jerryshao commented on code in PR #14508:
URL: https://github.com/apache/gravitino/pull/14508#discussion_r4465104238
##########
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:
**Minor:** if a concurrent register wins the race after the pre-check,
`store.put` throws `EntityAlreadyExistsException`, and this catch-all wraps it
into a `GravitinoRuntimeException` (500) telling the user to `load`. But `load`
returns the racing entity and won't register this source object. Suggest
rethrowing `EntityAlreadyExistsException` unchanged (e.g. a dedicated catch
before the generic one), so callers get the proper already-exists error.
##########
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:
**Major:** this persists the raw source properties without the write-through
ownership check. If schema B copies schema A's secret URN and has no
`gravitino.identifier`, the import path (L694) generates a fresh id and skips
the identity check at L698. B then stores A's URN, and dropping B later deletes
A's secret through `SecretManager.deleteSecretsFromProperties`. Plaintext
properties also end up persisted.
Could we filter these properties with
`SecretPropertyUtils.isWriteThroughForEntity(key, value, "schema", uid)` (or
the equivalent helper used on the create path) before persisting, and add a
test where a second schema reuses another schema's secret URN?
--
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]
