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 V2 content controller separation #10106

Closed
1 task done
kirrg001 opened this issue Nov 6, 2018 · 5 comments
Closed
1 task done

API V2 content controller separation #10106

kirrg001 opened this issue Nov 6, 2018 · 5 comments
Assignees
Labels
affects:api Affects the Ghost API server / core Issues relating to the server or core of Ghost stale [triage] Issues that were closed to to lack of traction

Comments

@kirrg001
Copy link
Contributor

kirrg001 commented Nov 6, 2018

I see some good reasons why we should separate the controllers for the content API.

  1. You don't have a limited access if the controllers are generic. For example: the V2 content API allows the theme developer to fetch "authors" {{#get "authors"}}. The get helper requires the target API version (require(.../api/).v2), but we require all of the controllers. That means we either need to disallow this behaviour in the helper level, so that you can't do {{#get "users"}}, which is very hard to maintain or the API returns just a set of controllers you can use for the content API.

  2. Admin controllers allow more than content controllers. e.g. the users controller allows to fetch roles & permissions. In v0.1 there was one controller who just allowed all the includes (roles, permissions, couting posts) and the model layer had added this condition. to avoid that a public access can fetch roles. This is very hard to maintain and it's IMO not the correct approach. If we would separate the controllers, we could just allow that the content user controller can fetch count.posts, nothing more.

  3. We always need to take care that we do a correct differentiation between content and admin access e.g. in serializers.

  4. Imagine you want to develop both API's separately. You can't. Because they both use the same controller base.

I can imagine if we separate both, we can optimise the API frame step by step to avoid having too many duplicate code.

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

naz commented Dec 14, 2018

As model separation is introduced with #10124, there has surfaced a need to separate tags controller for tags model used in public context. Wanted to discuss a folder structure for new controllers that are only meant to be used by the Content API.

Would it make sense to divide v2 controllers to a structure similar to the one used in routes /web/api/v2/(admih|content)? The reason for this would be to avoid naming controllers in the same folder like tagsPublic or postsPublic if such controllers are introduced in the future.

@kirrg001
Copy link
Contributor Author

Would it make sense to divide v2 controllers to a structure similar to the one used in routes /web/api/v2/(admih|content)

Yes makes sense to me 👍

kirrg001 pushed a commit to naz/Ghost that referenced this issue Jan 3, 2019
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
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Feb 21, 2019
kevinansfield pushed a commit that referenced this issue Feb 22, 2019
refs #10438, refs #10106

* Renamed existing pages ctrl
* Splitted posts & pages for Admin API v2
* Added pages JSON input schema for Admin API v2
* Removed single author for Content & Admin API v2
  - single author is not documented
  - single author usage is deprecated in v0.1
  - single author usage is removed in API v2
* Splitted posts & postsPublic controller for v2
* Removed requirement to send `status=all` from Admin API v2
* Removed `status` option from pages Content API v2
* Removed `status` options from Users Admin API v2
@kirrg001
Copy link
Contributor Author

kirrg001 commented May 6, 2019

State of this issue

API v2 fully separated the Admin & Content controllers.

  • pagesPublic -> /content/pages
  • postsPublic -> /content/posts
  • tagsPublic -> /content/tags
  • settingsPublic -> /content/settings
  • authorsPublic -> /content/authors

The rest of the controllers belong to the Admin API. This was a first good step into the right direction.

Further or existing problems

  • (1) is not solved yet
  • (3) is not solved yet
  • If you require the API, you get all Content & Admin API controllers, but you need to know that api.postsPublic is the Content API implementation (specific knowledge)

Options

Option 1

We could create api/v2/content and api/v2/admin and keep the current structure (e.g. from api/v2/utils/validators/ to api/v2/content/utils/validators).

See a working prototype on Github.

Adavantages:

  • Solves (1) and (3)
  • The pipeline & stages implemention doesn't need a refactoring
  • e.g. Apps could only get access to the Content API

Challenges:

  • We need to take care that the "shared" implementation must be valid for Content & Admin API requests, which it is currently.
  • Dynamic routing needs to require the Content API. Some bit's need to be refactored out e.g. this piece. Would need to collect more places.
  • We need to handle a lot more code and files. We already have many files inside the API layer.
  • API v2 Content & Admin implementation would need to share a lot of logic e.g. url decoration etc. We would need a shared folder for both (e.g. api/v2/shared). Increases complexity.
  • Refactoring should only happen when we've dropped v0.1, because e.g. dynamic routing is generic and API v0.1 has no concept of a content and admin API folder. Otherwise we would need to support a controller configuration per API version with folders e.g. from postsPublic -> content.postsPublic.

Option 2

Leave as is and improve what we have to get rid of the problems e.g. which controllers the theme helpers can access.

@stale
Copy link

stale bot commented Aug 4, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale [triage] Issues that were closed to to lack of traction label Aug 4, 2019
@naz naz removed the stale [triage] Issues that were closed to to lack of traction label Aug 5, 2019
@stale
Copy link

stale bot commented Nov 19, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale [triage] Issues that were closed to to lack of traction label Nov 19, 2019
@stale stale bot closed this as completed Nov 26, 2019
daniellockyer pushed a commit to TryGhost/framework that referenced this issue Jun 15, 2021
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 TryGhost/Ghost#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
daniellockyer pushed a commit to TryGhost/framework that referenced this issue Jun 15, 2021
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 TryGhost/Ghost#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
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 stale [triage] Issues that were closed to to lack of traction
Projects
None yet
Development

No branches or pull requests

3 participants