mchades commented on code in PR #12384:
URL: https://github.com/apache/gravitino/pull/12384#discussion_r3756961137
##########
server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectTagOperations.java:
##########
@@ -270,12 +296,24 @@ public Response associateTagsForObject(
httpRequest,
() -> {
request.validate();
+ TagValue[] tagsToAdd;
+ TagValue[] tagsToRemove;
+ if (request instanceof TagsAssociateRequest) {
+ TagsAssociateRequest tagsAssociateRequest =
(TagsAssociateRequest) request;
+ tagsToAdd = toNoValue(tagsAssociateRequest.getTagsToAdd());
+ tagsToRemove = toNoValue(tagsAssociateRequest.getTagsToRemove());
+ } else {
+ TagValuesAssociateRequest tagValuesAssociateRequest =
+ (TagValuesAssociateRequest) request;
+ tagsToAdd = tagValuesAssociateRequest.tagValuesToAdd();
+ tagsToRemove = tagValuesAssociateRequest.tagValuesToRemove();
+ }
MetadataObject object =
MetadataObjects.parse(
fullName,
MetadataObject.Type.valueOf(type.toUpperCase(Locale.ROOT)));
String[] tagNames =
- tagDispatcher.associateTagsForMetadataObject(
- metalake, object, request.getTagsToAdd(),
request.getTagsToRemove());
+ tagDispatcher.associateTagValuesForMetadataObject(
Review Comment:
**[P1] Preserve pair-specific removal for V2 valueless entries**
`TagValuesAssociateRequest` maps an omitted or null value to
`TagValue.noValue`, but `TagMetaService.associateTagValuesWithMetadataObject`
treats any valueless removal as a wildcard and puts the tag ID in
`tagIdsToRemove`, which deletes every row for this tag and object. If
`domain=finance` and `domain=risk` are active, V2 `tagsToRemove: [{"name":
"domain"}]` therefore silently removes both instead of only the `(domain,
null)` pair, which may not exist. The V2 design defines removal as
pair-specific and a missing pair as an idempotent no-op. Please keep the V1
name-only remove-all path separate from V2 pair removal and add this regression
case.
##########
common/src/main/java/org/apache/gravitino/dto/requests/TagValuesAssociateRequest.java:
##########
@@ -82,32 +85,28 @@ public TagValue[] tagValuesToRemove() {
@Override
public void validate() throws IllegalArgumentException {
Preconditions.checkArgument(
- tagsToAdd != null || tagsToRemove != null,
- "tagsToAdd and tagsToRemove cannot both be null");
+ tagsToAdd.length > 0 || tagsToRemove.length > 0,
+ "tagsToAdd and tagsToRemove cannot both be empty");
validateTagValues(tagsToAdd, "tagsToAdd");
Review Comment:
**[P2] Reject an exact pair present in both arrays**
This validates each array independently but never checks their intersection.
`TagManager` later removes common pairs from both sets, so adding and removing
the same `(name, value)` in one request returns 200 as a no-op. The documented
V2 contract requires this conflict to return 400. Please validate the
cross-array intersection here and add a request-level regression test.
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/TagIT.java:
##########
@@ -298,6 +301,42 @@ public void testCreateGetAndListTag() {
Assertions.assertEquals(tag2, tag3);
}
+ @Test
+ public void testAssignmentValues() {
+ String tagName = GravitinoITUtils.genRandomName("tag_it_assignment_value");
+ TagValueConstraint constraint =
TagValueConstraint.ofAllowedValues("finance", "risk");
+ Tag tag = metalake.createTag(tagName, "comment", Collections.emptyMap(),
constraint);
+ Assertions.assertEquals(constraint, tag.valueConstraint());
+
+ TagValue[] values =
+ new TagValue[] {TagValue.of(tagName, "finance"), TagValue.of(tagName,
"risk")};
+ Assertions.assertArrayEquals(
+ new String[] {tagName}, table.supportsTags().associateTags(values,
null));
Review Comment:
**[P2] Keep all-valueless V2 additions idempotent**
A V2 request containing only `TagValue.noValue(...)` reaches
`RelationUpdate.hasRelationValues() == false`, so `RelationalEntityStore`
routes it through the legacy `associateTagsWithMetadataObject` path, which
enables duplicate failure. Repeating the request then returns 409, while the
same valueless pair becomes idempotent if its batch happens to contain any
unrelated valued pair. V2 behavior should not depend on other batch elements.
Please select the V1 versus V2 semantics explicitly and extend this test with a
repeated all-valueless request.
##########
server/src/main/java/org/apache/gravitino/server/web/filter/authorization/AssociateTagAuthorizationExecutor.java:
##########
@@ -63,37 +66,51 @@ public boolean execute(AuthorizationRequestContext context)
throws Exception {
context.setOriginalAuthorizationExpression(expression);
Entity.EntityType targetType =
Entity.EntityType.TAG; // Tags are the only supported batch target here
+
Preconditions.checkArgument(
- request instanceof TagsAssociateRequest,
+ request instanceof TagsAssociateRequest || request instanceof
TagValuesAssociateRequest,
"Only tag can use AssociateTagAuthorizationExecutor, please contact
the administrator.");
- TagsAssociateRequest tagsAssociateRequest = (TagsAssociateRequest) request;
- tagsAssociateRequest.validate();
+
+ TagValue[] tagsToAdd;
+ TagValue[] tagsToRemove;
+ if (request instanceof TagsAssociateRequest) {
+ TagsAssociateRequest tagsAssociateRequest = (TagsAssociateRequest)
request;
+ tagsAssociateRequest.validate();
+ tagsToAdd = toNoValue(tagsAssociateRequest.getTagsToAdd());
+ tagsToRemove = toNoValue(tagsAssociateRequest.getTagsToRemove());
+ } else {
+ TagValuesAssociateRequest tagValuesAssociateRequest =
(TagValuesAssociateRequest) request;
+ tagValuesAssociateRequest.validate();
Review Comment:
**[P2] Preserve 400 responses for invalid V2 bodies when authorization is
enabled**
A deserialized but invalid V2 body, such as both arrays empty or a blank or
overlong value, throws here before the resource method runs.
`GravitinoInterceptionService` catches that exception and converts it to
`Utils.internalError`, so the request returns 500 instead of the REST layer
400. Please avoid full request validation in the authorizer or map validation
failures to Bad Request, and add an authorization-enabled endpoint test.
--
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]
