Fix duplicate resources inside included in compound document. #1239
Conversation
|
@hut8 Could you test this patch? |
beauby
force-pushed
the
fix-duplicate-jsonapi
branch
7 times, most recently
from
aa5a000 to
65c82ce
Compare
|
@beauby The test LGTM. It does appear to stop producing the duplicate I saw in the original issue. Thanks! |
| hash[:included] |= result[:included] | ||
| end | ||
| hash[:included] ||= [] | ||
| hash[:included] |= result[:included] |
There was a problem hiding this comment.
[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
There was a problem hiding this comment.
woah. that is fancy. TIL
beauby
force-pushed
the
fix-duplicate-jsonapi
branch
2 times, most recently
from
c301577 to
16440f1
Compare
|
Rebased. |
| end | ||
|
|
||
| included.delete_if { |resource| hash[:data].include?(resource) } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
@beauby just checked it, made few comments on it for you to check |
|
@joaomdmoura Answered ;) |
beauby
force-pushed
the
fix-duplicate-jsonapi
branch
from
16440f1 to
f4bb4c8
Compare
|
LGTM 👍 |
| @@ -1,11 +1,16 @@ | |||
| require 'test_helper' | |||
|
|
|||
| NestedPost = Class.new(Model) | |||
| class NestedPostSerializer < ActiveModel::Serializer | |||
There was a problem hiding this comment.
I'd say because this model and serializer are created here, and specific to this test, they should be namespaced under the test. :-\
There was a problem hiding this comment.
I think we agree we'll start doing that soon
There was a problem hiding this comment.
Yes. Let's leave them there for now, and take care of it during the Great Test Refactor.
|
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 |
|
@bf4 We agree. I'm happy to work on a refactor once this is merged. |
Fix duplicate resources inside included in compound document.
This fixes #1202.