Skip to content
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

Merged
merged 9 commits into from
Sep 13, 2024
Merged

[Fix] Fix deserialization of 401/403 errors #758

merged 9 commits into from
Sep 13, 2024

Conversation

mgyucht
Copy link
Contributor

@mgyucht mgyucht commented Sep 12, 2024

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 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 #755.

Tests

  • make test run locally
  • make fmt applied
  • relevant integration tests applied

Copy link

This PR breaks backwards compatibility for databrickslabs/blueprint downstream. See build logs for more details.

Running from downstreams #459

Copy link

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):
Copy link
Contributor Author

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)


Copy link
Contributor Author

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.

databricks/sdk/errors/parser.py Outdated Show resolved Hide resolved
databricks/sdk/core.py Outdated Show resolved Hide resolved
databricks/sdk/errors/customizer.py Outdated Show resolved Hide resolved
databricks/sdk/errors/customizer.py Outdated Show resolved Hide resolved
databricks/sdk/errors/customizer.py Outdated Show resolved Hide resolved
databricks/sdk/errors/parser.py Outdated Show resolved Hide resolved
databricks/sdk/errors/deserializer.py Outdated Show resolved Hide resolved
databricks/sdk/errors/deserializer.py Outdated Show resolved Hide resolved
Copy link
Contributor

@renaudhartert-db renaudhartert-db left a 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.

databricks/sdk/errors/customizer.py Outdated Show resolved Hide resolved
@mgyucht mgyucht enabled auto-merge September 13, 2024 13:15
@mgyucht mgyucht added this pull request to the merge queue Sep 13, 2024
Merged via the queue into main with commit f06bb27 Sep 13, 2024
14 checks passed
@mgyucht mgyucht deleted the issue-755 branch September 13, 2024 13:22
tanmay-db added a commit that referenced this pull request Sep 16, 2024
### 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)).
github-merge-queue bot pushed a commit that referenced this pull request Sep 17, 2024
### 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)).
aravind-segu pushed a commit to aravind-segu/databricks-sdk-py that referenced this pull request Sep 18, 2024
## 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
aravind-segu pushed a commit to aravind-segu/databricks-sdk-py that referenced this pull request Sep 18, 2024
### 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)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ISSUE] Regression in PermissionDefined error handling
3 participants