Clinical-Genomics / loqusdbapi

A simple REST api for loqusdb
2 stars 0 forks source link

The app returns an HTTP error when the variant is not found #4

Closed northwestwitch closed 3 years ago

northwestwitch commented 3 years ago

And gives the impression that there is something wrong in the query, even if it's fine. Why not returning success (status code 200) and informing that the variant is not found? I've never used the loqus command line, but what does it return when the variant is not found? I'm asking because API query errors in scout and in the vast majority of the other apps end up digested in a different way than successful responses with no results. Scout example:

https://github.com/Clinical-Genomics/scout/blob/dfec1c0c670c15f7e4babad75cf7a1261870c374/scout/utils/scout_requests.py#L64

northwestwitch commented 3 years ago

You could even return {}

moonso commented 3 years ago

When I read about REST I found that it is the common way to return a http error when a requested resource is not found. It could be discussed if 404 Not Found or 204 No Content suits better in this case but I think this is the correct way of doing it.

moonso commented 3 years ago

In this case it will be handled in the Loqusdbapi interaction so should not be a problem to handle right?

northwestwitch commented 3 years ago

It's not a problem to handle it in Scout, but it's more a philosophical question. Yes you get 404 when you don't find a resource but the resource is intended as a web page or file on the web server, not an entry in the database. The not found in the database is not an error, but a valid result. I don't know, I would handle it differently, like returning the query executed and that you found no entries in the database..

moonso commented 3 years ago

Feel free to delve into the topic :)

https://stackoverflow.com/questions/11746894/what-is-the-proper-rest-response-code-for-a-valid-request-but-an-empty-data

northwestwitch commented 3 years ago

@moonso your link says I'm right: 😄

image

northwestwitch commented 3 years ago

From the same response that was heavily upvoted:

image

moonso commented 3 years ago

Well, I think that depends on how much of the text and answers you want to read

moonso commented 3 years ago

There is a comment from the author of the first answer:

@MatthewPatrickCashatt you're free to downvote as you please. I now finally understand why people are downvoting me, but the rationale is still wrong. When receiving a 404 it doesn't mean the route doesn't make sense, it means there is no resource at that location. Full stop. This is true if you're requesting /badurl or /user/9 when such a user doesn't exist. A developer can help by adding a better reason phrase than 'Not Found', but is not required to. – Chris Pfohl Sep 16 '16 at 17:26

northwestwitch commented 3 years ago

I still agree with the most upvoted answer. 404 is an error code and a record not found in a database is not an error, but a valid result. But if you like it this way is fine, I can fix Scout to handle the 404 error in a different way