Open factyy opened 3 months ago
Important note: the current implementation doesn't support concurrency, will work on it a bit later
Added configuration compatibility checks + tests
@jkeen , just a humble reminder :)
BTW do you have any ideas what is wrong with the tests? It seems like a few of them are failing almost randomly...
@factyy Thanks for this effort!
With your description it definitely sounds like a bug, and therefore whatever fix it involves shouldn't be behind a feature flag. That being said, adding de-duplication logic that runs all the time sounds like a potential performance bottleneck, so I want to be very careful with how we proceed with this fix as this will affect every render (and json-api rendering speed in graphiti is… already an area that needs raw speed improvements IMO).
Can you describe other scenarios this bug might appear without using a remote resource? Or is it only with remote resources?
Also, I think whatever fix this involves should support concurrency out of the gate? Does this current PR include concurrency considerations?
And yeah, there are some intermittent failures in the tests it seems that I haven't tracked down quite yet.
@jkeen , I previously considered fixing it in a phase when resources are rendered (even filed a PR I mentioned earlier) but decided to go this way (de-duplication when resources are populated) since in this case we lose only a tiny bit of performance.
The problem is not anyhow related to remote resources. Just use any field that has to be included that is both non-DB provided (i.e. it can be populated in the resource code and not in the model code) and is located in an included resource itself. It can be reproduced in a project with no remote resources (if you meant resources retrieved from 3rd parties using separate APIs for example), just plain ActiveRecord
and Graphiti.
Regarding concurrency it is a bit tricky: as far as I understand we may either:
So this PR doesn't allow de-duplication to be used alongside concurrency - there are checks for such cases.
Regarding tests: got it. Maybe we'll find out the problem too but right now we are severely time-constrained sadly...
I completely get your point. Basically why this fix is behind a feature flag? It is for the very same reasons you described above: fixing an edge case with performance penalties (causing compatibility problems along the way).
I'd be happy to re-engineer it somehow that would allow it to be applied as a general fix (without a feature flag) but I don't know how to be honest.
@jkeen , just got my hands on the O(n) issue, fixed now.
Regarding incompatibility with concurrency: I remember encountering a weird deadlock problem with concurrency (before PRs affecting the mechanism) so we decided to just turn it off for a while, but never got back to this issue. Could you check that everything works? If this is the case I can just synchronize everything and we'll see how much it will affect performance.
With the current implementation in Graphiti it is possible to end up with incomplete objects in the
included
section.The easiest way is to have a non-DB backed relationship in one of your resources. Then query this resource as a child (by multiple paths) of a parent resource and then query this relationship from the child.
The easiest way to describe it is to just provide the following test case (added to tests as part of this PR):
And then the test case itself:
The result will contain
"data": null
in thecustom_department
object in theincluded
list.The problem is that when constructing the response tree these different associations (
positions
andcurrent_position
) are represented by separate objects (creating duplicates) although they can contain the same object multiple times. Non-DB backed relationships are populated only for respective objects (leaving other copies with empty relationships since they most probably are just plainattr_accessor
fields in the models). When rendered only the first copy of the object is rendered thus resulting in partial objects in theincluded
list (if you are not lucky and the first copy doesn't contain all the data needed).This problem can be solved either when rendering (https://github.com/jsonapi-rb/jsonapi-renderer/pull/41) or when constructing a response (approach taken by this PR).
Changes:
deduplicate_entities
which is disabled by default for backward compatibility);P.S: this PR changes a fair amount of internal response constructing mechanics so any feedback is really welcome!