Skip to content

Datastore Query order should use an explicit ascending/descending parameter instead of "-" in a string. #1061

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

Closed
krisnye opened this issue Jan 14, 2016 · 5 comments · Fixed by #1069
Assignees
Labels
api: datastore Issues related to the Datastore API.

Comments

@krisnye
Copy link

krisnye commented Jan 14, 2016

Currently:

// Sort by size ascendingly.
var companiesAscending = companyQuery.order('size');

// Sort by size descendingly.
var companiesDescending = companyQuery.order('-size');

This is bad. It is mixing the properties of a string based query language with a json based query API.

Instead, consider just adding a boolean ascending / descending property that defaults to ascending.

var companiesAscendingImplicit = companyQuery.order('size');
var companiesAscendingExplicit = companyQuery.order('size', true);
var companiesDescending = companyQuery.order('size', false);
@callmehiphop callmehiphop added the api: datastore Issues related to the Datastore API. label Jan 15, 2016
@callmehiphop
Copy link
Contributor

This seems like a pretty reasonable request, @krisnye. I think there might be a typo in your example and you were going for this..?

var companiesDescending = companyQuery.order('size', false);

I'm not totally sold on using a boolean, only because at first glance it's not very obvious what it does and it may end up warranting a trip to the docs. I like the idea of separating the two, however I feel that a literal object (probably overkill) or simply passing in an optional sign would be more intuitive.

var companiesDescending = companyQuery.order('size', { ascending: false });
var companiesDescending = companyQuery.order('size', '-');

@stephenplusplus thoughts?

@stephenplusplus
Copy link
Contributor

I prefer the options object.

@krisnye
Copy link
Author

krisnye commented Jan 21, 2016

Yes, I had a typo. I fixed it. My honest thoughts are that a JSON based API should have a JSON based query structure. One that is easy to read and easy to write, while being explicit.

Here is the JSON query structure that I use to represent filters, limits, offsets and sorts.

{
    "offset": 20,
    "limit": 100,
    "where": {
        "name": "foo",
        "age": { ">=": 10 },
        "percent": { ">": 0, "<": 100}
    },
    "sort": [
        { "age": true },
        { "name": false }
    ] 
}

I actually combine this query format with a TYPE/ID path which captures ancestor searches in order to represent all queries.

Here are some examples. They are all both REST urls and concise query descriptions:

/Project/23              # gets a single entity
/Project                 # gets all Projects
/Project?{"limit":50}    # gets first 50 projects
/Project/23/Task/2       # gets a single descendant entity
/Project/23/Task?{"where":{"percentComplete":{">=":1,"<=":99"}}     # get all tasks that are in progress

@stephenplusplus
Copy link
Contributor

If I was to design the API today, I definitely would have went with that approach as well. I'm not sure why we went with the builder approach. Obviously, at the time, it was thought to be more convenient for the developer. In the end, it's really just a style choice, so to completely change to just a plain JS object now would be an unfair amount of work to put on developers without any clear rewards.

@krisnye
Copy link
Author

krisnye commented Jan 21, 2016

True, but you can build such an object behind the scenes using the builder for backward compatibility and then expose the simpler interface when appropriate.

Something to think about for the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants