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

Sort Extension: simple format for GET #513

Merged
merged 26 commits into from
Nov 13, 2019
Merged

Conversation

philvarner
Copy link
Collaborator

@philvarner philvarner commented Jun 24, 2019

  1. added a slightly different, non-JSON format for the GET parameter case. @joshfix
  2. changed the description so that field from the root of Item rather than properties. This is so that a user can provide a stable sort over properties.created ascending, id (ascending or descending). Without the id as the secondary sort, items with the same created timestamp would result in a non-stable sort, which may give users incorrect results with trying to page through an entire result set.

PR Checklist:

  • This PR has no breaking changes.
  • I have added my changes to the CHANGELOG or a CHANGELOG entry is not required.
  • API only: I have run npm run generate-all. to update the generated OpenAPI files.

@joshfix
Copy link
Contributor

joshfix commented Jun 25, 2019

I like the GET support! :)

I have no idea if this is a good idea or bad idea, but what about making the GET values use a similar syntax as the proposed fields values, where a + denotes ascending and a - denotes descending:

GET /stac/search?sort=+properties.created,-id&fields=+properties.created,+xxx

@philvarner
Copy link
Collaborator Author

As I wrote up Fields, it's only nothing or -, which I think is fine for sort also ==> ascending is the default, with - prefix meaning descending.

@m-mohr m-mohr added this to the 0.8.0 milestone Jul 22, 2019
@cholmes cholmes modified the milestones: 0.8.0, 0.9.0 Aug 19, 2019
@cholmes
Copy link
Contributor

cholmes commented Aug 19, 2019

Moving to 0.9.0 - as there's more thinking to do on this. Sean will sound in with some more thoughts on doing things in headers, and we want to try to align with WFS.


Examples of `sort` parameter:

GET /stac/search?sort=created|asc,id|desc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about replacing | with ; ? Like GET /stac/search?sort=eo:cloud_cover;desc. | may be ambiguous (expressing this or that property, say)

where <direction> is either "asc" (ascending) or "desc" (descending). The `sort` value is an array, so multiple sort fields can be defined which will be used to sort the data in the order provided (e.g., first by `datetime`, then by `eo:cloud_cover`).
```json
{
"sort": [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Borrowing from OGC approaches, what about:

    "sort": [
        {
            "property": "created",
            "order": "asc"
        }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomkralidis can you point me to the documentation for those OGC approaches?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomkralidis
Copy link
Contributor

Note relevant conversation in opengeospatial/ogcapi-features#157

@philvarner philvarner changed the title add to Sort Extension a format for GET, qualify field off of root Item rather than Properties Sort Extension: simple format for GET Nov 6, 2019
@matthewhanson
Copy link
Collaborator

@philvarner Looks like this might need some updates, thought colon was going to be avoided in favor of semi-colon?

@philvarner
Copy link
Collaborator Author

I think I have all the necessary changes in now @joshfix @tomkralidis @matthewhanson

Copy link
Collaborator

@m-mohr m-mohr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be what we have discussed. 👍 Only thing to change is the endpoint path (see comment).

@m-mohr m-mohr self-requested a review November 11, 2019 10:24
@m-mohr
Copy link
Collaborator

m-mohr commented Nov 11, 2019

I'm just wondering whether we should use an approach that can be modeled in OpenAPI. We can't properly describe the current behavior except for textual descriptions. I went through the OpenAPI specification and found we could model it properly this way:

{
  "name": "sortby",
  "in": "query",
  "schema": {
    "type": "object",
    "additionalProperties": {
      "type": "string",
      "enum": [
        "asc",
        "desc"
      ]
    }
  },
  "style": "deepObject"
}

This would translate to the following GET query parameters:
sortby[properties.datetime]=desc&sortby[id]=asc

This translates to the following object, which could also be used in POST:

{
  "properties.datetime": "desc",
  "id": "asc"
}

I'm aware that this is maybe not as short as the other approach, but follows a well-defined behavior.

@m-mohr m-mohr mentioned this pull request Nov 11, 2019
3 tasks
@m-mohr m-mohr requested a review from joshfix November 13, 2019 17:05
@philvarner philvarner merged commit fb8d7a2 into radiantearth:dev Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants