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: Apply 'pipeline' refactor to the other endpoints #5508

Closed
11 of 13 tasks
ErisDS opened this issue Jul 2, 2015 · 1 comment
Closed
11 of 13 tasks

API: Apply 'pipeline' refactor to the other endpoints #5508

ErisDS opened this issue Jul 2, 2015 · 1 comment
Labels
affects:api Affects the Ghost API help wanted [triage] Ideal issues for contributors to help with server / core Issues relating to the server or core of Ghost

Comments

@ErisDS
Copy link
Member

ErisDS commented Jul 2, 2015

In 51ac3f6 I refactored the post, user and tag endpoints to use our pipeline promise utility.

This means that serving an API request becomes a matter of taking inputs (either just options, or object & options) and passing them through a series of functions which either return a value or a promise for a value. The output form one function is passed as the argument to the next, forming a chain somewhat similar to a middleware stack.

If at any step, there is an error or promise rejection, the pipeline returns an error, meaning that validation, permission handling, and data fetching can all be steps which return their own errors and prevent the process from continuing if they go wrong.

The best thing about this approach is that it wraps individual lines of endpoint code in functions which describe what happens, like validation and handlePermissions rather than having a large block of promise code. This has reduced the codeclimate score, as the vast amounts of code duplication are now obvious, whereas before they were hidden by lines being in slightly different orders.

It would be great to continue rolling out this refactor across the other endpoints. This could be done as a single task, or as one PR per endpoint - some of them are trickier than others (and in rough order of priority):

  • Posts
  • Users
  • Tags
  • Settings
  • Authentication
  • Roles
  • Configuration (too simple to need it)
  • Themes
  • Upload
  • Slugs
  • Notifications
  • DB
  • Mail
@ErisDS ErisDS added server / core Issues relating to the server or core of Ghost affects:api Affects the Ghost API labels Jul 2, 2015
@ErisDS ErisDS added this to the Current Backlog milestone Jul 2, 2015
halfdan added a commit to halfdan/Ghost that referenced this issue Jul 2, 2015
@ErisDS ErisDS mentioned this issue Jul 7, 2015
31 tasks
halfdan added a commit to halfdan/Ghost that referenced this issue Jul 22, 2015
acburdine added a commit to acburdine/Ghost that referenced this issue Aug 10, 2015
refs TryGhost#5508
- adds pipeline to the add and destroy methods of the notifications api
@ErisDS ErisDS modified the milestone: Current Backlog Oct 9, 2015
@ErisDS ErisDS added the help wanted [triage] Ideal issues for contributors to help with label Oct 9, 2015
@ErisDS ErisDS added this to the Public API v1 milestone Oct 13, 2015
@ErisDS ErisDS mentioned this issue Oct 20, 2015
24 tasks
@vadimdemedes
Copy link

I'd like to pick this one up and finish the parts, that are unchecked.

I think a refactoring to pipeline utility itself would also be a bonus (as a separate PR). At the moment, it is very confusing to figure out how it works by looking at its implementation. I'd also like to write tests for the pipeline util, because grep could not find any in core/test, so I assume they don't exist.

halfdan pushed a commit to halfdan/Ghost that referenced this issue Mar 28, 2016
@ErisDS
Copy link
Member Author

ErisDS commented Mar 28, 2016

This issue is almost done, with the exception of upload (#6648) and settings (#6028).

Upload I think is close, but the tests possibly need some work.
Settings is such a weird endpoint, it works sooo totally different to the others that it doesn't quite follow the same pattern. When #6028 was raised I self-assigned and marked it as hold because there were some changes in there that didn't quite look right.

Particularly to do with the change from settingsResult to formatResult where type seems to be missing, and handlePermissions not being split out for read and browse. Again I think the tests perhaps need some extra work to ensure the behaviour isn't changing in obscure cases.

I've tried to update, fixup & rebase #6028 but am not feeling confident with it yet. If anyone else fancies a stab grab it, else I'll try again soon.

@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 help wanted [triage] Ideal issues for contributors to help with server / core Issues relating to the server or core of Ghost
Projects
None yet
Development

No branches or pull requests

2 participants