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

Content API V2: Don't return authors and tags without posts #10124

Closed
5 tasks done
kirrg001 opened this issue Nov 7, 2018 · 3 comments
Closed
5 tasks done

Content API V2: Don't return authors and tags without posts #10124

kirrg001 opened this issue Nov 7, 2018 · 3 comments
Assignees
Labels
affects:api Affects the Ghost API server / core Issues relating to the server or core of Ghost

Comments

@kirrg001
Copy link
Contributor

kirrg001 commented Nov 7, 2018

(refs #9866)

This is a change for API v2 only.
The Content API should not return authors or tags with no posts.

Use Case

You have an administrator which organises the blog, but does not write posts.

Things to take care of

  • sitemaps service should not return these urls for v2
  • sitemaps service should return these urls for v0.1

Requirements/Underlying problem

GQL does not support filtering and couting e.g. filter=count.posts:>0. We first need to finish NQL and replace GQL with NQL in Ghost.

The sitemap service depends on the url service. And the url service is independent of API versions. The url service fetches data in bootstrap straight from the model layer. That means urls are being created independently from the API result/behaviour. e.g. v0.1 should show author urls with posts count 0 and v2 should not show them.

IMO we need to refactor the url service to request the configured (by the theme) API version. And if the theme changes the api version, we need to purge the url service and rebuild the cache. The challenge here is that we need to execute raw knex queries, because bookshelf is too slow e.g. api.v2.posts.all()

This needs some more thoughts and architecture ideas.

Hacky solution

It's possible to hack this feature into V2 without resolving the requirements, but it's really ugly and hard to maintain. See PR: #10123

TODO

@kirrg001 kirrg001 added server / core Issues relating to the server or core of Ghost affects:api Affects the Ghost API refactoring/cleanup labels Nov 7, 2018
@ErisDS
Copy link
Member

ErisDS commented Dec 3, 2018

We discussed an alternative solution to this in slack: model splitting.

Pre API versioning, Ghost used a single 1:1:1 table:model:api for each of Posts, Tags and Users.
We no longer have a single API, and there is no reason why a table must only have one model. Bookshelf will allow us to have multiple models, have models that extend one another, have models that share functionality through plugins, and have different APIs depend on different models, but refer to the same tables.

We can represent Published posts, Public Tags (includes internal tags!) and Authors as separate models, which automatically have filters built into them, implemented very similarly to a soft delete plugin.

This will allow us to more easily have frontend business logic defaults for ordering, filtering, limits etc for the Content API, and a different set of more generic defaults for the Admin API.

@naz naz self-assigned this Dec 12, 2018
kirrg001 pushed a commit to naz/Ghost that referenced this issue Jan 3, 2019
refs TryGhost#10124

- Author model returns only users that have published non page posts
naz added a commit that referenced this issue Jan 3, 2019
refs #10124

- Author model returns only users that have published non-page posts
- Added a public controller for tags (should be extracted to separate Content API controller #10106)
- Made resource configuration dynamic based on current theme engine
- This needs a follow-up PR with fixes to the problems described in the PR
@naz
Copy link
Contributor

naz commented Jan 3, 2019

As d3f3b3d has landed in master there are 2 main problems that need to be addressed before the release described in detail here. In short:

  • sitemaps problem has it's root in url service not being able to distinguish that related resources were changed. We need a way to pass information to Resources.js to event's like _onResourceUpdated telling about relational data being changed e.g.: tag was removed (@gargol)
  • routing needs 2 configurations similarly to resource configurations introduced in d3f3b3d (@kirrg001 )

kirrg001 added a commit to kirrg001/Ghost that referenced this issue Jan 4, 2019
refs TryGhost#10124

- one clean v0.1 and v2 config file for routing
kirrg001 added a commit that referenced this issue Jan 4, 2019
refs #10124

- one clean v0.1 and v2 config file for routing!
- solves one underlying bug reported in #10124
- the alias handling was just a hotfix to support v2 for the site
- but it was hard to read, ugly
- now we have two clean configs
- we'll see how useful it is
- need to do proper manual testing on Monday
naz added a commit to naz/Ghost that referenced this issue Jan 5, 2019
naz added a commit that referenced this issue Jan 8, 2019
…ed resources (#10336)

refs #10124

- This PR introduced additional db calls in URL service due to the need for a model recalculation (we can't rely on the objects that come with events)
@naz
Copy link
Contributor

naz commented Jan 8, 2019

All checkboxes ticked, time to close this issue 🎉

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 server / core Issues relating to the server or core of Ghost
Projects
None yet
Development

No branches or pull requests

3 participants