mchades commented on code in PR #12384:
URL: https://github.com/apache/gravitino/pull/12384#discussion_r3758898030
##########
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 — I agree the generic `Accept` parser can be handled separately. The
malformed-body case is still a contract mismatch introduced on this endpoint,
though: `tags.yaml` says the 400 response covers a malformed body and
advertises `application/vnd.gravitino.v2+json`, while
`JsonParseExceptionMapper` runs before `withMediaType` and returns
`application/json`. Could we either make that pre-resource 400 use the V2 media
type, or update the OpenAPI response to document the actual media type and
remove malformed-body from this V2 promise? Then the broader negotiation
behavior can remain a follow-up.
##########
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:
Retaining V1 behavior for the V1 endpoint is compatible with V2 returning
404. The checked-in design already states that a tag referenced by either V2
array but not found returns `404 Not Found`
(`design-docs/tag-assignment-values.md`, line 816), while the current
`TagMetaService` path silently `continue`s and returns 200. This is therefore a
runtime/contract mismatch, not an undecided stricter option. Please implement
the documented 404 behavior, or, if the semantics are intentionally changing,
update the design, OpenAPI, and tests in this PR; otherwise client typos appear
to have succeeded.
##########
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:
The checked-in contract already distinguishes request validation from this
active-state conflict: malformed or invalid values return 400, while adding a
valueless pair with remaining non-null assignments returns `409 Conflict`
(`design-docs/tag-assignment-values.md`, lines 730–733 and 814). This request
is pair-valid and fails only because of current assignment state; routing it
through `IllegalArgumentException` collapses that documented distinction.
Please return 409 and add a REST regression, or revise the design, OpenAPI, and
tests in this PR if the product semantics are intentionally changing.
--
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]
