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

Gracefully handle null JSON responses from API #641

Closed
wants to merge 3 commits into from

Conversation

sodle-splunk
Copy link
Contributor

@sodle-splunk sodle-splunk commented May 10, 2024

Changes

For some DELETE requests, notably when deleting an Alert, the API returns a JSON response containing only null. This is not seen as an empty response, because the content length is greater than zero.

When calling WorkspaceClient.alerts.delete(alertId), this results in the alert being deleted as expected. However, a TypeError is raised when trying to unpack the response at databricks/sdk/core.py:170.

To fix this, we will null-check the decoded JSON before attempting to unpack it. If it is None, we will simply return the response dict like we do for empty responses.

Tests

  • make test run locally
  • make fmt applied
    • This seems to have modified far more lines than expected.
    • Actual code changes are here
    • New test is here
  • relevant integration tests applied
    • Added a test which shows the null response received when calling DELETE /api/2.0/preview/sql/alerts/alertid. This test fails before applying my fix, and passes with the fix in place.

@mgyucht
Copy link
Contributor

mgyucht commented May 15, 2024

I believe @tanmay-db was working on a fix for this case: #579.

@tanmay-db
Copy link
Contributor

Hi @sodle-splunk, thanks for the PR. The formatting changes might be due to different versions of yapf being used, I ran make fmt on main branch and there were no diffs.

The test looks good and I tried to revert the make fmt commit so we could merge this PR but couldn't since I don't have the permission to push to your fork:

To github.com:sodle-splunk/databricks-sdk-py.git
 ! [remote rejected]   head -> sodle-splunk/main (permission denied)
error: failed to push some refs to 'github.com:sodle-splunk/databricks-sdk-py.git'

I have cherry picked the commits and added them to the PR: #579

@sodle-splunk
Copy link
Contributor Author

Ah, I could have sworn I'd allowed maintainer commits on this PR. Oh well.

Thanks for cherry-picking. I'll close this one.

github-merge-queue bot pushed a commit that referenced this pull request May 16, 2024
## Changes
<!-- Summary of your changes that are easy to understand -->
Some delete end points return null instead of empty body, we fix that in
SDK Client since the APIs are not available publicly and support isn't
available for them yet.

## Tests
<!-- 
How is this tested? Please see the checklist below and also describe any
other relevant tests
-->
Thanks to @sodle-splunk for contributing the unit test
(#641)
- [x] `make test` run locally
- [x] `make fmt` applied
- [ ] relevant integration tests applied

---------

Co-authored-by: Scott Odle <sodle@splunk.com>
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.

None yet

4 participants