Copilot commented on code in PR #12162:
URL: https://github.com/apache/gravitino/pull/12162#discussion_r3655060573
##########
api/src/main/java/org/apache/gravitino/tag/TagOperations.java:
##########
@@ -70,6 +70,31 @@ public interface TagOperations {
Tag createTag(String name, String comment, Map<String, String> properties)
throws TagAlreadyExistsException;
+ /**
+ * Create a tag under a metalake with an assignment value constraint.
+ *
+ * @param name The name of the tag.
+ * @param comment The comment of the tag.
+ * @param properties The properties of the tag.
+ * @param valueConstraint The assignment value constraint of the tag.
+ * @return The created tag.
+ * @throws TagAlreadyExistsException If the tag already exists.
+ * @throws UnsupportedOperationException If non-default value constraints
are not supported.
+ */
+ default Tag createTag(
+ String name,
+ String comment,
+ Map<String, String> properties,
+ TagValueConstraint valueConstraint)
+ throws TagAlreadyExistsException {
+ if (valueConstraint == null ||
TagValueConstraint.anyValue().equals(valueConstraint)) {
+ return createTag(name, comment, properties);
+ }
Review Comment:
This default overload treats TagValueConstraint.anyValue() as the
legacy/default constraint and delegates to the old createTag(..). However,
legacy behavior is creating tags that only support valueless associations,
which corresponds to TagValueConstraint.withoutValues(). As written, callers
passing withoutValues() will unexpectedly get UnsupportedOperationException.
##########
api/src/main/java/org/apache/gravitino/tag/SupportsTags.java:
##########
@@ -54,17 +54,30 @@ public interface SupportsTags {
Tag getTag(String name) throws NoSuchTagException;
/**
- * Associate tags to the specific object. The tagsToAdd will be added to the
object, and the
- * tagsToRemove will be removed from the object. Note that: 1) Adding or
removing tags that are
- * not existed will be ignored. 2) If the same name tag is in both tagsToAdd
and tagsToRemove, it
- * will be ignored. 3) If the tag is already associated with the object, it
will throw {@link
- * TagAlreadyAssociatedException}
+ * Associate valueless tags to the specific object. The tagsToAdd will be
added to the object, and
+ * the tagsToRemove will be removed from the object. Missing tags are
ignored. If the same tag is
+ * in both tagsToAdd and tagsToRemove, it will be ignored. Repeated existing
assignments are
+ * idempotent.
Review Comment:
The Javadoc says repeated existing assignments are idempotent, but the
current core implementation throws TagAlreadyAssociatedException when a tag is
already associated (e.g., TagManager catches EntityAlreadyExistsException and
rethrows TagAlreadyAssociatedException). This doc change is misleading and
contradicts the exception contract.
##########
api/src/main/java/org/apache/gravitino/tag/Tag.java:
##########
@@ -54,6 +54,25 @@ public interface Tag extends Auditable {
*/
Map<String, String> properties();
+ /**
+ * @return The assignment value constraint of the tag.
+ */
+ default TagValueConstraint valueConstraint() {
+ return TagValueConstraint.anyValue();
+ }
Review Comment:
The default value constraint is set to anyValue(), but the existing tagging
APIs and implementations associate tags without values (String[]-based).
Returning ANY_VALUE by default implies the tag requires/accepts valued
assignments, which can break backward compatibility and mislead clients;
defaulting to withoutValues() better matches the current valueless tag model.
##########
api/src/main/java/org/apache/gravitino/tag/TagValue.java:
##########
@@ -0,0 +1,114 @@
+/*
+ * 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.tag;
+
+import com.google.common.base.Preconditions;
+import java.util.Objects;
+import java.util.Optional;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.annotation.Evolving;
+
+/** Represents one tag assignment value in tag association requests. */
+@Evolving
+public final class TagValue {
+
+ private final String name;
+
+ @Nullable private final String value;
+
+ private TagValue(String name, @Nullable String value) {
+ this.name = name;
+ this.value = value;
+ }
Review Comment:
TagValue is intended as a public API model, but with the current
client/server ObjectMapper defaults (no creator visibility changes) it is not
Jackson-deserializable: it has only a private constructor and final fields, and
the test suite explicitly enforces “no JavaBean accessors”. If TagValue is
expected to appear in request/response JSON, this will fail at runtime;
consider adding a JSON-friendly representation (e.g., a public/protected no-arg
ctor + setters, or another DTO) and aligning/removing the test constraints
accordingly.
--
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]
