This is an automated email from the ASF dual-hosted git repository.
jerryshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 38de02edaf [#12324] fix(client-python): serialize property field in
RemoveCatalogPropertyRequest (#12325)
38de02edaf is described below
commit 38de02edafd0c0ab0aa777ba00dedad973115091
Author: Phước <[email protected]>
AuthorDate: Wed Aug 12 12:49:59 2026 +0700
[#12324] fix(client-python): serialize property field in
RemoveCatalogPropertyRequest (#12325)
### What changes were proposed in this pull request?
`RemoveCatalogPropertyRequest` in
`clients/client-python/gravitino/dto/requests/catalog_update_request.py`
was serialized without the `property` field, so
`GravitinoClient.alter_catalog(...)` with a
`CatalogChange.remove_property(...)` always failed server-side with:
```
IllegalArgumentException: "property" field is required and cannot be empty
```
The class had two compounding defects vs. its siblings: it was missing
the `@dataclass` decorator, and its field was declared as `property:
Optional[str] = None` while `__init__` set `self._property` (a different
attribute). It now matches the sibling pattern
(`RemoveTablePropertyRequest`, `RemoveSchemaPropertyRequest`):
```python
@dataclass
class RemoveCatalogPropertyRequest(CatalogUpdateRequestBase):
_property: Optional[str] = field(
default=None, metadata=config(field_name="property")
)
```
Tests added:
- `tests/unittests/dto/requests/test_catalog_update_request.py` —
serialize/validate coverage for all four catalog update request types
(catalog update requests previously had no unit-test coverage, unlike
table/tag/view/schema).
- `test_alter_catalog_remove_property` integration test in
`tests/integration/test_catalog.py` — sets then removes a catalog
property end-to-end.
### Why are the changes needed?
Any `alter_catalog` call that needs to remove a property is broken (e.g.
config-driven provisioners reconciling catalog properties).
Fix: #12324
### Does this PR introduce _any_ user-facing change?
No. It fixes existing broken behavior for removing catalog properties
via the Python SDK; no API or property-key change.
### How was this patch tested?
- New unit test `test_remove_catalog_property_request_serialize` was red
before the fix (`{"@type": "removeProperty"}`) and green after
(`{"@type": "removeProperty", "property": "prop1"}`).
- Full `tests/unittests/dto/requests/` suite: 91 passed.
- `ruff format --check` passes on changed files.
- Integration test `test_alter_catalog_remove_property` verifies
set+remove end-to-end (requires a running Gravitino server via
`GRAVITINO_HOME`).
Co-authored-by: phuocho <[email protected]>
---
.../dto/requests/catalog_update_request.py | 5 +-
.../tests/integration/test_catalog.py | 20 ++++
.../dto/requests/test_catalog_update_request.py | 108 +++++++++++++++++++++
3 files changed, 132 insertions(+), 1 deletion(-)
diff --git
a/clients/client-python/gravitino/dto/requests/catalog_update_request.py
b/clients/client-python/gravitino/dto/requests/catalog_update_request.py
index 1ea4d9953c..b4ba71c546 100644
--- a/clients/client-python/gravitino/dto/requests/catalog_update_request.py
+++ b/clients/client-python/gravitino/dto/requests/catalog_update_request.py
@@ -105,10 +105,13 @@ class CatalogUpdateRequest:
if not self._value:
raise ValueError('"value" field is required and cannot be
empty')
+ @dataclass
class RemoveCatalogPropertyRequest(CatalogUpdateRequestBase):
"""Request to remove a property from a catalog."""
- property: Optional[str] = None
+ _property: Optional[str] = field(
+ default=None, metadata=config(field_name="property")
+ )
"""The property to remove."""
def __init__(self, catalog_property: str):
diff --git a/clients/client-python/tests/integration/test_catalog.py
b/clients/client-python/tests/integration/test_catalog.py
index d99acb8e84..44464ec7fe 100644
--- a/clients/client-python/tests/integration/test_catalog.py
+++ b/clients/client-python/tests/integration/test_catalog.py
@@ -133,6 +133,26 @@ class TestCatalog(MetalakeTestMixin, IntegrationTestEnv):
)
self.catalog_name = self.catalog_name + "_new"
+ def test_alter_catalog_remove_property(self):
+ self.create_catalog(self.catalog_name)
+
+ property_key = "catalog_remove_property_key"
+ property_value = "catalog_remove_property_value"
+
+ # Set a custom property first so that there is something to remove.
+ catalog = self.gravitino_client.alter_catalog(
+ self.catalog_name,
+ CatalogChange.set_property(property_key, property_value),
+ )
+ self.assertEqual(property_value,
catalog.properties().get(property_key))
+
+ # Remove the property and assert it is gone from the catalog.
+ catalog = self.gravitino_client.alter_catalog(
+ self.catalog_name,
+ CatalogChange.remove_property(property_key),
+ )
+ self.assertNotIn(property_key, catalog.properties())
+
def test_drop_catalog(self):
self.create_catalog(self.catalog_name)
self.gravitino_client.disable_catalog(self.catalog_name)
diff --git
a/clients/client-python/tests/unittests/dto/requests/test_catalog_update_request.py
b/clients/client-python/tests/unittests/dto/requests/test_catalog_update_request.py
new file mode 100644
index 0000000000..afe16e5cc1
--- /dev/null
+++
b/clients/client-python/tests/unittests/dto/requests/test_catalog_update_request.py
@@ -0,0 +1,108 @@
+# 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 json as _json
+import unittest
+
+from gravitino.api.catalog_change import CatalogChange
+from gravitino.dto.requests.catalog_update_request import CatalogUpdateRequest
+
+
+class TestCatalogUpdateRequest(unittest.TestCase):
+ def test_rename_catalog_request_validate(self) -> None:
+ invalid_request = CatalogUpdateRequest.RenameCatalogRequest("")
+
+ with self.assertRaises(ValueError):
+ invalid_request.validate()
+
+ def test_rename_catalog_request_serialize(self) -> None:
+ request = CatalogUpdateRequest.RenameCatalogRequest("newCatalog")
+ json_str = _json.dumps(
+ {
+ "@type": "rename",
+ "newName": "newCatalog",
+ },
+ ensure_ascii=False,
+ )
+
+ self.assertEqual(json_str, request.to_json())
+
+ def test_update_catalog_comment_request_serialize(self) -> None:
+ request = CatalogUpdateRequest.UpdateCatalogCommentRequest("new
comment")
+ json_str = _json.dumps(
+ {
+ "@type": "updateComment",
+ "newComment": "new comment",
+ },
+ ensure_ascii=False,
+ )
+
+ self.assertEqual(json_str, request.to_json())
+
+ def test_set_catalog_property_request_validate(self) -> None:
+ invalid_request1 = CatalogUpdateRequest.SetCatalogPropertyRequest("",
"value")
+ invalid_request2 =
CatalogUpdateRequest.SetCatalogPropertyRequest("key", "")
+
+ with self.assertRaises(ValueError):
+ invalid_request1.validate()
+
+ with self.assertRaises(ValueError):
+ invalid_request2.validate()
+
+ def test_set_catalog_property_request_serialize(self) -> None:
+ request = CatalogUpdateRequest.SetCatalogPropertyRequest("key",
"value1")
+ json_str = _json.dumps(
+ {
+ "@type": "setProperty",
+ "property": "key",
+ "value": "value1",
+ },
+ ensure_ascii=False,
+ )
+
+ self.assertEqual(json_str, request.to_json())
+
+ def test_remove_catalog_property_request_validate(self) -> None:
+ invalid_request = CatalogUpdateRequest.RemoveCatalogPropertyRequest("")
+
+ with self.assertRaises(ValueError):
+ invalid_request.validate()
+
+ # A non-empty property must pass validation.
+ valid_request =
CatalogUpdateRequest.RemoveCatalogPropertyRequest("key")
+ valid_request.validate()
+
+ def test_remove_catalog_property_request_serialize(self) -> None:
+ request = CatalogUpdateRequest.RemoveCatalogPropertyRequest("prop1")
+ json_str = _json.dumps(
+ {
+ "@type": "removeProperty",
+ "property": "prop1",
+ },
+ ensure_ascii=False,
+ )
+
+ self.assertEqual(json_str, request.to_json())
+
+ def test_remove_catalog_property_request_catalog_change(self) -> None:
+ request = CatalogUpdateRequest.RemoveCatalogPropertyRequest("prop1")
+
+ self.assertIsInstance(request.catalog_change(),
CatalogChange.RemoveProperty)
+
+
+if __name__ == "__main__":
+ unittest.main()
