[Cherry-pick to branch-1.2] [#10753] fix(client-python): use PUT method for alter_tag API call (#10754)#10892
[Cherry-pick to branch-1.2] [#10753] fix(client-python): use PUT method for alter_tag API call (#10754)#10892diqiu50 wants to merge 1 commit intobranch-1.2from
Conversation
…od for alter_tag API call (#10754)
There was a problem hiding this comment.
Pull request overview
Cherry-pick to branch-1.2 to fix the Python client’s alter_tag behavior (intended to use HTTP PUT) and add unit tests around Tag APIs.
Changes:
- Implement
GravitinoMetalake.alter_tag()to issue a PUT request to the tag endpoint. - Add a new
test_tag_api.pyunit test suite covering client- and REST-level tag operations (including alter-tag via PUT).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| clients/client-python/gravitino/client/gravitino_metalake.py | Replaces alter_tag stub with a PUT-based implementation and request-building logic. |
| clients/client-python/tests/unittests/test_tag_api.py | Adds unit tests for tag operations, including asserting HTTPClient.put is used for alter-tag. |
| with patch( | ||
| "gravitino.utils.http_client.HTTPClient.delete", | ||
| return_value=mock_resp, | ||
| ): | ||
| is_dropped = client.delete_tag("tag1") | ||
| self.assertTrue(is_dropped) |
There was a problem hiding this 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.
| Precondition.check_argument( | ||
| StringUtils.is_not_blank(tag_name), |
There was a problem hiding this 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.
| Precondition.check_argument( | |
| StringUtils.is_not_blank(tag_name), | |
| Precondition.check_string_not_empty( | |
| tag_name, |
| 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, | ||
| ) | ||
|
|
There was a problem hiding this 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.
| 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, | |
| ) | |
| raise NotImplementedError( | |
| "alter_tag is not supported in this branch because tag update request " | |
| "DTOs and converters are not implemented." | |
| ) |
| 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]) |
There was a problem hiding this 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.
Cherry-pick Information:
branch-1.2