jerryshao commented on PR #13354:
URL: https://github.com/apache/gravitino/pull/13354#issuecomment-5754414599
**Verdict:** blocking issues — the registry fallback also changes the *omit*
path, which silently drops properties on catalogs that pass raw metadata
through (Glue), and the new registration precondition fails connectors whose
keys live outside core.
### Findings
**1.
`core/src/main/java/org/apache/gravitino/connector/HiddenPropertyMaskUtils.java:110`
— registry `hidden && reserved` now omits keys the catalog never declared,
silently dropping visible metadata (verified by: read the new `inMetadata ? … :
RegisteredPropertyKeys.isHidden/isReserved` branches at lines 110-116, then
traced one affected catalog end to end).**
`RegisteredPropertyKeys.java:150-156` registers `comment`, `EXTERNAL` and
`presto_view` as `reservedHidden`, so for any entity whose own metadata does
not declare them they now land in `keysToOmit` (line 117) instead of being
returned as-is. Glue is the concrete case: `GlueTable.java:174-176` copies all
of `Table.parameters()` into the Gravitino properties, and
`GlueTablePropertiesMetadata.java:41-45` states explicitly that unknown
parameters "are passed through transparently" — it declares only
`table-format`, `metadata_location`, `format`, `input-format`, `output-format`,
`serde-lib`. A Glue table carrying `EXTERNAL=TRUE` (the normal case for tables
created by Spark/Athena) or `comment` will now lose those entries from
`properties()`. For Glue Iceberg tables the loss is Gravitino's own:
`GlueIcebergTableHelper.java:345-346` writes `comment` into the Iceberg table
properties and `GlueIcebergTableHelper.java:211-213` merges those properties
back on load.
The asymmetry makes it worse: validation still uses only the catalog
metadata (`PropertiesMetadataHelpers.java:54-61` and `83-86`), so a user can
still *set* `comment`/`EXTERNAL` on a Glue table and it will never come back on
read. Suggest limiting the registry fallback to the mask path (`hidden`) and
keeping omission driven by the entity's own metadata, or restricting the
omit-capable registry entries to Gravitino-internal keys
(`gravitino.identifier`, `in-use-metalake`). Either way this belongs in the
PR's user-facing-change section, which currently mentions only keys becoming
*more* visible.
**2.
`core/src/main/java/org/apache/gravitino/connector/BasePropertiesMetadata.java:82`
— the registration precondition makes core the gatekeeper for connector keys
defined outside core, including a ServiceLoader SPI (verified by: read
`checkConnectorSpecificPropertiesRegistered` at lines 103-119 and followed the
one metadata class whose entries are contributed dynamically).**
`GenericTablePropertiesMetadata.java:68-76` builds
`specificPropertyEntries()` from
`LakehouseTableDelegatorFactory.tableDelegators()`, which is a
`ServiceLoader`-discovered SPI (`LakehouseTableDelegatorFactory.java:44-51`).
Any delegator shipped outside this repo — and likewise any catalog extending
the `@Evolving` `BasePropertiesMetadata` out of tree — now throws
`IllegalArgumentException` on first property access unless its keys are
hard-coded into core's `RegisteredPropertyKeys`. The failure is also lazy: it
surfaces at catalog load, not at build time. A consistency test over the
in-repo metadata classes (or a `LOG.warn`) would catch drift without turning an
unknown key into a runtime catalog failure. I checked every in-repo
`specificPropertyEntries()` (including
`catalogs-contrib/catalog-jdbc-clickhouse`) against the registry and found no
key missing today, so this is about the contract, not a current break.
**3.
`core/src/main/java/org/apache/gravitino/connector/RegisteredPropertyKeys.java:93`
— the registry hand-copies hidden/reserved flags that each connector owns,
with nothing tying the two together (verified by: compared registry entries
against the connectors' declarations, e.g.
`optionalHidden("jdbc.pool.test-on-borrow")` vs
`JdbcCatalogPropertiesMetadata.java:107-114`, `reservedHidden("comment")` vs
`HiveTablePropertiesMetadata.java:52`).**
The flags all match today, but they are now security-relevant defaults for
*every* catalog: if a connector later flips a `hidden` flag, nothing fails and
the registry silently keeps the stale semantics for catalogs that do not
declare the key. Worth a test asserting the registry agrees with each declaring
`PropertiesMetadata` it mirrors. (I also checked the reverse direction for
leaks: every registry entry that is non-hidden but sensitive-named —
`s3-access-key-id`, `aws-access-key-id`, `dlf-access-key-id`,
`gcs-service-account-file`, `s3-token-service-endpoint`, `token-provider` — is
non-hidden in its owning metadata too, and all real secrets stay `hidden`, so
the unmasking direction of this change looks sound.)
**4.
`core/src/main/java/org/apache/gravitino/secret/SecretPropertyUtils.java:138` —
stale javadoc on the two-arg `buildSecrets` (verified by: read it against the
new `shouldRecoverSensitiveNamedSecret` at lines 111-120).**
It still says the empty metadata means "every key is treated as undeclared",
which the registry branch (lines 117-118) makes untrue — official keys are now
classified even with empty metadata. The three-arg overload's javadoc was
updated; this one was not.
### Tests
The new `TestRegisteredPropertyKeys` covers membership, hidden/reserved
flags and the throwing registration check, and
`TestHiddenPropertyMaskUtils.testMaskHiddenPropertiesOfficialKeysConsistentAcrossCatalogMetadata`
covers the cross-catalog mask case. Two gaps:
- The new *omit* branch is untested.
`TestHiddenPropertyMaskUtils.java:95-129` only exercises omission for
`gravitino.identifier`, which is declared via `BASIC_PROPERTY_ENTRIES`; nothing
covers a registry-only `reservedHidden` key (`comment`, `EXTERNAL`) on metadata
that does not declare it — i.e. exactly the behavior in finding 1.
- The registration check at `BasePropertiesMetadata.java:103` is only
exercised against synthetic metadata in core, which cannot see the catalog
modules. A key missing from the registry therefore only fails inside that
connector's own module tests; a test that walks the delegator SPI entries would
close the gap that matters most.
### Nits
- `BasePropertiesMetadata.java:113` — `Preconditions.checkArgument(false,
…)` inside the loop; a plain `throw new
IllegalArgumentException(String.format(…))` (or a `checkArgument` on the real
condition) reads better.
- `RegisteredPropertyKeys.java:72` (`LOCATION_PROPERTY_PREFIX`) and `:282`
(`isSharedCloudOrCredentialKey`) have no production callers — only
`TestRegisteredPropertyKeys`.
- `TestFilesetPropertiesMetadata.java:30` renames the fixture key to a real
official key (`jdbc-password`), coupling an unrelated fixture to the registry;
a test-only name plus an explicit registry test keeps them independent.
---
_Generated by [Claude Code](https://claude.ai/code)_
--
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]
