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

Configurable inclusion for relationship object #159

Closed
bjornharrtell opened this issue Jun 12, 2017 · 8 comments
Closed

Configurable inclusion for relationship object #159

bjornharrtell opened this issue Jun 12, 2017 · 8 comments
Labels

Comments

@bjornharrtell
Copy link
Contributor

As described by http://jsonapi.org/format/#document-resource-object-relationships a "relationship object" MUST contain at least one of links, data or meta. When desired to lazy load related data it makes sense to only include the links key and omit data. This does not appear current possible with Saule so I propose to make that configurable.

Somewhat related to #121.

@joukevandermaas
Copy link
Owner

I think this is supported (although maybe not in the way you expect):

  1. If there is a property for the relationship on the object, and the value is not null, include it
  2. If there is a property for the relationship and the value is null, serialize data: null.
  3. If the property for the relationship is not there, serialize only links.

I think there are tests that verify this behavior.

I think that when the user sets DisableDefaultIncluded and the user does not include the resource, the data hash should be omitted in favor of a links hash. I would prefer not to add any configuration or settings beyond that.

@bjornharrtell
Copy link
Contributor Author

I cannot verify that behaviour. At least for case 3 it seems I get data: [] (regardless of DisableDefaultIncluded). Perhaps there is only a need for a fix for when using DisableDefaultIncluded.

@joukevandermaas
Copy link
Owner

Sorry, the last paragraph refers to how I think it should work. I can see now that I was not entirely clear 😄

@bjornharrtell
Copy link
Contributor Author

Ah :) Trying to fix that I find that it's a problem that a Resource class is not strictly related to the model it represents so I can't see if it has a property that represents the relationship. Have thought about that earlier but it had no real impact until now... to me it would perhaps make sense to point a Resource to a Model type, but that brings up the idea to bring Resource and Model closer in other ways (auto-defining attributes from properties etc) and would rather not go into that now.

@bjornharrtell
Copy link
Contributor Author

FYI will continue to try to find the easiest way to fix no 3.

@bjornharrtell
Copy link
Contributor Author

I think I've found a fix for no 3 in #161 but it's currently failing the test case "Handles recursive properties on resource objects" where I'm not sure if the old behaviour should/need to be reproduced as is by making the check recursive.

@bjornharrtell
Copy link
Contributor Author

Looked at the test case "Handles recursive properties on resource objects" again and can't make sense of it really, at least in the context of how it relates to resource inclusion. :( @joukevandermaas can you shed some light?

@bjornharrtell
Copy link
Contributor Author

With #161 this issue is no longer relevant, at least for me, so I will close it.

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

No branches or pull requests

2 participants