Copilot commented on code in PR #10554:
URL: https://github.com/apache/gravitino/pull/10554#discussion_r3013753617
##########
clients/client-python/gravitino/client/gravitino_metalake.py:
##########
@@ -544,8 +550,24 @@ def list_tags_info(self) -> List[Tag]:
Raises:
NoSuchMetalakeException: If the metalake does not exist.
"""
- # TODO implement list_tags_info
- raise NotImplementedError()
+ url = self.API_METALAKES_TAGS_PATH.format(encode_string(self.name()))
+
+ response = self.rest_client.get(
+ url,
+ params={"details": "true"},
+ error_handler=TAG_ERROR_HANDLER,
+ )
+ list_info_resp = TagListResponse.from_json(response.body,
infer_missing=True)
+ list_info_resp.validate()
+
+ return [
+ GenericTag(
+ self.name,
Review Comment:
In `list_tags_info`, `GenericTag` is being constructed with `self.name` (a
bound method) instead of the metalake name string. This will propagate an
incorrect metalake identifier (e.g., "<bound method ...>") into `GenericTag`
and break any API calls that format endpoints with the metalake name. Use
`self.name()` here, consistent with `get_tag`/`create_tag`/`alter_tag`.
```suggestion
self.name(),
```
##########
clients/client-python/tests/unittests/mock_base.py:
##########
@@ -154,3 +191,113 @@ class Wrapper(cls):
def mock_name_identifier_json(name, namespace):
return json.dumps({"name": name, "namespace": namespace}).encode("utf-8")
+
+
+class MockTagRepo:
+ def __init__(self) -> None:
+ self.tag_store = {
+ "tagA": build_tag_dto("tagA", "mock tag A"),
+ "tagB": build_tag_dto("tagB", "mock tag B"),
+ }
+
+ def mock_list_tags(self) -> list[str]:
+ return list(self.tag_store.keys())
+
+ def mock_list_tags_info(self) -> list[TagDTO]:
+ return list(self.tag_store.values())
+
+ def mock_get_tag(self, tag_name: str) -> TagDTO:
+ if tag_name not in self.tag_store:
+ raise ValueError(f"Tag {tag_name} does not exist")
+ return self.tag_store[tag_name]
+
+ def mock_create_tag(
+ self,
+ tag_name: str,
+ comment: str = "",
+ properties=None,
+ ) -> TagDTO:
+ if tag_name in self.tag_store:
+ raise ValueError(f"Tag {tag_name} already exists")
+ self.tag_store[tag_name] = build_tag_dto(tag_name, comment, properties)
+ return self.tag_store[tag_name]
+
+ def mock_alter_tag(self, tag_name: str, *changes) -> TagDTO:
+ if tag_name not in self.tag_store:
+ raise ValueError(f"Tag {tag_name} does not exist")
+
+ for change in changes:
+ current_tag_obj = self.tag_store[tag_name]
+
+ if isinstance(change, TagChange.RenameTag):
+ self.tag_store[change.new_name] = build_tag_dto(
+ change.new_name,
+ current_tag_obj.comment(),
+ dict(current_tag_obj.properties()),
+ )
+ del self.tag_store[tag_name]
+ tag_name = change.new_name
+
+ elif isinstance(change, TagChange.UpdateTagComment):
+ self.tag_store[tag_name] = build_tag_dto(
+ current_tag_obj.name(),
+ change.new_comment,
+ dict(current_tag_obj.properties()),
+ )
+
+ elif isinstance(change, TagChange.RemoveProperty):
+ new_properties = dict(current_tag_obj.properties())
+ new_properties.pop(change.removed_property, None)
+
+ self.tag_store[tag_name] = build_tag_dto(
+ current_tag_obj.name(),
+ current_tag_obj.comment(),
+ new_properties,
+ )
+
+ elif isinstance(change, TagChange.SetProperty):
+ new_properties = dict(current_tag_obj.properties())
+ new_properties[change.name] = change.value
+
+ self.tag_store[tag_name] = build_tag_dto(
+ current_tag_obj.name(),
+ current_tag_obj.comment(),
+ new_properties,
+ )
+
+ else:
+ raise ValueError(f"Unknown tag change type: {change}")
+
+ return self.tag_store[tag_name]
+
+ def mock_delete_tag(self, tag_name: str) -> bool:
+ if tag_name not in self.tag_store:
+ return False
+ del self.tag_store[tag_name]
+ return True
+
+
+@contextmanager
+def mock_tag_methods():
+ repo = MockTagRepo()
+
+ with patch.multiple(
+ GravitinoMetalake,
+ list_tags=MagicMock(side_effect=repo.mock_list_tags),
+ list_tags_info=MagicMock(side_effect=repo.mock_list_tags_info),
+ get_tag=MagicMock(side_effect=repo.mock_get_tag),
+ create_tag=MagicMock(side_effect=repo.mock_create_tag),
+ alter_tag=MagicMock(side_effect=repo.mock_alter_tag),
+ delete_tag=MagicMock(side_effect=repo.mock_delete_tag),
+ ) as mocks:
+ yield mocks, repo
+
+
+def mock_http_response(json_str: str) -> Response:
+ mock_http_resp = Mock(HTTPResponse)
+ mock_http_resp.getcode.return_value = 200
+ mock_http_resp.read.return_value = json_str
+ mock_http_resp.info.return_value = None
+ mock_http_resp.url = None
+ mock_resp = Response(mock_http_resp)
+ return mock_resp
Review Comment:
`mock_http_response()` sets `HTTPResponse.read()` to return a Python `str`,
but the real `HTTPResponse.read()` returns `bytes` and `Response.json()`
assumes `bytes` (calls `.decode('utf-8')`). Returning `str` here can mask
encoding/decoding issues and will break any test paths that start using
`Response.json()`. Make the mock return `json_str.encode('utf-8')` (or accept
`bytes` input) to match real behavior.
##########
clients/client-python/gravitino/utils/random_utils.py:
##########
@@ -0,0 +1,38 @@
+# 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.
+
+
+from __future__ import annotations
+
+import random
+import string
+
+
+class RandomStringUtils:
+ @staticmethod
+ def random_string(length: int = 8) -> str:
+ """
+ Generate a random string of specified length.
+
+ Args:
+ length (int, optional): The length of string. Defaults to 8.
+
+ Returns:
+ str: The random string.
+ """
+ chars = string.ascii_letters + string.digits
+ return "".join(random.choices(chars, k=length))
Review Comment:
`RandomStringUtils.random_string()` uses the `random` module, which is not
suitable for security-sensitive identifiers (predictable PRNG). Since this is
being exported as a general-purpose utility (`gravitino.utils`), prefer
`secrets`-based generation (or clearly document that it is not
cryptographically secure) to avoid accidental misuse.
##########
clients/client-python/gravitino/client/dto_converters.py:
##########
@@ -285,3 +293,39 @@ def to_job_template_update_request(
return UpdateJobTemplateContentRequest(template_update_dto)
raise ValueError(f"Unknown change type: {type(change).__name__}")
+
+ @staticmethod
+ def to_tag_update_request(
+ change: TagChange,
+ ) -> TagUpdateRequestBase:
+ """
+ Convert the tag change to the corresponding tag update request.
+
+ Args:
+ change (TagChange): The tag change.
+
+ Raises:
+ ValueError: if the change type is not supported.
+
+ Returns:
+ tp.Union[ TagUpdateRequest.RenameTagRequest,
+ TagUpdateRequest.UpdateTagCommentRequest,
+ TagUpdateRequest.SetTagPropertyRequest,
+ TagUpdateRequest.RemoveTagPropertyRequest,
+ ]: The tag update request.
+ """
+ if isinstance(change, TagChange.RenameTag):
+ return TagUpdateRequest.RenameTagRequest(change.new_name)
+
+ if isinstance(change, TagChange.UpdateTagComment):
Review Comment:
The docstring for `to_tag_update_request` says it raises `ValueError` and
references `tp.Union[...]`, but the implementation raises
`IllegalArgumentException` and there is no `tp` import. Update the docstring to
match the actual exception type and simplify the return type description to
avoid confusing/incorrect documentation.
```suggestion
IllegalArgumentException: if the change type is not supported.
Returns:
TagUpdateRequestBase: The tag update request.
"""
if isinstance(change, TagChange.RenameTag):
return TagUpdateRequest.RenameTagRequest(change.new_name)
if isinstance(change, TagChange.UpdateTagComment):
if isinstance(change, TagChange.RenameTag):
return TagUpdateRequest.RenameTagRequest(change.new_name)
if isinstance(change, TagChange.UpdateTagComment):
```
##########
clients/client-python/gravitino/client/gravitino_metalake.py:
##########
@@ -623,8 +645,30 @@ def alter_tag(self, tag_name, *changes) -> Tag:
NoSuchTagException: If the tag does not exist.
NoSuchMetalakeException: If the metalake does not exist.
"""
- # TODO implement alter_tag
- raise NotImplementedError()
+ Precondition.check_argument(
+ StringUtils.is_not_blank(tag_name),
+ "tag name must not be null or empty",
+ )
Review Comment:
`alter_tag` currently relies on `TagUpdatesRequest.validate()` to fail when
no `changes` are provided, which results in a confusing `ValueError("updates
must not be null")`. Add an explicit precondition that `changes` is non-empty
(with a clearer message like "at least one change is required") so callers get
a more actionable error.
```suggestion
)
Precondition.check_argument(
changes is not None and len(changes) > 0,
"at least one change is required",
)
```
##########
clients/client-python/gravitino/utils/__init__.py:
##########
@@ -16,11 +16,13 @@
# under the License.
from gravitino.utils.http_client import HTTPClient, Response, unpack
+from gravitino.utils.random_utils import RandomStringUtils
from gravitino.utils.string_utils import StringUtils
__all__ = [
"Response",
"HTTPClient",
"unpack",
"StringUtils",
+ "RandomStringUtils",
Review Comment:
`RandomStringUtils` is added to the public `gravitino.utils` namespace
(`__all__`) but is not referenced anywhere in the client or tests. If this
utility is not required for tag operations, consider removing it (or moving it
under test utilities) to avoid expanding the public API surface with unused
code.
--
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]
