Add regression test for MockPayloadGenerator nested-array bug with abstract InlineFragment (#5258)#5273
Closed
markselby9 wants to merge 2 commits into
Closed
Add regression test for MockPayloadGenerator nested-array bug with abstract InlineFragment (#5258)#5273markselby9 wants to merge 2 commits into
markselby9 wants to merge 2 commits into
Conversation
…stract InlineFragment (facebook#5258) Adds a regression test in RelayMockPayloadGenerator-test.js demonstrating the bug reported in facebook#5258: when a query field returns a concrete type and the same plural linked field is selected both directly (concrete path) and via a fragment spread on an interface the concrete type implements (abstract path), MockPayloadGenerator produces a nested-array shape [[e1,e2,e3], [e1,e2,e3], ...] instead of a flat [e1, e2, e3]. Root cause (per the issue): _traverseSelections processes the concrete LinkedField and the abstract InlineFragment sequentially. On the second pass, _mockLink reads the already-generated array out of data[applicationName] and passes the whole array as prevData to each new element generated by generateMockList. The OSS test schema does not naturally model a concrete-vs-abstract return-type divergence on the same field name (the compiler dedupes the two selections when types match), so this test mirrors the issue's "Standalone Reproduction Script" and hand-constructs a ConcreteRequest matching the AST shape from the issue body. MockPayloadGenerator.generate walks the AST and does not consult a schema, so the type names need not exist in the test schema. Test asserts current (buggy) behavior. The fix is left to the maintainer; flip the assertion when the underlying bug is fixed. Refs facebook#5258.
Address review feedback on the regression test: - Replace weak `Array.isArray(edges[i])` checks with a strict structural assertion of the exact buggy payload — three copies of the original edges array, each Array instance also carrying the abstract path's `__typename`/`cursor` properties bolted on. Likely partial fixes (collapsed nested arrays, single-edge-wrapped elements, missing properties) now break the test loudly instead of silently passing. - Rename test to `BUG: produces nested-array shape instead of a flat array (facebook#5258)` so the name reads as a bug record, matching the sibling `(facebook#5119)` style. - Add a scope note acknowledging that the issue's "Root Cause Analysis" point facebook#3 (`mockData[TYPENAME_KEY] = selection.type` forcing abstract type onto already-generated concrete data) is a separate symptom not asserted here. - Note in the header that a schema-extension prototype was tried — the OSS compiler folded the abstract InlineFragment because the parent field's static type pinned the narrowing — explaining why the hand-constructed AST is retained.
Contributor
Author
|
Closing for now — I want to verify the compiler actually emits the AST shape this test asserts before adding to the review queue. Following up on #5258 to ask for a compiler-generated artifact, will revisit once that's in hand. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a regression test for #5258: when a query selects a plural linked field on both a concrete type AND an abstract InlineFragment at the same selection level,
MockPayloadGenerator.generateproduces a nested-array payload ([[edge, edge, edge], [edge, edge, edge], [edge, edge, edge]]) instead of the expected flat[edge, edge, edge]shape, with__typename/cursorproperties also bolted onto each Array instance.The test asserts current (buggy) behavior so it flips when the underlying bug is fixed — same pattern as #5268 and #5269.
Notes
ConcreteRequestAST rather than using agraphqltemplate literal. The OSS test schema doesn't naturally model the concrete-vs-abstract divergent-return-type pattern that triggers this bug (every plural field returning an interface returns the same[Actor]from both interface and implementer, so the compiler dedupes the selections). I tried a schema extension addinginterface AbstractConnection { edges: [AbstractEdge] }+type ConcreteConnection implements AbstractConnection { edges: [ConcreteEdge] }, but the relay compiler folds the abstract InlineFragment when the parent field's static type pins the abstract narrowing. Happy to revisit if you have a preferred path; the test header documents this attempt so future contributors don't re-tread it.__typenameoverride on the abstract path) is a separate adjacent symptom and is explicitly out of scope here.Test plan
yarn jest RelayMockPayloadGenerator— full file passes (69 tests)yarn linton the test file — clean__typename/cursorproperties via deeptoEqualRefs #5258