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

API: Order parameter #5602

Closed
ErisDS opened this issue Jul 25, 2015 · 4 comments
Closed

API: Order parameter #5602

ErisDS opened this issue Jul 25, 2015 · 4 comments
Labels
affects:api Affects the Ghost API

Comments

@ErisDS
Copy link
Member

ErisDS commented Jul 25, 2015

As mentioned in #5463, we want to add a new order top-level parameter to API browse requests.

The order parameter allows a user to specify which order they'd like their results to be returned in.

The default orders across the 3 key resources are currently:

  • Posts: "posts"."status" ASC, "posts"."published_at" DESC, "posts"."updated_at" DESC, "posts"."id" DESC
  • Users: "users"."last_login" DESC, "users"."name" ASC, "users"."created_at" DESC
  • Tags: none (database default, usually by ID)

The order parameter should accept any valid SQL order by clause, which does not contain quote marks, e.g. users.last_login DESC, users.name ASC, users.created_at DESC. The comma-space separator between rules, and the space separator between property and direction are required.

This means that a valid value for the order parameter can be validated to contain only alphanumeric characters, dots, underscores and spaces, with the property being any string with those characters (except the space), and the direction is always either ASC or DESC (upper or lower) It should be possible to design a regex to match this pattern.

The order parameter should also validate that the provided order properties are valid attributes for the model. Any field with doesn't specify which model it belongs to should default to whichever model is being queried, and this should be set explicitly.

It should be possible to translate this format into a JSON format that matches the one returned from orderDefaultOptions using a split on the comma and space. The model must be included for each property (this will be added to models soon).

E.g. for posts, the default order would be translated to the following JSON objet:

{
    'post.status': 'ASC',
    'post.published_at': 'DESC',
    'post.updated_at': 'DESC',
     'post.id': 'DESC'
};

That object can then be used to apply the order, much as is already done, by reading options.order, and using the value of orderDefaultOptions if it doesn't exist: https://github.com/TryGhost/Ghost/blob/master/core/server/models/base/index.js#L293

@ErisDS ErisDS added the affects:api Affects the Ghost API label Jul 25, 2015
ErisDS added a commit to ErisDS/Ghost that referenced this issue Aug 28, 2015
refs TryGhost#5727, TryGhost#5602

- refactored to maintain the order in which tags are added
- will require a further, more complex refactor to handle re-ordering
ErisDS added a commit to ErisDS/Ghost that referenced this issue Sep 1, 2015
refs TryGhost#5727, TryGhost#5602

- Add new 'order' column to posts_tags table
- Migrate all existing posts_tags to have a correct value for 'order'
- Rewrite updateTags to not remove all tags, and to correctly maintain order
- Add transaction support for tag operations
- Many tests
ErisDS added a commit to ErisDS/Ghost that referenced this issue Sep 2, 2015
refs TryGhost#5727, TryGhost#5602

- Add new 'order' column to posts_tags table
- Migrate all existing posts_tags to have a correct value for 'order'
- Rewrite updateTags to not remove all tags, and to correctly maintain order
- Add transaction support for tag operations
- Many tests
@ErisDS ErisDS mentioned this issue Oct 6, 2015
31 tasks
@ErisDS ErisDS modified the milestone: Public API v1 Oct 13, 2015
@vadimdemedes
Copy link

The comma-space separator between rules, and the space separator
between property and direction are required.

I think it would be better, if we did not force spaces between rules. This will cause errors among developers, because they normally don't expect, that a lack of space in url can cause an error. I think we should accept both types of inputs, with and without spaces. And not being strict on lower/upper case should also be a plus.

Ideally, these should be treated equally: last_login desc,name asc, last_login DESC, name ASC.

@ErisDS ErisDS mentioned this issue Oct 20, 2015
24 tasks
@vadimdemedes vadimdemedes mentioned this issue Oct 22, 2015
3 tasks
@vadimdemedes
Copy link

Should transformed order object contain the model name prefix?

{
    title: 'DESC'
}

vs:

{
    'post.title': 'DESC'
}

defaultOrderOptions() in Post model returns attributes without this prefix:
screen shot 2015-10-22 at 3 03 20 pm

@ErisDS
Copy link
Member Author

ErisDS commented Oct 23, 2015

Yes I think it needs to so that it will play nicely with joins?

@vadimdemedes
Copy link

Ok, then orderDefaultOptions() will need to be updated to insert a table prefix.

@ErisDS
Copy link
Member Author

ErisDS commented Oct 23, 2015

Note: There are one or two 'hard' problems here when it comes to ordering by counts. Counts aren't implemented yet and so the open PR can be merged without support for that, but I'm just thinking out loud about what the solution might look like.

There is one, cheeky line of ordering code in the stuff I added to make filters work: https://github.com/TryGhost/Ghost/blob/master/core/server/models/base/utils.js#L52

It's hardcoded atm, but what it is supposed to is replace the default sort order automatically when a query is made asking for all posts which match more than one tag. This is the special rule for 'IN' defined in here: #5604. It should still be possible to override this. The problem is, that to order by a count you have to call .query('orderByRaw', ...) instead of .query('orderBy', ...) and I'm not sure how to manage getting all of this to work together just yet?

The API for ordering by a count would still be e.g. order="posts.count, ASC" but internally, we might need to use raw again... I'm not sure as I haven't played much with count implementations yet.

This is probably a separate issue - just brain-dumping :)

@vadimdemedes
Copy link

@ErisDS Turns out bookshelf is prefixing attributes with table names automatically. This happened when I added table prefixes:

screen shot 2015-10-26 at 11 02 23 am

Update: Now I found out that we prefix attributes, not bookshelf: https://github.com/TryGhost/Ghost/blob/master/core/server/models/base/pagination.js#L171

@ErisDS
Copy link
Member Author

ErisDS commented Oct 26, 2015

I was just about to reply and say, I think this was done by the order-handling code in the pagination plugin and it probably needs updating to only add the table prefix if it is not already present.

@ErisDS ErisDS modified the milestone: Public API v1 Dec 3, 2015
@ErisDS ErisDS added the later [triage] Things we intend to work but are not immediate priority label Sep 20, 2016
@ErisDS
Copy link
Member Author

ErisDS commented Sep 20, 2016

I'm closing most API issues temporarily with the later label.

JSON API Overhaul & OAuth access are currently scheduled next on the roadmap

@ErisDS ErisDS closed this as completed Sep 20, 2016
@ErisDS ErisDS removed later [triage] Things we intend to work but are not immediate priority labels Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:api Affects the Ghost API
Projects
None yet
Development

No branches or pull requests

2 participants