Copilot commented on code in PR #12384:
URL: https://github.com/apache/gravitino/pull/12384#discussion_r3755503533
##########
server/src/main/java/org/apache/gravitino/server/web/rest/TagOperations.java:
##########
@@ -157,7 +157,11 @@ public Response createTag(
request.validate();
Tag tag =
tagDispatcher.createTag(
- metalake, request.getName(), request.getComment(),
request.getProperties());
+ metalake,
+ request.getName(),
+ request.getComment(),
+ request.getProperties(),
+ request.valueConstraint());
Review Comment:
Document the new tag constraint fields in the OpenAPI contract.
`docs/open-api/tags.yaml:354-379` still omits constraint/assignment fields from
`Tag`, and `docs/open-api/tags.yaml:405-423` omits `allowedValues` from
`TagCreateRequest`, so generated clients cannot discover or model this
user-facing capability.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/TagOperations.java:
##########
@@ -268,14 +272,20 @@ public Response deleteTag(
public Response listMetadataObjectsForTag(
@PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
String metalake,
- @PathParam("tag") @AuthorizationMetadata(type = Entity.EntityType.TAG)
String tagName) {
- LOG.info("Received list objects for tag: {} under metalake: {}", tagName,
metalake);
+ @PathParam("tag") @AuthorizationMetadata(type = Entity.EntityType.TAG)
String tagName,
+ @QueryParam("value") String value) {
Review Comment:
Validate a non-null `value` before forwarding it. The Java API rejects blank
values and values over 256 characters, but this REST endpoint accepts them; in
particular, `?value=` queries the storage sentinel used for no-value
assignments, so raw REST callers can observe behavior that the public
`objects(String)` API explicitly disallows. Apply the same nonblank/length
rules here so invalid filters return 400.
##########
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:
Add this V2 representation to the OpenAPI contract. The association
operation at `docs/open-api/tags.yaml:221-244` currently exposes only the V1
string-array request and V1 response media type, leaving the new valued request
shape and `application/vnd.gravitino.v2+json` response undiscoverable.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/TagOperations.java:
##########
@@ -268,14 +272,20 @@ public Response deleteTag(
public Response listMetadataObjectsForTag(
@PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
String metalake,
- @PathParam("tag") @AuthorizationMetadata(type = Entity.EntityType.TAG)
String tagName) {
- LOG.info("Received list objects for tag: {} under metalake: {}", tagName,
metalake);
+ @PathParam("tag") @AuthorizationMetadata(type = Entity.EntityType.TAG)
String tagName,
+ @QueryParam("value") String value) {
+ LOG.info(
+ "Received list objects for tag: {} and value: {} under metalake: {}",
+ tagName,
+ value,
+ metalake);
try {
return Utils.doAs(
httpRequest,
() -> {
- MetadataObject[] objects =
tagDispatcher.listMetadataObjectsForTag(metalake, tagName);
+ MetadataObject[] objects =
+ tagDispatcher.listMetadataObjectsForTag(metalake, tagName,
value);
Review Comment:
Document the new `value` query parameter. `docs/open-api/tags.yaml:307-338`
still describes this operation without any parameters beyond metalake and tag,
so REST consumers and generated clients cannot use the new filtering feature
from the published contract.
##########
server/src/test/java/org/apache/gravitino/server/web/filter/authorization/TestAssociateTagAuthorizationExecutor.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.server.web.filter.authorization;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyMap;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.lang.reflect.Method;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.authorization.AuthorizationRequestContext;
+import org.apache.gravitino.dto.requests.TagValuesAssociateRequest;
+import org.apache.gravitino.dto.requests.TagsAssociateRequest;
+import
org.apache.gravitino.server.authorization.annotations.AuthorizationRequest;
+import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator;
+import org.apache.gravitino.tag.TagValue;
+import org.junit.jupiter.api.Test;
+
+public class TestAssociateTagAuthorizationExecutor {
+
+ @Test
+ public void testAuthorizesV1TagNames() throws Exception {
+ TagsAssociateRequest request =
+ new TagsAssociateRequest(new String[] {"pii", "data_domain"}, null);
+ assertAuthorizesAllTags("associateV1", TagsAssociateRequest.class,
request);
+ }
+
+ @Test
+ public void testAuthorizesV2TagValues() throws Exception {
+ TagValuesAssociateRequest request =
+ new TagValuesAssociateRequest(
+ new TagValue[] {TagValue.noValue("pii"),
TagValue.of("data_domain", "finance")}, null);
+ assertAuthorizesAllTags("associateV2", TagValuesAssociateRequest.class,
request);
Review Comment:
This V2 authorization test puts both tags in `tagsToAdd` and leaves
`tagsToRemove` null, so it would still pass if removal tags were never
authorized. Exercise one addition and one removal to cover both
security-sensitive branches.
--
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]
