Add support for registering views#3288
Conversation
| try: | ||
| response.raise_for_status() | ||
| except HTTPError as exc: | ||
| _handle_non_200_response(exc, {409: ViewAlreadyExistsError}) |
There was a problem hiding this comment.
Wonder if here we can catch a TableAlreadyExists if there is a table already registered with the same identifier?
There was a problem hiding this comment.
I agree with this, this behavior is seen in the java impl:
This is also what the rest spec defines
Following your implementation you're making a register view requests w/o the existence checks which makes it hard to tell if the 409 came from table or view.
There was a problem hiding this comment.
Wonder if here we can catch a TableAlreadyExists if there is a table already registered with the same identifier?
The ViewAlreadyExistsError in the YAML file is just an example. The actual exception type may differ in each REST catalog implementation.
If we want to distinguish between tables and views, we should use self.table_exists instead of relying on an exception-driven approach, as Drew already said.
| try: | ||
| response.raise_for_status() | ||
| except HTTPError as exc: | ||
| _handle_non_200_response(exc, {409: ViewAlreadyExistsError}) |
There was a problem hiding this comment.
I agree with this, this behavior is seen in the java impl:
This is also what the rest spec defines
Following your implementation you're making a register view requests w/o the existence checks which makes it hard to tell if the 409 came from table or view.
| @retry(**_RETRY_ARGS) | ||
| def register_view(self, identifier: str | Identifier, metadata_location: str) -> View: | ||
| self._check_endpoint(Capability.V1_REGISTER_VIEW) | ||
| namespace_and_view = self._split_identifier_for_path(identifier) |
There was a problem hiding this comment.
self._split_identifier_for_path(identifier, IdentifierKind.VIEW) here?
Then you can do:
namespace_and_view = self._split_identifier_for_path(identifier, IdentifierKind.VIEW)
request = RegisterViewRequest(name=namespace_and_view["view"], metadata_location=metadata_location)
| identifier=("default", "registered_view"), | ||
| metadata=ViewMetadata(**example_view_metadata_rest_json["metadata"]), | ||
| ) | ||
| assert actual.metadata.model_dump() == expected.metadata.model_dump() |
There was a problem hiding this comment.
You can follow the test from test_create_view_200 and just do a assert actual == expected since the __eq__ already passes
|
Does the REST fixture support register_view? An integration test on this would be great. |
|
@rambleraptor No, the current test target 1.10.x doesn't support the endpoint. |
ebyhr
force-pushed
the
ebi/register_view
branch
2 times, most recently
from
cd7d26a to
f389b33
Compare
ebyhr
force-pushed
the
ebi/register_view
branch
from
f389b33 to
af7a73c
Compare
5 participants
Rationale for this change
Are these changes tested?
tests/catalog/test_rest.py contains new tests.
Are there any user-facing changes?
This PR adds a new
register_viewmethod tocatalog.