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

Feature/multi level includes #287

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Tronix117
Copy link
Collaborator

This PR allows to have nested includes.

Exemple:

GET /posts/1?filter[include][comments]=author

will retrieve all comments, as well as the author of each comments


I also removed the unecessary "requestedIncludes" option.

This option should not handle the way this adapter serialize the resources, if there is embedded resources, even if they do not appear in this option, recommended behavior is to have them in the include property as well.

Security consideration: nested includes were previously not handled, it is now the case, like in the loopback documentation, users need to ensure it is secured their side, like they do when using loopback without JSONApi.

BREAKING CHANGE: remove "requestedIncludes" option
This option should not handle the way this adapter serialize the resources, if there is embedded resources, even if they do not appear in this option, recommended behavior is to have them in the include property as well.
Security consideration: nested includes were previously not handled, it is now the case, like in the loopback documentation, you need to ensure it is secured your side.
@coveralls
Copy link

coveralls commented Aug 21, 2018

Coverage Status

Coverage increased (+0.4%) to 94.911% when pulling dc36ee0 on Tronix117:feature/multi-level-includes into 40f9282 on digitalsadhu:master.

@Tronix117
Copy link
Collaborator Author

@digitalsadhu some feedback ?

@Leito987
Copy link

Leito987 commented Oct 10, 2018

if you make a call with relationship like:

GET /company-users/{id}/user

and that the ressource linked don't existe we have a error. Should be catch it.
TypeError: Cannot read property 'relationships' of null\n at /loopback-component-jsonapi/lib/serializer.js:373:18

@digitalsadhu
Copy link
Owner

@Tronix117 I'm afraid I don't have time to get my head back into this project. I'm thinking also with the new version of Loopback being a very major breaking change this project might naturally reach an end of life before too long as I won't be attempting to port anything. I'm happy for you to merge PRs as you see fit and continue maintenance if you have the time.

@Tronix117
Copy link
Collaborator Author

That’s what I though, from our side we are using it on a big production project and forme time to time we have some evolutions to push. I will do what you say, is npm publishing also fully automatic ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants