Fix and refactor error handling in rest catalog #2404
Conversation
|
more of a meta question but I wonder if we could auto generate some testing based off of the openapi spec upstream? |
geruh
left a comment
geruh
left a comment
There was a problem hiding this comment.
Overall, this looks good to me!!
There is a lot of refactoring here, and it centralizes all of the error handling into one place, which makes it easier to add new exceptions as they come from upstream. As we can see with this ViewAlreadyExistsError you added.
If there isn't a good way to test with the current rest catalog tests, maybe it would be good to add some additional unit tests. For example, NamespaceNotEmpty is properly handled instead of throwing a BadRequestError Or same for like the table and namespace already exists handling.
WDYT
| assert "Table does not exist" in str(e.value) | ||
|
|
||
|
|
||
| def test_load_table_404_non_existent_namespace(rest_mock: Mocker) -> None: |
There was a problem hiding this comment.
The mocked tests predate the time when we didn't have the IcebergTestFixtures Docker image. I think it would be best to test against the Docker image instead in the integration tests.
|
Thanks @gabeiglio for working on this, and thanks @geruh for bringing this back up in my mailbox :) I agree with what Drew is saying, but in general, I think this is a great improvement 👍 Could you follow up, so we can get this in? |
gabeiglio
force-pushed
the
error-handlers
branch
from
b922e1f to
acfafb5
Compare
geruh
left a comment
geruh
left a comment
There was a problem hiding this comment.
This looks good to me! Just have the one comment below
| ) | ||
|
|
||
| with pytest.raises(NoSuchNamespaceError) as e: | ||
| RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN).drop_table(("fokko", "does_not_exists")) |
geruh
left a comment
geruh
left a comment
There was a problem hiding this comment.
Awesome thanks for this @gabeiglio!
|
@Fokko bringing this back up to your mailbox :) |
4 participants
Rationale for this change
Error handling differed with the Java implementation in the following:
NoSuchNamespaceErrorif the namespace does not exists (same as Java)ViewAlreadyExistsErrorfor completeness even though we don't support creating views atmAre these changes tested?
Added two unit tests for testing the exception thrown between NoSuchNamespaceError and NoSuchTableError
Are there any user-facing changes?
No