mchades commented on code in PR #12384:
URL: https://github.com/apache/gravitino/pull/12384#discussion_r3757764286
##########
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] V1 REST still bypasses the new `TAG_NAMES` path**
The core split is not reached from the V1 endpoint.
`associateTagsForObjectInternal` converts a `TagsAssociateRequest` into
`TagValue.noValue(...)` and then unconditionally invokes
`associateTagValuesForMetadataObject`, so V1 REST and Java-client calls still
select `TAG_VALUES`. As a result, repeating a V1 add is now an idempotent 200
instead of the legacy 409, and `tagsToRemove: ["domain"]` only attempts to
delete `(domain, no-value)` rather than all active valued rows. This also
contradicts the existing `TagIT` assertion that a repeated V1 add throws
`TagAlreadyAssociatedException`. Please invoke the string overload for V1,
reserve the valued overload for V2, and add a V2-valued-assignment to V1
name-removal regression.
##########
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:
**[P2] Preserve wildcard `Accept` semantics and V2 media on error paths**
The versioned `Content-Type` disambiguates the request body only when
clients also send a versioned `Accept`. With `Content-Type:
application/vnd.gravitino.v2+json` and an omitted `Accept` or `Accept: */*`,
`VersioningFilter` sees no vendor token and replaces `Accept` with V1; no
resource method then satisfies both V2 `@Consumes` and the injected V1 response
type, so a wildcard request receives 406. The new success test covers only
callers that explicitly send both V2 headers. In addition, validation,
not-found, and conflict branches still return explicit `application/json`
through `Utils` even for a V2-only `Accept`, while the OpenAPI operation
documents only V1 error media types and no 400 response. Please let the vendor
request type select V2 when `Accept` is missing or wildcard (or preserve
wildcard negotiation), return and document the V2 error media type, and add
coverage for both cases.
--
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]
