Copilot commented on code in PR #12751:
URL: https://github.com/apache/gravitino/pull/12751#discussion_r3903827232
##########
core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java:
##########
@@ -142,24 +141,17 @@ protected <R, E1 extends Throwable, E2 extends Throwable>
R doWithCatalog(
}
}
- protected Set<String> getHiddenPropertyNames(
+ protected Map.Entry<Set<String>, Set<String>> getHiddenPropertyNames(
NameIdentifier catalogIdent,
ThrowableFunction<HasPropertyMetadata, PropertiesMetadata> provider,
Map<String, String> properties) {
return doWithCatalog(
catalogIdent,
c ->
c.doWithPropertiesMeta(
- p -> {
- PropertiesMetadata propertiesMetadata = provider.apply(p);
- return properties.entrySet().stream()
- .filter(
- e ->
- propertiesMetadata.isHiddenProperty(e.getKey())
- ||
SecretPropertyUtils.isSecretProperty(e.getKey(), e.getValue()))
- .map(Map.Entry::getKey)
- .collect(Collectors.toSet());
- }),
+ p ->
+ HiddenPropertyMaskUtils.classifyHiddenProperties(
+ properties, provider.apply(p))),
IllegalArgumentException.class);
}
Review Comment:
`getHiddenPropertyNames` now returns a `(keysToMask, keysToOmit)` pair
rather than “hidden property names”. This is misleading and makes call sites
error-prone (it’s no longer clear what “hidden” means, and
`getKey()`/`getValue()` ordering must be remembered). Rename this method to
reflect the new semantics (e.g., `getPropertyResponseClassification`,
`getMaskAndOmitKeys`, etc.).
##########
core/src/main/java/org/apache/gravitino/catalog/EntityCombinedSchema.java:
##########
@@ -70,8 +72,15 @@ public static EntityCombinedSchema of(Schema schema) {
return of(schema, null);
}
- public EntityCombinedSchema withHiddenProperties(Set<String>
hiddenProperties) {
- this.hiddenProperties = hiddenProperties == null ? Collections.emptySet()
: hiddenProperties;
+ public EntityCombinedSchema withHiddenProperties(Map.Entry<Set<String>,
Set<String>> classified) {
+ if (classified == null) {
+ this.keysToMask = Collections.emptySet();
+ this.keysToOmit = Collections.emptySet();
+ } else {
+ this.keysToMask = classified.getKey() == null ? Collections.emptySet() :
classified.getKey();
+ this.keysToOmit =
+ classified.getValue() == null ? Collections.emptySet() :
classified.getValue();
+ }
Review Comment:
Using `Map.Entry<Set<String>, Set<String>>` as a public “tuple” for `(mask,
omit)` is ambiguous (`getKey()`/`getValue()` don’t communicate intent) and
makes accidentally swapping mask/omit sets easy. Prefer introducing a dedicated
value type (e.g., a small `record`/class like `PropertyResponsePolicy {
Set<String> keysToMask; Set<String> keysToOmit; }`) and accept that here (and
in the other `EntityCombined*` wrappers).
##########
core/src/main/java/org/apache/gravitino/connector/HiddenPropertyMaskUtils.java:
##########
@@ -71,53 +79,79 @@ public static void validateNoMaskedPlaceholders(Map<String,
String> properties)
}
/**
- * Returns a mutable copy of {@code properties} with values for {@code
keysToMask} replaced by
- * {@link #MASKED_VALUE}. Entries with null keys or values are dropped.
+ * Classifies property keys for API responses.
*
- * <p>The returned map is always mutable so callers can add defaults such as
{@code in-use}.
+ * @return entry of {@code (keysToMask, keysToOmit)}
*/
- public static Map<String, String> maskHiddenProperties(
- Map<String, String> properties, Set<String> keysToMask) {
+ public static Map.Entry<Set<String>, Set<String>> classifyHiddenProperties(
+ @Nullable Map<String, String> properties, PropertiesMetadata metadata) {
+ Objects.requireNonNull(metadata, "metadata");
if (properties == null || properties.isEmpty()) {
- return new HashMap<>();
+ return Map.entry(Collections.emptySet(), Collections.emptySet());
}
- Set<String> mask = keysToMask == null ? Collections.emptySet() :
keysToMask;
- Map<String, String> result = new HashMap<>(properties.size());
+
+ Set<String> keysToMask = new HashSet<>();
+ Set<String> keysToOmit = new HashSet<>();
for (Map.Entry<String, String> entry : properties.entrySet()) {
String key = entry.getKey();
String value = entry.getValue();
if (key == null || value == null) {
continue;
}
- result.put(key, mask.contains(key) ? MASKED_VALUE : value);
+ boolean hidden = metadata.isHiddenProperty(key);
+ boolean reserved = metadata.isReservedProperty(key);
+ if (hidden && reserved) {
+ keysToOmit.add(key);
+ } else if (hidden || SecretPropertyUtils.isSecretProperty(key, value)) {
+ keysToMask.add(key);
+ }
}
- return result;
+ return Map.entry(Set.copyOf(keysToMask), Set.copyOf(keysToOmit));
}
/**
- * Returns a mutable API-response copy of {@code properties}.
+ * Returns a mutable copy of {@code properties} with values for {@code
keysToMask} replaced by
+ * {@link #MASKED_VALUE}. Entries with null keys or values are dropped. No
keys are omitted.
*
- * <p>Values are replaced with {@link #MASKED_VALUE} when {@link
- * PropertiesMetadata#isHiddenProperty(String)} is true (credential and
other sensitive keys) or
- * {@link SecretPropertyUtils#isSecretProperty(String, String)} is true.
Reserved keys are not
- * removed.
+ * <p>The returned map is always mutable so callers can add defaults such as
{@code in-use}.
*/
public static Map<String, String> maskHiddenProperties(
- Map<String, String> properties, PropertiesMetadata metadata) {
+ Map<String, String> properties, Set<String> keysToMask) {
+ return maskHiddenProperties(properties, keysToMask,
Collections.emptySet());
+ }
+
+ /**
+ * Like {@link #maskHiddenProperties(Map, Set)}, and also drops {@code
keysToOmit} from the
+ * result.
+ */
+ public static Map<String, String> maskHiddenProperties(
+ Map<String, String> properties,
+ @Nullable Set<String> keysToMask,
+ @Nullable Set<String> keysToOmit) {
if (properties == null || properties.isEmpty()) {
return new HashMap<>();
}
+ Set<String> mask = keysToMask == null ? Collections.emptySet() :
keysToMask;
+ Set<String> omit = keysToOmit == null ? Collections.emptySet() :
keysToOmit;
Map<String, String> result = new HashMap<>(properties.size());
for (Map.Entry<String, String> entry : properties.entrySet()) {
String key = entry.getKey();
String value = entry.getValue();
- if (key == null || value == null) {
+ if (key == null || value == null || omit.contains(key)) {
continue;
}
- boolean shouldMask =
- metadata.isHiddenProperty(key) ||
SecretPropertyUtils.isSecretProperty(key, value);
- result.put(key, shouldMask ? MASKED_VALUE : value);
+ result.put(key, mask.contains(key) ? MASKED_VALUE : value);
}
return result;
}
+
+ /**
+ * Returns a mutable API-response copy of {@code properties}:
reserved+hidden keys are omitted;
+ * other hidden keys and secret-manager URN values are replaced with {@link
#MASKED_VALUE}.
+ */
+ public static Map<String, String> maskHiddenProperties(
+ Map<String, String> properties, PropertiesMetadata metadata) {
+ Map.Entry<Set<String>, Set<String>> classified =
classifyHiddenProperties(properties, metadata);
+ return maskHiddenProperties(properties, classified.getKey(),
classified.getValue());
+ }
Review Comment:
The PR description mentions renaming the API-response helper to
`HiddenPropertyMaskUtils.forApiResponse(...)`, but the implementation still
exposes multiple `maskHiddenProperties(...)` overloads (including lower-level
`(properties, keysToMask, keysToOmit)` variants). To align the code with the PR
intent and reduce misuse risk, consider adding the promised
`forApiResponse(...)` entry point (and potentially narrowing the visibility of
the lower-level overloads if they’re not intended to be called directly).
--
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]
