Skip to content

fix(dataconnect): Refactor CRUD helpers to use GraphQL variables and @allow directive#3182

Open
stephenarosaj wants to merge 16 commits into
mainfrom
rosa/enum-serialization
Open

fix(dataconnect): Refactor CRUD helpers to use GraphQL variables and @allow directive#3182
stephenarosaj wants to merge 16 commits into
mainfrom
rosa/enum-serialization

Conversation

@stephenarosaj

@stephenarosaj stephenarosaj commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Description

✨ Refactored Data Connect CRUD operations to execute parameterized GraphQL mutations with query variables and @allow directives. This fixes the bug described by Issue #3041 where enums would not be serialized properly by the [insert,upsert](many) APIs.

To verify this fix and harden integration tests, refactored tests so that before checking for equality between the expected and actual input query strings, they are normalized.

Changes

  • Removed objectToString inline GraphQL serializer
  • Added getTableNames and getFieldsString helpers to handle
  • Refactored CRUD operations (insert, insertMany, upsert, upsertMany) to use GraphQL variables and @allow directives, and to not use duplicated code
  • Added Data Connect emulator test instructions in CONTRIBUTING.md
  • Updated ignored emulator debug logs in .gitignore so that no matter where they show up, they are properly ignored. This makes it so that even when running integration tests from the root of the SDK repo they are not tracked.

Testing

  • Added expectNormalizedExecuteGraphqlCall test helper
  • Updated CRUD unit tests to verify parameterized mutations and variables
  • Added unit tests for enum field serialization and @allow directives, including complex nested input types, and arrays of complex nested input types with different selection sets on each item
  • Updated string serialization tests to check equality between normalized expected and actual query strings

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request refactors the Data Connect API client to use GraphQL variables and the @allow(fields: ...) directive for insert, insertMany, upsert, and upsertMany operations, replacing the previous manual JSON-to-GraphQL-string serialization. It also updates the unit tests to validate this new variable-based execution and adds Data Connect emulator testing instructions to the contribution guide. The review feedback highlights potential GraphQL injection vulnerabilities and syntax errors due to direct interpolation of table names and object keys into the mutation strings, recommending validation against standard GraphQL identifier patterns.

Comment thread src/data-connect/data-connect-api-client-internal.ts Outdated
Comment thread src/data-connect/data-connect-api-client-internal.ts Outdated
Comment thread src/data-connect/data-connect-api-client-internal.ts Outdated
@stephenarosaj

Copy link
Copy Markdown
Contributor Author

From offline review by @mtr002:

By default (admin usage), we build a flat @allow fields list containing only the top-level database columns. To explicitly perform nested relational mutations, the developer is required to explicitly set the allowed fields in the @allow(fields: "...") to lock down the nested relationship structure

Also, since this feature is for admin usage, a limit of 100 seems very low, especially given that there is no way to override it. We currently have an upper limit of 10k set

const { capitalized, camelCase } = this.getTableNames(tableName);
const keys = this.getFieldsString(data);
const mutation =
`mutation($data: [${capitalized}_Data!]! @allow(fields: "${keys}")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

While we correctly validate that the input array doesn't exceed 10,000 items, the generated mutation doesn't specify maxCount inside the @Allow directive.

By default, the fsql backend defaults maxCount to 100 if it is omitted from the @Allow directive. Therefore, if a user attempts to bulk import 150 items, it will pass the validation but fail at the backend with a payload limit error.

We should hardcode maxCount: 10000 inside the @Allow directive

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch - but quick question - this should only added to the @allow directive for bulk inserts (like array variables) right??

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes! MaxCount is only allowed on array variables

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.

2 participants