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

Undefined grammar for sort query parameter #1266

Open
ethanresnick opened this issue Apr 7, 2018 · 8 comments
Open

Undefined grammar for sort query parameter #1266

ethanresnick opened this issue Apr 7, 2018 · 8 comments
Labels
profile Related to existing and proposed profiles as well as profiles in general

Comments

@ethanresnick
Copy link
Member

ethanresnick commented Apr 7, 2018

The spec for the sort query parameter doesn't specify what format/grammar a "sort field" must follow. However, it might be good to define a grammar now that works with existing implementations, to preserve our extensibility options in the future.

The genesis of this issue is that I wanted my API to support sorting the returned resources by their distance from some point. To accomplish that, I was thinking about having a ?sort=distanceFrom[lng,lat] syntax or similar. This syntax clearly would involve a minus sign (if the longitude or latitude is negative), but the minus sign would only appear in a context where my parser can distinguish it from the minus sign that can prefix a sort field. Is that valid? [Edit: it probably has to be, as disallowing the minus sign within sort fields would make it impossible to filter on kebab-case field names.]

What about the comma in my proposed syntax, which can likewise be contextually distinguished from the comma that separates sort fields?

The current spec says that sort fields don't need to correspond to field names, so it's not clear that the member naming rules would automatically apply to sort fields. Also, strictly speaking, the spec shows an example of a sort field that contains a "." in it, which would be invalid as a member name.

I guess the most minimal thing we could do is say that the comma can't appear directly in a sort field, but can appear urlencoded. The problem with that is, I suspect, that some implementations are going to unconditionally percent encode the comma that separates sort fields (even though that's not strictly necessary), which would then make it impossible to use whether a comma is url encoded as a way to determine whether the comma is part of the sort field or a separator between sort fields. [Edit: this is wrong.] So, a better solution might be to disallow commas within a sort field altogether, which seems quite reasonable given the current spec (even though it would be quite convenient for my use case). If we do that, I guess the question is also just whether we want to reserve some other characters at the same time for future extensions.

@dgeb
Copy link
Member

dgeb commented Apr 8, 2018

It seems fine to spell out that url-encoded commas may be used within sort fields.

I don't think we need to specify anything else re: minus signs, because the spec is pretty clearly speaking only about their significance at the beginning of sort fields:

The sort order for each sort field MUST be ascending unless it is prefixed with a minus (U+002D HYPHEN-MINUS, “-“), in which case it MUST be descending.

I think it would be breaking, and unnecessary, to try to disallow minuses within sort fields at this point.

@ethanresnick
Copy link
Member Author

ethanresnick commented Apr 9, 2018

Sorry, yesterday was a long day and my OP was quite rambly. I've edited it to incorporate what I wrote in my (now deleted) follow up comment and to strike the incorrect part about browsers unconditionally encoding query param commas.

I agree that we can't disallow minus (except at the beginning of a sort field, which is already implicit but might be worth stating).

The open questions at this point are:

  • Are there other characters we do want to disallow that are so rarely used that disallowing them wouldn't be a problem, but that would be useful for us to have in the future?

  • Do we want to allow unencoded commas within sort fields, when the server can determine that the comma isn't a sort field separator? (Encoded commas are definitely allowed, I agree.) I'm pretty sure we should allow them, unless there's a compelling reason why a generic client/client library would need to be able to inspect the URI and extract the sort fields. I think a syntax like ?sort=distanceFrom(lat,lng),otherField is very useful, and not allowing unencoded commas would mean I'd have to ditch that in favor of some less-intuitive, comma-less argument syntax. (I can't just encode the comma within the parentheses, because then I'd have to double encode commas to use them within the arguments, which would be a confusing special case tricky for clients.)

@ethanresnick
Copy link
Member Author

FYI, I plan to implement a format for the sort parameter in my json-api library that uses unencoded parentheses, backticks, and commas within sort fields. If that's a problem, please speak up now :)

@krainboltgreene
Copy link
Contributor

I know this PR isn't specific to geolocation and only uses that as an example, but just in case it was missed another way to handle this would be https://tools.ietf.org/html/draft-daviel-http-geo-header-05:

GET /events?sort=events.distance-from-input,events.start_at
Host: tourism.example.com
Geo-Position: -10.28;60.84 epu=50 hdn=45

@krainboltgreene
Copy link
Contributor

After reflection I have some questions about this: If we give sort this type of syntax, won't we be forcing an untenable situation on servers? Lets say you have this request:

GET /events?sort=events.start_at,friends(2),distanceFrom(-10.28, 60.84)&page[offset]=1&page[limit]=2
Host: travel.example.com
Accept: application/vnd.api+json

And a table of:

events {
  start_at TIMESTAMP
  end_at TIMESTAMP
  latitude FLOAT
  longitude FLOAT
}

In this example we're going to want to do two things:

  1. Sort by the start at
  2. Sort by how many friends (to the second degree, aka friend of friends)
  3. Sort by distance from given location
  4. Paginate the values

Now, 1 and 4 can be done at the database level and should for efficiency sake. Sometimes 3 can be done a the database level, but not always and not easily. It takes a special kind of database to make 2 easy.

As soon as you have a sorting mechanism that doesn't operate in the database, they all have to be done in the application layer, right?

@ethanresnick
Copy link
Member Author

The spec isn't saying that servers have to support sort fields that take arguments, or any other kind of sort field. This issue is just about (eventually) describing a grammar for what exactly is allowed in the ?sort parameter. And I think we definitely want to allow servers to support these kinds of dynamic sort fields if they so desire. Your right that they won't always be possible to implement efficiently, so servers presumably won't support those kinds of sorts.

@krainboltgreene
Copy link
Contributor

Right, I understand that end-developers are allowed to say no, but you and I are implementors. We have to at least support the possibility, yes?

@ethanresnick
Copy link
Member Author

I'm not really sure I follow... I guess you're saying that server libraries need to provide some way for the library's user to customize how the ?sort param is converted into a DB query, and/or allow for some pre-response processing step to support application level sorting? If so, then yes, I think that's right, but I imagine most libraries will end up developing similar facilities anyway for other features.

E.g., from experience working on my library, I've found that a pre-response processing step is helpful for doing things like hiding response fields from users who don't have permission to see them, or adding "virtual" relationships to the serialized output. Likewise, a step for transforming the default generated query is useful for adding filter constraints that can't be inferred from the request directly (e.g., the query for GET /projects should SELECT from projects, but would get transformed by the user to add WHERE is_deleted = false if soft deletions are in use), among many other things.

@jelhan jelhan added the profile Related to existing and proposed profiles as well as profiles in general label Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profile Related to existing and proposed profiles as well as profiles in general
Projects
None yet
Development

No branches or pull requests

4 participants