{{ message }}
Repository navigation
Fix GraphQL JSON arrays, nested variables, and integer precision - #13667
Merged
Merged
Conversation
HarshMN2345
marked this pull request as ready for review
September 14, 2026 12:58
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…e errors Write the overflow expectations as literals instead of re-running json_decode on the parser's own input, so the rows assert the numeric result rather than the production expression. Pin that a nested JSON-encoded string stays a string under Assoc. That is the one semantics decision in the fix rather than a bug fix, and a recursion that decodes nested strings opportunistically passed the suite unchanged. Assert the missing-variable message in the e2e failure case. An errors key alone is present with or without the fix, since the unfixed parser answers the same mutation with an internal error. Explain why the recursion is self:: — it is what keeps Assoc's decode on the outermost node, and nothing mechanical catches a change back to $this->.
ChiragAgg5k
approved these changes
Sep 21, 2026
Leave the account preferences regression as the coverage for the parser. It drives parseLiteral through a real mutation with an inline array, a nested object, a nested variable and an integer past 2^53. The boundary rows go with it: overflow to float, the platform integer bounds, and a nested JSON-encoded string staying a string are no longer pinned.
Comment on lines
+31
to
+53
Contributor
There was a problem hiding this comment.
The focused Json/Assoc unit suite added for this parser fix was deleted, leaving overflow boundaries, negative integers, nested JSON-looking strings, nullable nested variables, and top-level encoded Assoc strings without direct tests. The remaining account E2E covers only 9007199254740993 and one nested-variable path. The repository requires a unit regression when an E2E exposes a local library bug that no unit test catches, so independent observable-behavior tests for these parser cases must be restored before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/GraphQL/Types/Json.php
Line: 31-53
Comment:
**Parser regressions lost coverage**
The focused `Json`/`Assoc` unit suite added for this parser fix was deleted, leaving overflow boundaries, negative integers, nested JSON-looking strings, nullable nested variables, and top-level encoded `Assoc` strings without direct tests. The remaining account E2E covers only `9007199254740993` and one nested-variable path. The repository requires a unit regression when an E2E exposes a local library bug that no unit test catches, so independent observable-behavior tests for these parser cases must be restored before merging.
**Context Used:** AGENTS.md ([source](https://github.com/appwrite/appwrite/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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.
GraphQL JSON array literals raised an internal error because the parser passed an AST NodeList to array_map. Nested variables became null, string values in inline Assoc objects were decoded as standalone JSON, and integer literals were converted to floats before requests reached the API.
Traverse object/list nodes with their variables, preserve nested strings, and decode integer literals using PHP JSON semantics. Native-range integers remain exact; larger values retain the same floating-point representation as JSON request bodies. Delegate other scalar values to the GraphQL library parser. Top-level JSON-encoded Assoc strings remain supported.
The regression matrix covers arrays, nested objects and variables, falsy values, native integer bounds, overflow, and encoded strings. An API regression updates account preferences using inline JSON, reads them back through REST, and checks that an omitted required variable leaves preferences unchanged.
Validation: 16 unit tests pass with 32 assertions; the original parser produces five errors and five failures on the same cases. PHP syntax, Pint, targeted PHPStan, Rector dry-run, and diff checks pass. On commit 398f215, GitHub CI passed Checks, the Unit suite (1,215 tests, 6,056 assertions), and all six GraphQL lanes across dedicated/shared MariaDB, PostgreSQL, and MongoDB. Each GraphQL lane ran 308 tests with 12 skips; the new literal regression ran in each.
The full CI run is now successful on the same commit. The PostgreSQL/shared Usage job initially failed before tests when its MongoDB dependency could not bind port 27017; a single retry passed 10 tests with 327 assertions and one skip.