laserninja commented on code in PR #12867:
URL: https://github.com/apache/gravitino/pull/12867#discussion_r4126215106
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataIdConverter.java:
##########
@@ -50,7 +50,8 @@ public class MetadataIdConverter {
MetadataObject.Type.MODEL, Capability.Scope.MODEL,
MetadataObject.Type.FILESET, Capability.Scope.FILESET,
MetadataObject.Type.TOPIC, Capability.Scope.TOPIC,
- MetadataObject.Type.COLUMN, Capability.Scope.COLUMN);
+ MetadataObject.Type.COLUMN, Capability.Scope.COLUMN,
+ MetadataObject.Type.SEMANTIC_MODEL, Capability.Scope.SEMANTIC_MODEL);
Review Comment:
Fixed in 3875436b3. Added the SEMANTIC_MODEL reverse lookup with a batch
identity query. The H2 regression test persists all three object-level grants,
reloads the role, checks name resolution after a model rename, updates the
privileges, and revokes them. The storage tests pass.
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java:
##########
@@ -97,7 +97,10 @@ public class AuthorizationUtils {
MetadataObject.Type.JOB_TEMPLATE,
MetadataObject.Type.TAG,
MetadataObject.Type.POLICY,
- MetadataObject.Type.VIEW);
+ MetadataObject.Type.VIEW,
+ // Semantic models live only in Gravitino, underlying connectors
know nothing about
+ // them, so there is no privilege to push down to an authorization
plugin.
+ MetadataObject.Type.SEMANTIC_MODEL);
Review Comment:
Fixed in 3875436b3. Semantic Model privilege names are filtered both when
selecting connector callbacks and when building the role passed to a plugin.
Semantic-only grants skip dispatch. Tests cover metalake, catalog, and schema
grants, including mixed roles where SELECT_TABLE is retained and Semantic Model
privileges are removed.
##########
core/src/main/java/org/apache/gravitino/hook/SemanticModelHookDispatcher.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.hook;
+
+import java.util.Map;
+import java.util.function.Supplier;
+import javax.annotation.Nullable;
+import org.apache.gravitino.Entity;
+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.SemanticModelDispatcher;
+import org.apache.gravitino.exceptions.IllegalSemanticModelException;
+import org.apache.gravitino.exceptions.NoSuchSchemaException;
+import org.apache.gravitino.exceptions.NoSuchSemanticModelException;
+import org.apache.gravitino.exceptions.SemanticModelAlreadyExistsException;
+import org.apache.gravitino.semantic.SemanticModel;
+import org.apache.gravitino.semantic.SemanticModelChange;
+import org.apache.gravitino.semantic.SemanticModelDefinition;
+import org.apache.gravitino.utils.NameIdentifierUtil;
+import org.apache.gravitino.utils.PrincipalUtils;
+
+/**
+ * {@code SemanticModelHookDispatcher} is a decorator for {@link
SemanticModelDispatcher} that not
+ * only delegates Semantic Model operations to the underlying dispatcher but
also executes some hook
+ * operations before or after the underlying operations.
+ */
+public class SemanticModelHookDispatcher implements SemanticModelDispatcher {
+
+ private final SemanticModelDispatcher dispatcher;
+ private final Supplier<OwnerDispatcher> ownerDispatcher;
+
+ /**
+ * Creates a Semantic Model hook dispatcher.
+ *
+ * @param dispatcher The underlying dispatcher.
+ * @param ownerDispatcher Supplies the owner dispatcher, or null when
authorization is disabled.
+ */
+ public SemanticModelHookDispatcher(
+ SemanticModelDispatcher dispatcher, Supplier<OwnerDispatcher>
ownerDispatcher) {
+ this.dispatcher = dispatcher;
+ this.ownerDispatcher = ownerDispatcher;
+ }
+
+ @Override
+ public NameIdentifier[] listSemanticModels(Namespace namespace) throws
NoSuchSchemaException {
+ return dispatcher.listSemanticModels(namespace);
+ }
+
+ @Override
+ public SemanticModel loadSemanticModel(NameIdentifier ident) throws
NoSuchSemanticModelException {
+ return dispatcher.loadSemanticModel(ident);
+ }
+
+ @Override
+ public boolean semanticModelExists(NameIdentifier ident) {
+ return dispatcher.semanticModelExists(ident);
+ }
+
+ @Override
+ public SemanticModel createSemanticModel(
+ NameIdentifier ident,
+ @Nullable String comment,
+ SemanticModelDefinition definition,
+ Map<String, String> properties)
+ throws NoSuchSchemaException, SemanticModelAlreadyExistsException,
+ IllegalSemanticModelException {
+ SemanticModel semanticModel =
+ dispatcher.createSemanticModel(ident, comment, definition, properties);
+
+ // Set the creator as the owner of the Semantic Model.
+ OwnerDispatcher ownerManager = ownerDispatcher.get();
+ if (ownerManager != null) {
+ ownerManager.setOwner(
+ ident.namespace().level(0),
+ NameIdentifierUtil.toMetadataObject(ident,
Entity.EntityType.SEMANTIC_MODEL),
+ PrincipalUtils.getCurrentUserName(),
+ Owner.Type.USER);
+ }
+ return semanticModel;
+ }
+
+ @Override
+ public SemanticModel alterSemanticModel(NameIdentifier ident,
SemanticModelChange... changes)
+ throws NoSuchSemanticModelException, SemanticModelAlreadyExistsException,
+ IllegalSemanticModelException {
+ return dispatcher.alterSemanticModel(ident, changes);
Review Comment:
Fixed in 3875436b3. Successful rename invalidates both the old and new
identifiers; successful drop invalidates the dropped identifier. Added a
deterministic test using the Jcasbin cache for rename, drop, and name reuse,
plus coverage that failed mutations and non-rename changes do not invalidate
it. These tests pass.
##########
core/src/main/java/org/apache/gravitino/hook/SemanticModelHookDispatcher.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.hook;
+
+import java.util.Map;
+import java.util.function.Supplier;
+import javax.annotation.Nullable;
+import org.apache.gravitino.Entity;
+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.SemanticModelDispatcher;
+import org.apache.gravitino.exceptions.IllegalSemanticModelException;
+import org.apache.gravitino.exceptions.NoSuchSchemaException;
+import org.apache.gravitino.exceptions.NoSuchSemanticModelException;
+import org.apache.gravitino.exceptions.SemanticModelAlreadyExistsException;
+import org.apache.gravitino.semantic.SemanticModel;
+import org.apache.gravitino.semantic.SemanticModelChange;
+import org.apache.gravitino.semantic.SemanticModelDefinition;
+import org.apache.gravitino.utils.NameIdentifierUtil;
+import org.apache.gravitino.utils.PrincipalUtils;
+
+/**
+ * {@code SemanticModelHookDispatcher} is a decorator for {@link
SemanticModelDispatcher} that not
+ * only delegates Semantic Model operations to the underlying dispatcher but
also executes some hook
+ * operations before or after the underlying operations.
+ */
+public class SemanticModelHookDispatcher implements SemanticModelDispatcher {
+
+ private final SemanticModelDispatcher dispatcher;
+ private final Supplier<OwnerDispatcher> ownerDispatcher;
+
+ /**
+ * Creates a Semantic Model hook dispatcher.
+ *
+ * @param dispatcher The underlying dispatcher.
+ * @param ownerDispatcher Supplies the owner dispatcher, or null when
authorization is disabled.
+ */
+ public SemanticModelHookDispatcher(
+ SemanticModelDispatcher dispatcher, Supplier<OwnerDispatcher>
ownerDispatcher) {
+ this.dispatcher = dispatcher;
+ this.ownerDispatcher = ownerDispatcher;
+ }
+
+ @Override
+ public NameIdentifier[] listSemanticModels(Namespace namespace) throws
NoSuchSchemaException {
+ return dispatcher.listSemanticModels(namespace);
+ }
+
+ @Override
+ public SemanticModel loadSemanticModel(NameIdentifier ident) throws
NoSuchSemanticModelException {
+ return dispatcher.loadSemanticModel(ident);
+ }
+
+ @Override
+ public boolean semanticModelExists(NameIdentifier ident) {
+ return dispatcher.semanticModelExists(ident);
+ }
+
+ @Override
+ public SemanticModel createSemanticModel(
+ NameIdentifier ident,
+ @Nullable String comment,
+ SemanticModelDefinition definition,
+ Map<String, String> properties)
+ throws NoSuchSchemaException, SemanticModelAlreadyExistsException,
+ IllegalSemanticModelException {
+ SemanticModel semanticModel =
+ dispatcher.createSemanticModel(ident, comment, definition, properties);
+
+ // Set the creator as the owner of the Semantic Model.
+ OwnerDispatcher ownerManager = ownerDispatcher.get();
+ if (ownerManager != null) {
+ ownerManager.setOwner(
+ ident.namespace().level(0),
+ NameIdentifierUtil.toMetadataObject(ident,
Entity.EntityType.SEMANTIC_MODEL),
+ PrincipalUtils.getCurrentUserName(),
+ Owner.Type.USER);
+ }
+ return semanticModel;
+ }
+
+ @Override
+ public SemanticModel alterSemanticModel(NameIdentifier ident,
SemanticModelChange... changes)
+ throws NoSuchSemanticModelException, SemanticModelAlreadyExistsException,
+ IllegalSemanticModelException {
+ return dispatcher.alterSemanticModel(ident, changes);
+ }
+
+ @Override
+ public boolean dropSemanticModel(NameIdentifier ident) {
+ return dispatcher.dropSemanticModel(ident);
Review Comment:
Fixed in 3875436b3. Registered semantic_model_meta / semantic_model_id with
orphaned relation cleanup. The relational regression test verifies that
dropped-model owner and role relations are collected, live-model relations
remain intact, and repeated cleanup is a no-op. Passed locally with H2.
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataIdConverter.java:
##########
@@ -153,6 +156,12 @@ void testConvert() throws IllegalAccessException {
MetadataIdConverter.normalizeCaseSensitive(
eq(ident8), eq(null), eq(mockCatalogManager)))
.thenReturn(ident8);
+ mockedStatic
+ .when(
+ () ->
+ MetadataIdConverter.normalizeCaseSensitive(
+ eq(ident9), eq(Capability.Scope.SEMANTIC_MODEL),
eq(mockCatalogManager)))
Review Comment:
Fixed in 3875436b3. Semantic Model ID lookup now normalizes only the parent
namespace and preserves the model leaf. Added a dedicated test through the real
normalization path with a case-insensitive capability: SCHEMA becomes schema
while SalesModel retains its case. The test passes.
##########
core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:
##########
@@ -302,6 +302,13 @@ public static void checkMetadataObject(String metalake,
MetadataObject object) {
check(env.internalViewDispatcher().viewExists(identifier),
exceptionToThrowSupplier);
break;
+ case SEMANTIC_MODEL:
+ NameIdentifierUtil.checkSemanticModel(identifier);
+ check(
+ env.semanticModelDispatcher().semanticModelExists(identifier),
Review Comment:
Fixed in 3875436b3. Added internalSemanticModelDispatcher with Normalize ->
Operation and routed checkMetadataObject through it. The public full-server
chain remains Normalize -> Hook -> Operation. Tests assert the dispatcher
chains and verify that metadata existence checks use the internal dispatcher.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
