jerryshao commented on code in PR #13512:
URL: https://github.com/apache/gravitino/pull/13512#discussion_r4244277431
##########
server/src/main/java/org/apache/gravitino/server/web/rest/SemanticModelOperations.java:
##########
@@ -208,9 +239,14 @@ public Response importOssieDocument(
MediaType.APPLICATION_JSON_TYPE.isCompatible(headers.getMediaType())
? OssieDocument.json(document)
: OssieDocument.yaml(document);
+ ImportedSemanticModel imported =
OssieDocumentConverter.importDocument(ossieDocument);
+ sourceValidator.validate(metalake, imported.definition());
SemanticModel semanticModel =
- dispatcher.importOssieDocument(
- NamespaceUtil.ofSemanticModel(metalake, catalog, schema),
ossieDocument);
+ dispatcher.createSemanticModel(
Review Comment:
[Nit] This duplicates the body of
`SemanticModelDispatcher.importOssieDocument`.
SemanticModelDispatcher.java:40-47 is exactly `importDocument` plus
`createSemanticModel`, and after this change nothing in the server reaches it.
Two copies of the convert-then-create sequence can drift, and a catalog that
later overrides the documented `SemanticModelCatalog.importOssieDocument`
(api/src/main/java/org/apache/gravitino/semantic/SemanticModelCatalog.java:112)
would silently never be called from REST. Either drop the now-unreachable core
default, or keep the dispatcher call and pass the validation hook into it, so
the import contract has one home. Behavior today is unchanged, which is why
this is only a nit.
Verified by: grep for `importOssieDocument` across the repo at 33b6fd9
returns only SemanticModelOperations.java:222, SemanticModelCatalog.java:112
and SemanticModelDispatcher.java:40; confirmed no dispatcher in the chain
(normalize, hook, event, operation) overrides it, so the inlined sequence is
equivalent to the default it replaces.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/SemanticModelSourceValidator.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.server.web.rest;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+import javax.inject.Inject;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.GravitinoEnv;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.authorization.AuthorizationRequestContext;
+import org.apache.gravitino.authorization.GravitinoAuthorizer;
+import org.apache.gravitino.catalog.TableDispatcher;
+import org.apache.gravitino.catalog.ViewDispatcher;
+import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.exceptions.IllegalSemanticModelException;
+import org.apache.gravitino.exceptions.NotFoundException;
+import org.apache.gravitino.rel.Column;
+import org.apache.gravitino.rel.ViewCatalog;
+import org.apache.gravitino.semantic.Dataset;
+import org.apache.gravitino.semantic.Relationship;
+import org.apache.gravitino.semantic.SemanticModelDefinition;
+import org.apache.gravitino.server.authorization.GravitinoAuthorizerProvider;
+import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants;
+import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator;
+import org.apache.gravitino.utils.NameIdentifierUtil;
+
+/** Validates source visibility, existence, and explicit column references
before model writes. */
+public class SemanticModelSourceValidator {
+ private final TableDispatcher tables;
+ private final ViewDispatcher views;
+ private final Supplier<GravitinoAuthorizer> authorizer;
+ private final Predicate<NameIdentifier> supportsViews;
+
+ /**
+ * Creates a validator using the server's authorizer, including its
disabled-mode pass-through.
+ *
+ * @param tables table metadata dispatcher
+ * @param views view metadata dispatcher
+ */
+ @Inject
+ public SemanticModelSourceValidator(TableDispatcher tables, ViewDispatcher
views) {
+ this(tables, views, () ->
GravitinoAuthorizerProvider.getInstance().getGravitinoAuthorizer());
+ }
+
+ SemanticModelSourceValidator(
+ TableDispatcher tables, ViewDispatcher views,
Supplier<GravitinoAuthorizer> authorizer) {
+ this(
+ tables,
+ views,
+ authorizer,
+ ident ->
+ GravitinoEnv.getInstance()
+ .catalogManager()
+ .doWithCatalog(
+ NameIdentifierUtil.getCatalogIdentifier(ident),
+ catalog -> catalog.ops() instanceof ViewCatalog));
+ }
+
+ SemanticModelSourceValidator(
+ TableDispatcher tables,
+ ViewDispatcher views,
+ Supplier<GravitinoAuthorizer> authorizer,
+ Predicate<NameIdentifier> supportsViews) {
+ this.supportsViews = supportsViews;
+ this.tables = tables;
+ this.views = views;
+ this.authorizer = authorizer;
+ }
+
+ /**
+ * Validates every dataset source and key or relationship column under the
current caller.
+ *
+ * <p>This method must run even when authorization is disabled. It does not
interpret SQL
+ * expressions or validate transitive view dependencies. Connection failures
propagate unchanged.
+ *
+ * @param metalake the request's metalake, also used for cross-catalog
source references
+ * @param definition the complete definition to validate before persistence
+ */
+ public void validate(String metalake, SemanticModelDefinition definition) {
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ GravitinoAuthorizer currentAuthorizer = authorizer.get();
+ Map<String, Set<String>> datasetColumns = new HashMap<>();
+ Map<NameIdentifier, Set<String>> sourceColumns = new HashMap<>();
+ for (Dataset dataset : definition.datasets()) {
+ NameIdentifier source = dataset.source();
+ if (source.namespace().length() != 2) {
+ throw new IllegalSemanticModelException("Source must contain
catalog.schema.name");
+ }
+ NameIdentifier ident =
+ NameIdentifier.of(
+ metalake, source.namespace().level(0),
source.namespace().level(1), source.name());
+ Set<String> columns =
+ sourceColumns.computeIfAbsent(ident, key -> loadColumns(key,
currentAuthorizer, context));
+ datasetColumns.put(dataset.name(), columns);
+ checkColumns(dataset.name(), dataset.primaryKey(), columns);
+ if (dataset.uniqueKeys() != null) {
+ for (String[] key : dataset.uniqueKeys()) {
+ checkColumns(dataset.name(), key, columns);
+ }
+ }
+ }
+ if (definition.relationships() != null) {
+ for (Relationship relationship : definition.relationships()) {
+ checkColumns(
+ relationship.from(),
+ relationship.fromColumns(),
+ datasetColumns.get(relationship.from()));
+ checkColumns(
+ relationship.to(), relationship.toColumns(),
datasetColumns.get(relationship.to()));
+ }
+ }
+ }
+
+ private Set<String> loadColumns(
+ NameIdentifier ident,
+ GravitinoAuthorizer currentAuthorizer,
+ AuthorizationRequestContext context) {
+ boolean tableAllowed =
+ canLoad(
+ ident,
+ Entity.EntityType.TABLE,
+
AuthorizationExpressionConstants.LOAD_TABLE_AUTHORIZATION_EXPRESSION,
+ currentAuthorizer,
+ context);
+ if (tableAllowed) {
+ try {
+ return columnNames(ident, tables.loadTable(ident).columns());
+ } catch (NotFoundException missing) {
+ // The untyped source can also refer to a logical view. Authorize that
lookup separately.
+ }
+ }
Review Comment:
[Important] A dataset source in a catalog without table support returns 501,
not the documented 400.
`tables.loadTable(ident)` runs with no table-capability guard, and only
`NotFoundException` is caught. For a catalog whose ops implement neither
`TableCatalog` nor `ViewCatalog` - `KafkaCatalogOperations`
(catalogs/catalog-kafka/.../KafkaCatalogOperations.java:95, `CatalogOperations,
SupportsSchemas, TopicCatalog`) is one - the chain is:
TableOperationDispatcher.java:616 calls `catalog.doWithTableOps`, which throws
`UnsupportedOperationException("Catalog does not support table operations")`
(CatalogManager.java:308-316); OperationDispatcher.java:115-116 rethrows
RuntimeExceptions unchanged; nothing here catches it, so
`SemanticModelExceptionHandler` maps it to 501
(ExceptionHandlers.java:1036-1037). Nothing earlier guards it either:
`TableNormalizeDispatcher.loadTable` only resolves capabilities and the
physical name (CapabilityHelpers.java:78-97). This hits create, Ossie import
and every `replaceDefinition`, and when authorization is disabled
`tableAllowed` is always true, so it
hits every caller. The docs paragraph this PR adds says an unresolvable source
is 400, and this is the same defect class as the View case fixed in 9cc73861a.
Suggested fix, symmetric to the View handling: extend the capability
callback to report table support too - one `doWithCatalog` lease can answer
both, `catalog.ops() instanceof TableCatalog` - skip the table lookup when
tables are unsupported, and treat a source whose catalog supports neither kind
as a missing source. Keep not swallowing `UnsupportedOperationException` from a
supported operation, as `testSupportedViewOperationFailureIsNotHidden`
requires. Worth one regression case per entry point: create, import and
`replaceDefinition`.
Verified by: reading `loadColumns` in full at HEAD 33b6fd9, then the call
chain in TableOperationDispatcher.java:610-619, CatalogManager.java:308-316,
OperationDispatcher.java:105-120 and ExceptionHandlers.java:1019-1047;
confirmed TestSemanticModelSourceValidator.java:203-248 exercises the View side
only.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/SemanticModelOperations.java:
##########
@@ -308,6 +354,12 @@ public Response alterSemanticModel(
request.getUpdates().stream()
.map(SemanticModelUpdateRequest::semanticModelChange)
.toArray(SemanticModelChange[]::new);
+ for (SemanticModelChange change : changes) {
+ if (change instanceof SemanticModelChange.ReplaceDefinition) {
+ sourceValidator.validate(
+ metalake, ((SemanticModelChange.ReplaceDefinition)
change).getDefinition());
+ }
Review Comment:
[Nit] Source validation runs before the model's existence is established on
alter.
`validate` runs here, before `dispatcher.alterSemanticModel` on the
following line. `MODIFY_SEMANTIC_MODEL_AUTHORIZATION_EXPRESSION` is satisfied
by a parent-scope `ANY_MODIFY_SEMANTIC_MODEL` grant
(AuthorizationExpressionConstants.java:338-343), so a `replaceDefinition` aimed
at a model that does not exist passes authorization and then does catalog I/O:
the caller sees 400 for a bad source, or 502 if a source catalog is
unreachable, where 404 is the accurate answer. Resolving the model first would
keep the status accurate and avoid remote lookups for a request that cannot
succeed either way.
Verified by: reading the alter body at HEAD 33b6fd9 - the validation loop is
the first thing after the changes array is built, with no existence check
before it - and the MODIFY expression at
AuthorizationExpressionConstants.java:338-343.
--
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]
