Fix traverse exiting early after custom-scalar array conversion#631
Merged
zth merged 7 commits intoJun 1, 2026
Merged
Conversation
Owner
|
Hi Thank you for this! I don't mind LLM PRs as long as you have thought it through and verified it all yourself. I believe this issue is related to this as well: #582 This looks good to me, but I would like you to have your LLM add more tests in general. Not necessarily for the concrete fix itself, but for the behavior around it, so we can be a bit more sure that this does not regress anything. Ask your LLM about this and see what it says. |
ValdemarGr's stale PR zth#583 added a schema input shape, two custom scalar modules, and a relay-config registration to reproduce issue zth#582 (encoding doesn't always apply scalar encoders). This commit lifts those fixtures in preparation for a full integration test for zth#582 and broader regression coverage in this PR. ValdemarGr's ObjectScalar serializers and parsers also log to the console, which the harness in a later commit uses to assert which converters actually ran for a given operation. The Datetime and Number serializers in TestsUtils gain similar console.log lines (matching the existing pattern in IntString) so a follow-up zth#407 regression test can verify that a non-array custom scalar variable still has its serializer run when it follows an array-backed scalar variable in the same operation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ation input Reproduces the bug ValdemarGr reported in issue zth#582 using SerializeMultipleCustomScalars: the input has `os1s: [ObjectScalar1!]!` followed by `os2: ObjectScalar2`. On master the early-return in the `ca` branch of traverse (utils.js:109-113) skips the `c` instruction for `os2`, so ObjectScalar2.serialize is never called. The harness asserts that both serializers' log lines fire. The fix for zth#582 lands later in this PR; on this commit the test fails for the right reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Read-side analog of the bug ValdemarGr's zth#582 reports on the encoding side: a `c` custom-scalar field (createdAt: Datetime!) follows a `ca` custom-scalar array (intStrings) in a query selection. On master the early-return in traverse (utils.js:109-113) leaks the raw wire string through Object.assign untouched, so createdAt never reaches Datetime.parse. The harness renders `createdAt->Date.getTime->Float.toString` — Date.getTime on a raw string throws, so the assertion never resolves on master. Fails on this commit, passes once the fix lands later in this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `c && Array.isArray` branch in traverse (utils.js:134-138) was added by PR zth#434 to close issue zth#407 ("Array scalars are not well behaved"). It correctly handles single custom scalars whose ReScript representation is an array — but it terminates with `return` instead of `continue`, so sibling fields processed after the array-backed scalar in the same selection set have their converters skipped. This test runs a query with two variables: `$aaNumber: Number!` (custom scalar whose underlying type is array<int>) and `$beforeDate: Datetime`. relay-compiler sorts variables alphabetically in the generated record, so the `aa` prefix forces the array-backed scalar to be iterated first. On master only Number.serialize runs; Datetime.serialize is silently skipped. The fix later in this PR extends zth#434's intent so trailing fields are preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…m-scalar array A query selects `intStrings` (a `[IntString!]`, `ca`) followed by nullable plain scalars on the same parent. On master the early- return in the `ca` branch of traverse (utils.js:109-113) leaves those siblings copied through Object.assign untouched, so their raw wire `null` reaches the caller without going through the null coercion at the top of the per-key loop. ReScript then reads the leaked `null` as `Some(null)` instead of `None` (pattern matches take the wrong branch). The harness uses Type.classify (via Obj.magic + Null.toOption) on the `private` payload so the rendered DOM literally reads `private=Some(null)` on master — the smoking-gun assertion. The fix later in this PR turns the `return` into `continue` and the DOM reads `private=None`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Maps the converter-instruction surface area covered by this PR. Twenty-five direct calls into `traverser` covering each `c` and `ca` wire-payload shape in both directions (read with nullableValue=undefined, write with nullableValue=null), plus explicit regression cells for the three open/closed issues the fix addresses: - zth#631 — `ca` followed by null sibling - zth#582 — `ca` followed by another `c` - zth#407 — single `c` whose underlying type is an array Plus sequencing cells that document order-dependence (two `ca`s in a row, `ca` followed by a nested object with its own `c`, and a null sibling BEFORE `ca` as a positive control showing the bug only fires when `ca` precedes other fields). Two cells under "ca — custom scalar array (read/write)" explicitly lock in the CURRENT BEHAVIOR for `[T]!` and `[T]` (nullable-element) arrays. relay-compiler emits the same `ca` instruction for all four GraphQL array nullability variants, and the existing fast-path calls `converter(null)` on null elements. That is almost certainly a separate latent bug — out of scope for this PR. The cells make it explicit so a future fix is a deliberate change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The two custom-scalar branches in `traverse` (utils.js) exit the
per-key loop with `return newObj` instead of moving on with
`continue`:
- line 112 — `shouldConvertCustomFieldArray && Array.isArray(...)`
(`ca` instruction, e.g. `[IntString!]`).
- line 137 — `shouldConvertCustomField && Array.isArray(...)`
(`c` instruction whose runtime value is an array, e.g. a custom
scalar like `Number = array<int>`. This branch was added by
PR zth#434 to close issue zth#407.)
In both cases the shallow `Object.assign({}, currentObj)` done by
`getNewObj` has already copied subsequent keys to `newObj`, so
their raw values reach the caller without going through the
null-coercion at the top of the loop (lines 76-79) or the rest of
the per-key logic. The visible symptoms surfaced in three open
issues:
- zth#631 (this PR): ReScript reads leaked response-side `null` as
`Some(null)` instead of `None`; pattern matches take the wrong
branch.
- zth#582 (ValdemarGr): the next variable's `c` instruction never
runs, so the raw ReScript value ships on the wire instead of
the serialized form.
- zth#407 trailing-field regression: same as above but for a single
array-backed scalar followed by another converted field —
silently leaked.
The leading comment on each branch ("Ensures we don't accidentally
move into the array when we're not supposed to") matches the intent
of skipping the remaining branches for the current key, i.e.
`continue`. `return` exits the whole function and was unintended.
Closes zth#582. Extends zth#434's fix for zth#407.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
drewwestphal
force-pushed
the
fix-traverse-after-custom-scalar-array
branch
from
505455b to
3080219
Compare
Owner
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Owner
|
Thank you @drewwestphal ! I'll get this out in a new release shortly. Closes #582 |
Owner
|
@drewwestphal 4.5.1 is out with this fix. |
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
2 participants
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
We had a bug in a project of mine where some of our custom scalars were coming back as Some(null) and breaking our frontend. The below is from a session with claude code where it isolated the issue. I then worked with LLM to get the repo and the toolchain for rescript-relay running running locally, run all the tests (including ci), build a breaking test in line with repo conventions, then provide a patch that fixes the error.
Anyway, first time pull-requester long-time-user and thanks to the maintainers for all the work they do. Below from the maw of the machine:
================
traverseinpackages/rescript-relay/src/utils.jsexits the loop withreturn newObjafter processing a[CustomScalar!]field, instead of moving on to the next key withcontinue. Every key that follows in the samerecord's selection set is then copied through unchanged via the shallow
Object.assigndone bygetNewObj,bypassing the null-coercion at the top of the loop. ReScript reads the leaked raw
nullasSome(null)instead ofNone.Root Cause
Two
return newObj;statements inside the per-keyfor (var key in currentObj)loop should becontinue;:shouldConvertCustomFieldArray && Array.isArray(...)branch (instruction"ca", e.g.[IntString!]).shouldConvertCustomField && Array.isArray(...)branch (instruction"c"whose value is unexpectedlyan array).
Both blocks already write the converted value to
newObj[key]before the exit. The leading comment on each ("Ensureswe don't accidentally move into the array when we're not supposed to") matches the intent of skipping the remaining
branches for the current key, i.e.
continue.returnexits the whole function and skips every later key.The lines 76-79 null-coercion (
if (currentObj[key] == null) { newObj[key] = nullableValue; continue; }) — whichturns
nullintoundefinedfor fragment reads — never runs for any key that comes after the custom-scalar-arrayfield in the selection set.
Changes
packages/rescript-relay/src/utils.js: two-line fix.packages/rescript-relay/__tests__/Test_nullableScalarsAfterCustomScalarArray.res+ jest harness: regression testthat selects
intStringsfollowed by nullableavatarUrl,isOnline,private,onlineStatusonloggedInUser.The
privatebranch runsType.classify(viaObj.magic+Null.toOption) on the option's payload, so onmasterthe rendered DOM literally reads
private=Some(null). Fails on master; passes after the fix.CHANGELOG.md: one bullet under# master.Validation
yarn build:test(idempotent — no extra files modified)yarn buildyarn jest __tests__/Test_nullableScalarsAfterCustomScalarArray-tests.js --runInBandyarn test:ci— all 40 suites / 111 tests passLLM Disclaimer
I know there are many slop PRs out there. This is a real bug I've encountered and this fix seems to address it. We've been using a shim called "fixBrokenRelayNull" in our codebase when this occurs for a while (over 50 occurrences). While I have my reservations about LLM technology and its uses, this was something that would have taken me long enough to get moving on my local machine that I would probably have continued to suffer the problem (or submitted a much less helpful bug report in the discord or something).