-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Improvements to query language #2066
Comments
I like many of the ideas from an HTTP standpoint, but they'd be such a big breaking change(consider all client libraries we have now) that we might as well change our name if we did them. Many users will wonder why they should not just try to migrate to a GraphQL/OData(which are behind standards) solution if we do such a change.
Yes, this is why it's important to document our REST syntax(ref), maybe we could do a RESTQL/RSQL/RESQL V2 if we'd ever adopt such a change. But even if we were to support both our v1 and v2 at the same time, I think that would still create doubts about our stability. So I think before doing any major breaking changes to our REST syntax, we should offer extensibility(#1909 (comment)) to provide a way to be compatible with other standards(GraphQL/Odata). Some comments:
This looks better from a REST/HTTP standpoint, but there are many crippled http clients out there that can't send headers, we would leave those out or require even more use cases for proxies.
Ah, isn't
This one looks good and is actionable, will take it to a new issue. |
No, they are specifically allowed for that purpose: https://datatracker.ietf.org/doc/html/rfc3986#section-3.3
|
I certainly don't want to propose a general breaking change in the API here. I think there is value in "providing the most HTTP-standard compliant interface" in itself, but also many of those proposals solve actual problems we have.
If we offer an extensible interface for the query language and have both our V1 and V2 to make use of that interface / prove that it's working well - I don't see us creating doubts about stability. It's the other way around, I think. It increases confidence in a stable non-breaking interface, because new features, that would imply breaking changes can much easier be developed while keeping backwards compatibility in other versions of our language. |
This comment was marked as outdated.
This comment was marked as outdated.
Once #1970 (comment) is implemented, I think we could support both the old and new filter syntax without a breaking change.
Looking at the parsers I think it'd be easier to do
|
Some ideas..
Doesn't embedding just add attributes to the entity? You still do a Edit: Totally misread the RFC. It was related to media type parameters not query parameters. Still the new media type below might be worth considering.
Considering the above, it sounds possible to document the embedding possibilities for a resource. In OpenAPI we can list the Accept media types. Using paths:
/films:
get:
...
content:
application/vnd.pgrst.related+json; rel=films>-directors
application/vnd.pgrst.related+json; rel=films-<nominations
application/vnd.pgrst.related+json; rel=films-<roles
application/vnd.pgrst.related+json; rel=films>-<actors
application/vnd.pgrst.related+json; rel=films>-<competitions
So say we get the following request(with nesting): GET /films?select=*,roles(*, actors(*)),directors(*) We would respond with our usual json, but with the added header: Content-Type: application/vnd.pgrst.related+json; rel=films-<roles,films>-directors,roles>-actors The gain here is that we could document the relationships a resource has. Which was previously impossible in OpenAPI. |
This would only make sense if the embedding were to take place via accept header / mediatype. I don't think that would be correct. Adding an embedding will return a new entity - it will not merely change the return format. That means it does not belong in the header.
Really what I suggested above about embeddings and the path component would be the way forward. Just requesting people is different from just requesting jobs is different from requesting a combination of both. If we had something like |
As another datapoint, this also helps when user input is passed to a stored procedure via Currently, There doesn't seem to be a simple way to force the function param interpretation (quoting required preprocessing in the stored procedure to remove the quotes). |
Just noticed that even the
Now that we're moving on from doing disambiguation on the URL(#2863), the syntax can be much simpler so we don't need the FK or the cardinality in the path. So we could just go with There's a problem in expressing nested embedding on the path, since it needs to shape the JSON output. For this I believe we should keep the GET /actors,roles,films?select=roles(character,films(title,year)) Or later as a media type parameter too: GET /actors,roles,films
Accept: application/json;select=roles(character,films(title,year))
Yes, and we could do this now without a breaking change. Another syntax: GET /users(projects,tasks(subtasks)) That would allow us to express joins on resources. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Another improvement for the query language: use /a?col.~eq=3 |
We have discussed a few shortcomings of our current query language (expressions in the query string). Taking those suggestions together - and adding a few more points, that seem to be inconsistent to me in the current implementation.
I wonder whether we should try to implement all those improvements as non-breaking separated changes - or just invent a query language v2, where we can chose via config option which language to use. Maybe #1804 can lead us to have a cleanly separated
QueryLanguage
module that we can replace with a new module.Here we go:
Change operator syntax from
col=op.val
tocol.op=val
: Swagger: GET record from table with uuid primary key: failed to parse filter #1970. This will allow to useswagger-ui
to send requests. It will also allow plain-html requests without javascript like this:This was not possible before, because the value would have to be constructed dynamically.
Separating filter logic from values: Swagger: GET record from table with uuid primary key: failed to parse filter #1970. This will allow doing more complex queries in
swagger-ui
involving logic conditions, too. It will also simplify a lot of our qouting requirements already. Let's put all logic into alogic=...
parameter. Maybefilters=
is even better.Replace current
in
withcol.eq.any=<array>
as suggested in Normalize double quoting across all operators #1943. I'd go one step further and removein
completely. We are usingANY
under the hood anyway and together withlogic=
, we would remove all manual quoting we'd need to do. We'd rely only on PG array quoting.Move
select
andorder
to parameters of the mimetype in theAccept
header. Both parameters only shape the response - but the underlying entity is the same. As such, I think they don't really fit in the query string, which should describe which entity to receive. This would also remove the strange interaction we have between theselect=
and theAccept
header already: When you request a custom mimetype, e.g.application/octet-stream
ortext/plain
, the wholeselect=
doesn't make any sense. So the availability ofselect
is already dependent on the mediatype.Example: Both of the following requests will return the same person. Returning fewer or more columns is not much different to just returning the same entity in a different format.
would become
I'm not sure whether we can still use the mimetype
application/json
here. The registration mentions no parameters at all: https://www.iana.org/assignments/media-types/application/json. It specifically hints thatcharset
is not a valid parameter, but we are returning it anyway. At the same time RFC6838 says:As we would still be returning valid json, I think we would not change existing functionality, so adding parameters should be fine. If we end up not wanting to do this, we could still do so with our own custom media type, though.
Remove
limit
andoffset
parameters entirely - this can be done via the respective range headers.Replace
!inner
on the embedding with anexists
filter as suggested in Inaccurate total record count returned when top-level filtering with embedded resource filters #2009 (comment). Note, this does not really play well with moving the embedding to theAccept
header, yet. I don't think referencing a definition that is sent in the header from the query string is a wise idea. But given how the SQL for an exists query actually looks like (EXISTS (... sub query...)
), maybe we can do something likeexists=<embedding-spec>
:This means that in some cases the embedding + hint needs to be repeated between query string and header, but this makes thing much more explicit. Later on, we could even support true filtering via
EXISTS (...)
, which could then be combined with other filters in thelogic=
part. And only if it's part of a top-levelAND
and requested in theselect=
part, then we would make it aINNER JOIN
.Move
columns
to a parameter of the mimetype in theContent-Type
header, which describes the request body. Same arguments apply as above.Move
on_conflict
to a parameter of thePrefer: resolution
header. POSTing the same request once withon_conflict
and once without really does not change the returned entity. Parameters on prefer headers can be given like this, according to the RFC:Prefer: resolution=merge-duplicates;col1;col2;col3
Change the
Accept: application/vnd.pgrst.object+json
way to request a single resource to a path parameter something like this:This is because, the ability to query for a single row instead of multiple rows is really not inherent to the media type requested. Right now, it's not possible to query for a single row in
text/csv
,application/octet-stream
,text/plain
and other formats, which is a severe limitation. Changing it like this would also allow to people to rewrite those PK-equality requests easily via nginx from something like/people/123
. And since there is no interaction with the mimetype, this will not conflict with custom accept headers that might be sent.This would leave only filters (identified by at least one
.
before the=
), arguments (without.
) andlogic=
query parameters, reducing the amount of "reserved keywords" by a fair bit.Using parameters to all kinds of things etc. is also much more consistent with what the RFCs allow for those, instead of cramping everything into the query string.
Not sure whether everything in here can really be done. I haven't found a good solution for filters / limits / offsets on embedded resources, yet. It doesn't make much sense to move those to the accept header - but keeping them in the query string would create a strange interaction between query string and accept header once again.
One idea, that I haven't fully thought through, yet, could be to split the embeddings from the
select
part. If the embeddings were defined in the query string, but theselect=
in the accept header would reference those, that would be fine, I think. Having an interaction in this direction between header and query string would make sense, imho.Embedding other resources really changes the entity we are requesting, too - so they should not be moved to the header. What about using the flexibility the path specification of a URI gives us for embedding?
This would also play nicely with the
;single
or;one
parameter for requesting a single entity, which is kind of similar to a hint of relationship type to embedd!m2m
,!o2m
, etc.:They both do kind of the same thing in returning an object instead of an array.
The text was updated successfully, but these errors were encountered: