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

Output error when doing GET with plurality=singular and more than one row is returned #748

Closed
steve-chavez opened this issue Nov 23, 2016 · 10 comments
Milestone

Comments

@steve-chavez
Copy link
Member

postgREST current behavior is to do a LIMIT 1 OFFSET 0 when doing a GET with plurality=singular, I think it will be more appropriate to give an error when more than one row would be returned.

This would help with client errors, for example a while back I got a case of a duplicate key and postgREST could have helped me there since I mostly use plurality=singular when explicitly requesting a single row with filters or a limit.

Also this would be a more consistent behavior since POST and PATCH already give an error when returning more than one mutated row.

@begriffs
Copy link
Member

We had discussed that behavior in the original pull request (this comment). Your proposal is behavior number 2 from that discussion. It looks like we chose behavior 1 only for expediency. I understand why it would be helpful to do things the way you propose.

Thinking about it again, if we want to raise errors when a client asks for a singular response to a resource with multiple rows then we're technically not talking about a client preference, but more properly an expectation. RFC7240 says, "The Prefer request header field is used to indicate that particular server behaviors are preferred by the client but are not required for successful completion of the request." Whereas RFC2616 says, "The Expect request-header field is used to indicate that particular server behaviors are required by the client."

So here's my proposal:

  • If the client sends Expect: plurality=singular then the server will return single row responses as a JSON object with no surrounding array. However if the result to be returned has multiple rows then the server will respond with HTTP 417 (Expectation Failed) and an error message explaining the violation.
  • Then either ignore Prefer: plurality=singular or use behavior 3 from the linked PR comment.
  • Apply this behavior uniformly across GET, POST, PATCH

My one concern about the Expect header at this point is this comment:

While the Expect header field is end to end, the HTTP
specification requires that the header be processed hop by hop.
That is, every interceding intermediary that handles a request
between the client and the origin server is required to process
an expectation and determine whether it is capable of
appropriately handling it.

@steve-chavez
Copy link
Member Author

steve-chavez commented Nov 26, 2016

I've been testing sending an Expect header to postgREST through nginx 1.10.1, and nginx doesn't proxy pass that particular header even with the ignore_invalid_headers off directive.

This article http://www.fallingcanbedeadly.com/posts/http-100-continue-latency-and-you also points that nginx behaves this way.

Also from the code https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_proxy_module.c#L1938 I think something can be intuited. Though to confirm this I'd have to compile nginx with debug logs enabled.

@ruslantalpa
Copy link
Contributor

@steve-chavez wow, good catch

@begriffs
Copy link
Member

Hm, good research. That's too bad about the lack of HTTP support. This is a perplexing issue, I've had a few more thoughts about the semantics. Now is a good time to figure it out so we can do the right thing in the big release.

  1. It expresses more than a preference to throw errors when unable to comply. The Prefer header, at least the way we're treating it, is the wrong mechanism. The only behavior for the Prefer header consistent with the spec would be to return a single object when there is only one row, or the unchanged array if more rows.
  2. In REST terms Is the singular object a distinct representation or an entirely new resource? When we return HTTP 200 and [] for a regular request but 404 for a singular request we are treating them as two resources. One exists (the filtering of a known collection) and the other does not.
  3. Considering singular and plural as distinct resources means they should have distinct URLs. Our current behavior is inconsistent.
  4. Currently all query params work equally well on non-root routes, but if some routes are singular-only then they don't support stuff like filtering. Such routes require specialized logic.
  5. Additionally, enforcing the singular requirement is useful beyond supporting select-by-id (e.g. /things/1). It is convenient to apply it to the results of RPC, or to filtering on a column which is not a primary key but does have a uniqueness constraint.
  6. The problem is not as severe for CSV since there is no such thing as a "singular CSV" response, it's always a list of rows, sometimes with only one row in it. I don't think clients get thrown off by this.
  7. The crux of the problem seems to be that it is inconvenient for clients to deal with arrays of length one when all they want is an object. (It's also important to enforce uniqueness on the data, but the database itself can do that.)
  8. What the client wants to say is, "I accept JSON, but just objects actually, not arrays, numbers, or strings." I propose this requirement be expressed in the Accept header. We should either make a new MIME type and register it with IANA, or find a creative use of mime type parameters like application/json; type=object.
  9. It solves the 200/404 resource inconsistency. If the client accepts only JSON objects and the server sees that the response is plural then the server would respond with 406 Not Acceptable.

I'm curious what you think of this. Here is an article about the structure of mime types and how to make new ones. Technically what I wrote above with the type=object is not a valid param for the json type as registered with IANA: https://www.iana.org/assignments/media-types/application/json

We could also continue using the Prefer header but remove the server side errors. The client would discover that the resource is plural because it would get back an array instead of an object...

@steve-chavez
Copy link
Member Author

What is the reason for suppressing the Content-Location, Content-Range headers when plurality=singular is set?

According to that restriction getting the total count from GET /items?limit=1 Prefer:plurality=singular Prefer:count=exact is not considered a valid use case.

@begriffs
Copy link
Member

begriffs commented Nov 30, 2016

Actually what do you all think about removing the plurality=singular behavior for the 0.4 release and investigating a new mime type for a future release like 0.4.1?

@steve-chavez
Copy link
Member Author

@begriffs Regarding your proposal, if we were to keep Prefer and respect semantics in all HTTP verbs, it would be better to respond with a Preference-Applied like suggested in another issue, though keeping the Prefer with this behavior wouldn't be of much use for the client.

For me the Accept: application/json; type=object is the better solution but I don't know how lengthy is the process of registering the optional parameter with IANA.

I don't think that removing the feature is the best solution for the users, maybe we could leave just like it is or how about just agreeing on a mime type now and applying it.
Not sure of the process but seeing https://tools.ietf.org/html/rfc6838 a Provisional Registration can be used.
It maybe harder to succeed in a modification to a standard tree, so maybe a vnd.postgrest or the like could be another option.

I guess we could also ignore the IANA requirement for now, I've seen an unregistered application/json; odata=verbose before, if our proposed mime type doesn't get accepted we could change it later.

@begriffs begriffs added this to the v0.4 milestone Dec 5, 2016
@deinspanjer
Copy link

I'll +1 the option of having the type=object mime-type. That would be easy enough to implement on the client side.

@jackfirth
Copy link

jackfirth commented Dec 6, 2016

A quick side note: the issue of what to do with multiple rows with Prefer: plurality=singular could be resolved using the standard "handling" preference (one of either "strict" or "lenient"), with Prefer: plurality=singular, handling=strict resulting in errors on multiple rows and Prefer: plurality=singular, handling=lenient returning the multiple rows unchanged. However, this doesn't really help the problem of the same resource either existing (200 Okay containing empty array) or not existing (404 with plurality=singular) depending on an HTTP preference. Semantically that seems very strange.

@steve-chavez
Copy link
Member Author

I think the handling=strict is a good option until the mime type is settled.

Seeing that the only postgREST Prefer value that has a potential error condition is plurality=singular
we can just apply it as suggested and ignore it if is applied when no plurality=singular is specified; handling=lenient could be the default, in this case return single object for one row, the unchanged array for multiple rows or a 200 with a {} body instead of 404 for zero rows.

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

No branches or pull requests

5 participants