zachahn
commented
1 year ago Thanks for the review @grxy! I've addressed your comments.
I definitely understand your concern around a performance regression, but I don't think I'll have the time to make this as "efficient" as before in the near term (I don't believe it's correct to call a broken implementation "efficient"). I suspect that the worst case scenario is somewhat unlikely since the bug has been around for 8 months with no issues I'm aware of. This leads me to assume that most calls ask for a single type.
I'm quite sure that it's possible to maintain a similar performance characteristic as the broken code. I have a vague idea of an approach, and I'd be happy to work on it when I have some more time, but I'd also be happy to see someone else take on this challenge if they beat me to it! I believe the new test case I added should help to ensure there is no future regression.
Do feel free to edit the code in my branch—but I do think that this bug is impactful! We noticed this when the items in our catalog had mismatching prices.
grxy
Hello!
I ran into this bug a couple weeks ago. It took me a little while to find the cause of this fix, but this seems to be working well for us.
The first commit is a stylistic change—I'm happy to revert it. The second commit demonstrates the failing case, and the last fixes it. I wrote a detailed description of the change in the final commit as well.
I do know that this solution is a little less efficient than the previous version. Theoretically, if there is a request asking for N entities of
[TypeA, TypeB, TypeA, TypeB, TypeA, TypeB, ...], this would result in N calls toresolve_references.I'm quite sure that it would be possible to make this almost as efficient as before (I mean, 1 call to
resolve_referencesper distinct type). However, I'd like to consider that out of scope of this PR for now.Additional details from last commit message
Fix ordering of entities returned via `.resolve_references` The previous implementation incorrectly shuffled the order of entities when different `__typename`s were requested and interleaved. Here are some examples: ``` working_case_1 = [ # all of one type { __typename: typename, id: 1 }, { __typename: typename, id: 2 }, { __typename: typename, id: 3 }, ] working_case_2 = [ # not interleaved { __typename: typename, id: 1 }, { __typename: typename, id: 2 }, { __typename: another_typename, id: 3 }, { __typename: another_typename, id: 4 }, ] broken_case = [ # interleaved { __typename: typename, id: 1 }, { __typename: typename, id: 2 }, { __typename: another_typename, id: 3 }, { __typename: another_typename, id: 4 }, { __typename: typename, id: 5 }, ] ``` The `broken_case` example would likely return a response where the IDs are `[1, 2, 5, 3, 4]`, that is, the all of `typename`s first, then followed by all `another_typename`s. However, the correct response would be one that returns IDs in the order of `[1, 2, 3, 4, 5]`, exactly as requested. This is problematic because Apollo Federation joins responses together only by the order of those responses. If any response is out of order, then the final response would also be out of order. Whereas the broken implementation would have called each type's `resolve_references` only once, this commit's implementation will now call it once per type's "chunk" or "consecutive streak".