Related resources

[bjornharrtell]

Rebased from previous PR #136.

[bjornharrtell]

Implementation seems to work in some limited tests. Some test cases was failing but I've now managed to fix them. @joukevandermaas this is now ready for review.

[bjornharrtell]

There are two behavioral changes that I've seen with this PR:

1. Before it seems the default behavior was to return all relationship data as included. With this PR relationship data will not be included by default and must be explicitly requested with the include param. I think this behavior was added intentionally by @yohanmishkin but I'm not sure about the reasoning.

2. Before empty relationships was included. With this PR they are not. I can't find if the spec says one of these two is the correct behavior.

@joukevandermaas can you clarify "Is there some way to dis-allow the client from including resources? The endpoint still has to support it for it to work."?

[joukevandermaas]

@bjornharrtell

1. I think that's fine in theory, but it's a breaking change. I'd rather keep the current behavior and allow users of Saule (API writers) to opt into the new behavior, e.g. with an [AllowClientInclude] attribute on the action method, or a parameter to the BelongsTo and HasMany functions in ApiResource.

This is also what I meant by "is there some way to dis-allow the client from including resources? The endpoint still has to support it for it to work." The API writer should be able to control whether the client can or cannot include a given relationship.

2. IIRC empty relationships were converted into a link (they were serialized with a links hash that allowed clients to fetch the data async).

{
  "attributes": { ... },
  "relationships": {
    "job": {
      "links": {
        "related": "http://.../job",
        "self": "http://.../relationships/job"
      }
    }
  }
}

I think that should still happen if the client did not ask to include the resource or if the resource property does not exist on the model.

[bjornharrtell]

Thanks for the explanations, I think it makes sense.

[yohanmishkin]

1. We had some heavy objects with many nested relationships so that's why I made inclusions opt-in by default, but it makes sense to me to avoid introducing breaking changes.

2. To be honest, I'm not positive why I removed empty relationships, but I think it may have been ember-cli-mirage inspired.

[bjornharrtell]

Ready for review once more @joukevandermaas. Things done:

1. Should now be without any breaking change for API consumers
2. Default is to include related data but can be opt-out via action attribute (this concerns only the case where the include param is not used)
3. Added test for included resource referenced in multiple resources

[bjornharrtell]

I've refactored to keep the state of DisableDefaultIncluded on IncludedContext instead of QueryContext, and assuming false if IncludedContext is null. Ready for review yet again @joukevandermaas.

[joukevandermaas]

@bjornharrtell Thanks so much for seeing this through! I'm going to merge this and publish a new beta release with the changes.

@yohanmishkin Thanks for your contributions to this!