yuqi1129 commented on code in PR #14508:
URL: https://github.com/apache/gravitino/pull/14508#discussion_r4465938256
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -684,13 +725,15 @@ private void importSchema(NameIdentifier identifier) {
}
}
+ // Persist source properties, not the masked client view, so a recovered
registration retains
+ // secret URNs. The EntityCombinedSchema returned to the caller still
applies masking.
+ Map<String, String> sourceProperties = schema.schema().properties();
SchemaEntity schemaEntity =
SchemaEntity.builder()
.withId(uid)
.withName(identifier.name())
.withNamespace(identifier.namespace())
- .withProperties(
- schema.properties() == null ? Collections.emptyMap() :
schema.properties())
+ .withProperties(sourceProperties == null ? Collections.emptyMap()
: sourceProperties)
Review Comment:
Fixed in `084754f2d3`. Import validates every three-segment write-through
URN with isWriteThroughForEntity against the schema type, stable ID and exact
property key before storing raw properties. A foreign reference is rejected
rather than persisted. New cases cover copied URNs without the identifier
marker, wrong entity type and wrong property key; they verify that the rejected
copy has no registration/getSecrets access and dropping it leaves the original
secret readable. Matching partial-create recovery and external references
remain supported.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -684,13 +725,15 @@ private void importSchema(NameIdentifier identifier) {
}
}
+ // Persist source properties, not the masked client view, so a recovered
registration retains
+ // secret URNs. The EntityCombinedSchema returned to the caller still
applies masking.
+ Map<String, String> sourceProperties = schema.schema().properties();
Review Comment:
Fixed in `084754f2d3`. The import now rejects write-through URNs whose
entity type, ID or exact property key does not match the imported schema,
before any store registration. Tests cover the copied-URN/no-marker case plus
wrong type/key, denied getSecrets on the rejected copy, and preserving the
original secret when the source copy is dropped. Existing matching
partial-create recovery still passes, and external references are retained.
Ordinary inline source properties retain the existing create/import storage
behavior and client masking; that plaintext-at-rest limitation remains explicit
in the PR description rather than claiming this change encrypts them.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -229,12 +266,12 @@ public Schema createSchema(
store.put(schemaEntity, true /* overwrite */);
} catch (Exception e) {
Review Comment:
Fixed in `084754f2d3`. A dedicated registration-conflict catch now preserves
the store exception as the cause of SchemaAlreadyExistsException; the table
counterpart uses TableAlreadyExistsException. I used these public domain types
rather than rethrowing EntityAlreadyExistsException unchanged because the REST
handlers map the schema/table types to 409, while the internal entity exception
would still fall through to 500. The conflict message requests explicit
identity reconciliation and does not suggest that load can resolve it. New
regressions verify cause/type/message and retention of credentials referenced
by the already-created 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]
