Skip to content

fix: keep GraphQL variables in the documented JS SDK call - #1897

Open
HarshMN2345 wants to merge 2 commits into
mainfrom
fix/graphql-query-overload
Open

fix: keep GraphQL variables in the documented JS SDK call#1897
HarshMN2345 wants to merge 2 commits into
mainfrom
fix/graphql-query-overload

Conversation

@HarshMN2345

@HarshMN2345 HarshMN2345 commented Sep 11, 2026

Copy link
Copy Markdown
Member

What does this PR do?

The JS SDKs (web, node, react-native) drop variables from the GraphQL call shown in the Appwrite docs.

graphql.query and graphql.mutation take a single free-form object parameter named query. The params-object overload is selected when 'query' in paramsOrFirst, and a GraphQL request object has a query key as well, so the documented call is read as the params form:

graphql.mutation({
    query: 'mutation CreateAccount($email: String!) { ... }',
    variables: { email: '...' },
});

Before:

{"query":"mutation CreateAccount($email: String!) { ... }"}

After:

{"query":{"query":"mutation CreateAccount($email: String!) { ... }","variables":{"email":"..."}}}

Current Appwrite servers answer the flat body with HTTP 500 (appwrite/appwrite#13605 turns it into a 400), so every documented GraphQL call fails with the current SDKs.

When a method's only parameter is a free-form object, getOverloadCondition now adds typeof paramsOrFirst.<name> !== 'string'. Generated from the Appwrite 2.0.x spec, this operand in graphql.query and graphql.mutation is the only change in web, node and react-native:

             'query' in paramsOrFirst &&
+            typeof paramsOrFirst.query !== 'string'

The params form ({ query: { query, variables } }, { query: [...] }), positional batches, { query: null } and the Missing required parameter throw for { query: undefined } behave as before. The documented shape resolves to the positional overload, which stays marked @deprecated.

Test Plan

  • Generated web (client), node (server) and react-native (client) from open-api3-2.0.x.json before and after the change: only src/services/graphql.ts differs.
  • npm run format:check, lint, analyse and build pass for all three; npm run test passes for node (832 tests).
  • Ran the generated graphql.ts before and after with a recording client and replayed the web payloads against an Appwrite server: the documented mutation reaches the resolver with its variables, and the documented query returns data instead of HTTP 500.
  • vendor/bin/phpunit --testsuite Generation, phpcs and rector --dry-run pass.
  • e2e: a general.createQuery fixture takes a lone free-form query object; the node, web and react-native scripts call it with { query, variables } and { query: { query } }, and the mock echoes what it received. Node20 and WebNode pass; with Web.php reverted, Node20 fails with Invalid `query` param: Value must be a valid object.

graphql.query({ query: '...', variables }) matched the params-object
overload because the GraphQL request object also has a `query` key, so
the SDK sent only the query string and dropped `variables`. When a
method's only parameter is a free-form object, a string under its own
name now selects the positional form.
@greptile-apps

greptile-apps Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge; the overload correction is covered through observable request behavior across all affected SDK targets.

Fix All in Claude CodeFindings

  1. P2 Missing behavior coverage
Fix with agent prompt
### Issue 1
src/SDK/Language/Web.php:1076-1078
This subtle overload-dispatch fix affects every SDK inheriting Web behavior, but no automated test exercises a lone free-form object through the documented GraphQL call or verifies that `variables` reaches the request body. Add a runtime behavior test using the generated SDK and a recording or mock client. An assertion against the emitted condition would only mirror the implementation and would not protect the actual request contract.

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!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

  • Adds a string-value discriminator to the generated params-object overload condition.
  • Adds a mock endpoint and OpenAPI fixture for a lone free-form object parameter.
  • Adds observable end-to-end request-payload coverage for Web, Node, and React Native runtimes.

Reviews (2) · Last reviewed commit: "test: cover a lone object param that car..."

Comment thread src/SDK/Language/Web.php
Comment on lines +1076 to +1078
if (count($params) === 1 && $firstParamType === 'object') {
$operands[] = 'typeof paramsOrFirst.' . $this->escapeKeyword($this->toCamelCase($params[0]->name)) . " !== 'string'";
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing behavior coverage

This subtle overload-dispatch fix affects every SDK inheriting Web behavior, but no automated test exercises a lone free-form object through the documented GraphQL call or verifies that variables reaches the request body. Add a runtime behavior test using the generated SDK and a recording or mock client. An assertion against the emitted condition would only mirror the implementation and would not protect the actual request contract.

Knowledge Base Used: Generation quality assurance

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/SDK/Language/Web.php
Line: 1076-1078

Comment:
**Missing behavior coverage**

This subtle overload-dispatch fix affects every SDK inheriting Web behavior, but no automated test exercises a lone free-form object through the documented GraphQL call or verifies that `variables` reaches the request body. Add a runtime behavior test using the generated SDK and a recording or mock client. An assertion against the emitted condition would only mirror the implementation and would not protect the actual request contract.

**Knowledge Base Used:** [Generation quality assurance](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/sdk-generator/-/docs/generation-quality-assurance.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!

Fix in Claude Code Fix in Codex

Adds a general.createQuery fixture whose only parameter is a free-form
`query` object, called from the node, web and react-native e2e scripts
with the graphql.query({ query, variables }) shape and the params form.
The mock echoes the received object, so a flattened string or dropped
`variables` fails the run.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant