roryqi commented on code in PR #12384:
URL: https://github.com/apache/gravitino/pull/12384#discussion_r3758826950
##########
server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectTagOperations.java:
##########
@@ -260,6 +257,35 @@ public Response associateTagsForObject(
@PathParam("fullName") @AuthorizationFullName String fullName,
@AuthorizationRequest(type =
AuthorizationRequest.RequestType.ASSOCIATE_TAG)
TagsAssociateRequest request) {
+ return associateTagsForObjectInternal(metalake, type, fullName, request);
+ }
+
+ /**
+ * Associates tag values with a metadata object using the v2 request
representation.
+ *
+ * @param metalake The metalake name.
+ * @param type The metadata object type.
+ * @param fullName The metadata object full name.
+ * @param request The tag values association request.
+ * @return The response containing associated tag names.
+ */
+ @POST
+ @Produces("application/vnd.gravitino.v2+json")
+ @Timed(name = "associate-object-tags." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
+ @ResponseMetered(name = "associate-object-tags", absolute = true)
+ @AuthorizationExpression(expression = CAN_ACCESS_METADATA_AND_TAG)
+ public Response associateTagValuesForObject(
+ @PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
+ String metalake,
+ @PathParam("type") @AuthorizationObjectType String type,
+ @PathParam("fullName") @AuthorizationFullName String fullName,
+ @AuthorizationRequest(type =
AuthorizationRequest.RequestType.ASSOCIATE_TAG)
+ TagValuesAssociateRequest request) {
+ return associateTagsForObjectInternal(metalake, type, fullName, request);
Review Comment:
Thanks for the detailed callout. I agree stricter Accept negotiation and
applying the negotiated media type to pre-resource JSON mapping would be more
complete, but that changes the generic versioning/filter and global JSON
exception mapper behavior beyond tag assignment values. I would prefer to keep
this PR scoped to the tag values API surface and leave the broader
negotiation/error-media behavior to a separate follow-up if we want to tighten
it consistently across endpoints.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectTagOperations.java:
##########
@@ -270,26 +299,65 @@ public Response associateTagsForObject(
httpRequest,
() -> {
request.validate();
- MetadataObject object =
- MetadataObjects.parse(
- fullName,
MetadataObject.Type.valueOf(type.toUpperCase(Locale.ROOT)));
+ MetadataObject object = parseMetadataObject(type, fullName);
String[] tagNames =
tagDispatcher.associateTagsForMetadataObject(
metalake, object, request.getTagsToAdd(),
request.getTagsToRemove());
tagNames = tagNames == null ? new String[0] : tagNames;
- LOG.info(
- "Associated tags: {} for object type: {}, full name: {} under
metalake: {}",
- Arrays.toString(tagNames),
- type,
- fullName,
- metalake);
+ logAssociatedTags(type, fullName, metalake, tagNames);
return Utils.ok(new NameListResponse(tagNames));
});
} catch (Exception e) {
return ExceptionHandlers.handleTagException(OperationType.ASSOCIATE, "",
fullName, e);
}
}
+ private Response associateTagValuesForObjectInternal(
+ String metalake, String type, String fullName, TagValuesAssociateRequest
request) {
+ LOG.info(
+ "Received associate tag values request for object type: {}, full name:
{} under metalake: {}",
+ type,
+ fullName,
+ metalake);
+ try {
+ return Utils.doAs(
+ httpRequest,
+ () -> {
+ request.validate();
+ MetadataObject object = parseMetadataObject(type, fullName);
+ String[] tagNames =
+ tagDispatcher.associateTagValuesForMetadataObject(
Review Comment:
Thanks. For this PR I would prefer to keep the association semantics aligned
with the existing name-only API: missing tag names in batch add/remove are
ignored to keep the operation idempotent. V2 extends the assignment
representation with values, but I do not think we need to make missing pair
names strict in this change. If we decide the V2 contract should be stricter
here, I think it is better handled as a focused follow-up with the API contract
and tests updated together.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectTagOperations.java:
##########
@@ -270,26 +299,65 @@ public Response associateTagsForObject(
httpRequest,
() -> {
request.validate();
- MetadataObject object =
- MetadataObjects.parse(
- fullName,
MetadataObject.Type.valueOf(type.toUpperCase(Locale.ROOT)));
+ MetadataObject object = parseMetadataObject(type, fullName);
String[] tagNames =
tagDispatcher.associateTagsForMetadataObject(
metalake, object, request.getTagsToAdd(),
request.getTagsToRemove());
tagNames = tagNames == null ? new String[0] : tagNames;
- LOG.info(
- "Associated tags: {} for object type: {}, full name: {} under
metalake: {}",
- Arrays.toString(tagNames),
- type,
- fullName,
- metalake);
+ logAssociatedTags(type, fullName, metalake, tagNames);
return Utils.ok(new NameListResponse(tagNames));
});
} catch (Exception e) {
return ExceptionHandlers.handleTagException(OperationType.ASSOCIATE, "",
fullName, e);
}
}
+ private Response associateTagValuesForObjectInternal(
+ String metalake, String type, String fullName, TagValuesAssociateRequest
request) {
+ LOG.info(
+ "Received associate tag values request for object type: {}, full name:
{} under metalake: {}",
+ type,
+ fullName,
+ metalake);
+ try {
+ return Utils.doAs(
+ httpRequest,
+ () -> {
+ request.validate();
+ MetadataObject object = parseMetadataObject(type, fullName);
+ String[] tagNames =
+ tagDispatcher.associateTagValuesForMetadataObject(
+ metalake, object, request.tagValuesToAdd(),
request.tagValuesToRemove());
Review Comment:
Good point that 409 can be argued for this state conflict. I am leaning to
keep the current mapping in this PR because this path is still validation of an
invalid assignment transition and is currently handled with the same 400
IllegalArgumentException flow as the other tag value constraint violations. I
would rather avoid introducing a special conflict mapping for only this case
unless we decide to revise the tag-assignment error contract more broadly.
--
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]
