Fix duplicate resources inside included in compound document.

[beauby]

This fixes #1202.

[beauby]

@hut8 Could you test this patch?

[hut8]

@beauby The test LGTM. It does appear to stop producing the duplicate I saw in the original issue. Thanks!

[NullVoxPopuli]

Thanks for verifying @hut8.

@beauby this looks good to me.

[NullVoxPopuli]

why bitwise or here?

[hut8]

[7] pry(main)> lol ||= []
[]
[8] pry(main)> lol |= [1,2,3]
[
    [0] 1,
    [1] 2,
    [2] 3
]
[9] pry(main)> lol
[
    [0] 1,
    [1] 2,
    [2] 3
]

It's fancy syntax for a union
Sauce: http://ruby-doc.org/core-2.1.2/Array.html#method-i-7C

[NullVoxPopuli]

woah. that is fancy. TIL

[beauby]

Rebased.

[joaomdmoura]

I see we are deleting here the duplicated resource, but it seems hacky, it's not addressing the problem itself, just working around it, did you found the main issue causing this?

[beauby]

It is a structural issue. When the primary resource is a single resource (serializable_hash_for_single_resource), we make sure we do not add to included any related resource that is the primary resource. However, when the primary resource is a collection (serializable_hash_for_collection), the adapter works by computing the hash for every resource independently, and then combining the hashes into a big one. This has the advantage of clear code, but the disadvantage that when serializing [a, b] where a includes b, we have no other choice than manually removing duplicates when aggregating.

[joaomdmoura]

got it 😉

[bf4]

deletes resource from included if is a root resource

this included stuff is getting messy. needs a refactor before it sends even more vines through the class

[beauby]

I believe it is the best we can achieve without refactoring the adapter so that it does not call itself recursively for collections (which I'm in favor of, in a subsequent PR). The scope of this PR was to fix a behavior breaking JSON API spec, without modifying the adapter too much.

[joaomdmoura]

@beauby just checked it, made few comments on it for you to check

[beauby]

@joaomdmoura Answered ;)

[joaomdmoura]

LGTM 👍

[NullVoxPopuli]

I'd say because this model and serializer are created here, and specific to this test, they should be namespaced under the test. :-\

[bf4]

I think we agree we'll start doing that soon

[beauby]

Yes. Let's leave them there for now, and take care of it during the Great Test Refactor.

[bf4]

I'm ok to merge, but we need to admit that this inclusion stuff has been a source of bugs and increasing complexity. IMHO, it deserves its own object

[beauby]

@bf4 We agree. I'm happy to work on a refactor once this is merged.