Copilot commented on code in PR #12442:
URL: https://github.com/apache/gravitino/pull/12442#discussion_r3781575009
##########
clients/client-python/gravitino/dto/requests/tag_associate_request.py:
##########
@@ -27,45 +28,149 @@
@dataclass_json
@dataclass
-class TagsAssociateRequest(RESTRequest):
- """
- Represents a request to associate tags.
- """
+class TagValuePairRequest(RESTRequest):
+ """Represents a tag assignment value pair request."""
+
+ _name: str = field(metadata=config(field_name="name"))
+ _value: Optional[str] = field(default=None,
metadata=config(field_name="value"))
+
+ @property
+ def name(self) -> str:
+ """Gets the tag name."""
+ return self._name
+
+ @property
+ def value(self) -> Optional[str]:
+ """Gets the tag assignment value."""
+ return self._value
+
+ def validate(self) -> None:
+ """Validates the request."""
+ Precondition.check_argument(
+ StringUtils.is_not_blank(self._name),
+ "Tag name must not be null or empty",
+ )
+ if self._value is not None:
+ Precondition.check_argument(
+ self._value.strip() != "",
+ "Tag value must not be empty",
+ )
+ Precondition.check_argument(
+ len(self._value) <= 256,
+ "Tag value must not be longer than 256 characters",
+ )
- _tags_to_add: list[str] = field(metadata=config(field_name="tagsToAdd"))
- _tags_to_remove: list[str] =
field(metadata=config(field_name="tagsToRemove"))
+
+@dataclass_json
+@dataclass
+class TagNamesAssociateRequest(RESTRequest):
+ """Represents a request to associate tag names."""
+
+ _tags_to_add: Optional[list[str]] = field(
+ default=None, metadata=config(field_name="tagsToAdd")
+ )
+ _tags_to_remove: Optional[list[str]] = field(
+ default=None, metadata=config(field_name="tagsToRemove")
+ )
@property
- def tags_to_add(self) -> list[str]:
- """
- Gets the tags to add.
- """
+ def tags_to_add(self) -> Optional[list[str]]:
+ """Gets the tags to add."""
return self._tags_to_add
@property
- def tags_to_remove(self) -> list[str]:
- """
- Gets the tags to remove.
- """
+ def tags_to_remove(self) -> Optional[list[str]]:
+ """Gets the tags to remove."""
return self._tags_to_remove
def validate(self) -> None:
- """
- Validates the request.
- """
+ """Validates the request."""
Precondition.check_argument(
- self._tags_to_add is not None or self._tags_to_remove is not None,
- "tagsToAdd and tagsToRemove cannot both be null",
+ bool(self._tags_to_add) or bool(self._tags_to_remove),
+ "tagsToAdd and tagsToRemove cannot both be null or empty",
Review Comment:
This changes the legacy `associate_tags([], [])` behavior even though the PR
promises that API remains compatible. The previous request validator—and the
server's v1 `TagsAssociateRequest`—only reject two `None` fields, so a no-op
with two empty lists previously passed. Preserve that null check here; the
stricter non-empty rule should apply only to the new value-pair request.
##########
clients/client-python/gravitino/dto/requests/tag_associate_request.py:
##########
@@ -27,45 +28,149 @@
@dataclass_json
@dataclass
-class TagsAssociateRequest(RESTRequest):
- """
- Represents a request to associate tags.
- """
+class TagValuePairRequest(RESTRequest):
+ """Represents a tag assignment value pair request."""
+
+ _name: str = field(metadata=config(field_name="name"))
+ _value: Optional[str] = field(default=None,
metadata=config(field_name="value"))
+
+ @property
+ def name(self) -> str:
+ """Gets the tag name."""
+ return self._name
+
+ @property
+ def value(self) -> Optional[str]:
+ """Gets the tag assignment value."""
+ return self._value
+
+ def validate(self) -> None:
+ """Validates the request."""
+ Precondition.check_argument(
+ StringUtils.is_not_blank(self._name),
+ "Tag name must not be null or empty",
+ )
+ if self._value is not None:
+ Precondition.check_argument(
+ self._value.strip() != "",
+ "Tag value must not be empty",
+ )
+ Precondition.check_argument(
+ len(self._value) <= 256,
+ "Tag value must not be longer than 256 characters",
+ )
- _tags_to_add: list[str] = field(metadata=config(field_name="tagsToAdd"))
- _tags_to_remove: list[str] =
field(metadata=config(field_name="tagsToRemove"))
+
+@dataclass_json
+@dataclass
+class TagNamesAssociateRequest(RESTRequest):
+ """Represents a request to associate tag names."""
+
+ _tags_to_add: Optional[list[str]] = field(
+ default=None, metadata=config(field_name="tagsToAdd")
+ )
+ _tags_to_remove: Optional[list[str]] = field(
+ default=None, metadata=config(field_name="tagsToRemove")
+ )
@property
- def tags_to_add(self) -> list[str]:
- """
- Gets the tags to add.
- """
+ def tags_to_add(self) -> Optional[list[str]]:
+ """Gets the tags to add."""
return self._tags_to_add
@property
- def tags_to_remove(self) -> list[str]:
- """
- Gets the tags to remove.
- """
+ def tags_to_remove(self) -> Optional[list[str]]:
+ """Gets the tags to remove."""
return self._tags_to_remove
def validate(self) -> None:
- """
- Validates the request.
- """
+ """Validates the request."""
Precondition.check_argument(
- self._tags_to_add is not None or self._tags_to_remove is not None,
- "tagsToAdd and tagsToRemove cannot both be null",
+ bool(self._tags_to_add) or bool(self._tags_to_remove),
+ "tagsToAdd and tagsToRemove cannot both be null or empty",
)
- self._validate_tags(self._tags_to_add, "tagsToAdd")
- self._validate_tags(self._tags_to_remove, "tagsToRemove")
+ self._validate_tag_names(self._tags_to_add, "tagsToAdd")
+ self._validate_tag_names(self._tags_to_remove, "tagsToRemove")
- def _validate_tags(self, tags: list[str] | None, field_name: str) -> None:
- if tags is None:
+ def _validate_tag_names(
+ self, tag_names: Optional[list[str]], field_name: str
+ ) -> None:
+ if tag_names is None:
return
Precondition.check_argument(
- all(StringUtils.is_not_blank(tag) for tag in tags),
+ all(StringUtils.is_not_blank(tag_name) for tag_name in tag_names),
f"{field_name} must not contain null or empty tag names",
)
+
+
+@dataclass_json
+@dataclass
+class TagsAssociateRequest(RESTRequest):
+ """Represents a request to associate tags."""
+
+ _tags_to_add: Optional[list[TagValuePairRequest]] = field(
+ default=None, metadata=config(field_name="tagsToAdd")
+ )
+ _tags_to_remove: Optional[list[TagValuePairRequest]] = field(
+ default=None, metadata=config(field_name="tagsToRemove")
+ )
+
+ def __post_init__(self) -> None:
+ self._tags_to_add = self._normalize_pairs(self._tags_to_add)
+ self._tags_to_remove = self._normalize_pairs(self._tags_to_remove)
+
+ @property
+ def tags_to_add(self) -> Optional[list[TagValuePairRequest]]:
+ """Gets the tags to add."""
+ return self._tags_to_add
+
+ @property
+ def tags_to_remove(self) -> Optional[list[TagValuePairRequest]]:
+ """Gets the tags to remove."""
+ return self._tags_to_remove
+
+ def validate(self) -> None:
+ """Validates the request."""
+ Precondition.check_argument(
+ bool(self._tags_to_add) or bool(self._tags_to_remove),
+ "tagsToAdd and tagsToRemove cannot both be null or empty",
+ )
+
+ self._validate_pairs(self._tags_to_add, "tagsToAdd")
+ self._validate_pairs(self._tags_to_remove, "tagsToRemove")
Review Comment:
The v2 REST contract rejects an exact tag/value pair appearing in both
`tagsToAdd` and `tagsToRemove`, but this validator accepts that request and
sends it to the server. Add an intersection check after validating the pairs,
matching the server/Java request validation, so `assign_tags` consistently
rejects invalid input locally.
--
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]
