jerryshao commented on code in PR #13791:
URL: https://github.com/apache/gravitino/pull/13791#discussion_r4255363142
##########
core/src/main/java/org/apache/gravitino/listener/SemanticModelEventDispatcher.java:
##########
@@ -131,6 +140,44 @@ public SemanticModel createSemanticModel(
}
}
+ @Override
+ public SemanticModel importOssieDocument(
Review Comment:
[Important] The event dispatcher overrides `importOssieDocument` but not the
new `importSemanticModel` entry point, so
`semanticModelDispatcher().importSemanticModel(ident, model)` - a public
default method on the interface this class implements - writes a model through
Normalize -> Hook -> Operation and emits no pre, success or failure event at
all, i.e. no audit record. That is the one outcome this PR exists to prevent,
and the pre-refactor REST code shape (parse the document, then call the write)
is exactly what a future caller would reproduce.
Overriding `importSemanticModel` here to emit the import events cannot
double-emit: in the 3-arg path the default `importOssieDocument` body runs on
the Normalize instance, so its `importSemanticModel(...)` call dispatches on
`SemanticModelNormalizeDispatcher` (`SemanticModelNormalizeDispatcher.java:90`)
and never re-enters this class. Alternatively, keep the parsed-model entry
point out of the interface shared with the top-level dispatcher.
Verified by: traced the chain built at `GravitinoEnv.java:1149-1163`;
confirmed this class overrides only `importOssieDocument(Namespace,
OssieDocument, Consumer)` and `exportOssieDocument` (lines 143-179), and that
`SemanticModelDispatcher.java:79-81` provides the default `importSemanticModel`
used mid-chain.
##########
core/src/main/java/org/apache/gravitino/catalog/SemanticModelDispatcher.java:
##########
@@ -26,30 +28,63 @@
import org.apache.gravitino.semantic.OssieFormat;
import org.apache.gravitino.semantic.SemanticModel;
import org.apache.gravitino.semantic.SemanticModelCatalog;
+import org.apache.gravitino.semantic.SemanticModelDefinition;
/** A dispatcher specialization for schema-scoped Semantic Model operations. */
public interface SemanticModelDispatcher extends SemanticModelCatalog {
/**
* {@inheritDoc}
*
- * <p>Delegates to this dispatcher's native create operation so that the
document follows the same
- * normalization and validation chain as a structured create request.
+ * <p>Uses the import pipeline without caller-specific source validation.
Server callers must use
+ * the overload accepting a source validator, even when authorization is
disabled.
*/
@Override
default SemanticModel importOssieDocument(Namespace namespace, OssieDocument
document) {
+ return importOssieDocument(namespace, document, definition -> {});
Review Comment:
[Important] This override makes a full import run with a no-op source
validator, which silently skips `SemanticModelSourceValidator.validate`. That
validator is not a convenience check: it resolves every dataset source through
`GravitinoAuthorizer` and verifies existence and explicit column references,
and its own Javadoc says it "must run even when authorization is disabled"
(`server/src/main/java/org/apache/gravitino/server/web/rest/SemanticModelSourceValidator.java:104-130`).
The requirement to use the 3-arg overload lives only in the prose above this
line.
Nothing reaches it today (the REST endpoint at
`SemanticModelOperations.java:242-245` passes the validator, and no other
main-source caller exists), but the no-op path is the signature inherited from
the public `SemanticModelCatalog` API, so it is the natural one for the next
caller to pick - a CLI, a job, or a second REST handler - and the failure mode
is an imported model referencing sources the caller cannot see. Note also that
create and alter still validate at the REST layer
(`SemanticModelOperations.java:359` and `:486`) while import now validates
inside core, so the rule is no longer uniform.
Suggestion: make this override fail loudly instead of defaulting to a no-op,
for example by throwing `UnsupportedOperationException` here with a message
pointing at the 3-arg overload.
`TestSemanticModelImportExportEvent.testTwoArgumentImportUsesTheSameEventAndHookPipeline`
would need to call the 3-arg form instead.
Verified by: read `SemanticModelSourceValidator.validate` in full, and
grepped the repo for `importOssieDocument` callers (only
`SemanticModelOperations.java:242` in main source).
--
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]
