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 layer refactoring && avoid bypassing filtering options in User model #9068

Merged
merged 2 commits into from
Sep 28, 2017

Conversation

kirrg001
Copy link
Contributor

@kirrg001 kirrg001 commented Sep 27, 2017

Please merge both commits separate.

First commit: Refactored the API layer: do not handle API response after pipelining

  • this has a big underlying problem
  • each task in the pipeline can modify the options
  • e.g. add a proper permission context
  • if we chain after the pipeline, we don't have access to the modified options object
  • and then we pass the wrong options into the toJSON function of a model
  • the toJSON function decides what to return based on options
  • this is the easiest solution for now, but i am going to write a spec if we can solve this problem differently
  • if we want to use the toJSON function to determine which model fields to return, we cannot do that without this change!

NOTE: I thought about a util for the response format, but i decided against at the moment, because i am not sure if we have to change a fundamental concept of how the API works (who returns what, who returns JSON, how do we guarantee that the options object is everywhere accessible, how are headers generated etc.). See for example this here - it only happens because the outer API level wants to add headers based on the state of a model.

Second commit: Removed bypassing option filtering in User model

  • the tests were red on the first and i investigated why
  • i had to create a PR with both commits, because the second commit is a result of the correctness of the first commit
  • the logic in the User model bypasses filtering options!
  • that is wrong, because if we filter out certain options e.g. include
  • the tests from the previous commit fail because of this
  • if we don't fix this logic, the tests won't pass, because as said, you can bypass certain logic e.g. remove roles from include
  • this has worked before, because we passed the wrong options via the API layer
  • was introduced here 014e2c8, because of Add count.posts support to user model on client #6122
  • add proper tests to proof that these queries work!!

no issue

- this has a big underlying problem
- each task in the pipeline can modify the options
- e.g. add a proper permission context
- if we chain after the pipeline, we don't have access to the modified options object
- and then we pass the wrong options into the `toJSON` function of a model
- the toJSON function decides what to return based on options
- this is the easiest solution for now, but i am going to write a spec if we can solve this problem differently
no issue

- the logic here bypasses filtering options!
- that is wrong, because if we filter out certain options e.g. include
- the tests from the previous commit fail because of this
- if we don't fix this logic, the tests won't pass, because as said, you can bypass certain logic e.g. remove roles from include
- this has worked before, because we passed the wrong options via the API layer
- was introduced here TryGhost@014e2c8, because of TryGhost#6122
- add proper tests to proof that these queries work!!
@kirrg001 kirrg001 changed the title [WIP] API layer refactoring && avoid bypassing filtering options in User model API layer refactoring && avoid bypassing filtering options in User model Sep 27, 2017
@kirrg001
Copy link
Contributor Author

@ErisDS Ready for review.

@ErisDS ErisDS merged commit e347163 into TryGhost:master Sep 28, 2017
@ErisDS
Copy link
Member

ErisDS commented Sep 28, 2017

🎉 This looks really good 👍

kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Sep 28, 2017
closes TryGhost#9077

- because of our API layer refactoring, see TryGhost#9068
- we can now see that code was written wrong because of this horrible API bug
- this fixes the formats parameter for querying a single post
kevinansfield pushed a commit that referenced this pull request Sep 28, 2017
closes #9077

- because of our API layer refactoring, see #9068
- we can now see that code was written wrong because of this horrible API bug
- this fixes the formats parameter for querying a single post
@ErisDS ErisDS removed their assignment Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants