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

Server sends 404 for request with malformed parameters #376

Open
hlapp opened this issue Jan 28, 2021 · 8 comments
Open

Server sends 404 for request with malformed parameters #376

hlapp opened this issue Jan 28, 2021 · 8 comments
Labels

Comments

@hlapp
Copy link
Member

hlapp commented Jan 28, 2021

curl -D - -X GET "https://kb.phenoscape.org/api/taxon?foo=http%3A%2F%2Fpurl.obolibrary.org%2Fobo%2FVTO_9020379" -H "accept: application/json"
HTTP/1.1 404 Not Found
Date: Thu, 28 Jan 2021 00:01:25 GMT
Server: akka-http/10.1.11
Content-Type: text/plain; charset=UTF-8
Content-Length: 49

Request is missing required query parameter 'iri'

This shouldn't be a 404 but a 400, right?

@balhoff
Copy link
Member

balhoff commented Feb 1, 2021

I can find arguments on the web for both 400 and 404. Personally I think 404 is correct here. I would return 400 if the query argument could not be parsed to the correct format. In this case parameter names are part of the URL location and there is none that matches.

We can try to change, but the current behavior is automatically provided by the routing API.

@hlapp
Copy link
Member Author

hlapp commented Feb 1, 2021

Well, I think we need to decide on a succinct and precise definition of what 404 Not Found means for the KB. I would say there are big advantages if it means one and only one thing, such as the provided resource (IRI) cannot be found in the database. The search returned no results but that might be because you made a user error is much squishier, and requires further hacky confirmatory checking on the client library side (see phenoscape/rphenoscape#157).

@hlapp
Copy link
Member Author

hlapp commented Feb 1, 2021

I can find arguments on the web for both 400 and 404.

And can you provide refs for either side.

@balhoff
Copy link
Member

balhoff commented Feb 2, 2021

And can you provide refs for either side.

The selected answer here supports your viewpoint in favor of 400: https://stackoverflow.com/questions/51970600/what-status-code-to-use-when-a-parameter-is-missing-in-the-query-string

There is a very reasonable sounding answer farther down in favor of 404.

There is discussion of this issue here in an http4s issue: http4s/http4s#1868

People see it both ways. The fact that the 404 result is baked into the http4s API (and akka-http, which we're using here) suggests that many people land on the same expectation I have.

I just don't think there is anything obvious about the choice. I think that for example if we want the /taxon endpoint to return 404 for an IRI that's not in the database (#377), then we're taking the whole URL as identifying a resource rather than being a method returning a value. So then I think the parameter names must be part of the resource identification as well.

Alternatively if the taxon IRI there is just an argument to a "method", then a 200 success response with some kind of empty value returned is more appropriate. If your IRI argument violates IRI syntax and can't be processed, then I think 400 is appropriate.

@hlapp
Copy link
Member Author

hlapp commented Feb 2, 2021

My argument isn't that the choice is obvious, let alone for everyone. Instead, in a nutshell it is, if there are multiple defensible choices, use the one that is best for usability, i.e., in this case makes client programming most robust and uncomplicated. I do believe that this means defining the reasons for 404 more narrowly.

Just based on experience with this API, the majority of problems that surfaced for Rphenoscape were caused by behavior changes of the API, not original bugs. Based on this experience, as a client having to rely on a server error message for properly handling a 404 condition plants a future failure. I do feel quite strongly that this is bad.

If you don't agree, let's discuss at the next SCATE software engineering meeting, which we should now reinstate.

@balhoff
Copy link
Member

balhoff commented Feb 2, 2021

Based on this experience, as a client having to rely on a server error message for properly handling a 404 condition plants a future failure. I do feel quite strongly that this is bad.

I agree with you here. Anything 4xx indicates a client error. I think we may be making a mistake to say the client makes an error to ask for info about an IRI we don't know about—it is a valid request. What I don't understand is how Rphenoscape would ever use the wrong query parameter. That doesn't seem like a situation you would need to handle as part of ordinary operation.

@hlapp
Copy link
Member Author

hlapp commented Feb 2, 2021

What I don't understand is how Rphenoscape would ever use the wrong query parameter.

Simply as a result of a Rphenoscape programmer error. (The programmer passes in the list of parameters.) If this happens, the test that hits it should abort (stop() in R parlance). This is in contrast to Rphenoscape user input, which might or might not be in error – Rphenoscape indeed can't infer that.

That doesn't seem like a situation you would need to handle as part of ordinary operation.

Yeah that's what will continue to happen (including after phenoscape/rphenoscape#157), but as I said, it now requires checking the error message returned by the server.

@balhoff
Copy link
Member

balhoff commented Feb 24, 2021

We will look into how we can configure akka-http routes to know that a required parameter is missing for a given path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants