Copilot commented on code in PR #12380:
URL: https://github.com/apache/gravitino/pull/12380#discussion_r3725433074
##########
core/src/main/java/org/apache/gravitino/tag/TagManager.java:
##########
@@ -315,32 +340,37 @@ public Tag getTagForMetadataObject(String metalake,
MetadataObject metadataObjec
public String[] associateTagsForMetadataObject(
String metalake, MetadataObject metadataObject, String[] tagsToAdd,
String[] tagsToRemove)
throws NoSuchMetadataObjectException, TagAlreadyAssociatedException {
+ return associateTagValuesForMetadataObject(
+ metalake, metadataObject, toNoValue(tagsToAdd),
toNoValue(tagsToRemove));
+ }
+
+ @Override
+ public String[] associateTagValuesForMetadataObject(
+ String metalake, MetadataObject metadataObject, TagValue[] tagsToAdd,
TagValue[] tagsToRemove)
+ throws NoSuchMetadataObjectException, TagAlreadyAssociatedException {
Preconditions.checkArgument(
SUPPORTED_METADATA_OBJECT_TYPES_FOR_TAGS.contains(metadataObject.type()),
"Cannot associate tags for unsupported metadata object type %s",
metadataObject.type());
+ validateTagValuesToAdd(tagsToAdd);
Review Comment:
associateTagValuesForMetadataObject validates tagsToAdd but does not
validate tagsToRemove elements; a null entry in tagsToRemove will flow into the
LinkedHashSet and later cause an NPE in toRelationEdgeTargets
(tagValue.name()). Add a null check for tagsToRemove similar to tagsToAdd to
fail fast with a clear error.
##########
common/src/main/java/org/apache/gravitino/dto/requests/TagCreateRequest.java:
##########
@@ -72,5 +106,12 @@ public TagCreateRequest() {
public void validate() throws IllegalArgumentException {
Preconditions.checkArgument(
StringUtils.isNotBlank(name), "\"name\" is required and cannot be
empty");
+
+ if (allowedValues != null) {
+ for (String value : allowedValues) {
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(value), "allowedValues cannot contain null
or empty values");
+ }
Review Comment:
TagCreateRequest.validate() accepts allowedValues longer than 256
characters, but tag assignment values are stored in tag_relation_meta.tag_value
as VARCHAR(256) and TagValuesAssociateRequest already enforces a 256-character
limit. This can allow creating tags whose allowedValues can never be assigned
(or can fail when bypassing REST validation). Add a length check consistent
with the assignment value constraint.
##########
core/src/main/java/org/apache/gravitino/tag/TagDispatcher.java:
##########
@@ -96,6 +117,19 @@ public interface TagDispatcher {
*/
MetadataObject[] listMetadataObjectsForTag(String metalake, String name);
+ /**
+ * List all metadata objects associated with the specified tag and exact
assignment value.
+ *
+ * @param metalake The name of the metalake.
+ * @param name The name of the tag.
+ * @param value The exact assignment value to match, or null to return all
objects for the tag.
+ * @return The array of metadata objects associated with the specified tag
and value.
+ */
Review Comment:
The new listMetadataObjectsForTag(metalake, name, value) overload uses null
to mean "no filter", but it isn't clear how callers should query for valueless
assignments (which are represented in storage as the empty string). Clarifying
this in the Javadoc will help avoid misuse.
##########
common/src/test/java/org/apache/gravitino/dto/requests/TestTagCreateRequest.java:
##########
@@ -46,4 +47,32 @@ public void testTagCreateRequestSerDe() throws
JsonProcessingException {
Assertions.assertEquals(request1, deserRequest1);
Assertions.assertEquals(properties, deserRequest1.getProperties());
}
+
+ @Test
+ public void testTagCreateRequestSerDeWithAllowedValues() throws
JsonProcessingException {
+ String[] allowedValues = new String[] {"finance", "risk"};
+ TagCreateRequest request = new TagCreateRequest("tag_test", "tag comment",
null, allowedValues);
+
+ String serJson = JsonUtils.objectMapper().writeValueAsString(request);
+ TagCreateRequest deserRequest =
+ JsonUtils.objectMapper().readValue(serJson, TagCreateRequest.class);
+
+ Assertions.assertEquals(request, deserRequest);
+ Assertions.assertArrayEquals(allowedValues,
deserRequest.getAllowedValues());
+ }
+
+ @Test
+ public void testTagCreateRequestValidateAllowedValues() {
+ new TagCreateRequest("tag_test", "tag comment", null, new String[]
{"finance"}).validate();
+
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> new TagCreateRequest("tag_test", null, null, new String[]
{""}).validate());
+ char[] longValueChars = new char[257];
+ Arrays.fill(longValueChars, 'a');
+ Assertions.assertDoesNotThrow(
+ () ->
+ new TagCreateRequest("tag_test", null, null, new String[] {new
String(longValueChars)})
+ .validate());
Review Comment:
This test currently asserts that allowedValues of length 257 is valid, but
tag assignment values are constrained to VARCHAR(256) (and
TagValuesAssociateRequest enforces 256). Update the test to treat 256 as the
maximum accepted length and to reject 257+.
--
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]
