PostgREST / postgrest

REST API for any Postgres database
https://postgrest.org
MIT License
23.41k stars 1.03k forks source link

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

Closed wolfgangwalther closed 1 year ago

wolfgangwalther commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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:

Nah.

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

taimoorzaeem commented 1 year ago

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 commented 1 year ago

@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 https://github.com/PostgREST/postgrest/pull/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 commented 1 year ago

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 https://github.com/PostgREST/postgrest/pull/2164).

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

PeteDevoy commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Closing here as https://github.com/PostgREST/postgrest/pull/2876 got merged. Let's continue on https://github.com/PostgREST/postgrest/issues/2887 - different status for n=0 can be discussed there.