Copilot commented on code in PR #10892: URL: https://github.com/apache/gravitino/pull/10892#discussion_r3154927495
########## clients/client-python/tests/unittests/test_tag_api.py: ########## @@ -0,0 +1,482 @@ +# 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 unittest +from unittest.mock import patch + +from gravitino import GravitinoClient +from gravitino.api.tag import Tag +from gravitino.api.tag.tag_change import TagChange +from gravitino.dto.responses.drop_response import DropResponse +from gravitino.dto.responses.tag_response import ( + TagListResponse, + TagNamesListResponse, + TagResponse, +) +from tests.unittests import mock_base + + +@mock_base.mock_data +class TestTagAPI(unittest.TestCase): + _metalake_name: str = "metalake_demo" + + def test_client_get_tag(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagA") + self._check_default_tag_a(retrieved_tag) + + def test_client_list_tag(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tags = client.list_tags() + self.assertEqual(2, len(retrieved_tags)) + self.assertTrue("tagA" in retrieved_tags) + self.assertTrue("tagB" in retrieved_tags) + + client.create_tag("tagC", "mock tag C", None) + + retrieved_tags = client.list_tags() + self.assertEqual(3, len(retrieved_tags)) + self.assertTrue("tagA" in retrieved_tags) + self.assertTrue("tagB" in retrieved_tags) + self.assertTrue("tagC" in retrieved_tags) + + def test_client_list_tag_info(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tags = client.list_tags_info() + self.assertEqual(2, len(retrieved_tags)) + tag_names = [tag.name() for tag in retrieved_tags] + + self.assertTrue("tagA" in tag_names) + self.assertTrue("tagB" in tag_names) + + tag_comments = [tag.comment() for tag in retrieved_tags] + self.assertTrue("mock tag A" in tag_comments) + self.assertTrue("mock tag B" in tag_comments) + + def test_client_remove_tag(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tags = client.list_tags() + self.assertEqual(2, len(retrieved_tags)) + self.assertTrue("tagA" in retrieved_tags) + self.assertTrue("tagB" in retrieved_tags) + + client.delete_tag("tagA") + retrieved_tags = client.list_tags() + self.assertEqual(1, len(retrieved_tags)) + self.assertTrue("tagA" not in retrieved_tags) + self.assertTrue("tagB" in retrieved_tags) + + def test_client_create_tag(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tags = client.list_tags() + self.assertEqual(2, len(retrieved_tags)) + self.assertTrue("tagA" in retrieved_tags) + self.assertTrue("tagB" in retrieved_tags) + + client.create_tag("tagC", "mock tag C", None) + retrieved_tags = client.list_tags() + self.assertEqual(3, len(retrieved_tags)) + self.assertTrue("tagA" in retrieved_tags) + self.assertTrue("tagB" in retrieved_tags) + self.assertTrue("tagC" in retrieved_tags) + + def test_client_alter_tag_with_name(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagA") + self._check_default_tag_a(retrieved_tag) + + change = TagChange.rename("tagA-new") + client.alter_tag("tagA", change) + + with self.assertRaises(ValueError): + client.get_tag("tagA") + + retrieved_tag = client.get_tag("tagA-new") + self.assertEqual("tagA-new", retrieved_tag.name()) + self.assertEqual("mock tag A", retrieved_tag.comment()) + self.assertEqual( + { + "key1": "value1", + "key2": "value2", + }, + retrieved_tag.properties(), + ) + + def test_client_alter_tag_with_comment(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagA") + self._check_default_tag_a(retrieved_tag) + + change = TagChange.update_comment("new comment") + client.alter_tag("tagA", change) + + retrieved_tag = client.get_tag("tagA") + self.assertEqual("tagA", retrieved_tag.name()) + self.assertEqual("new comment", retrieved_tag.comment()) + self.assertEqual( + { + "key1": "value1", + "key2": "value2", + }, + retrieved_tag.properties(), + ) + + def test_client_alter_tag_with_remove_property(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagA") + self._check_default_tag_a(retrieved_tag) + + change = TagChange.remove_property("key1") + client.alter_tag("tagA", change) + + retrieved_tag = client.get_tag("tagA") + self.assertEqual("tagA", retrieved_tag.name()) + self.assertEqual("mock tag A", retrieved_tag.comment()) + self.assertEqual( + { + "key2": "value2", + }, + retrieved_tag.properties(), + ) + + def test_client_alter_tag_with_add_property(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagA") + self._check_default_tag_a(retrieved_tag) + + change = TagChange.set_property("key3", "value3") + client.alter_tag("tagA", change) + + retrieved_tag = client.get_tag("tagA") + self.assertEqual("tagA", retrieved_tag.name()) + self.assertEqual("mock tag A", retrieved_tag.comment()) + self.assertEqual( + { + "key1": "value1", + "key2": "value2", + "key3": "value3", + }, + retrieved_tag.properties(), + ) + + def test_client_alter_tag_with_replace_property(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagA") + self._check_default_tag_a(retrieved_tag) + + change = TagChange.set_property("key1", "value3") + client.alter_tag("tagA", change) + + retrieved_tag = client.get_tag("tagA") + self.assertEqual("tagA", retrieved_tag.name()) + self.assertEqual("mock tag A", retrieved_tag.comment()) + self.assertEqual( + { + "key1": "value3", + "key2": "value2", + }, + retrieved_tag.properties(), + ) + + def test_client_alter_tag_with_all_operations(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagB") + self._check_default_tag_b(retrieved_tag) + + changes = [ + TagChange.set_property("key1", "value3"), + TagChange.remove_property("key2"), + TagChange.update_comment("mock tag B updated"), + TagChange.rename("new_tag_B"), + ] + + client.alter_tag("tagB", *changes) + retrieved_tag = client.get_tag("new_tag_B") + + self.assertEqual("new_tag_B", retrieved_tag.name()) + self.assertEqual("mock tag B updated", retrieved_tag.comment()) + self.assertEqual( + { + "key1": "value3", + }, + retrieved_tag.properties(), + ) + + def test_gravitino_list_tag_api(self, *mock_method) -> None: + resp = TagNamesListResponse(0, ["tagA", "tagB", "tagC"]) + json_str = resp.to_json() + mock_resp = mock_base.mock_http_response(json_str) + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + with patch( + "gravitino.utils.http_client.HTTPClient.get", + return_value=mock_resp, + ): + tags = client.list_tags() + self.assertEqual(3, len(tags)) + self.assertTrue("tagA" in tags) + self.assertTrue("tagB" in tags) + self.assertTrue("tagC" in tags) + + def test_gravitino_list_tag_info_api(self, *mock_method) -> None: + tag = mock_base.build_tag_dto() + resp = TagListResponse(0, [tag]) + json_str = resp.to_json() + mock_resp = mock_base.mock_http_response(json_str) + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + with patch( + "gravitino.utils.http_client.HTTPClient.get", + return_value=mock_resp, + ): + tags = client.list_tags_info() + self.assertEqual(1, len(tags)) + self.check_tag_equal(tag, tags[0]) + + def test_gravitino_metalake_delete_tag_api(self, *mock_method) -> None: + resp = DropResponse(0, True) + json_str = resp.to_json() + mock_resp = mock_base.mock_http_response(json_str) + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + with patch( + "gravitino.utils.http_client.HTTPClient.delete", + return_value=mock_resp, + ): + is_dropped = client.delete_tag("tag1") + self.assertTrue(is_dropped) Review Comment: This test exercises `client.delete_tag()`, but `GravitinoMetalake.delete_tag` currently raises `NotImplementedError` in this branch, so the test will fail even though `HTTPClient.delete` is mocked. Either cherry-pick/implement `delete_tag`, or remove/skip this test until the API is implemented. ########## clients/client-python/gravitino/client/gravitino_metalake.py: ########## @@ -623,8 +623,34 @@ 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", + ) + Precondition.check_argument( + changes is not None and len(changes) > 0, + "at least one change is required", + ) + updates = [DTOConverters.to_tag_update_request(change) for change in changes] + update_req = TagUpdatesRequest(updates) + update_req.validate() + + url = self.API_METALAKES_TAG_PATH.format( + encode_string(self.name()), encode_string(tag_name) + ) + response = self.rest_client.put( + url, + json=update_req, + error_handler=TAG_ERROR_HANDLER, + ) + tag_resp: TagResponse = TagResponse.from_json(response.body, infer_missing=True) + + tag_resp.validate() + return GenericTag( + self.name(), + tag_resp.tag(), + self.rest_client, + ) Review Comment: `DTOConverters.to_tag_update_request` and `TagUpdatesRequest` are not implemented anywhere in this branch (search finds only these references), so `alter_tag` will fail with `AttributeError`/`NameError` before making the PUT request. Add the missing DTO converter and request classes (similar to `CatalogUpdateRequest`/`CatalogUpdatesRequest`) and import `TagUpdatesRequest` here, or adjust `alter_tag` to use existing request/DTO types. ```suggestion raise NotImplementedError( "alter_tag is not supported in this branch because tag update request " "DTOs and converters are not implemented." ) ``` ########## clients/client-python/tests/unittests/test_tag_api.py: ########## @@ -0,0 +1,482 @@ +# 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 unittest +from unittest.mock import patch + +from gravitino import GravitinoClient +from gravitino.api.tag import Tag +from gravitino.api.tag.tag_change import TagChange +from gravitino.dto.responses.drop_response import DropResponse +from gravitino.dto.responses.tag_response import ( + TagListResponse, + TagNamesListResponse, + TagResponse, +) +from tests.unittests import mock_base + + +@mock_base.mock_data +class TestTagAPI(unittest.TestCase): + _metalake_name: str = "metalake_demo" + + def test_client_get_tag(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagA") + self._check_default_tag_a(retrieved_tag) + + def test_client_list_tag(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tags = client.list_tags() + self.assertEqual(2, len(retrieved_tags)) + self.assertTrue("tagA" in retrieved_tags) + self.assertTrue("tagB" in retrieved_tags) + + client.create_tag("tagC", "mock tag C", None) + + retrieved_tags = client.list_tags() + self.assertEqual(3, len(retrieved_tags)) + self.assertTrue("tagA" in retrieved_tags) + self.assertTrue("tagB" in retrieved_tags) + self.assertTrue("tagC" in retrieved_tags) + + def test_client_list_tag_info(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tags = client.list_tags_info() + self.assertEqual(2, len(retrieved_tags)) + tag_names = [tag.name() for tag in retrieved_tags] + + self.assertTrue("tagA" in tag_names) + self.assertTrue("tagB" in tag_names) + + tag_comments = [tag.comment() for tag in retrieved_tags] + self.assertTrue("mock tag A" in tag_comments) + self.assertTrue("mock tag B" in tag_comments) + + def test_client_remove_tag(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tags = client.list_tags() + self.assertEqual(2, len(retrieved_tags)) + self.assertTrue("tagA" in retrieved_tags) + self.assertTrue("tagB" in retrieved_tags) + + client.delete_tag("tagA") + retrieved_tags = client.list_tags() + self.assertEqual(1, len(retrieved_tags)) + self.assertTrue("tagA" not in retrieved_tags) + self.assertTrue("tagB" in retrieved_tags) + + def test_client_create_tag(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tags = client.list_tags() + self.assertEqual(2, len(retrieved_tags)) + self.assertTrue("tagA" in retrieved_tags) + self.assertTrue("tagB" in retrieved_tags) + + client.create_tag("tagC", "mock tag C", None) + retrieved_tags = client.list_tags() + self.assertEqual(3, len(retrieved_tags)) + self.assertTrue("tagA" in retrieved_tags) + self.assertTrue("tagB" in retrieved_tags) + self.assertTrue("tagC" in retrieved_tags) + + def test_client_alter_tag_with_name(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagA") + self._check_default_tag_a(retrieved_tag) + + change = TagChange.rename("tagA-new") + client.alter_tag("tagA", change) + + with self.assertRaises(ValueError): + client.get_tag("tagA") + + retrieved_tag = client.get_tag("tagA-new") + self.assertEqual("tagA-new", retrieved_tag.name()) + self.assertEqual("mock tag A", retrieved_tag.comment()) + self.assertEqual( + { + "key1": "value1", + "key2": "value2", + }, + retrieved_tag.properties(), + ) + + def test_client_alter_tag_with_comment(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagA") + self._check_default_tag_a(retrieved_tag) + + change = TagChange.update_comment("new comment") + client.alter_tag("tagA", change) + + retrieved_tag = client.get_tag("tagA") + self.assertEqual("tagA", retrieved_tag.name()) + self.assertEqual("new comment", retrieved_tag.comment()) + self.assertEqual( + { + "key1": "value1", + "key2": "value2", + }, + retrieved_tag.properties(), + ) + + def test_client_alter_tag_with_remove_property(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagA") + self._check_default_tag_a(retrieved_tag) + + change = TagChange.remove_property("key1") + client.alter_tag("tagA", change) + + retrieved_tag = client.get_tag("tagA") + self.assertEqual("tagA", retrieved_tag.name()) + self.assertEqual("mock tag A", retrieved_tag.comment()) + self.assertEqual( + { + "key2": "value2", + }, + retrieved_tag.properties(), + ) + + def test_client_alter_tag_with_add_property(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagA") + self._check_default_tag_a(retrieved_tag) + + change = TagChange.set_property("key3", "value3") + client.alter_tag("tagA", change) + + retrieved_tag = client.get_tag("tagA") + self.assertEqual("tagA", retrieved_tag.name()) + self.assertEqual("mock tag A", retrieved_tag.comment()) + self.assertEqual( + { + "key1": "value1", + "key2": "value2", + "key3": "value3", + }, + retrieved_tag.properties(), + ) + + def test_client_alter_tag_with_replace_property(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagA") + self._check_default_tag_a(retrieved_tag) + + change = TagChange.set_property("key1", "value3") + client.alter_tag("tagA", change) + + retrieved_tag = client.get_tag("tagA") + self.assertEqual("tagA", retrieved_tag.name()) + self.assertEqual("mock tag A", retrieved_tag.comment()) + self.assertEqual( + { + "key1": "value3", + "key2": "value2", + }, + retrieved_tag.properties(), + ) + + def test_client_alter_tag_with_all_operations(self, *mock_method) -> None: + with mock_base.mock_tag_methods(): + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + retrieved_tag = client.get_tag("tagB") + self._check_default_tag_b(retrieved_tag) + + changes = [ + TagChange.set_property("key1", "value3"), + TagChange.remove_property("key2"), + TagChange.update_comment("mock tag B updated"), + TagChange.rename("new_tag_B"), + ] + + client.alter_tag("tagB", *changes) + retrieved_tag = client.get_tag("new_tag_B") + + self.assertEqual("new_tag_B", retrieved_tag.name()) + self.assertEqual("mock tag B updated", retrieved_tag.comment()) + self.assertEqual( + { + "key1": "value3", + }, + retrieved_tag.properties(), + ) + + def test_gravitino_list_tag_api(self, *mock_method) -> None: + resp = TagNamesListResponse(0, ["tagA", "tagB", "tagC"]) + json_str = resp.to_json() + mock_resp = mock_base.mock_http_response(json_str) + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + with patch( + "gravitino.utils.http_client.HTTPClient.get", + return_value=mock_resp, + ): + tags = client.list_tags() + self.assertEqual(3, len(tags)) + self.assertTrue("tagA" in tags) + self.assertTrue("tagB" in tags) + self.assertTrue("tagC" in tags) + + def test_gravitino_list_tag_info_api(self, *mock_method) -> None: + tag = mock_base.build_tag_dto() + resp = TagListResponse(0, [tag]) + json_str = resp.to_json() + mock_resp = mock_base.mock_http_response(json_str) + client = GravitinoClient( + uri="http://localhost:8090";, + metalake_name=self._metalake_name, + check_version=False, + ) + + with patch( + "gravitino.utils.http_client.HTTPClient.get", + return_value=mock_resp, + ): + tags = client.list_tags_info() + self.assertEqual(1, len(tags)) + self.check_tag_equal(tag, tags[0]) Review Comment: This test exercises `client.list_tags_info()`, but `GravitinoMetalake.list_tags_info` currently raises `NotImplementedError` in this branch, so the test will fail regardless of the mocked HTTP response. Either cherry-pick/implement `list_tags_info` in the client, or remove/skip this test until the API is implemented. ########## clients/client-python/gravitino/client/gravitino_metalake.py: ########## @@ -623,8 +623,34 @@ 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), Review Comment: `alter_tag` references `StringUtils.is_not_blank`, but `StringUtils` is not defined/imported anywhere in the Python client (search shows only this usage). This will raise `NameError` at runtime. Use the existing `Precondition.check_string_not_empty(tag_name, ...)` pattern (as used in `get_tag`) or inline `tag_name is not None and tag_name.strip() != ""` instead. ```suggestion Precondition.check_string_not_empty( tag_name, ``` -- 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]
