paultanay opened a new issue, #12685:
URL: https://github.com/apache/gravitino/issues/12685
### Version
main branch
### Describe what's wrong
In `PolicyManager.updatePolicyEntity()`, the validation that prevents
changing `supportedObjectTypes` for built-in policies uses
`Sets.difference(old, new).isEmpty()` (lines 503–508). `Sets.difference(A, B)`
returns elements in `A` not in `B`, so the check only blocks removing types —
it silently allows adding new ones (the set `new ⊇ old` case passes). The
`Preconditions.checkArgument` call on line 508 also omits the format arguments
for its two `%s` placeholders, so if the check does throw, the exception
message prints literal `%s` instead of the actual type sets.
### Error message and/or stacktrace
If the check fires (e.g., removal case), the exception message is:
```
java.lang.IllegalArgumentException: Policy content type mismatch: expected
%s but got %s
```
instead of the actual expected and received type sets.
### How to reproduce
In `TestPolicyManager`, call `alterPolicy` with a
`PolicyChange.UpdateContent` for an `ICEBERG_COMPACTION` policy where the new
content carries a superset of `supportedObjectTypes`. The `Sets.difference`
check passes. Downstream, `PolicyEntity.build()` throws a different, unrelated
error about content class mismatch - obscuring the actual validation failure.
The intent is documented in the code itself (`// cannot change the supported
object types for built-in policies`), and the fix is to use
`Sets.symmetricDifference(...).isEmpty()` (equivalent to set equality) plus
pass the format args:
```java
Preconditions.checkArgument(
Sets.symmetricDifference(
policyEntity.content().supportedObjectTypes(),
updateContent.getContent().supportedObjectTypes())
.isEmpty(),
"Policy content type mismatch: expected %s but got %s",
policyEntity.content().supportedObjectTypes(),
updateContent.getContent().supportedObjectTypes());
### Additional context
- Introduced in the validation added alongside `supportedObjectTypes` moving
into `PolicyContent` (related to PR #8065 / commit `8484e783`).
- No existing test covers the `UpdateContent` path for built-in policies —
the test gap is what let this through.
- `TestPolicyManager` currently only tests the rename, comment,
enable/disable, and custom-content update paths.
--
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]
