mchades commented on code in PR #12442:
URL: https://github.com/apache/gravitino/pull/12442#discussion_r3796640677
##########
clients/client-python/gravitino/dto/requests/tag_associate_request.py:
##########
@@ -27,45 +28,162 @@
@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"))
- _tags_to_add: list[str] = field(metadata=config(field_name="tagsToAdd"))
- _tags_to_remove: list[str] =
field(metadata=config(field_name="tagsToRemove"))
+ @property
+ def name(self) -> str:
+ """Gets the tag name."""
+ return self._name
@property
- def tags_to_add(self) -> list[str]:
- """
- Gets the tags to add.
- """
+ 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",
+ )
+
+
+@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) -> 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",
)
- 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):
Review Comment:
[P2] Preserve the legacy `TagsAssociateRequest` wire contract
This reuses the public `TagsAssociateRequest` name for the V2 tag-value
payload. At the merge-base, `TagsAssociateRequest(["tagA"], ["tagB"])`
serializes to string arrays; at this head the same call serializes to `{name,
value}` objects. Downstream code that constructs this DTO and posts it as
`application/json` will therefore send the wrong schema to the V1 endpoint.
Please keep `TagsAssociateRequest` as the name-array DTO and introduce a
separately named `TagValuesAssociateRequest` for `assign_tags`, matching the
Java request types.
##########
clients/client-python/gravitino/dto/tag_dto.py:
##########
@@ -33,6 +33,12 @@ class TagDTO(Tag):
_name: str = field(metadata=config(field_name="name"))
_comment: str = field(metadata=config(field_name="comment"))
_properties: dict[str, str] =
field(metadata=config(field_name="properties"))
+ _allowed_values: Optional[list[str]] = field(
Review Comment:
[P2] Keep the existing positional constructor arguments stable
Inserting these fields before `_audit` and `_inherited` changes the meaning
of the existing `TagDTO(name, comment, properties, audit, inherited)` call. On
this head, `audit` is bound to `_allowed_values`, `inherited` is bound to
`_assignment_values`, `audit_info()`/`inherited()` return `None`, and `hash()`
can raise `TypeError`. Please append the new fields after the existing
positional fields, or provide a compatibility initializer; using keyword
arguments in the builder would also prevent this from recurring.
##########
clients/client-python/gravitino/api/tag/supports_tags.py:
##########
@@ -61,6 +61,23 @@ def get_tag(self, name: str) -> Tag:
"""
pass
+ @abstractmethod
Review Comment:
[P2] Provide compatibility defaults for the new tag APIs
Making this method abstract prevents every existing custom `SupportsTags`
implementation from being instantiated after an upgrade, even when it only uses
the legacy APIs. The new abstract `Tag.allowed_values()` and
`Tag.assignment_values()` methods have the same problem. Please make the new
APIs concrete compatibility defaults—`assign_tags()` can raise
`UnsupportedOperationException`, while the two `Tag` accessors can return
`None`—and override them in the built-in implementations. This also matches the
default-method strategy used by the Java interfaces.
--
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]
