included parameter potentially includes the object itself

[hb9hnt]

Let's say we have a class Room and a class Person with a foreign key to Room. Now I want to create a new person in the room and reload all the people already in the room because adding the person might have changed them for some reason. Therefor I send a POST request containing the new person to

persons/?include=room.persons`

This lead to the newly created person to be included twice, once in the data property of the reply and once in the included property. Even though this isn't explicitly forbidden by jsonapi.org, Ember doesn't like this. IMHO an object shouldn't be included twice in a reply - meaning that this should be fixed on the DJA side of things.

[sliverc]

I think this is a tricky issue as it might be that different frameworks have different expectations. Question is what the specification defines in the end to see where it needs to be fixed.

Specification of discussion would be on compound documents which sound a bit ambiguous on this issue.

This said do you have any other discussion links within the Ember or JSON API community where this issue has been discussed?

[hb9hnt]

I didn't post this anywhere else. When I re-read the specification I thought it is pretty clear on this:

A compound document MUST NOT include more than one resource object for each type and id pair.

Note: In a single document, you can think of the type and id as a composite key that uniquely references resource objects in another part of the document.

Note: This approach ensures that a single canonical resource object is returned with each response, even when the same resource is referenced multiple times.

[sliverc]

In a compound document, all included resources MUST be represented as an array of resource objects in a top-level included member.

Here the specification states that compound document is the top-level included member. This doesn't include the default resource object, so the statement that A compound document MUST NOT include more than one resource is as I understand it only relevant for the included member.

If it is understood this way how DJA implements it now would be OK.

I think in terms of payload fixing this would actually make sense, I am just not so sure on the clarity of the specification on this point, so it would be good to clarify it.

[hb9hnt]

The crucial point is that it says in your quote in a compound document. It doesn't say that the included top level member is the compound document itself.

IMHO the specification is quite clear about what a compound document is:

To reduce the number of HTTP requests, servers MAY allow responses that include related resources along with the requested primary resources. Such responses are called “compound documents”.

i.e. the full response, including the data and the included top level member are part of the response called a compound document.

However, I will open an issue on the JSON API side later today.

[sliverc]

I have overread this part, but with this in mind I agree that a resource object should not be added twice including primary data.

I guess it never can hurt to double check though with JSON API. Marking this as bug and a Pull Request is very welcome.

[hb9hnt]

I didn't have the time to create the issue at JSON API. I'll leave it at that and look into fixing the problem. Would you happen to have some pointers for me to where to start looking to fix this?

[sliverc]

The resource itself should be part of the included_cache so it will be skipped in the renderer if it gets included again.

See https://github.com/django-json-api/django-rest-framework-json-api/blob/master/rest_framework_json_api/renderers.py#L383

[hb9hnt]

I'm almost ready to submit the PR. Before I do a couple of questions:

• I wrote a integration test which tests this case but no unit test. Do we also need to unit-test this case?

• Adding the resource itself to the included_cache is IMHO not the best approach as we then have to removed it again before building the included attribute of the response. Therefor I decided to just check in the end whether it is in the cache and remove it in this case. Could this lead to any problems I didn't yet fully grasp?

[hb9hnt]

I just sent a PR for you to check out what I did. Feel free to reject the PR if it doesn't include everything I should have provided.