fix(dataconnect): Refactor CRUD helpers to use GraphQL variables and @allow directive#3182
fix(dataconnect): Refactor CRUD helpers to use GraphQL variables and @allow directive#3182stephenarosaj wants to merge 16 commits into
Conversation
… artifact ignores
There was a problem hiding this comment.
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.
|
From offline review by @mtr002: By default (admin usage), we build a flat 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}")) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Good catch - but quick question - this should only added to the @allow directive for bulk inserts (like array variables) right??
There was a problem hiding this comment.
Yes! MaxCount is only allowed on array variables
Description
✨ Refactored Data Connect CRUD operations to execute parameterized GraphQL mutations with query variables and
@allowdirectives. 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
objectToStringinline GraphQL serializergetTableNamesandgetFieldsStringhelpers to handleinsert,insertMany,upsert,upsertMany) to use GraphQL variables and@allowdirectives, and to not use duplicated codeCONTRIBUTING.md.gitignoreso 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
expectNormalizedExecuteGraphqlCalltest helper@allowdirectives, including complex nested input types, and arrays of complex nested input types with different selection sets on each item