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: Next and Previous post #4262

Closed
ErisDS opened this issue Oct 10, 2014 · 7 comments · Fixed by #4772
Closed

API: Next and Previous post #4262

ErisDS opened this issue Oct 10, 2014 · 7 comments · Fixed by #4772
Labels
affects:api Affects the Ghost API help wanted [triage] Ideal issues for contributors to help with

Comments

@ErisDS
Copy link
Member

ErisDS commented Oct 10, 2014

Many moons ago, #529 was raised to describe a future in which theme developers were able to access information about the next and previous post, according to the chronological order which posts are published on the blog.

Back then, bookshelf didn't support subqueries, we didn't have a clear plan for the API, and we didn't have any idea about having a query helper. All of these things combined to make this issue quite tricky to solve.

The solution that was put forward was an always-on addition to the API which meant making 2 extra queries for every post page. This seemed like unnecessary overhead for the GET /posts/ request for a feature that wouldn't be used by everyone. Not having performance tests made it riskier.

Now that we have a clearer idea of what our API is supposed to look like (in short, JSON API but with inline objects rather than linked), it is much easier to design clean solutions to problems like this one. The proposal for adding next & previous support to the API is to update the posts endpoint to accept next and previous as includes E.g.

GET /posts/?include=next,previous

It should be possible to add one or both of these options, and get the corresponding post returned in the response in the following format:

{
    posts: [{
        id: 1,
        title: 'Something here',
        ...
        next: { 
             id: 6,
             title: 'Another post thing',
             ...
        }
    }]
}

Implementing this at the model level is going to require some fancy bookshelf-ery. May be worth using the subquery support here. Performance testing would be very much appreciated.

Notes:

  • next is the post published after the current post, so should be more recent
  • previous is the post published before
  • If the next or previous posts are requested but don't exist because we are at one end of the list or another, the key should be included, but the body should be null.
  • To make the API consistent, we should always return next and previous keys with the id of the post, such that adding them to include causes the whole object to be included, rather than the keys to magically appear. This definitely needs perf tests and will likely be a separate issue.
  • This is just an API addition and won't make this feature available to themes
@ErisDS ErisDS added the affects:api Affects the Ghost API label Oct 10, 2014
@ErisDS ErisDS added this to the 0.5.x Backlog milestone Oct 10, 2014
@sondr3
Copy link

sondr3 commented Oct 16, 2014

I'm curious if it would be possible to also get next/previous for posts in tags as well? It'd make it possible to iterate over posts the same way say Medium does it with their collections, so when you've scrolled to the bottom of a post in the tag markdown it could display the next post in that tag, so a reader could go either backwards to the beginning of the series or forward in it. I do realise it does add "some" complexity (when do you go forward vs backward, or at all vs next chronologically etc). Just curious 😄

@novaugust
Copy link
Contributor

That'd sure be cool. "next is the post published after the current post as specified by some query/filter"

@ErisDS
Copy link
Member Author

ErisDS commented Oct 16, 2014

@sondr3 this is a potential upgrade / improvement / extension to the behaviour described by this issue but is not expected. For now, next/previous refers only to the global chronological list.

@ErisDS ErisDS modified the milestones: Current backlog, Next Backlog Oct 21, 2014
@ErisDS ErisDS added the help wanted [triage] Ideal issues for contributors to help with label Oct 22, 2014
@ekulabuhov
Copy link
Contributor

Starting to work on this one.

@ekulabuhov
Copy link
Contributor

Progress update. I've hit 2 issues so far. One in bookshelf and one in knex.
Current API call will generate something along these lines:
select * from posts where slug = ? and status = 'published'

to get the next post I wanted to extend the query like this:
select * from posts where (slug = ? and status = 'published') or id = (select min(id) from posts where published_at > (select published_at from posts where slug = ?))

First problem: knex doesn't support subqueries with any operator other than 'in'. There is no way that I'm aware of to generate this in knex: select min(id) from posts where published_at > (select published_at from posts where slug = ?). To workaround this I've used knex.raw queries which are not very optimal. Opened an issue here knex/knex#614

Second problem: bookshelf doesn't allow me to relax constraints on generated query.
where (slug = ? and status = 'published') is coming from properties set on the model by Posts.forge(data). Tapping into query builder will only allow me to append constraints (i.e. (slug = ? and status = 'published') AND ...), no way to OR them.

There's clearly an easier path to the solution that I don't see yet. But I'm new to all three: knex, bookshelf & ghost, so oh well, the battle goes on.

@ErisDS
Copy link
Member Author

ErisDS commented Jan 7, 2015

@ekulabuhov not sure if it will help, but there was a previous PR which had a set of queries which worked: https://github.com/TryGhost/Ghost/pull/1545/files. The main reason for not merging was that it was before we had the concept of include?= in our API and the queries would have been called every single time you fetched a post.

@ekulabuhov
Copy link
Contributor

@ErisDS thank you, I saw that. My main concern about that implementation is that it will do 3 requests to the database instead of one. But maybe that's okay for the first version.

ekulabuhov added a commit to ekulabuhov/Ghost that referenced this issue Jan 13, 2015
closes TryGhost#4262
- implementation based on TryGhost#1545
- added integration test. Modified mocked posts because code requires published_at timestamps to be different.
- fixed 2 broken tests that depended on mocked posts to have "new Date()" as their timestamps
- added checks to only query db if next/previous post requested
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 help wanted [triage] Ideal issues for contributors to help with
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants