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

Raise HTTP errors #125

Merged
merged 2 commits into from
Nov 28, 2023
Merged

Raise HTTP errors #125

merged 2 commits into from
Nov 28, 2023

Conversation

bjchambers
Copy link
Contributor

This fixes #124.

Copy link
Collaborator

@erichare erichare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This is awesome and i think we do need this. The only caveat that might be worth looking into is, the API sometimes returns an "error" (as a valid JSON response). I think in all these cases the response code is in the 200 range, and hence this would be just fine. But i know that the error sometimes is parsed in order to determine the next steps. Particularly in upsert() - the tests pass though, and i think this would only impact a true API response error code, so i like this a lot :)

@hemidactylus i'm still out until wednesday, but was intrigued by this enough to at least quickly test and comment haha. Do you have any concerns?

Also, i believe the failing tests are just a result of the repo secrets - the tests pass for me locally.

@hemidactylus
Copy link
Collaborator

First, thank you @bjchambers for spotting and improving the behaviour on errors!

I ran extensive testing with the patched code and it all LGTM - including the upsert (the reason being that "errors" such as document already exist is not an HTTP error in these API, rather a legitimate returned response the caller has the responsibility to check. So we're good).

So, TLDR = LGTM.

But if you want to know more (@erichare , please do) read on, since I pushed further changes (mostly the tests) to this PR to bring a flawless test suite to master, including the Ops tests (which we seldom run and not in the CI, but still.)

Enabling the raise_for_status is all good and a welcome thing to do, but it uncovered some trouble in the Ops test which I improved.

  • The DB creation test was silently failing in the "delete the newly-created db" part. I took that out [1]
  • The create keyspace test was meaningless, since it was targeting a nonexisting DB and then not checking the obvious call failure that came with that. Fixed.
  • More importantly, while the JSON API always return a top-level dict, the DevOps is more heterogeneous. Some calls, such as get_databases, return a list. This god in the way with typing and made me add some stuff to correctly handle that. But I just patched so that it always works, with a few casts around to satisfy mypy. More work will be needed in the ops sector. [2]

[1] = on the roadmap we should have a more thorough Ops testing, which is not a trivial thing. There, I'd like to test db deletion as well, probably with a very long-running test that polls and waits until the DB is deletable (as long as it's being created you can't). And other similar operations.
[2] = In particular, I added a union type covering lists as well, adapted the docstring of the get_databases method, and the corresponding code. I think the right (and type-sensible way) would be to have two top-level return types, the dicty and the listy, and have two separate _json_ops_request, one per type. This would "document in code" what the Ops API does in the proper way.

Copy link
Collaborator

@erichare erichare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hemidactylus Very good catch on the ops stuff. I'm approving because, even though the proper typing etc is something we need to do, the ops client itself is already in kind of a, shall we say, less than ideal state, and i think this gets us further and raising for errors is such an important change (so the user gets more feedback about the result)

We still have the separate issue discussed with Enrico about errors that produce a successful HTTP response, but this doesn't impact it.

I say let's merge. At some point i'll bump the version to 0.6.3 and start a changelog (no plans to push to pypi of course) but just so we can eventually push a pre-release or just install from git easily and know we're on the latest version.

Thanks @bjchambers !!

@erichare erichare merged commit 31eee92 into datastax:master Nov 28, 2023
1 check failed
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.

Errors in a request hidden by JSON decoding error
3 participants