-
Notifications
You must be signed in to change notification settings - Fork 421
fix: Add check table UUID to detect table replacement #2890
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,7 @@ | |
| from pyiceberg.table.sorting import SortField, SortOrder | ||
| from pyiceberg.transforms import IdentityTransform, TruncateTransform | ||
| from pyiceberg.typedef import RecursiveDict | ||
| from pyiceberg.types import StringType | ||
| from pyiceberg.utils.config import Config | ||
|
|
||
| TEST_URI = "https://iceberg-test-catalog/" | ||
|
|
@@ -1100,6 +1101,9 @@ def test_create_staged_table_200( | |
| example_table_metadata_with_no_location: dict[str, Any], | ||
| example_table_metadata_no_snapshot_v1_rest_json: dict[str, Any], | ||
| ) -> None: | ||
| expected_table_uuid = example_table_metadata_with_no_location["metadata"]["table-uuid"] | ||
| example_table_metadata_no_snapshot_v1_rest_json["metadata"]["table-uuid"] = expected_table_uuid | ||
|
|
||
| rest_mock.post( | ||
| f"{TEST_URI}v1/namespaces/fokko/tables", | ||
| json=example_table_metadata_with_no_location, | ||
|
|
@@ -2155,3 +2159,87 @@ def test_view_endpoints_enabled_with_config(self, requests_mock: Mocker) -> None | |
| # View endpoints should be supported when enabled | ||
| catalog._check_endpoint(Capability.V1_LIST_VIEWS) | ||
| catalog._check_endpoint(Capability.V1_DELETE_VIEW) | ||
|
|
||
|
|
||
| def test_table_uuid_check_on_commit(rest_mock: Mocker, example_table_metadata_v2: dict[str, Any]) -> None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a big fan of mocking this out, since I think this should already work as @kevinjqliu pointed out. When performing the update,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea, as the other comment pointed out, this is purely defensive in case the catalog isnt doing the right thing 🤷 I dont like the mocks either, but i dont see another way to test this logic haha |
||
| """Test that UUID mismatch is detected on commit response (matches Java RESTTableOperations behavior).""" | ||
| original_uuid = "9c12d441-03fe-4693-9a96-a0705ddf69c1" | ||
| different_uuid = "550e8400-e29b-41d4-a716-446655440000" | ||
| metadata_location = "s3://warehouse/database/table/metadata.json" | ||
|
|
||
| rest_mock.get( | ||
| f"{TEST_URI}v1/namespaces/namespace/tables/table_name", | ||
| json={ | ||
| "metadata-location": metadata_location, | ||
| "metadata": example_table_metadata_v2, | ||
| "config": {}, | ||
| }, | ||
| status_code=200, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
|
|
||
| catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN) | ||
| table = catalog.load_table(("namespace", "table_name")) | ||
|
|
||
| assert str(table.metadata.table_uuid) == original_uuid | ||
|
|
||
| metadata_with_different_uuid = {**example_table_metadata_v2, "table-uuid": different_uuid} | ||
|
|
||
| rest_mock.post( | ||
| f"{TEST_URI}v1/namespaces/namespace/tables/table_name", | ||
| json={ | ||
| "metadata-location": metadata_location, | ||
| "metadata": metadata_with_different_uuid, | ||
| }, | ||
| status_code=200, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
|
|
||
| with pytest.raises(ValueError) as exc_info: | ||
| table.update_schema().add_column("new_col", StringType()).commit() | ||
|
|
||
| assert "Table UUID does not match" in str(exc_info.value) | ||
| assert f"current={original_uuid}" in str(exc_info.value) | ||
| assert f"refreshed={different_uuid}" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_table_uuid_check_on_refresh(rest_mock: Mocker, example_table_metadata_v2: dict[str, Any]) -> None: | ||
| original_uuid = "9c12d441-03fe-4693-9a96-a0705ddf69c1" | ||
| different_uuid = "550e8400-e29b-41d4-a716-446655440000" | ||
| metadata_location = "s3://warehouse/database/table/metadata.json" | ||
|
|
||
| rest_mock.get( | ||
| f"{TEST_URI}v1/namespaces/namespace/tables/table_name", | ||
| json={ | ||
| "metadata-location": metadata_location, | ||
| "metadata": example_table_metadata_v2, | ||
| "config": {}, | ||
| }, | ||
| status_code=200, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
|
|
||
| catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN) | ||
| table = catalog.load_table(("namespace", "table_name")) | ||
|
|
||
| assert str(table.metadata.table_uuid) == original_uuid | ||
|
|
||
| metadata_with_different_uuid = {**example_table_metadata_v2, "table-uuid": different_uuid} | ||
|
|
||
| rest_mock.get( | ||
| f"{TEST_URI}v1/namespaces/namespace/tables/table_name", | ||
| json={ | ||
| "metadata-location": metadata_location, | ||
| "metadata": metadata_with_different_uuid, | ||
| "config": {}, | ||
| }, | ||
| status_code=200, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
|
|
||
| with pytest.raises(ValueError) as exc_info: | ||
| table.refresh() | ||
|
|
||
| assert "Table UUID does not match" in str(exc_info.value) | ||
| assert f"current={original_uuid}" in str(exc_info.value) | ||
| assert f"refreshed={different_uuid}" in str(exc_info.value) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i feel like this comment is confusing without context
maybe something like ?