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

Return 404 instead of 406 for json object response with no rows? #1655

Closed
wolfgangwalther opened this issue Nov 18, 2020 · 8 comments
Closed
Labels
difficulty: beginner Pure Haskell task enhancement a feature, ready for implementation messages user-facing error/informative messages

Comments

@wolfgangwalther
Copy link
Member

Currently when making a request with Accept: application/vnd.pgrst.object+json, we get the following response, when <> 1 rows are returned:

{
    "details": "Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row",
    "message": "JSON object requested, multiple (or no) rows returned"
}

This is status code 406.

With no rows returned, I would expect to get a 404 Not Found. Maybe we can return 404 for no rows, and 406 for multiple rows?

This would help a lot in differentiating those two cases. 404 (no rows) is something that I expect in my case and can handle on the client side. 406 (multiple rows) would mean something went terribly wrong.

@steve-chavez
Copy link
Member

Not sure if changing the status would be a good idea, specially when 406 Not Acceptable is the best semantic fit for a failed Accept.

Perhaps we could add a field(or use the hint) on the json error to make it easier for clients to detect the number of rows affected?

I've also looked at other REST mechanisms. I found this REST API: https://www.hl7.org/fhir/http.html#patch. It seems it responds 404 for zero rows and 412 Precondition Failed for multiple rows. Perhaps we can have another Prefer that makes the operations stricter?

(Their implementation also has a Prefer: return=OperationOutcome, interesting to find another option besides minimal/representation)

@wolfgangwalther
Copy link
Member Author

Not sure if changing the status would be a good idea, specially when 406 Not Acceptable is the best semantic fit for a failed Accept.

I think the underlying problem is, that we don't really have a concept of "requesting a single resource". We do have a concept of "requesting a list of resources", with a special case of n=1, where we can use a different output format. At least in terms of what Accept etc. means.

However, when using Accept: application/vnd.pgrst.object+json, I assume almost everyone using PostgREST to think about requesting a single resource.

In terms of Content-Negotiation and Accept headers, 406 is a semantically better response. In terms of requesting a single resource, it doesn't fit at all. In this case the 404/412 mentioned below are a much better fit.

Perhaps we could add a field(or use the hint) on the json error to make it easier for clients to detect the number of rows affected?

That might be the easiest way out. I think we should do that in any case, even if we were to handle n=0 differently. The number of rows is there, so we can as well send it to the client.

I've also looked at other REST mechanisms. I found this REST API: https://www.hl7.org/fhir/http.html#patch. It seems it responds 404 for zero rows and 412 Precondition Failed for multiple rows. Perhaps we can have another Prefer that makes the operations stricter?

404/412 immediately resonates with me. The whole concept of "requesting a single resource and failing if the query returns multiple resources" has a lot of similarities to the idea of conditional requests.

(Their implementation also has a Prefer: return=OperationOutcome, interesting to find another option besides minimal/representation)

We do have return=headers, although only implicitly. As of right now.


Just some brainstorming here:

Conceptually a list of resources is itself another resource. The URI identifies a resource. By setting the Accept header we are implicitly changing to a different resource, but that's not the right way to do it.

The most natural way to make a distinction in the URL I can think of is by doing GET /items?.. vs. GET /item?... But I don't really see that in our case.

Some other ideas:

  • GET /one/items?... (kinda similar to the rpc prefix)
  • GET /items/one?...

Nah.

Let's add the number of rows to the 406... ;)

@wolfgangwalther wolfgangwalther added the enhancement a feature, ready for implementation label Nov 28, 2020
@wolfgangwalther wolfgangwalther added the difficulty: beginner Pure Haskell task label Mar 9, 2021
@steve-chavez steve-chavez added the messages user-facing error/informative messages label Jun 21, 2022
@taimoorzaeem
Copy link
Collaborator

I think the idea of 404/412 could be implemented. After naively thinking about this, I am thinking of defining a new Error besides SingularityError called PreConditionFailed and assign this the Http.status code of 412 and change the SingularityError status code to 404.

Then for the request Accept: application/vnd.pgrst.object+json if 0 rows are returned, we throw a singularityError and if more than 1 rows are returned, we could throw the preConditionFailed error. We will also need to add a new ErrorCode for the new Error type and as for MediaType we could keep it same as MTSingularJSON.

@steve-chavez Let me know your thoughts on this.

@steve-chavez
Copy link
Member

@taimoorzaeem I think that the 412 Precondition Failed error is more fit when we're using an If-None-Exist header (like on the FHIR standard) or when we use a custom Prefer header (like the idea about Prefer: max-changes on #2164).

Also, when we fail on a:

GET /projects?id=gt.1
Accept: application/vnd.pgrst.object+json

The resource collection does exist (hence a 404 Not Found is inaccurate) but we can't comply with the requested resource representation (a json object).

So I believe we should stick to the 406 Not Acceptable as it's more accurate and only add the number of affected rows to the hint of the error.

@steve-chavez
Copy link
Member

I think that the 412 Precondition Failed error is more fit when we're using an If-None-Exist header (like on the FHIR standard) or when we use a custom Prefer header (like the idea about Prefer: max-changes on #2164).

A better explanation would be that 412 is meant for conditional requests.

@PeteDevoy
Copy link

PeteDevoy commented Jul 30, 2023

This may sound insane but maybe if we are working on the premise that the resource is always a collection then maybe a 204 No Content would be the appropriate response?

Because the collection still has the properties of being found and empty but after stripping away the [ and ] there is nothing left to send.

Something like this:

application/json application/vnd.pgrst.object+json
n = 0 200 OK + [] 204 No Content + <empty body>
n = 1 200 OK + [{...}] 200 OK + {...}
n > 1 200 OK + [{...}, ...] 406 Not Acceptable + <error response>

This approach would allow clients to use HEAD to indicate the state of the resource and avoid an error on GET, which is not possible with the current implementation.

@steve-chavez
Copy link
Member

steve-chavez commented Aug 2, 2023

n = 0 | 204 No Content + <empty body>

Unfortunately this would be a breaking change as some clients are relying on the 4xx status for n = 0.

This approach would allow clients to use HEAD to be used to indicate the state of the resource and an avoid an error on GET, which is not possible with the current implementation.

@PeteDevoy Is it really necessary to have a different status for a n = 0 and n > 1? Another way might be also including Prefer: count with HEAD, which would let you confirm if it's 0 or not.

@steve-chavez
Copy link
Member

Closing here as #2876 got merged. Let's continue on #2887 - different status for n=0 can be discussed there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: beginner Pure Haskell task enhancement a feature, ready for implementation messages user-facing error/informative messages
Development

No branches or pull requests

4 participants