LiJie20190102 commented on code in PR #12743:
URL: https://github.com/apache/gravitino/pull/12743#discussion_r3909821578
##########
core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:
##########
@@ -500,12 +500,14 @@ private PolicyEntity updatePolicyEntity(PolicyEntity
policyEntity, PolicyChange.
if (policyType != Policy.BuiltInType.CUSTOM) {
// cannot change the supported object types for built-in policies
+ Set<String> oldTypes = policyEntity.content().supportedObjectTypes();
+ Set<String> newTypes =
updateContent.getContent().supportedObjectTypes();
Preconditions.checkArgument(
- Sets.difference(
- policyEntity.content().supportedObjectTypes(),
- updateContent.getContent().supportedObjectTypes())
- .isEmpty(),
- "Policy content type mismatch: expected %s but got %s");
+ Sets.difference(oldTypes, newTypes).isEmpty()
Review Comment:
Issue #12685 explicitly states:
_No_ existing test covers the **UpdateContent** path for built-in policies —
the test gap is what let this through.
This PR fixes the bug but adds no tests. **TestPolicyManager** only covers
rename, comment, enable/disable, and custom-content paths today. Please add
regression tests for the **UpdateContent** path on built-in policies:
- Adding a new **supportedObjectType** to a built-in policy → reject
- Removing an existing **supportedObjectType** from a built-in policy →
reject
- **supportedObjectTypes** unchanged → pass
Without these, the same regression can silently re-enter.
--
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]
