-
Notifications
You must be signed in to change notification settings - Fork 498
Add rename_view to REST Catalog #2149
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
1c4ddef
f67788c
e81f9eb
df809ab
ac2e809
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 |
|---|---|---|
|
|
@@ -158,6 +158,7 @@ class Endpoints: | |
| register_view: str = "namespaces/{namespace}/register-view" | ||
| drop_view: str = "namespaces/{namespace}/views/{view}" | ||
| view_exists: str = "namespaces/{namespace}/views/{view}" | ||
| rename_view: str = "views/rename" | ||
| plan_table_scan: str = "namespaces/{namespace}/tables/{table}/plan" | ||
| fetch_scan_tasks: str = "namespaces/{namespace}/tables/{table}/tasks" | ||
|
|
||
|
|
@@ -187,6 +188,7 @@ class Capability: | |
| V1_VIEW_EXISTS = Endpoint(http_method=HttpMethod.HEAD, path=f"{API_PREFIX}/{Endpoints.view_exists}") | ||
| V1_REGISTER_VIEW = Endpoint(http_method=HttpMethod.POST, path=f"{API_PREFIX}/{Endpoints.register_view}") | ||
| V1_DELETE_VIEW = Endpoint(http_method=HttpMethod.DELETE, path=f"{API_PREFIX}/{Endpoints.drop_view}") | ||
| V1_RENAME_VIEW = Endpoint(http_method=HttpMethod.POST, path=f"{API_PREFIX}/{Endpoints.rename_view}") | ||
| V1_SUBMIT_TABLE_SCAN_PLAN = Endpoint(http_method=HttpMethod.POST, path=f"{API_PREFIX}/{Endpoints.plan_table_scan}") | ||
| V1_TABLE_SCAN_PLAN_TASKS = Endpoint(http_method=HttpMethod.POST, path=f"{API_PREFIX}/{Endpoints.fetch_scan_tasks}") | ||
|
|
||
|
|
@@ -216,6 +218,7 @@ class Capability: | |
| Capability.V1_LIST_VIEWS, | ||
| Capability.V1_LOAD_VIEW, | ||
| Capability.V1_DELETE_VIEW, | ||
| Capability.V1_RENAME_VIEW, | ||
| ) | ||
| ) | ||
|
|
||
|
|
@@ -1471,6 +1474,28 @@ def drop_view(self, identifier: str | Identifier) -> None: | |
| except HTTPError as exc: | ||
| _handle_non_200_response(exc, {404: NoSuchViewError}) | ||
|
|
||
| @retry(**_RETRY_ARGS) | ||
| def rename_view(self, from_identifier: str | Identifier, to_identifier: str | Identifier) -> None: | ||
|
Member
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. Is |
||
| payload = { | ||
| "source": self._split_identifier_for_json(from_identifier), | ||
| "destination": self._split_identifier_for_json(to_identifier), | ||
| } | ||
|
|
||
| # Ensure source and destination namespaces exist before rename. | ||
| source_namespace = self._split_identifier_for_json(from_identifier)["namespace"] | ||
| dest_namespace = self._split_identifier_for_path(to_identifier)["namespace"] | ||
|
Member
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. Why do we use |
||
|
|
||
| if not self.namespace_exists(source_namespace): | ||
| raise NoSuchNamespaceError(f"Source namespace does not exist: {source_namespace}") | ||
| if not self.namespace_exists(dest_namespace): | ||
| raise NoSuchNamespaceError(f"Destination namespace does not exist: {dest_namespace}") | ||
|
|
||
| response = self._session.post(self.url(Endpoints.rename_view), json=payload) | ||
| try: | ||
| response.raise_for_status() | ||
| except HTTPError as exc: | ||
| _handle_non_200_response(exc, {404: NoSuchViewError, 409: ViewAlreadyExistsError}) | ||
|
|
||
| def close(self) -> None: | ||
| """Close the catalog and release Session connection adapters. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3167,3 +3167,122 @@ def test_load_table_without_storage_credentials( | |
| ) | ||
| assert actual.metadata.model_dump() == expected.metadata.model_dump() | ||
| assert actual == expected | ||
|
|
||
|
Member
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. Can we add an integration test? I've added initial tests in #3406. |
||
|
|
||
| def test_rename_view_204(rest_mock: Mocker) -> None: | ||
| from_identifier = ("some_namespace", "old_view") | ||
| to_identifier = ("some_namespace", "new_view") | ||
| rest_mock.head( | ||
| f"{TEST_URI}v1/namespaces/some_namespace", | ||
| status_code=200, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
| rest_mock.post( | ||
| f"{TEST_URI}v1/views/rename", | ||
| json={ | ||
| "source": {"namespace": ["some_namespace"], "name": "old_view"}, | ||
| "destination": {"namespace": ["some_namespace"], "name": "new_view"}, | ||
| }, | ||
| status_code=204, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
| catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN) | ||
| catalog.rename_view(from_identifier, to_identifier) | ||
| assert ( | ||
| rest_mock.last_request.text == """{"source": {"namespace": ["some_namespace"], "name": "old_view"}, """ | ||
| """"destination": {"namespace": ["some_namespace"], "name": "new_view"}}""" | ||
| ) | ||
|
|
||
|
|
||
| def test_rename_view_404(rest_mock: Mocker) -> None: | ||
| from_identifier = ("some_namespace", "non_existent_view") | ||
| to_identifier = ("some_namespace", "new_view") | ||
| rest_mock.head( | ||
| f"{TEST_URI}v1/namespaces/some_namespace", | ||
| status_code=200, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
| rest_mock.post( | ||
| f"{TEST_URI}v1/views/rename", | ||
| json={ | ||
| "error": { | ||
| "message": "View does not exist: some_namespace.non_existent_view", | ||
| "type": "NoSuchViewException", | ||
| "code": 404, | ||
| } | ||
| }, | ||
| status_code=404, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
| catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN) | ||
| with pytest.raises(NoSuchViewError) as exc_info: | ||
| catalog.rename_view(from_identifier, to_identifier) | ||
| assert "View does not exist: some_namespace.non_existent_view" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_rename_view_409(rest_mock: Mocker) -> None: | ||
| from_identifier = ("some_namespace", "old_view") | ||
| to_identifier = ("some_namespace", "existing_view") | ||
| rest_mock.head( | ||
| f"{TEST_URI}v1/namespaces/some_namespace", | ||
| status_code=200, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
| rest_mock.post( | ||
| f"{TEST_URI}v1/views/rename", | ||
| json={ | ||
| "error": { | ||
| "message": "View already exists: some_namespace.existing_view", | ||
| "type": "ViewAlreadyExistsException", | ||
| "code": 409, | ||
| } | ||
| }, | ||
| status_code=409, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
| catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN) | ||
| with pytest.raises(ViewAlreadyExistsError) as exc_info: | ||
| catalog.rename_view(from_identifier, to_identifier) | ||
| assert "View already exists: some_namespace.existing_view" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_rename_view_source_namespace_does_not_exist(rest_mock: Mocker) -> None: | ||
| from_identifier = ("non_existent_namespace", "old_view") | ||
| to_identifier = ("some_namespace", "new_view") | ||
|
|
||
| rest_mock.head( | ||
| f"{TEST_URI}v1/namespaces/non_existent_namespace", | ||
| status_code=404, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
| rest_mock.head( | ||
| f"{TEST_URI}v1/namespaces/some_namespace", | ||
| status_code=200, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
|
Comment on lines
+3258
to
+3262
Member
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. nit: This mock seems redundant because the catalog raises an exception before verifying the target namespace. No requested change, I suppose you put it intentionally. |
||
|
|
||
| catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN) | ||
| with pytest.raises(NoSuchNamespaceError) as exc_info: | ||
| catalog.rename_view(from_identifier, to_identifier) | ||
| assert "Source namespace does not exist: ('non_existent_namespace',)" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_rename_view_destination_namespace_does_not_exist(rest_mock: Mocker) -> None: | ||
| from_identifier = ("some_namespace", "old_view") | ||
| to_identifier = ("non_existent_namespace", "new_view") | ||
|
|
||
| rest_mock.head( | ||
| f"{TEST_URI}v1/namespaces/some_namespace", | ||
| status_code=200, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
| rest_mock.head( | ||
| f"{TEST_URI}v1/namespaces/non_existent_namespace", | ||
| status_code=404, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
|
|
||
| catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN) | ||
| with pytest.raises(NoSuchNamespaceError) as exc_info: | ||
| catalog.rename_view(from_identifier, to_identifier) | ||
| assert "Destination namespace does not exist: non_existent_namespace" 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.
Can we add
ViewAlreadyExistsError?