This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-c2e69d46-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 2f7a11d0b76882e441a68aeb4464b4718ca71b7d Author: roryqi <[email protected]> AuthorDate: Wed Sep 9 13:17:20 2026 +0800 [#13007] fix(core,iceberg): Restore event dispatcher ordering (#13000) ### What changes were proposed in this pull request? Restore the public dispatcher order to: `Event -> Normalize -> Hook -> Manager/OperationDispatcher` Keep internal dispatchers independent from hook and event dispatchers, and apply the same ordering to Iceberg REST dispatchers. Route ownership updates for bulk role creation through the internal owner dispatcher, remove redundant normalization from hook dispatchers, and add regression tests covering owner assignment failures. ### Why are the changes needed? PR #10834 placed the hook dispatcher outside the event dispatcher so that CREATE events were emitted before the SET_OWNER event. After PR #12893, automatic ownership assignment is routed through internal dispatchers and no longer emits a nested SET_OWNER event. Keeping the previous order would emit a successful CREATE event before owner assignment completes, even if the overall CREATE request subsequently fails. Bulk role creation was also missed when automatic ownership updates were moved to internal dispatchers, so it could still emit nested SET_OWNER events. Restoring the event dispatcher as the outermost layer makes the CREATE event reflect the result of the complete operation. Fix: #13007 ### Does this PR introduce _any_ user-facing change? Yes. If automatic owner assignment fails, the CREATE operation now emits a failure event instead of an earlier success event. Automatic ownership updates, including bulk role creation, no longer emit nested SET_OWNER events. There are no API or property changes. ### How was this patch tested? - `./gradlew :core:test -PskipITs` - `./gradlew :core:test --tests org.apache.gravitino.hook.TestAccessControlHookDispatcher --tests org.apache.gravitino.listener.api.event.TestTableEvent -PskipITs` - Targeted Iceberg REST dispatcher tests - Core and Iceberg REST compile tests - `./gradlew :core:spotlessApply :iceberg:iceberg-rest-server:spotlessApply` - `git diff --check` # Conflicts: # core/src/main/java/org/apache/gravitino/GravitinoEnv.java # core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java # core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java --- .../java/org/apache/gravitino/GravitinoEnv.java | 162 +++++++++++++++------ .../gravitino/catalog/CapabilityHelpers.java | 11 -- .../hook/AccessControlHookDispatcher.java | 24 +++ .../gravitino/hook/FilesetHookDispatcher.java | 13 +- .../gravitino/hook/FunctionHookDispatcher.java | 25 +--- .../apache/gravitino/hook/ModelHookDispatcher.java | 22 +-- .../gravitino/hook/SchemaHookDispatcher.java | 26 +++- .../apache/gravitino/hook/TableHookDispatcher.java | 20 +-- .../apache/gravitino/hook/TopicHookDispatcher.java | 13 +- .../apache/gravitino/hook/ViewHookDispatcher.java | 17 +-- .../hook/TestAccessControlHookDispatcher.java | 17 +++ .../gravitino/hook/TestFilesetHookDispatcher.java | 5 +- .../gravitino/hook/TestFunctionHookDispatcher.java | 31 ++-- .../gravitino/hook/TestModelHookDispatcher.java | 6 +- .../gravitino/hook/TestSchemaHookDispatcher.java | 5 +- .../gravitino/hook/TestTableHookDispatcher.java | 17 +-- .../gravitino/hook/TestTopicHookDispatcher.java | 5 +- .../gravitino/hook/TestViewHookDispatcher.java | 16 +- .../listener/api/event/TestTableEvent.java | 36 +++++ .../org/apache/gravitino/iceberg/RESTService.java | 33 ++--- 20 files changed, 287 insertions(+), 217 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/GravitinoEnv.java b/core/src/main/java/org/apache/gravitino/GravitinoEnv.java index b60914afcf..8988aa72fe 100644 --- a/core/src/main/java/org/apache/gravitino/GravitinoEnv.java +++ b/core/src/main/java/org/apache/gravitino/GravitinoEnv.java @@ -746,17 +746,32 @@ public class GravitinoEnv { // Tree lock this.lockManager = new LockManager(config); +<<<<<<< HEAD +======= + // Create and initialize Catalog related modules first so MetalakeManager can force-drop + // child catalogs through CatalogManager.dropCatalog (same path as FilesetCatalogOperations). + // CatalogEventDispatcher -> CatalogNormalizeDispatcher -> CatalogHookDispatcher -> + // CatalogManager + // CatalogManager registers its own change-log listener with the entity store (when the store + // supports it), so no poller wiring is needed here. + this.catalogManager = new CatalogManager(config, entityStore, idGenerator, secretManager); + +>>>>>>> c2e69d464 ([#13007] fix(core,iceberg): Restore event dispatcher ordering (#13000)) // Create and initialize metalake related modules, the operation chain is: - // MetalakeHookDispatcher -> MetalakeEventDispatcher -> MetalakeNormalizeDispatcher -> + // MetalakeEventDispatcher -> MetalakeNormalizeDispatcher -> MetalakeHookDispatcher -> // MetalakeManager +<<<<<<< HEAD this.metalakeManager = new MetalakeManager(entityStore, idGenerator); +======= + this.metalakeManager = new MetalakeManager(entityStore, idGenerator, catalogManager); + this.internalMetalakeDispatcher = new MetalakeNormalizeDispatcher(metalakeManager); + MetalakeHookDispatcher metalakeHookDispatcher = new MetalakeHookDispatcher(metalakeManager); +>>>>>>> c2e69d464 ([#13007] fix(core,iceberg): Restore event dispatcher ordering (#13000)) MetalakeNormalizeDispatcher metalakeNormalizeDispatcher = - new MetalakeNormalizeDispatcher(metalakeManager); - this.internalMetalakeDispatcher = metalakeNormalizeDispatcher; - MetalakeEventDispatcher metalakeEventDispatcher = - new MetalakeEventDispatcher(eventBus, metalakeNormalizeDispatcher); - this.metalakeDispatcher = new MetalakeHookDispatcher(metalakeEventDispatcher); + new MetalakeNormalizeDispatcher(metalakeHookDispatcher); + this.metalakeDispatcher = new MetalakeEventDispatcher(eventBus, metalakeNormalizeDispatcher); +<<<<<<< HEAD // Create and initialize Catalog related modules, the operation chain is: // CatalogHookDispatcher -> CatalogEventDispatcher -> CatalogNormalizeDispatcher -> // CatalogManager @@ -764,41 +779,69 @@ public class GravitinoEnv { // supports it), so no poller wiring is needed here. this.catalogManager = new CatalogManager(config, entityStore, idGenerator); this.internalCatalogDispatcher = catalogManager; +======= + this.internalCatalogDispatcher = new CatalogNormalizeDispatcher(catalogManager); + CatalogHookDispatcher catalogHookDispatcher = new CatalogHookDispatcher(catalogManager); +>>>>>>> c2e69d464 ([#13007] fix(core,iceberg): Restore event dispatcher ordering (#13000)) CatalogNormalizeDispatcher catalogNormalizeDispatcher = - new CatalogNormalizeDispatcher(catalogManager); - this.internalCatalogDispatcher = catalogNormalizeDispatcher; - CatalogEventDispatcher catalogEventDispatcher = - new CatalogEventDispatcher(eventBus, catalogNormalizeDispatcher); - this.catalogDispatcher = new CatalogHookDispatcher(catalogEventDispatcher); + new CatalogNormalizeDispatcher(catalogHookDispatcher); + this.catalogDispatcher = new CatalogEventDispatcher(eventBus, catalogNormalizeDispatcher); this.credentialOperationDispatcher = +<<<<<<< HEAD new CredentialOperationDispatcher(catalogManager, entityStore, idGenerator); SchemaOperationDispatcher schemaOperationDispatcher = new SchemaOperationDispatcher(catalogManager, entityStore, idGenerator); this.internalSchemaDispatcher = schemaOperationDispatcher; SchemaNormalizeDispatcher schemaNormalizeDispatcher = +======= + new CredentialOperationDispatcher(catalogManager, entityStore, idGenerator, secretManager); + + this.secretPropertyOperationDispatcher = + new SecretPropertyOperationDispatcher( + catalogManager, entityStore, idGenerator, secretManager); + + // Fileset dispatcher is created before schema dispatcher so schema can take it directly. + FilesetOperationDispatcher filesetOperationDispatcher = + new FilesetOperationDispatcher(catalogManager, entityStore, idGenerator, secretManager); + FilesetNormalizeDispatcher internalFilesetNormalizeDispatcher = + new FilesetNormalizeDispatcher(filesetOperationDispatcher, catalogManager); + this.internalFilesetDispatcher = internalFilesetNormalizeDispatcher; + FilesetHookDispatcher filesetHookDispatcher = + new FilesetHookDispatcher(filesetOperationDispatcher); + FilesetNormalizeDispatcher filesetNormalizeDispatcher = + new FilesetNormalizeDispatcher(filesetHookDispatcher, catalogManager); + this.filesetDispatcher = new FilesetEventDispatcher(eventBus, filesetNormalizeDispatcher); + + SchemaOperationDispatcher schemaOperationDispatcher = + new SchemaOperationDispatcher( + catalogManager, + entityStore, + idGenerator, + secretManager, + internalFilesetNormalizeDispatcher); + this.internalSchemaDispatcher = +>>>>>>> c2e69d464 ([#13007] fix(core,iceberg): Restore event dispatcher ordering (#13000)) new SchemaNormalizeDispatcher(schemaOperationDispatcher, catalogManager); - this.internalSchemaDispatcher = schemaNormalizeDispatcher; - SchemaEventDispatcher schemaEventDispatcher = - new SchemaEventDispatcher(eventBus, schemaNormalizeDispatcher); - this.schemaDispatcher = new SchemaHookDispatcher(schemaEventDispatcher); + SchemaHookDispatcher schemaHookDispatcher = new SchemaHookDispatcher(schemaOperationDispatcher); + SchemaNormalizeDispatcher schemaNormalizeDispatcher = + new SchemaNormalizeDispatcher(schemaHookDispatcher, catalogManager); + this.schemaDispatcher = new SchemaEventDispatcher(eventBus, schemaNormalizeDispatcher); TableOperationDispatcher tableOperationDispatcher = new TableOperationDispatcher(catalogManager, entityStore, idGenerator); this.internalTableDispatcher = tableOperationDispatcher; - TableNormalizeDispatcher tableNormalizeDispatcher = - new TableNormalizeDispatcher(tableOperationDispatcher, catalogManager); TableOperationDispatcher internalTableOperationDispatcher = new TableOperationDispatcher( catalogManager, entityStore, idGenerator, () -> internalSchemaDispatcher); this.internalTableDispatcher = new TableNormalizeDispatcher(internalTableOperationDispatcher, catalogManager); - TableEventDispatcher tableEventDispatcher = - new TableEventDispatcher(eventBus, tableNormalizeDispatcher); - this.tableDispatcher = - new TableHookDispatcher( - tableEventDispatcher, this::internalOwnerDispatcher, catalogManager); + TableHookDispatcher tableHookDispatcher = + new TableHookDispatcher(tableOperationDispatcher, this::internalOwnerDispatcher); + TableNormalizeDispatcher tableNormalizeDispatcher = + new TableNormalizeDispatcher(tableHookDispatcher, catalogManager); + this.tableDispatcher = new TableEventDispatcher(eventBus, tableNormalizeDispatcher); // TODO: We can install hooks when we need, we only supports ownership post hook, // partition doesn't have ownership, so we don't need it now. @@ -818,28 +861,41 @@ public class GravitinoEnv { this.filesetDispatcher = new FilesetHookDispatcher(filesetEventDispatcher); TopicOperationDispatcher topicOperationDispatcher = +<<<<<<< HEAD new TopicOperationDispatcher(catalogManager, entityStore, idGenerator); TopicNormalizeDispatcher topicNormalizeDispatcher = +======= + new TopicOperationDispatcher(catalogManager, entityStore, idGenerator, secretManager); + TopicNormalizeDispatcher internalTopicNormalizeDispatcher = +>>>>>>> c2e69d464 ([#13007] fix(core,iceberg): Restore event dispatcher ordering (#13000)) new TopicNormalizeDispatcher(topicOperationDispatcher, catalogManager); - this.internalTopicDispatcher = topicNormalizeDispatcher; - TopicEventDispatcher topicEventDispatcher = - new TopicEventDispatcher(eventBus, topicNormalizeDispatcher); - this.topicDispatcher = new TopicHookDispatcher(topicEventDispatcher); + this.internalTopicDispatcher = internalTopicNormalizeDispatcher; + TopicHookDispatcher topicHookDispatcher = new TopicHookDispatcher(topicOperationDispatcher); + TopicNormalizeDispatcher topicNormalizeDispatcher = + new TopicNormalizeDispatcher(topicHookDispatcher, catalogManager); + this.topicDispatcher = new TopicEventDispatcher(eventBus, topicNormalizeDispatcher); ModelOperationDispatcher modelOperationDispatcher = +<<<<<<< HEAD new ModelOperationDispatcher(catalogManager, entityStore, idGenerator); ModelNormalizeDispatcher modelNormalizeDispatcher = +======= + new ModelOperationDispatcher(catalogManager, entityStore, idGenerator, secretManager); + ModelNormalizeDispatcher internalModelNormalizeDispatcher = +>>>>>>> c2e69d464 ([#13007] fix(core,iceberg): Restore event dispatcher ordering (#13000)) new ModelNormalizeDispatcher(modelOperationDispatcher, catalogManager); - this.internalModelDispatcher = modelNormalizeDispatcher; - ModelEventDispatcher modelEventDispatcher = - new ModelEventDispatcher(eventBus, modelNormalizeDispatcher); - this.modelDispatcher = new ModelHookDispatcher(modelEventDispatcher); + this.internalModelDispatcher = internalModelNormalizeDispatcher; + ModelHookDispatcher modelHookDispatcher = new ModelHookDispatcher(modelOperationDispatcher); + ModelNormalizeDispatcher modelNormalizeDispatcher = + new ModelNormalizeDispatcher(modelHookDispatcher, catalogManager); + this.modelDispatcher = new ModelEventDispatcher(eventBus, modelNormalizeDispatcher); // Create and initialize Function related modules, the operation chain is: - // FunctionHookDispatcher -> FunctionEventDispatcher -> FunctionNormalizeDispatcher -> + // FunctionEventDispatcher -> FunctionNormalizeDispatcher -> FunctionHookDispatcher -> // FunctionOperationDispatcher FunctionOperationDispatcher functionOperationDispatcher = new FunctionOperationDispatcher( +<<<<<<< HEAD catalogManager, schemaOperationDispatcher, entityStore, idGenerator); FunctionNormalizeDispatcher functionNormalizeDispatcher = new FunctionNormalizeDispatcher(functionOperationDispatcher, catalogManager); @@ -848,23 +904,33 @@ public class GravitinoEnv { new FunctionEventDispatcher(eventBus, functionNormalizeDispatcher); this.functionDispatcher = new FunctionHookDispatcher(functionEventDispatcher, this::ownerDispatcher, catalogManager); +======= + catalogManager, schemaOperationDispatcher, entityStore, idGenerator, secretManager); + FunctionNormalizeDispatcher internalFunctionNormalizeDispatcher = + new FunctionNormalizeDispatcher(functionOperationDispatcher, catalogManager); + this.internalFunctionDispatcher = internalFunctionNormalizeDispatcher; + FunctionHookDispatcher functionHookDispatcher = + new FunctionHookDispatcher(functionOperationDispatcher, this::internalOwnerDispatcher); + FunctionNormalizeDispatcher functionNormalizeDispatcher = + new FunctionNormalizeDispatcher(functionHookDispatcher, catalogManager); + this.functionDispatcher = new FunctionEventDispatcher(eventBus, functionNormalizeDispatcher); +>>>>>>> c2e69d464 ([#13007] fix(core,iceberg): Restore event dispatcher ordering (#13000)) - // View operation chain: ViewHookDispatcher -> ViewEventDispatcher -> ViewNormalizeDispatcher + // View operation chain: ViewEventDispatcher -> ViewNormalizeDispatcher -> ViewHookDispatcher // -> ViewOperationDispatcher. ViewOperationDispatcher viewOperationDispatcher = new ViewOperationDispatcher(catalogManager, entityStore, idGenerator); this.internalViewDispatcher = viewOperationDispatcher; - ViewNormalizeDispatcher viewNormalizeDispatcher = - new ViewNormalizeDispatcher(viewOperationDispatcher, catalogManager); ViewOperationDispatcher internalViewOperationDispatcher = new ViewOperationDispatcher( catalogManager, entityStore, idGenerator, () -> internalSchemaDispatcher); this.internalViewDispatcher = new ViewNormalizeDispatcher(internalViewOperationDispatcher, catalogManager); - ViewEventDispatcher viewEventDispatcher = - new ViewEventDispatcher(eventBus, viewNormalizeDispatcher); - this.viewDispatcher = - new ViewHookDispatcher(viewEventDispatcher, this::internalOwnerDispatcher, catalogManager); + ViewHookDispatcher viewHookDispatcher = + new ViewHookDispatcher(viewOperationDispatcher, this::internalOwnerDispatcher); + ViewNormalizeDispatcher viewNormalizeDispatcher = + new ViewNormalizeDispatcher(viewHookDispatcher, catalogManager); + this.viewDispatcher = new ViewEventDispatcher(eventBus, viewNormalizeDispatcher); this.statisticDispatcher = new StatisticEventDispatcher( @@ -876,9 +942,10 @@ public class GravitinoEnv { AccessControlManager accessControlManager = new AccessControlManager(entityStore, idGenerator, config); this.internalAccessControlDispatcher = accessControlManager; - AccessControlEventDispatcher accessControlEventDispatcher = - new AccessControlEventDispatcher(eventBus, accessControlManager); - this.accessControlDispatcher = new AccessControlHookDispatcher(accessControlEventDispatcher); + AccessControlHookDispatcher accessControlHookDispatcher = + new AccessControlHookDispatcher(accessControlManager); + this.accessControlDispatcher = + new AccessControlEventDispatcher(eventBus, accessControlHookDispatcher); OwnerDispatcher ownerManager = new OwnerManager(entityStore); this.internalOwnerDispatcher = ownerManager; this.ownerDispatcher = new OwnerEventManager(eventBus, ownerManager); @@ -897,21 +964,20 @@ public class GravitinoEnv { // Create and initialize Tag related modules TagManager tagManager = new TagManager(idGenerator, entityStore); this.internalTagDispatcher = tagManager; - TagEventDispatcher tagEventDispatcher = new TagEventDispatcher(eventBus, tagManager); - this.tagDispatcher = new TagHookDispatcher(tagEventDispatcher); + TagHookDispatcher tagHookDispatcher = new TagHookDispatcher(tagManager); + this.tagDispatcher = new TagEventDispatcher(eventBus, tagHookDispatcher); PolicyManager policyManager = new PolicyManager(idGenerator, entityStore); this.internalPolicyDispatcher = policyManager; - PolicyEventDispatcher policyEventDispatcher = - new PolicyEventDispatcher(eventBus, policyManager); - this.policyDispatcher = new PolicyHookDispatcher(policyEventDispatcher); + PolicyHookDispatcher policyHookDispatcher = new PolicyHookDispatcher(policyManager); + this.policyDispatcher = new PolicyEventDispatcher(eventBus, policyHookDispatcher); JobManager jobManager = new JobManager(config, entityStore, idGenerator); JobTemplateValidationDispatcher validationDispatcher = new JobTemplateValidationDispatcher(jobManager); this.internalJobOperationDispatcher = validationDispatcher; - JobEventDispatcher jobEventDispatcher = new JobEventDispatcher(eventBus, validationDispatcher); - this.jobOperationDispatcher = new JobHookDispatcher(jobEventDispatcher); + JobHookDispatcher jobHookDispatcher = new JobHookDispatcher(validationDispatcher); + this.jobOperationDispatcher = new JobEventDispatcher(eventBus, jobHookDispatcher); // Register built-in job template event listener to automatically register templates // when metalakes are created diff --git a/core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java b/core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java index 1a68e38a6c..e29f2596af 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java +++ b/core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java @@ -124,17 +124,6 @@ public class CapabilityHelpers { return NameIdentifier.of(namespace, name); } - /** - * Convenience overload that loads the catalog capability for {@code ident} and applies it to the - * identifier. Use this from call sites (e.g. HookDispatchers) that need a normalized identifier - * but do not already hold a {@link Capability} instance. - */ - public static NameIdentifier applyCapabilities( - NameIdentifier ident, Capability.Scope scope, CatalogManager catalogManager) { - Capability capability = getCapability(ident, catalogManager); - return applyCapabilities(ident, scope, capability); - } - public static NameIdentifier[] applyCaseSensitive( NameIdentifier[] idents, Capability.Scope scope, Capability capabilities) { return Arrays.stream(idents) diff --git a/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java index a22f81d721..4eca2d9def 100644 --- a/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java @@ -177,6 +177,30 @@ public class AccessControlHookDispatcher implements AccessControlDispatcher { } @Override +<<<<<<< HEAD +======= + public List<BulkItemResult<Role>> createRoles(String metalake, List<RoleAdd> roles) + throws NoSuchMetalakeException { + List<BulkItemResult<Role>> results = dispatcher.createRoles(metalake, roles); + OwnerDispatcher ownerDispatcher = GravitinoEnv.getInstance().internalOwnerDispatcher(); + if (ownerDispatcher != null) { + results.stream() + .filter(BulkItemResult::succeeded) + .forEach( + result -> + ownerDispatcher.setOwner( + metalake, + NameIdentifierUtil.toMetadataObject( + AuthorizationUtils.ofRole(metalake, result.name()), + Entity.EntityType.ROLE), + PrincipalUtils.getCurrentUserName(), + Owner.Type.USER)); + } + return results; + } + + @Override +>>>>>>> c2e69d464 ([#13007] fix(core,iceberg): Restore event dispatcher ordering (#13000)) public Role getRole(String metalake, String role) throws NoSuchRoleException, NoSuchMetalakeException { return dispatcher.getRole(metalake, role); diff --git a/core/src/main/java/org/apache/gravitino/hook/FilesetHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/FilesetHookDispatcher.java index 66f3b75642..d4794808ca 100644 --- a/core/src/main/java/org/apache/gravitino/hook/FilesetHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/FilesetHookDispatcher.java @@ -28,9 +28,7 @@ import org.apache.gravitino.Namespace; import org.apache.gravitino.authorization.AuthorizationUtils; import org.apache.gravitino.authorization.Owner; import org.apache.gravitino.authorization.OwnerDispatcher; -import org.apache.gravitino.catalog.CapabilityHelpers; import org.apache.gravitino.catalog.FilesetDispatcher; -import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.exceptions.FilesetAlreadyExistsException; import org.apache.gravitino.exceptions.NoSuchFilesetException; import org.apache.gravitino.exceptions.NoSuchLocationNameException; @@ -84,16 +82,9 @@ public class FilesetHookDispatcher implements FilesetDispatcher { // Set the creator as the owner of the fileset. OwnerDispatcher ownerManager = GravitinoEnv.getInstance().internalOwnerDispatcher(); if (ownerManager != null) { - // The inner NormalizeDispatcher case-folds the fileset name (and its schema namespace) - // based on catalog capabilities, so the entity is stored under the normalized identifier. - // Apply the same normalization here so the owner is attached to the same identifier the - // manager sees. - NameIdentifier normalizedIdent = - CapabilityHelpers.applyCapabilities( - ident, Capability.Scope.FILESET, GravitinoEnv.getInstance().catalogManager()); ownerManager.setOwner( - normalizedIdent.namespace().level(0), - NameIdentifierUtil.toMetadataObject(normalizedIdent, Entity.EntityType.FILESET), + ident.namespace().level(0), + NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.FILESET), PrincipalUtils.getCurrentUserName(), Owner.Type.USER); } diff --git a/core/src/main/java/org/apache/gravitino/hook/FunctionHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/FunctionHookDispatcher.java index 33c70bda57..895731d106 100644 --- a/core/src/main/java/org/apache/gravitino/hook/FunctionHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/FunctionHookDispatcher.java @@ -26,10 +26,7 @@ import org.apache.gravitino.Namespace; import org.apache.gravitino.authorization.AuthorizationUtils; import org.apache.gravitino.authorization.Owner; import org.apache.gravitino.authorization.OwnerDispatcher; -import org.apache.gravitino.catalog.CapabilityHelpers; -import org.apache.gravitino.catalog.CatalogManager; import org.apache.gravitino.catalog.FunctionDispatcher; -import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.exceptions.FunctionAlreadyExistsException; import org.apache.gravitino.exceptions.NoSuchFunctionException; import org.apache.gravitino.exceptions.NoSuchSchemaException; @@ -48,7 +45,6 @@ import org.apache.gravitino.utils.PrincipalUtils; public class FunctionHookDispatcher implements FunctionDispatcher { private final FunctionDispatcher dispatcher; private final Supplier<OwnerDispatcher> ownerDispatcher; - private final CatalogManager catalogManager; /** * Creates a function hook dispatcher. @@ -56,15 +52,11 @@ public class FunctionHookDispatcher implements FunctionDispatcher { * @param dispatcher the underlying function dispatcher * @param ownerDispatcher supplies the owner dispatcher, or {@code null} when authorization is * disabled - * @param catalogManager the catalog manager used to apply catalog capabilities */ public FunctionHookDispatcher( - FunctionDispatcher dispatcher, - Supplier<OwnerDispatcher> ownerDispatcher, - CatalogManager catalogManager) { + FunctionDispatcher dispatcher, Supplier<OwnerDispatcher> ownerDispatcher) { this.dispatcher = dispatcher; this.ownerDispatcher = ownerDispatcher; - this.catalogManager = catalogManager; } @Override @@ -101,15 +93,9 @@ public class FunctionHookDispatcher implements FunctionDispatcher { // Set the creator as the owner of the function. OwnerDispatcher ownerManager = ownerDispatcher.get(); if (ownerManager != null) { - // The inner NormalizeDispatcher case-folds the function name (and its schema namespace) - // based on catalog capabilities, so the entity is stored under the normalized identifier. - // Apply the same normalization here so the owner is attached to the same identifier the - // manager sees. - NameIdentifier normalizedIdent = - CapabilityHelpers.applyCapabilities(ident, Capability.Scope.FUNCTION, catalogManager); ownerManager.setOwner( - normalizedIdent.namespace().level(0), - NameIdentifierUtil.toMetadataObject(normalizedIdent, Entity.EntityType.FUNCTION), + ident.namespace().level(0), + NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.FUNCTION), PrincipalUtils.getCurrentUserName(), Owner.Type.USER); } @@ -126,13 +112,10 @@ public class FunctionHookDispatcher implements FunctionDispatcher { public boolean dropFunction(NameIdentifier ident) { boolean dropped = dispatcher.dropFunction(ident); if (dropped) { - NameIdentifier normalizedIdent = - CapabilityHelpers.applyCaseSensitive(ident, Capability.Scope.FUNCTION, catalogManager); // Function privileges are managed by Gravitino. Catalog authorization plugins such as // Ranger HadoopSQL do not support FUNCTION metadata objects, so only invalidate the built-in // authorizer's name-to-ID mapping here. - AuthorizationUtils.notifyEntityNameIdMappingChange( - normalizedIdent, Entity.EntityType.FUNCTION); + AuthorizationUtils.notifyEntityNameIdMappingChange(ident, Entity.EntityType.FUNCTION); } return dropped; } diff --git a/core/src/main/java/org/apache/gravitino/hook/ModelHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/ModelHookDispatcher.java index 2143030b7a..facad41909 100644 --- a/core/src/main/java/org/apache/gravitino/hook/ModelHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/ModelHookDispatcher.java @@ -26,9 +26,7 @@ import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; import org.apache.gravitino.authorization.Owner; import org.apache.gravitino.authorization.OwnerDispatcher; -import org.apache.gravitino.catalog.CapabilityHelpers; import org.apache.gravitino.catalog.ModelDispatcher; -import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.exceptions.ModelAlreadyExistsException; import org.apache.gravitino.exceptions.ModelVersionAliasesAlreadyExistException; import org.apache.gravitino.exceptions.NoSuchModelException; @@ -73,15 +71,9 @@ public class ModelHookDispatcher implements ModelDispatcher { // Set the creator as owner of the model. OwnerDispatcher ownerManager = GravitinoEnv.getInstance().internalOwnerDispatcher(); if (ownerManager != null) { - // The inner NormalizeDispatcher case-folds the model name based on catalog capabilities, - // so the entity is stored under the normalized identifier. Apply the same normalization - // here so the owner is attached to the same identifier the manager sees. - NameIdentifier normalizedIdent = - CapabilityHelpers.applyCapabilities( - ident, Capability.Scope.MODEL, GravitinoEnv.getInstance().catalogManager()); ownerManager.setOwner( - normalizedIdent.namespace().level(0), - NameIdentifierUtil.toMetadataObject(normalizedIdent, Entity.EntityType.MODEL), + ident.namespace().level(0), + NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.MODEL), PrincipalUtils.getCurrentUserName(), Owner.Type.USER); } @@ -167,15 +159,9 @@ public class ModelHookDispatcher implements ModelDispatcher { // Set the creator as owner of the model. OwnerDispatcher ownerManager = GravitinoEnv.getInstance().internalOwnerDispatcher(); if (ownerManager != null) { - // The inner NormalizeDispatcher case-folds the model name based on catalog capabilities, - // so the entity is stored under the normalized identifier. Apply the same normalization - // here so the owner is attached to the same identifier the manager sees. - NameIdentifier normalizedIdent = - CapabilityHelpers.applyCapabilities( - ident, Capability.Scope.MODEL, GravitinoEnv.getInstance().catalogManager()); ownerManager.setOwner( - normalizedIdent.namespace().level(0), - NameIdentifierUtil.toMetadataObject(normalizedIdent, Entity.EntityType.MODEL), + ident.namespace().level(0), + NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.MODEL), PrincipalUtils.getCurrentUserName(), Owner.Type.USER); } diff --git a/core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java index 3b3b54a643..654a431243 100644 --- a/core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java @@ -32,9 +32,7 @@ import org.apache.gravitino.SchemaChange; import org.apache.gravitino.authorization.AuthorizationUtils; import org.apache.gravitino.authorization.Owner; import org.apache.gravitino.authorization.OwnerDispatcher; -import org.apache.gravitino.catalog.CapabilityHelpers; import org.apache.gravitino.catalog.SchemaDispatcher; -import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.exceptions.NoSuchCatalogException; import org.apache.gravitino.exceptions.NoSuchSchemaException; import org.apache.gravitino.exceptions.NonEmptySchemaException; @@ -65,6 +63,7 @@ public class SchemaHookDispatcher implements SchemaDispatcher { @Override public Schema createSchema(NameIdentifier ident, String comment, Map<String, String> properties) throws NoSuchCatalogException, SchemaAlreadyExistsException { +<<<<<<< HEAD // The inner NormalizeDispatcher case-folds the schema name based on catalog capabilities, so // the entity is stored under the normalized identifier. Normalize here too so ownership is // attached to the identifiers the manager sees and ancestor probing matches stored names. @@ -72,6 +71,19 @@ public class SchemaHookDispatcher implements SchemaDispatcher { CapabilityHelpers.applyCapabilities( ident, Capability.Scope.SCHEMA, GravitinoEnv.getInstance().catalogManager()); +======= + return createSchema(ident, comment, properties, Collections.emptyMap(), Collections.emptyMap()); + } + + @Override + public Schema createSchema( + NameIdentifier ident, + String comment, + Map<String, String> properties, + Map<String, SecretBinding> secretBindings, + Map<String, SecretReference> secretReferences) + throws NoSuchCatalogException, SchemaAlreadyExistsException { +>>>>>>> c2e69d464 ([#13007] fix(core,iceberg): Restore event dispatcher ordering (#13000)) // Serialize probe -> create -> owner-assignment on the catalog so concurrent hierarchical // creates cannot both claim a shared, newly-created ancestor (which would let the later create // overwrite the first creator's ownership). We lock the catalog node -- the same node the inner @@ -79,8 +91,7 @@ public class SchemaHookDispatcher implements SchemaDispatcher { // deeper (branch-scoped) lock would hold the catalog node in READ mode and deadlock against // that inner WRITE acquisition. NameIdentifier catalogIdent = - NameIdentifierUtil.ofCatalog( - normalizedIdent.namespace().level(0), normalizedIdent.namespace().level(1)); + NameIdentifierUtil.ofCatalog(ident.namespace().level(0), ident.namespace().level(1)); return TreeLockUtils.doWithTreeLock( catalogIdent, LockType.WRITE, @@ -89,7 +100,7 @@ public class SchemaHookDispatcher implements SchemaDispatcher { // missing ancestor ("A", "A:B"). Probe BEFORE the create which ancestors are new, so // ownership is assigned only to schemas this request actually creates and a pre-existing // ancestor's owner is never overwritten. - List<NameIdentifier> newAncestors = findMissingAncestors(normalizedIdent); + List<NameIdentifier> newAncestors = findMissingAncestors(ident); Schema schema = dispatcher.createSchema(ident, comment, properties); @@ -104,11 +115,10 @@ public class SchemaHookDispatcher implements SchemaDispatcher { ownedObjects.add( NameIdentifierUtil.toMetadataObject(ancestor, Entity.EntityType.SCHEMA)); } - ownedObjects.add( - NameIdentifierUtil.toMetadataObject(normalizedIdent, Entity.EntityType.SCHEMA)); + ownedObjects.add(NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.SCHEMA)); // All objects are SCHEMA-typed, so the batch path (single object type) is valid. ownerManager.setOwners( - normalizedIdent.namespace().level(0), + ident.namespace().level(0), ownedObjects, PrincipalUtils.getCurrentUserName(), Owner.Type.USER); diff --git a/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java index bb923afdf2..9a1037e1ca 100644 --- a/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java @@ -27,10 +27,7 @@ import org.apache.gravitino.Namespace; import org.apache.gravitino.authorization.AuthorizationUtils; import org.apache.gravitino.authorization.Owner; import org.apache.gravitino.authorization.OwnerDispatcher; -import org.apache.gravitino.catalog.CapabilityHelpers; -import org.apache.gravitino.catalog.CatalogManager; import org.apache.gravitino.catalog.TableDispatcher; -import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.exceptions.NoSuchSchemaException; import org.apache.gravitino.exceptions.NoSuchTableException; import org.apache.gravitino.exceptions.TableAlreadyExistsException; @@ -52,7 +49,6 @@ import org.apache.gravitino.utils.PrincipalUtils; public class TableHookDispatcher implements TableDispatcher { private final TableDispatcher dispatcher; private final Supplier<OwnerDispatcher> ownerDispatcher; - private final CatalogManager catalogManager; /** * Creates a table hook dispatcher. @@ -60,15 +56,11 @@ public class TableHookDispatcher implements TableDispatcher { * @param dispatcher the underlying table dispatcher * @param ownerDispatcher supplies the owner dispatcher, or {@code null} when authorization is * disabled - * @param catalogManager the catalog manager used to apply catalog capabilities */ public TableHookDispatcher( - TableDispatcher dispatcher, - Supplier<OwnerDispatcher> ownerDispatcher, - CatalogManager catalogManager) { + TableDispatcher dispatcher, Supplier<OwnerDispatcher> ownerDispatcher) { this.dispatcher = dispatcher; this.ownerDispatcher = ownerDispatcher; - this.catalogManager = catalogManager; } @Override @@ -99,15 +91,9 @@ public class TableHookDispatcher implements TableDispatcher { // Set the creator as the owner of the table. OwnerDispatcher ownerManager = ownerDispatcher.get(); if (ownerManager != null) { - // The inner NormalizeDispatcher case-folds the table name (and its schema namespace) - // based on catalog capabilities, so the entity is stored under the normalized identifier. - // Apply the same normalization here so the owner is attached to the same identifier the - // manager sees. - NameIdentifier normalizedIdent = - CapabilityHelpers.applyCapabilities(ident, Capability.Scope.TABLE, catalogManager); ownerManager.setOwner( - normalizedIdent.namespace().level(0), - NameIdentifierUtil.toMetadataObject(normalizedIdent, Entity.EntityType.TABLE), + ident.namespace().level(0), + NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.TABLE), PrincipalUtils.getCurrentUserName(), Owner.Type.USER); } diff --git a/core/src/main/java/org/apache/gravitino/hook/TopicHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/TopicHookDispatcher.java index cae51e74fc..ad8440942b 100644 --- a/core/src/main/java/org/apache/gravitino/hook/TopicHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/TopicHookDispatcher.java @@ -27,9 +27,7 @@ import org.apache.gravitino.Namespace; import org.apache.gravitino.authorization.AuthorizationUtils; import org.apache.gravitino.authorization.Owner; import org.apache.gravitino.authorization.OwnerDispatcher; -import org.apache.gravitino.catalog.CapabilityHelpers; import org.apache.gravitino.catalog.TopicDispatcher; -import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.exceptions.NoSuchSchemaException; import org.apache.gravitino.exceptions.NoSuchTopicException; import org.apache.gravitino.exceptions.TopicAlreadyExistsException; @@ -70,16 +68,9 @@ public class TopicHookDispatcher implements TopicDispatcher { // Set the creator as the owner of the topic. OwnerDispatcher ownerManager = GravitinoEnv.getInstance().internalOwnerDispatcher(); if (ownerManager != null) { - // The inner NormalizeDispatcher case-folds the topic name (and its schema namespace) - // based on catalog capabilities, so the entity is stored under the normalized identifier. - // Apply the same normalization here so the owner is attached to the same identifier the - // manager sees. - NameIdentifier normalizedIdent = - CapabilityHelpers.applyCapabilities( - ident, Capability.Scope.TOPIC, GravitinoEnv.getInstance().catalogManager()); ownerManager.setOwner( - normalizedIdent.namespace().level(0), - NameIdentifierUtil.toMetadataObject(normalizedIdent, Entity.EntityType.TOPIC), + ident.namespace().level(0), + NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.TOPIC), PrincipalUtils.getCurrentUserName(), Owner.Type.USER); } diff --git a/core/src/main/java/org/apache/gravitino/hook/ViewHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/ViewHookDispatcher.java index 2273cd3d3e..1f871682e9 100644 --- a/core/src/main/java/org/apache/gravitino/hook/ViewHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/ViewHookDispatcher.java @@ -27,10 +27,7 @@ import org.apache.gravitino.Namespace; import org.apache.gravitino.authorization.AuthorizationUtils; import org.apache.gravitino.authorization.Owner; import org.apache.gravitino.authorization.OwnerDispatcher; -import org.apache.gravitino.catalog.CapabilityHelpers; -import org.apache.gravitino.catalog.CatalogManager; import org.apache.gravitino.catalog.ViewDispatcher; -import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.exceptions.NoSuchSchemaException; import org.apache.gravitino.exceptions.NoSuchViewException; import org.apache.gravitino.exceptions.ViewAlreadyExistsException; @@ -48,7 +45,6 @@ import org.apache.gravitino.utils.PrincipalUtils; public class ViewHookDispatcher implements ViewDispatcher { private final ViewDispatcher dispatcher; private final Supplier<OwnerDispatcher> ownerDispatcher; - private final CatalogManager catalogManager; /** * Creates a view hook dispatcher. @@ -56,15 +52,10 @@ public class ViewHookDispatcher implements ViewDispatcher { * @param dispatcher the underlying view dispatcher * @param ownerDispatcher supplies the owner dispatcher, or {@code null} when authorization is * disabled - * @param catalogManager the catalog manager used to apply catalog capabilities */ - public ViewHookDispatcher( - ViewDispatcher dispatcher, - Supplier<OwnerDispatcher> ownerDispatcher, - CatalogManager catalogManager) { + public ViewHookDispatcher(ViewDispatcher dispatcher, Supplier<OwnerDispatcher> ownerDispatcher) { this.dispatcher = dispatcher; this.ownerDispatcher = ownerDispatcher; - this.catalogManager = catalogManager; } @Override @@ -98,11 +89,9 @@ public class ViewHookDispatcher implements ViewDispatcher { OwnerDispatcher ownerManager = ownerDispatcher.get(); if (ownerManager != null) { - NameIdentifier normalizedIdent = - CapabilityHelpers.applyCapabilities(ident, Capability.Scope.VIEW, catalogManager); ownerManager.setOwner( - normalizedIdent.namespace().level(0), - NameIdentifierUtil.toMetadataObject(normalizedIdent, Entity.EntityType.VIEW), + ident.namespace().level(0), + NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.VIEW), PrincipalUtils.getCurrentUserName(), Owner.Type.USER); } diff --git a/core/src/test/java/org/apache/gravitino/hook/TestAccessControlHookDispatcher.java b/core/src/test/java/org/apache/gravitino/hook/TestAccessControlHookDispatcher.java index 308ea03444..3d587a05fe 100644 --- a/core/src/test/java/org/apache/gravitino/hook/TestAccessControlHookDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/hook/TestAccessControlHookDispatcher.java @@ -26,6 +26,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import java.util.Collections; +import java.util.List; import org.apache.commons.lang3.reflect.FieldUtils; import org.apache.gravitino.GravitinoEnv; import org.apache.gravitino.authorization.AccessControlDispatcher; @@ -34,6 +35,8 @@ import org.apache.gravitino.authorization.Group; import org.apache.gravitino.authorization.OwnerDispatcher; import org.apache.gravitino.authorization.Role; import org.apache.gravitino.authorization.User; +import org.apache.gravitino.bulk.BulkItemResult; +import org.apache.gravitino.bulk.RoleAdd; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; @@ -89,6 +92,20 @@ public class TestAccessControlHookDispatcher { verify(mockDispatcher).createRole(any(), any(), any(), any()); } + @Test + public void testCreateRolesSetsOwnerWithInternalDispatcher() { + List<RoleAdd> roles = + Collections.singletonList( + new RoleAdd("test_role", Collections.emptyMap(), Collections.emptyList())); + List<BulkItemResult<Role>> results = + Collections.singletonList(BulkItemResult.success(0, "test_role", mock(Role.class))); + when(mockDispatcher.createRoles("test_metalake", roles)).thenReturn(results); + + Assertions.assertSame(results, hookDispatcher.createRoles("test_metalake", roles)); + + verify(mockOwnerDispatcher).setOwner(any(), any(), any(), any()); + } + @Test public void testGrantRolesToUserInvalidatesUserRoleRelation() { User mockUser = mock(User.class); diff --git a/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java b/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java index f212adc504..2710ff8748 100644 --- a/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java @@ -55,6 +55,7 @@ import org.apache.gravitino.authorization.OwnerDispatcher; import org.apache.gravitino.catalog.CatalogManager; import org.apache.gravitino.catalog.CatalogTestUtils; import org.apache.gravitino.catalog.FilesetDispatcher; +import org.apache.gravitino.catalog.FilesetNormalizeDispatcher; import org.apache.gravitino.catalog.TestFilesetOperationDispatcher; import org.apache.gravitino.catalog.TestOperationDispatcher; import org.apache.gravitino.connector.BaseCatalog; @@ -121,7 +122,9 @@ public class TestFilesetHookDispatcher extends TestOperationDispatcher { GravitinoEnv.getInstance(), "internalOwnerDispatcher", mockOwnerDispatcher, true); try { - FilesetHookDispatcher localHook = new FilesetHookDispatcher(mockFilesetDispatcher); + FilesetDispatcher localHook = + new FilesetNormalizeDispatcher( + new FilesetHookDispatcher(mockFilesetDispatcher), mockCatalogManager); NameIdentifier ident = NameIdentifier.of(metalake, catalog, "SCHEMA_NORM", "MY_FILESET"); localHook.createMultipleLocationFileset( ident, diff --git a/core/src/test/java/org/apache/gravitino/hook/TestFunctionHookDispatcher.java b/core/src/test/java/org/apache/gravitino/hook/TestFunctionHookDispatcher.java index 6b375f2af1..e06a63a8eb 100644 --- a/core/src/test/java/org/apache/gravitino/hook/TestFunctionHookDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/hook/TestFunctionHookDispatcher.java @@ -38,6 +38,7 @@ import org.apache.gravitino.authorization.OwnerDispatcher; import org.apache.gravitino.catalog.CatalogManager; import org.apache.gravitino.catalog.CatalogTestUtils; import org.apache.gravitino.catalog.FunctionDispatcher; +import org.apache.gravitino.catalog.FunctionNormalizeDispatcher; import org.apache.gravitino.connector.BaseCatalog; import org.apache.gravitino.connector.authorization.AuthorizationPlugin; import org.apache.gravitino.connector.capability.Capability; @@ -61,8 +62,6 @@ public class TestFunctionHookDispatcher { Function registeredFunction = Mockito.mock(Function.class); OwnerDispatcher ownerDispatcher = Mockito.mock(OwnerDispatcher.class); - CatalogManager catalogManager = catalogManagerWith(Capability.DEFAULT); - Mockito.when( dispatcher.registerFunction( Mockito.eq(functionIdentifier), @@ -73,7 +72,7 @@ public class TestFunctionHookDispatcher { .thenReturn(registeredFunction); FunctionHookDispatcher hookDispatcher = - new FunctionHookDispatcher(dispatcher, () -> ownerDispatcher, catalogManager); + new FunctionHookDispatcher(dispatcher, () -> ownerDispatcher); Function result = hookDispatcher.registerFunction( functionIdentifier, "comment", FunctionType.SCALAR, true, definitions); @@ -109,9 +108,7 @@ public class TestFunctionHookDispatcher { Mockito.eq(definitions))) .thenReturn(registeredFunction); - CatalogManager catalogManager = Mockito.mock(CatalogManager.class); - FunctionHookDispatcher hookDispatcher = - new FunctionHookDispatcher(dispatcher, () -> null, catalogManager); + FunctionHookDispatcher hookDispatcher = new FunctionHookDispatcher(dispatcher, () -> null); Function result = hookDispatcher.registerFunction( functionIdentifier, "comment", FunctionType.SCALAR, true, definitions); @@ -119,7 +116,6 @@ public class TestFunctionHookDispatcher { assertSame(registeredFunction, result); Mockito.verify(dispatcher) .registerFunction(functionIdentifier, "comment", FunctionType.SCALAR, true, definitions); - Mockito.verifyNoInteractions(catalogManager); } @Test @@ -137,9 +133,10 @@ public class TestFunctionHookDispatcher { any(), any(), any(), Mockito.anyBoolean(), any())) .thenReturn(mockFunction); - FunctionHookDispatcher hook = - new FunctionHookDispatcher( - mockFunctionDispatcher, () -> mockOwnerDispatcher, catalogManager); + FunctionDispatcher hook = + new FunctionNormalizeDispatcher( + new FunctionHookDispatcher(mockFunctionDispatcher, () -> mockOwnerDispatcher), + catalogManager); NameIdentifier ident = NameIdentifier.of("metalake1", "catalog1", "SCHEMA_NORM", "MY_FUNC"); hook.registerFunction(ident, "comment", FunctionType.SCALAR, true, definitions); @@ -157,8 +154,6 @@ public class TestFunctionHookDispatcher { .when(mockOwnerDispatcher) .setOwner(any(), any(), any(), any()); - CatalogManager catalogManager = catalogManagerWith(Capability.DEFAULT); - FunctionDispatcher mockFunctionDispatcher = Mockito.mock(FunctionDispatcher.class); Function mockFunction = Mockito.mock(Function.class); FunctionDefinition[] definitions = new FunctionDefinition[] {}; @@ -168,8 +163,7 @@ public class TestFunctionHookDispatcher { .thenReturn(mockFunction); FunctionHookDispatcher hook = - new FunctionHookDispatcher( - mockFunctionDispatcher, () -> mockOwnerDispatcher, catalogManager); + new FunctionHookDispatcher(mockFunctionDispatcher, () -> mockOwnerDispatcher); NameIdentifier ident = NameIdentifier.of("metalake1", "catalog1", "schema_owner_fail", "func_owner_fail"); RuntimeException thrown = @@ -187,7 +181,7 @@ public class TestFunctionHookDispatcher { NameIdentifier normalizedIdentifier = NameIdentifier.of("metalake1", "catalog1", "schema1", "func1"); FunctionDispatcher dispatcher = Mockito.mock(FunctionDispatcher.class); - Mockito.when(dispatcher.dropFunction(functionIdentifier)) + Mockito.when(dispatcher.dropFunction(normalizedIdentifier)) .thenReturn(true, false) .thenThrow(new RuntimeException("Drop failed")); CatalogManager catalogManager = catalogManagerWith(new CaseInsensitiveCapability()); @@ -206,8 +200,9 @@ public class TestFunctionHookDispatcher { try (MockedStatic<GravitinoEnv> envStatic = Mockito.mockStatic(GravitinoEnv.class)) { envStatic.when(GravitinoEnv::getInstance).thenReturn(env); - FunctionHookDispatcher hookDispatcher = - new FunctionHookDispatcher(dispatcher, () -> null, catalogManager); + FunctionDispatcher hookDispatcher = + new FunctionNormalizeDispatcher( + new FunctionHookDispatcher(dispatcher, () -> null), catalogManager); assertTrue(hookDispatcher.dropFunction(functionIdentifier)); assertFalse(hookDispatcher.dropFunction(functionIdentifier)); @@ -222,7 +217,7 @@ public class TestFunctionHookDispatcher { // mock represents that integration boundary and must not receive a removal callback. Mockito.verifyNoInteractions(catalogAuthorizationPlugin); Mockito.verify(catalogManager, Mockito.never()).loadCatalog(any()); - Mockito.verify(catalogManager, Mockito.times(1)).doWithCatalog(any(), any()); + Mockito.verify(catalogManager, Mockito.times(3)).doWithCatalog(any(), any()); } } diff --git a/core/src/test/java/org/apache/gravitino/hook/TestModelHookDispatcher.java b/core/src/test/java/org/apache/gravitino/hook/TestModelHookDispatcher.java index 48fa0cfd06..856836a20d 100644 --- a/core/src/test/java/org/apache/gravitino/hook/TestModelHookDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/hook/TestModelHookDispatcher.java @@ -35,6 +35,7 @@ import org.apache.gravitino.authorization.OwnerDispatcher; import org.apache.gravitino.catalog.CatalogManager; import org.apache.gravitino.catalog.CatalogTestUtils; import org.apache.gravitino.catalog.ModelDispatcher; +import org.apache.gravitino.catalog.ModelNormalizeDispatcher; import org.apache.gravitino.connector.BaseCatalog; import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.connector.capability.CapabilityResult; @@ -47,7 +48,7 @@ import org.mockito.ArgumentCaptor; public class TestModelHookDispatcher { - private ModelHookDispatcher hookDispatcher; + private ModelDispatcher hookDispatcher; private ModelDispatcher mockDispatcher; private OwnerDispatcher mockOwnerDispatcher; private CatalogManager mockCatalogManager; @@ -74,7 +75,8 @@ public class TestModelHookDispatcher { FieldUtils.writeField( GravitinoEnv.getInstance(), "internalOwnerDispatcher", mockOwnerDispatcher, true); FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogManager", mockCatalogManager, true); - hookDispatcher = new ModelHookDispatcher(mockDispatcher); + hookDispatcher = + new ModelNormalizeDispatcher(new ModelHookDispatcher(mockDispatcher), mockCatalogManager); } @AfterEach diff --git a/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java b/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java index 3e340326ff..63d2a99f08 100644 --- a/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java @@ -48,6 +48,7 @@ import org.apache.gravitino.authorization.OwnerDispatcher; import org.apache.gravitino.catalog.CatalogManager; import org.apache.gravitino.catalog.CatalogTestUtils; import org.apache.gravitino.catalog.SchemaDispatcher; +import org.apache.gravitino.catalog.SchemaNormalizeDispatcher; import org.apache.gravitino.connector.BaseCatalog; import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.connector.capability.CapabilityResult; @@ -143,7 +144,9 @@ public class TestSchemaHookDispatcher { Schema mockSchema = mock(Schema.class); when(mockDispatcher.createSchema(any(), any(), any())).thenReturn(mockSchema); - hookDispatcher.createSchema(ident, "comment", Collections.emptyMap()); + SchemaDispatcher normalizedDispatcher = + new SchemaNormalizeDispatcher(hookDispatcher, mockCatalogManager); + normalizedDispatcher.createSchema(ident, "comment", Collections.emptyMap()); List<MetadataObject> owned = captureOwnedObjects(); Assertions.assertEquals(1, owned.size(), "A flat schema only assigns ownership to the leaf"); diff --git a/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java b/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java index 6abb334569..b1a61795a7 100644 --- a/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java @@ -37,6 +37,7 @@ import org.apache.gravitino.authorization.OwnerDispatcher; import org.apache.gravitino.catalog.CatalogManager; import org.apache.gravitino.catalog.CatalogTestUtils; import org.apache.gravitino.catalog.TableDispatcher; +import org.apache.gravitino.catalog.TableNormalizeDispatcher; import org.apache.gravitino.connector.BaseCatalog; import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.connector.capability.CapabilityResult; @@ -60,8 +61,7 @@ public class TestTableHookDispatcher { @Test public void testDropAuthorizationPrivilege() { TableDispatcher dispatcher = Mockito.mock(TableDispatcher.class); - TableHookDispatcher hook = - new TableHookDispatcher(dispatcher, () -> null, Mockito.mock(CatalogManager.class)); + TableHookDispatcher hook = new TableHookDispatcher(dispatcher, () -> null); NameIdentifier ident = NameIdentifier.of(METALAKE, CATALOG, "schema", "table"); List<String> locations = ImmutableList.of("/test"); Mockito.when(dispatcher.dropTable(ident)).thenReturn(true); @@ -93,8 +93,9 @@ public class TestTableHookDispatcher { Table createdTable = Mockito.mock(Table.class); Mockito.when(dispatcher.createTable(any(), any(), any(), any(), any(), any(), any(), any())) .thenReturn(createdTable); - TableHookDispatcher hook = - new TableHookDispatcher(dispatcher, () -> ownerDispatcher, catalogManager); + TableDispatcher hook = + new TableNormalizeDispatcher( + new TableHookDispatcher(dispatcher, () -> ownerDispatcher), catalogManager); NameIdentifier ident = NameIdentifier.of(METALAKE, CATALOG, "SCHEMA_NORM", "MY_TABLE"); assertSame( @@ -123,7 +124,7 @@ public class TestTableHookDispatcher { Table createdTable = Mockito.mock(Table.class); Mockito.when(dispatcher.createTable(any(), any(), any(), any(), any(), any(), any(), any())) .thenReturn(createdTable); - TableHookDispatcher hook = new TableHookDispatcher(dispatcher, () -> null, catalogManager); + TableHookDispatcher hook = new TableHookDispatcher(dispatcher, () -> null); assertSame( createdTable, @@ -153,8 +154,7 @@ public class TestTableHookDispatcher { BaseCatalog<?> catalog = Mockito.mock(BaseCatalog.class); Mockito.when(catalog.capability()).thenReturn(Capability.DEFAULT); CatalogTestUtils.mockDoWithCatalog(catalogManager, catalog); - TableHookDispatcher hook = - new TableHookDispatcher(dispatcher, () -> ownerDispatcher, catalogManager); + TableHookDispatcher hook = new TableHookDispatcher(dispatcher, () -> ownerDispatcher); RuntimeException thrown = assertThrows( @@ -176,8 +176,7 @@ public class TestTableHookDispatcher { @Test public void testRenameAuthorizationPrivilege() { TableDispatcher dispatcher = Mockito.mock(TableDispatcher.class); - TableHookDispatcher hook = - new TableHookDispatcher(dispatcher, () -> null, Mockito.mock(CatalogManager.class)); + TableHookDispatcher hook = new TableHookDispatcher(dispatcher, () -> null); NameIdentifier ident = NameIdentifier.of(METALAKE, CATALOG, "schema", "table"); Table alteredTable = Mockito.mock(Table.class); TableChange setChange = TableChange.setProperty("key", "value"); diff --git a/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java b/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java index 91da438bcf..e4294b2c17 100644 --- a/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java @@ -36,6 +36,7 @@ import org.apache.gravitino.catalog.CatalogTestUtils; import org.apache.gravitino.catalog.TestOperationDispatcher; import org.apache.gravitino.catalog.TestTopicOperationDispatcher; import org.apache.gravitino.catalog.TopicDispatcher; +import org.apache.gravitino.catalog.TopicNormalizeDispatcher; import org.apache.gravitino.connector.BaseCatalog; import org.apache.gravitino.connector.authorization.AuthorizationPlugin; import org.apache.gravitino.connector.capability.Capability; @@ -96,7 +97,9 @@ public class TestTopicHookDispatcher extends TestOperationDispatcher { GravitinoEnv.getInstance(), "internalOwnerDispatcher", mockOwnerDispatcher, true); try { - TopicHookDispatcher localHook = new TopicHookDispatcher(mockTopicDispatcher); + TopicDispatcher localHook = + new TopicNormalizeDispatcher( + new TopicHookDispatcher(mockTopicDispatcher), mockCatalogManager); NameIdentifier ident = NameIdentifier.of(metalake, catalog, "SCHEMA_NORM", "MY_TOPIC"); localHook.createTopic(ident, "comment", null, ImmutableMap.of()); diff --git a/core/src/test/java/org/apache/gravitino/hook/TestViewHookDispatcher.java b/core/src/test/java/org/apache/gravitino/hook/TestViewHookDispatcher.java index 9bb348c4e4..2041d9163e 100644 --- a/core/src/test/java/org/apache/gravitino/hook/TestViewHookDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/hook/TestViewHookDispatcher.java @@ -34,6 +34,7 @@ import org.apache.gravitino.authorization.OwnerDispatcher; import org.apache.gravitino.catalog.CatalogManager; import org.apache.gravitino.catalog.CatalogTestUtils; import org.apache.gravitino.catalog.ViewDispatcher; +import org.apache.gravitino.catalog.ViewNormalizeDispatcher; import org.apache.gravitino.connector.BaseCatalog; import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.connector.capability.CapabilityResult; @@ -66,8 +67,9 @@ public class TestViewHookDispatcher { View createdView = Mockito.mock(View.class); Mockito.when(dispatcher.createView(any(), any(), any(), any(), any(), any(), any())) .thenReturn(createdView); - ViewHookDispatcher hook = - new ViewHookDispatcher(dispatcher, () -> ownerDispatcher, catalogManager); + ViewDispatcher hook = + new ViewNormalizeDispatcher( + new ViewHookDispatcher(dispatcher, () -> ownerDispatcher), catalogManager); NameIdentifier ident = NameIdentifier.of(METALAKE, CATALOG, "SCHEMA_NORM", "MY_VIEW"); try (MockedStatic<PrincipalUtils> principalUtils = Mockito.mockStatic(PrincipalUtils.class)) { @@ -90,7 +92,7 @@ public class TestViewHookDispatcher { View createdView = Mockito.mock(View.class); Mockito.when(dispatcher.createView(any(), any(), any(), any(), any(), any(), any())) .thenReturn(createdView); - ViewHookDispatcher hook = new ViewHookDispatcher(dispatcher, () -> null, catalogManager); + ViewHookDispatcher hook = new ViewHookDispatcher(dispatcher, () -> null); assertSame( createdView, createView(hook, NameIdentifier.of(METALAKE, CATALOG, "schema", "view"))); @@ -111,8 +113,7 @@ public class TestViewHookDispatcher { ViewDispatcher dispatcher = Mockito.mock(ViewDispatcher.class); Mockito.when(dispatcher.createView(any(), any(), any(), any(), any(), any(), any())) .thenReturn(Mockito.mock(View.class)); - ViewHookDispatcher hook = - new ViewHookDispatcher(dispatcher, () -> ownerDispatcher, catalogManager); + ViewHookDispatcher hook = new ViewHookDispatcher(dispatcher, () -> ownerDispatcher); RuntimeException thrown = assertThrows( @@ -126,8 +127,7 @@ public class TestViewHookDispatcher { @Test public void testRenameViewUpdatesAuthorizationMapping() { ViewDispatcher dispatcher = Mockito.mock(ViewDispatcher.class); - ViewHookDispatcher hook = - new ViewHookDispatcher(dispatcher, () -> null, Mockito.mock(CatalogManager.class)); + ViewHookDispatcher hook = new ViewHookDispatcher(dispatcher, () -> null); NameIdentifier ident = NameIdentifier.of(METALAKE, CATALOG, "schema", "view"); View alteredView = Mockito.mock(View.class); ViewChange setChange = ViewChange.setProperty("key", "value"); @@ -148,7 +148,7 @@ public class TestViewHookDispatcher { } } - private View createView(ViewHookDispatcher hook, NameIdentifier ident) { + private View createView(ViewDispatcher hook, NameIdentifier ident) { Representation[] representations = new Representation[] { SQLRepresentation.builder().withDialect("trino").withSql("SELECT 1").build() diff --git a/core/src/test/java/org/apache/gravitino/listener/api/event/TestTableEvent.java b/core/src/test/java/org/apache/gravitino/listener/api/event/TestTableEvent.java index bd07402602..84598f69ca 100644 --- a/core/src/test/java/org/apache/gravitino/listener/api/event/TestTableEvent.java +++ b/core/src/test/java/org/apache/gravitino/listener/api/event/TestTableEvent.java @@ -20,6 +20,7 @@ package org.apache.gravitino.listener.api.event; import static org.mockito.Mockito.any; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -28,8 +29,10 @@ import java.util.Arrays; import java.util.Map; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; +import org.apache.gravitino.authorization.OwnerDispatcher; import org.apache.gravitino.catalog.TableDispatcher; import org.apache.gravitino.exceptions.GravitinoRuntimeException; +import org.apache.gravitino.hook.TableHookDispatcher; import org.apache.gravitino.listener.DummyEventListener; import org.apache.gravitino.listener.EventBus; import org.apache.gravitino.listener.TableEventDispatcher; @@ -102,6 +105,39 @@ public class TestTableEvent { Assertions.assertEquals(OperationStatus.UNPROCESSED, preEvent.operationStatus()); } + @Test + void testCreateTableOwnerFailureProducesCreateFailureEvent() { + DummyEventListener listener = new DummyEventListener(); + EventBus eventBus = new EventBus(Arrays.asList(listener)); + OwnerDispatcher ownerDispatcher = mock(OwnerDispatcher.class); + doThrow(new RuntimeException("Set owner failed")) + .when(ownerDispatcher) + .setOwner(any(), any(), any(), any()); + TableEventDispatcher dispatcherWithHook = + new TableEventDispatcher( + eventBus, new TableHookDispatcher(mockTableDispatcher(), () -> ownerDispatcher)); + NameIdentifier identifier = NameIdentifier.of("metalake", "catalog", "schema", table.name()); + + RuntimeException thrown = + Assertions.assertThrows( + RuntimeException.class, + () -> + dispatcherWithHook.createTable( + identifier, + table.columns(), + table.comment(), + table.properties(), + table.partitioning(), + table.distribution(), + table.sortOrder(), + table.index())); + + Assertions.assertEquals("Set owner failed", thrown.getMessage()); + Event event = listener.popPostEvent(); + Assertions.assertEquals(CreateTableFailureEvent.class, event.getClass()); + Assertions.assertTrue(listener.getPostEvents().isEmpty()); + } + @Test void testLoadTableEvent() { NameIdentifier identifier = NameIdentifier.of("metalake", "catalog", table.name()); diff --git a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java index 89f42ca0c2..78a58f71b7 100644 --- a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java +++ b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java @@ -150,38 +150,35 @@ public class RESTService implements GravitinoAuxiliaryService { IcebergNamespaceOperationDispatcher namespaceOperationDispatcher = new IcebergNamespaceOperationExecutor(icebergCatalogWrapperManager, cleanupManager); - // Table: HookDispatcher -> EventDispatcher -> OperationExecutor + // Table: EventDispatcher -> HookDispatcher -> OperationExecutor IcebergTableOperationDispatcher icebergTableOperationDispatcher = new IcebergTableOperationExecutor(icebergCatalogWrapperManager, cleanupManager); - IcebergTableOperationDispatcher icebergTableEventDispatcher = - new IcebergTableEventDispatcher(icebergTableOperationDispatcher, eventBus, metalakeName); if (authorizationContext.isAuthorizationEnabled()) { - icebergTableEventDispatcher = - new IcebergTableHookDispatcher(icebergTableEventDispatcher, namespaceOperationDispatcher); + icebergTableOperationDispatcher = + new IcebergTableHookDispatcher( + icebergTableOperationDispatcher, namespaceOperationDispatcher); } - IcebergTableOperationDispatcher icebergTableDispatcher = icebergTableEventDispatcher; + IcebergTableOperationDispatcher icebergTableDispatcher = + new IcebergTableEventDispatcher(icebergTableOperationDispatcher, eventBus, metalakeName); - // View: HookDispatcher -> EventDispatcher -> OperationExecutor + // View: EventDispatcher -> HookDispatcher -> OperationExecutor IcebergViewOperationDispatcher icebergViewOperationDispatcher = new IcebergViewOperationExecutor(icebergCatalogWrapperManager); - IcebergViewOperationDispatcher icebergViewEventDispatcher = - new IcebergViewEventDispatcher(icebergViewOperationDispatcher, eventBus, metalakeName); if (authorizationContext.isAuthorizationEnabled()) { - icebergViewEventDispatcher = + icebergViewOperationDispatcher = new IcebergViewHookDispatcher( - icebergViewEventDispatcher, namespaceOperationDispatcher, metalakeName); + icebergViewOperationDispatcher, namespaceOperationDispatcher, metalakeName); } - IcebergViewOperationDispatcher icebergViewDispatcher = icebergViewEventDispatcher; + IcebergViewOperationDispatcher icebergViewDispatcher = + new IcebergViewEventDispatcher(icebergViewOperationDispatcher, eventBus, metalakeName); - // Namespace: HookDispatcher -> EventDispatcher -> OperationExecutor - IcebergNamespaceOperationDispatcher icebergNamespaceEventDispatcher = - new IcebergNamespaceEventDispatcher(namespaceOperationDispatcher, eventBus, metalakeName); + // Namespace: EventDispatcher -> HookDispatcher -> OperationExecutor if (authorizationContext.isAuthorizationEnabled()) { - icebergNamespaceEventDispatcher = - new IcebergNamespaceHookDispatcher(icebergNamespaceEventDispatcher); + namespaceOperationDispatcher = + new IcebergNamespaceHookDispatcher(namespaceOperationDispatcher); } IcebergNamespaceOperationDispatcher icebergNamespaceDispatcher = - icebergNamespaceEventDispatcher; + new IcebergNamespaceEventDispatcher(namespaceOperationDispatcher, eventBus, metalakeName); config.register( new AbstractBinder() {
