-
Notifications
You must be signed in to change notification settings - Fork 127
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
[Fix] Fix deserialization of 401/403 errors #758
Conversation
This PR breaks backwards compatibility for databrickslabs/blueprint downstream. See build logs for more details. Running from downstreams #459 |
This PR breaks backwards compatibility for databrickslabs/ucx downstream. See build logs for more details. Running from downstreams #459 |
import requests | ||
|
||
|
||
class _ErrorDeserializer(abc.ABC): |
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.
Renamed to _ErrorDeserializer
and moved here from parser.py
.
from .mapper import _error_mapper | ||
from .private_link import (_get_private_link_validation_error, | ||
_is_private_link_redirect) | ||
|
||
|
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.
Moved to deserializers.py, and replaced Parser -> Deserializer.
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.
LGTM, just a small typo left.
### New Features and Improvements * Support Models in `dbutils.fs` operations ([#750](#750)). ### Bug Fixes * Do not specify --tenant flag when fetching managed identity access token from the CLI ([#748](#748)). * Fix deserialization of 401/403 errors ([#758](#758)). * Use correct optional typing in `WorkspaceClient` for `mypy` ([#760](#760)).
### New Features and Improvements * Support Models in `dbutils.fs` operations ([#750](#750)). ### Bug Fixes * Do not specify --tenant flag when fetching managed identity access token from the CLI ([#748](#748)). * Fix deserialization of 401/403 errors ([#758](#758)). * Use correct optional typing in `WorkspaceClient` for `mypy` ([#760](#760)).
## Changes databricks#741 introduced a change to how an error message was modified in `ApiClient._perform`. Previously, arguments to the DatabricksError constructor were modified as a dictionary in `_perform`. After that change, `get_api_error` started to return a `DatabricksError` instance whose attributes were modified. The `message` attribute referred to in that change does not exist in the DatabricksError class: there is a `message` constructor parameter, but it is not set as an attribute. This PR refactors the error handling logic slightly to restore the original behavior. In doing this, we decouple all error-parsing and customizing logic out of ApiClient. This also sets us up to allow for further extension of error parsing and customization in the future, a feature that I have seen present in other SDKs. Fixes databricks#755. ## Tests <!-- How is this tested? Please see the checklist below and also describe any other relevant tests --> - [ ] `make test` run locally - [ ] `make fmt` applied - [ ] relevant integration tests applied
### New Features and Improvements * Support Models in `dbutils.fs` operations ([databricks#750](databricks#750)). ### Bug Fixes * Do not specify --tenant flag when fetching managed identity access token from the CLI ([databricks#748](databricks#748)). * Fix deserialization of 401/403 errors ([databricks#758](databricks#758)). * Use correct optional typing in `WorkspaceClient` for `mypy` ([databricks#760](databricks#760)).
Changes
#741 introduced a change to how an error message was modified in
ApiClient._perform
. Previously, arguments to the DatabricksError constructor were modified as a dictionary in_perform
. After that change,get_api_error
started to return aDatabricksError
instance whose attributes were modified. Themessage
attribute referred to in that change does not exist in the DatabricksError class: there is amessage
constructor parameter, but it is not set as an attribute.This PR refactors the error handling logic slightly to restore the original behavior. In doing this, we decouple all error-parsing and customizing logic out of ApiClient. This also sets us up to allow for further extension of error parsing and customization in the future, a feature that I have seen present in other SDKs.
Fixes #755.
Tests
make test
run locallymake fmt
applied