Abstract findPage & add pagination Bookshelf plugin

[ErisDS]

This PR is intended as a discussion starter on where we should be going next with the API vs model layer.

In this PR, I've finished the work of generalising the code in findPage. This included moving repeated pagination-specific code into base/utils.js#pagination and filter-specific code into base/utils.js#filtering. I also moved the parts that were model-specific options or custom code into model functions with horrific names like findPageDefaultOptions(), and then moved the remainder of findPage into base/index.js, calling out to the utils & model specific functions where needs be.

It's now reasonably clean, but there are a lot of steps (especially around option handling), 4 new model-specific functions (which are kinda awkward) and 2 utils that don't really make sense. There's clearly a lot more to do to get this to be sane - and then I'll add some more tests as well of course 😉

Suggested changes

There are a few changes that I wanted to get a bit of feedback on before I forge ahead:

1) API-level option handling

More of the option handling should be done at the API level as described in #2758. At the API level, we need to add validation that the options provided are allowed and valid. Currently there is some option validation inside the model level but this is quite a weird mix between API-logic and model-logic. E.g. the model layer permits include (and uses it heavily), but it should be receiving withRelated - this is subsequently converted in multiple places. Instead, the API should be checking that the includes are correct and converting them to a withRelated call. Conversion from filters to joins & wheres should probably also happen in the API. That way the options passed to the model are the ones that bookshelf will understand.

Fundamentally, I think the API layer should speak api-ish and be able to translate it into bookshelfish. The model layer should only speak bookshelfish.

2) Remove weird filtering behaviour from models

Our findPage functionality does a slightly odd thing where it will fetch a related model (tag, author, role), use it to perform a filter, and then return that model in data.meta.filters. This has an additional annoyance in that it is inconsistent - if you request tag=thing, you get tags: [{thing}] and the same for roles. I could write an essay just on this bit, but the short version is that this behaviour is a bit of a hack so that themes get the data they need. Therefore it can be moved into the channel layer when we get there. Filtering is a whole other ball game and needs to be consistent and extensible - issue coming soon.

3) Split base/index.js up into feature-based 'plugins' or 'mixins'.

Bookshelf supports plugins but we don't necessarily need to go quite that far. However, it would make sense to have a set of mixins covering things like BREAD, pagination, filtering, validation etc. These would provide a place for the pagination & filtering code currently sat in a weird util. This is one of those things where it's hard to know what it looks like til you try it - but does it sound sane? I think care should be taken to not over-do it, but to split out logical groups of stuff.

4) Consider treating BR and EAD differently?

This is just something that's irking me - we treat all of our BREAD methods very similarly (both in the API and Model layer), but they are quite different. B&R need to fetch data with lots of options for determining what sort of data to get. The EAD parts have a lot less options but they change things. I don't even know how this would look - I was just throwing it out there to see if anyone else had any thoughts on refactoring this stuff?

Opinions, thoughts and ideas pretty please!!!

---

refs #2896

• move default options / custom code into model functions
• sepearate more pagination logic into base/utils.pagination
• move most of the filtering logic into base/utils.filtering
• move the remainder of findPage back into base/index.js and remove from posts/users&tags

[sebgie]

1. 👍
2. I think we agreed on using filter during the initial API discussion. The tag endpoint was probably missed/existed before that?
3. Am I understanding it right that 'plugins' or 'mixins' are different modules for different tasks in base/? Getting rid of util or at least change it's name is a good thing but I'm not sure if it makes sense to make it too sophisticated.
4. I don't see a benefit in splitting this up for all resources atm. I think that it would make it more complicated for new contributors to follow and there are more important refactors that could be done (e.g.: permission handling without a promise for every API method). The only module that would imo benefit from this is api/users.js. trasferOwnership, changePassword and sendInviteEmail could be worth a look.

[ErisDS]

2. I think we agreed on using filter during the initial API discussion. The tag endpoint was probably missed/existed before that?

Changing to use a filter param is a separate issue (and I've not written it yet 😉)
I was talking more about fetching extra stuff & particularly putting it into data.meta.filters inside the model layer. Seems like it's totally in the wrong place to me, and double checking with others before I start moving it.

3. Am I understanding it right that 'plugins' or 'mixins' are different modules for different tasks in base/? Getting rid of util or at least change it's name is a good thing but I'm not sure if it makes sense to make it too sophisticated.

Yes, but they'd stay inside the model/base folder for now. I am just imagining a file called, e.g. pagination.js which contains defaults & knows how to do paginated fetches. Bookshelf has fetch and fetchAll perhaps we could have fetchPage. The point is to hide away bits of logic we don't need to care about to give focus back to the bits of Ghost which are more specific - like coping with draft and published posts.
Here are some other nice examples of plugins from the community.

[ErisDS]

This PR has had a bit of an update - I've now split the pagination functionality out into a fully unit-tested plugin. I think this is making sense.

My plan is to add a bit more documentation into the plugin & to key bits of the model layer and call this PR 'done' as it closes #2896.

Next step is probably a new PR to do the work to move the filter logic & option handling for the API described in #2758.

Alternatively I can keep pushing commits here until the utils file is gone.

[ErisDS]

I've updated this PR again 😄

The new pagination.js file is both 100% unit tested and fully documented. (Try running grunt docs to see the nicely formatted version - will host this somewhere soon).

findPage is refactored out of individual models, and the code for handling pagination is tucked away in pagination.js.

I also started adding a bit more documentation to the base model file, as a little bit of an indicator of how I imagine clarifying the code in there and which parts are bookshelf settings, which parts override bookshelf functions, and which parts are Ghost-specific.

#2896 is now closable, I'm in two minds as to whether these 2 commits should be squashed or not.

Feedback would be very much appreciated.

[sebgie]

😍

I'm in two minds as to whether these 2 commits should be squashed or not.

+1 for squashing

[ErisDS]

Updated to move module.exports to the bottom & squashed the commits together