Copilot commented on code in PR #10670: URL: https://github.com/apache/gravitino/pull/10670#discussion_r3098473578
########## clients/client-python/tests/unittests/mock_base.py: ########## @@ -29,13 +29,79 @@ from gravitino.dto.audit_dto import AuditDTO from gravitino.dto.fileset_dto import FilesetDTO from gravitino.dto.metalake_dto import MetalakeDTO +from gravitino.dto.model_dto import ModelDTO from gravitino.dto.schema_dto import SchemaDTO from gravitino.dto.tag_dto import TagDTO from gravitino.namespace import Namespace from gravitino.utils import Response from gravitino.utils.http_client import HTTPClient +def build_schema_dto( + name: str = "demo_schema", + comment: str = "This is a demo model.", + properties: tp.Optional[dict[str, str]] = None, +) -> SchemaDTO: + """ + Build a schema DTO for testing. + + Args: + name (str, optional): The name of the schema. Defaults to "demo_schema". + comment (str, optional): The comment for the schema. Defaults to "This is a demo model.". + properties (tp.Optional[dict[str, str]], optional): The properties for the schema. + Review Comment: `build_schema_dto` defaults `comment` to "This is a demo model.", which is misleading for a schema DTO and is repeated in the docstring. Update the default text (and the docstring) to refer to a schema to avoid confusing future test authors. ########## clients/client-python/tests/unittests/test_generic_model.py: ########## @@ -0,0 +1,70 @@ +# 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. + +import unittest + +from gravitino.api.tag.supports_tags import SupportsTags +from gravitino.client.generic_model import GenericModel +from gravitino.name_identifier import NameIdentifier +from gravitino.namespace import Namespace +from gravitino.utils.http_client import HTTPClient +from tests.unittests.mock_base import build_model_dto + + +class TestGenericModel(unittest.TestCase): + _rest_client = HTTPClient("http://localhost:8080";) + _model_ident: NameIdentifier = NameIdentifier.of( + "demo_metalake", "demo_catalog", "demo_schema", "demo_model" + ) + + def test_equal_and_hash(self) -> None: + genenric_model = GenericModel( + build_model_dto(), + TestGenericModel._rest_client, + TestGenericModel._model_ident.namespace(), + ) + + generic_model2 = GenericModel( + build_model_dto(), + TestGenericModel._rest_client, + Namespace.of("demo_metalake", "demo_catalog", "demo_schema"), + ) + + self.assertEqual(genenric_model, generic_model2) + self.assertEqual(hash(genenric_model), hash(generic_model2)) Review Comment: Local variable name `genenric_model` is misspelled; it makes the test harder to read and easy to copy/paste into other tests. Rename to `generic_model` for consistency with the class name and other variables in the file. ########## clients/client-python/tests/unittests/mock_base.py: ########## @@ -29,13 +29,79 @@ from gravitino.dto.audit_dto import AuditDTO from gravitino.dto.fileset_dto import FilesetDTO from gravitino.dto.metalake_dto import MetalakeDTO +from gravitino.dto.model_dto import ModelDTO from gravitino.dto.schema_dto import SchemaDTO from gravitino.dto.tag_dto import TagDTO from gravitino.namespace import Namespace from gravitino.utils import Response from gravitino.utils.http_client import HTTPClient +def build_schema_dto( + name: str = "demo_schema", + comment: str = "This is a demo model.", + properties: tp.Optional[dict[str, str]] = None, +) -> SchemaDTO: + """ + Build a schema DTO for testing. + + Args: + name (str, optional): The name of the schema. Defaults to "demo_schema". + comment (str, optional): The comment for the schema. Defaults to "This is a demo model.". + properties (tp.Optional[dict[str, str]], optional): The properties for the schema. + + Returns: + SchemaDTO: The built schema DTO. + """ + if properties is None: + properties = { + "key1": "value1", + "key2": "value2", + } + return SchemaDTO.from_dict( + { + "name": name, + "comment": comment, + "properties": properties, + "audit": build_audit_info().to_dict(), + } + ) + + +def build_model_dto( + name: str = "demo_model", + comment: str = "This is a demo model.", + properties: tp.Optional[dict[str, str]] = None, + latest_version: int = 1, +) -> ModelDTO: + """ + Build a model DTO for testing. + + Args: + name (str, optional): The name of the model. Defaults to "demo_model". + comment (str, optional): The comment for the model. Defaults to "This is a demo model.". + properties (tp.Optional[dict[str, str]], optional): The properties for the model. + latest_version (int, optional): The latest version of the model. Defaults to 1. + + Returns: + ModelDTO: _description_ Review Comment: `build_model_dto` docstring has a placeholder return description (`ModelDTO: _description_`). Replace it with a real description (e.g., "The built model DTO") to keep test helpers self-explanatory. ```suggestion ModelDTO: The built model DTO. ``` ########## clients/client-python/gravitino/client/generic_model.py: ########## @@ -14,19 +14,47 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. + +from __future__ import annotations + from typing import Optional +from gravitino.api.metadata_object import MetadataObject +from gravitino.api.metadata_objects import MetadataObjects from gravitino.api.model.model import Model +from gravitino.api.tag import Tag +from gravitino.api.tag.supports_tags import SupportsTags +from gravitino.client.metadata_object_tag_operations import MetadataObjectTagOperations from gravitino.dto.audit_dto import AuditDTO from gravitino.dto.model_dto import ModelDTO +from gravitino.namespace import Namespace +from gravitino.utils import HTTPClient -class GenericModel(Model): +class GenericModel(Model, SupportsTags): _model_dto: ModelDTO """The model DTO object.""" - def __init__(self, model_dto: ModelDTO): + def __init__( + self, model_dto: ModelDTO, rest_client: HTTPClient, model_ns: Namespace + ) -> None: self._model_dto = model_dto + model_full_name = [model_ns.level(1), model_ns.level(2), model_dto.name()] + model_object: MetadataObject = MetadataObjects.of( + model_full_name, MetadataObject.Type.MODEL + ) + self._model_tag_opearions = MetadataObjectTagOperations( + model_ns.level(0), model_object, rest_client + ) Review Comment: Attribute name `_model_tag_opearions` looks like a typo ("operations") and is used throughout the class. Renaming it to `_model_tag_operations` would improve readability and avoid propagating the misspelling into future code changes. ########## clients/client-python/tests/unittests/client/test_metadata_object_tag_operations.py: ########## @@ -0,0 +1,188 @@ +# 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. + +import unittest +from unittest.mock import patch + +from gravitino.api.metadata_objects import MetadataObject, MetadataObjects +from gravitino.api.tag import Tag +from gravitino.client.generic_tag import GenericTag +from gravitino.client.metadata_object_tag_operations import MetadataObjectTagOperations +from gravitino.dto.requests.tag_associate_request import TagsAssociateRequest +from gravitino.dto.responses.tag_response import ( + TagListResponse, + TagNamesListResponse, + TagResponse, +) +from gravitino.exceptions.handlers.tag_error_handler import ( + TAG_ERROR_HANDLER, +) +from gravitino.rest.rest_utils import encode_string +from gravitino.utils import HTTPClient +from tests.unittests import mock_base + + +class TestMetadataObjectTagOperations(unittest.TestCase): + REST_CLIENT = HTTPClient("http://localhost:8090";) + METALAKE_NAME = "demo_metalake" + + def test_list_tag_request_path(self) -> None: + metadata_object = MetadataObjects.of( + ["catalog", "schema", "table"], MetadataObject.Type.TABLE + ) + table_tag_request_path = MetadataObjectTagOperations.TAG_REQUEST_PATH.format( + encode_string(TestMetadataObjectTagOperations.METALAKE_NAME), + metadata_object.type().name.lower(), + encode_string(metadata_object.full_name()), + ) + self.assertEqual( + "api/metalakes/demo_metalake/objects/table/catalog.schema.table/tags", + table_tag_request_path, + ) + + def test_get_tag_request_path(self) -> None: + metadata_object = MetadataObjects.of( + ["catalog", "schema", "table"], MetadataObject.Type.TABLE + ) + table_tag_request_path = MetadataObjectTagOperations.TAG_REQUEST_PATH.format( + encode_string(TestMetadataObjectTagOperations.METALAKE_NAME), + metadata_object.type().name.lower(), + encode_string(metadata_object.full_name()), + ) + self.assertEqual( + "api/metalakes/demo_metalake/objects/table/catalog.schema.table/tags/tag_name", + f"{table_tag_request_path}/tag_name", + ) + + def test_get_tag_from_metadata_object(self) -> None: + tag_operations = MetadataObjectTagOperations( + TestMetadataObjectTagOperations.METALAKE_NAME, + MetadataObjects.of( + ["catalog", "schema"], + MetadataObject.Type.SCHEMA, + ), + TestMetadataObjectTagOperations.REST_CLIENT, + ) + + tag = mock_base.build_tag_dto() + resp = TagResponse(0, tag) + json_str = resp.to_json() + mock_resp = mock_base.mock_http_response(json_str) + + with patch( + "gravitino.utils.http_client.HTTPClient.get", + return_value=mock_resp, + ) as mock_get: + retrieved_tag = tag_operations.get_tag(tag.name()) + self.check_tag_equal(tag, retrieved_tag) + mock_get.assert_called_once_with( + "api/metalakes/demo_metalake/objects/schema/catalog.schema/tags/tagA", + params={}, + error_handler=TAG_ERROR_HANDLER, + ) + + def test_list_tags(self) -> None: + tag_operations = MetadataObjectTagOperations( + TestMetadataObjectTagOperations.METALAKE_NAME, + MetadataObjects.of( + ["catalog", "schema", "table"], + MetadataObject.Type.TABLE, + ), + TestMetadataObjectTagOperations.REST_CLIENT, + ) + json_str = TagNamesListResponse(0, ["tagA", "tagB"]).to_json() + mock_resp = mock_base.mock_http_response(json_str) + + with patch( + "gravitino.utils.http_client.HTTPClient.get", + return_value=mock_resp, + ) as mock_get: + tags = tag_operations.list_tags() + + self.assertEqual(["tagA", "tagB"], tags) + mock_get.assert_called_once_with( + "api/metalakes/demo_metalake/objects/table/catalog.schema.table/tags", + params={}, + error_handler=TAG_ERROR_HANDLER, + ) + + def test_list_tags_info(self) -> None: + tag_operations = MetadataObjectTagOperations( + TestMetadataObjectTagOperations.METALAKE_NAME, + MetadataObjects.of( + ["catalog", "schema", "table"], + MetadataObject.Type.TABLE, + ), + TestMetadataObjectTagOperations.REST_CLIENT, + ) + tag1 = mock_base.build_tag_dto() + tag2 = mock_base.build_tag_dto(name="tagB", comment="commentB") + json_str = TagListResponse(0, [tag1, tag2]).to_json() + mock_resp = mock_base.mock_http_response(json_str) + + with patch( + "gravitino.utils.http_client.HTTPClient.get", + return_value=mock_resp, + ) as mock_get: + tags = tag_operations.list_tags_info() + + self.assertEqual(2, len(tags)) + self.assertTrue(all(isinstance(tag, GenericTag) for tag in tags)) + mock_get.assert_called_once_with( + "api/metalakes/demo_metalake/objects/table/catalog.schema.table/tags", + params={"details": "true"}, + error_handler=TAG_ERROR_HANDLER, + ) + + def test_associate_tags(self) -> None: + tag_operations = MetadataObjectTagOperations( + TestMetadataObjectTagOperations.METALAKE_NAME, + MetadataObjects.of( + ["catalog", "schema", "table"], + MetadataObject.Type.TABLE, + ), + TestMetadataObjectTagOperations.REST_CLIENT, + ) + json_str = TagNamesListResponse(0, ["tagA", "tagC"]).to_json() + mock_resp = mock_base.mock_http_response(json_str) + + with patch( + "gravitino.utils.http_client.HTTPClient.post", + return_value=mock_resp, + ) as mock_post: + tags = tag_operations.associate_tags(["tagA"], ["tagB"]) + + self.assertEqual(["tagA", "tagC"], tags) + mock_post.assert_called_once() + call_args = mock_post.call_args + self.assertEqual( + "api/metalakes/demo_metalake/objects/table/catalog.schema.table/tags", + call_args.args[0], + ) + + param = TagsAssociateRequest(["tagA"], ["tagB"]) + + mock_post.assert_called_once_with( + "api/metalakes/demo_metalake/objects/table/catalog.schema.table/tags", + json=param.to_json(), Review Comment: This test currently asserts `HTTPClient.post` is called with `json=param.to_json()`, but `HTTPClient.post` expects the `json` argument to be a request object (it calls `json.to_json()` internally). Once the production code is fixed to pass the request object, update the assertion to expect `json=param` (or assert against the DTO fields) so the test matches the actual HTTP client contract. ```suggestion json=param, ``` -- 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]
