roryqi commented on code in PR #12380:
URL: https://github.com/apache/gravitino/pull/12380#discussion_r3725530835
##########
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:
Thanks. By design, `null` means no value filter, while this overload only
supports exact lookup of non-blank assignment values. Valueless assignments are
not queryable through the value filter; the empty string is an internal storage
representation rather than a public query value.
##########
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:
Fixed in cba3dee89. `tagsToRemove` is now validated before set normalization
and relation conversion, and a regression test verifies that a null entry fails
fast with a clear message.
##########
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:
Updated in d56f47200. The test now covers both boundaries: a 256-character
allowed value succeeds and a 257-character allowed value is rejected.
##########
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:
Fixed in d56f47200. The validation applies to each individual allowed value:
values up to 256 characters are accepted, while values longer than 256
characters are rejected. The `allowedValues` array itself has no count or
aggregate-length limit.
--
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]
