[#12685] fix(core): Fix built-in policy supportedObjectTypes validation and error message - #12686
Conversation
Code Coverage Report
Files |
|
Hi @roryqi, could you please take a look at this PR when you have a chance? Thanks! |
| addEx.getMessage().contains("Policy content type mismatch"), | ||
| "expected mismatch message, got: " + addEx.getMessage()); | ||
| // Format arguments must be substituted — neither placeholder should survive literally. | ||
| Assertions.assertFalse( |
There was a problem hiding this comment.
Please verify the actual diagnostic values here, not only that the format placeholders disappeared. This assertion would still pass if the message dropped both sets or formatted the wrong pair, so it does not cover the requirement in #12685 to report the expected and received supported-object sets. Assert that the original set and withExtra (and likewise withFewer below) are present, or assert the complete messages.
There was a problem hiding this comment.
Pull request overview
This pull request fixes validation in PolicyManager.updatePolicyEntity() so built-in policies cannot change their supportedObjectTypes during an update, and improves the resulting validation error message to include the expected/actual sets. It also adds a regression test to cover both “added type” and “removed type” scenarios for a built-in policy update.
Changes:
- Enforce exact equality of
supportedObjectTypesfor built-in policy content updates (catching both additions and removals). - Fix the validation error message formatting by supplying the expected and actual sets.
- Add a regression test for attempted
supportedObjectTypeschanges on a built-in policy.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/src/main/java/org/apache/gravitino/policy/PolicyManager.java | Tightens built-in policy supportedObjectTypes validation and fixes error message formatting args. |
| core/src/test/java/org/apache/gravitino/policy/TestPolicyManager.java | Adds regression coverage for built-in policy content updates that try to add/remove supported object types. |
Suppressed comments (2)
core/src/test/java/org/apache/gravitino/policy/TestPolicyManager.java:409
- Same coverage gap for the removal case: assert the message includes both the expected and actual supportedObjectTypes sets, not just that "%s" was substituted.
Assertions.assertFalse(
removeEx.getMessage().contains("%s"),
"format args were not substituted: " + removeEx.getMessage());
core/src/test/java/org/apache/gravitino/policy/TestPolicyManager.java:403
- Avoid hard-coding the built-in policy type string here; using the enum constant keeps the test aligned if the policy type value ever changes.
PolicyChange.updateContent("system_iceberg_compaction", removedType)));
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| policyManager.alterPolicy( | ||
| METALAKE, | ||
| policyName, | ||
| PolicyChange.updateContent("system_iceberg_compaction", addedType))); |
| Assertions.assertTrue( | ||
| addEx.getMessage().contains("Policy content type mismatch"), | ||
| "expected mismatch message, got: " + addEx.getMessage()); | ||
| // Format arguments must be substituted — neither placeholder should survive literally. | ||
| Assertions.assertFalse( | ||
| addEx.getMessage().contains("%s"), | ||
| "format args were not substituted: " + addEx.getMessage()); |
|
only small comments on test class, so I would merge it |
What changes were proposed in this pull request?
Fix the validation of
supportedObjectTypeswhen updating built-in policies.PolicyManager.updatePolicyEntity()currently usesSets.difference(oldTypes, newTypes).isEmpty(), which only detects removed object types. When the updated set contains additional object types, the validation incorrectly passes.This change:
Why are the changes needed?
Built-in policies must not change their supported metadata object types during an update. The previous asymmetric set comparison did not enforce this invariant for additions.
Fixes #12685