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

DOCFIX or FEATURE: Status code returns for each request are undocumented #54

Open
ihs-nick opened this issue Nov 24, 2020 · 5 comments
Open

Comments

@ihs-nick
Copy link

Right now it is unclear exactly what each non 200 status code means precisely for each potential client method. For instance, if I try to find a key in a non existent key/value store will that give me a different response than if the key is missing in a existing key/value store? I see two (not necessarily mutually exclusive) options to make this more clear.

  1. Documentation Fix:
    For each method (or set of methods if they share status codes exactly), document the precise meanings of all non-200 status codes (including 204). This will allow anyone to then use "raise for status" properly and handle the codes after. Also, as a side note, it seems that raise_for_status should be not_raise_for_status, since the arguments are what are allowed.

  2. Custom Exceptions Feature:
    For all scenarios like ContainerNotFound, KeyValueTableNotFound, or KeyValueKeyNotFound allow the dataplane client to specify an optional argument - something like raise_exceptions: bool - to allow users to choose if they want status codes or for a meaningful exception to be raised. Then users can consult that documentation and use a try catch block around client calls. I personally favor this from a python library as opposed to status codes which can be hard to interpret and contextual.

@ihs-nick ihs-nick changed the title DOCFIX or FEATURE: Status code returns for each requests are undocumented DOCFIX or FEATURE: Status code returns for each request are undocumented Nov 24, 2020
@pavius
Copy link
Contributor

pavius commented Nov 24, 2020

Thanks for the feedback! Agreed on the need for all the points mentioned here.

(1) is something I'll get to soon - will alias raise_for_status because that indeed seems like a mistake. Can't change it because it'll break backwards compatibility. (2) is something that will require some more looking into. I'll mention this issue as the PRs are merged.

@ihs-nick
Copy link
Author

Hi Pavius!

Thanks for getting back so quickly. I'm glad that we agree on these, and that you will be prioritizing the documentation. I understand that you won't be able to change it for now - just wanted to point it out sooner rather than later. With (2), I look forward to seeing what you come up with. I do believe that you could cover many of the problems with a small number of exceptions each with a meaningful name and good docstring.

I appreciate your time and attention on this!

@pavius
Copy link
Contributor

pavius commented Nov 29, 2020

I dug into this along with folks from the data layer. For the time being there is no one place that maps a specific API's underlying error to that of an HTTP status code. There is a generic conversion between data layer errors to HTTP errors (e.g. EBADF is mapped to HTTP_NOT_FOUND, but so are EISDIR and ENOTDIR) - but this isn't why this issue exists. You need a high level of mapping of "if I call API X, such and such error can happen and it'll appear as status code Y".

Practically the way I'll approach this is with incremental experiment rather than just converting some documentation I thought existed.

@ihs-nick
Copy link
Author

ihs-nick commented Dec 3, 2020

Will it be possible to disentangle the different "missingness" if the status code (e.g. 404) is overloaded with multiple meanings? Does the data layer add anything to the header or body with the original errors?

@ihs-nick
Copy link
Author

@pavius, are there any updates on this?

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

No branches or pull requests

2 participants