Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix query from AppSync with an empty filter object #61

Conversation

danielfinke
Copy link
Contributor

Issue #, if available: #44

Description of changes:

Build the query/mutation's selection set using the graphql utilities, instead of building manually. This will avoid any edge cases with the provided arguments. The schema is assumed to be valid: validation should be taken care of ahead of time in schemaModelValidator and we also currently do not define the built-in directives (e.g. @id, @relationship) required to make it a valid schema.

  • break out lookup functions for type/field definitions
  • update outputReference resolvers
  • add unit tests
  • fix query result tests that were sensitive to object key order (use toEqual instead of strict JSON string equality)
  • also check output file size of .cjs resolver in test Case01.02
  • fix running test Case01.04 (use .cjs extension)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Build the query/mutation's selection set using the `graphql` utilities,
instead of building manually. This will avoid any edge cases with the
provided arguments. The schema is assumed to be valid: validation should
be taken care of ahead of time in `schemaModelValidator` and we also
currently do not define the built-in directives (e.g. `@id`,
`@relationship`) required to make it a valid schema.

- break out lookup functions for type/field definitions
- update `outputReference` resolvers
- add unit tests
- fix query result tests that were sensitive to object key order (use
  `toEqual` instead of strict JSON string equality)
- also check output file size of `.cjs` resolver in test `Case01.02`
- fix running test `Case01.04` (use `.cjs` extension)

closes aws#44
Resolve an "unexpected EOF" error from `gql`
(as a result of the resolver changes for queries with an empty filter
object)
@andreachild
Copy link
Contributor

Hi @danielfinke thanks for all of your recent contributions. This one is a bit more complex than the others so will take a bit more time to review, but I hope to review it this week.

@AndreaNassisi
Copy link
Contributor

Hello Daniel, thank you for submitting these changes. The team will look at them soon.

@danielfinke
Copy link
Contributor Author

@andreachild @AndreaNassisi no problem 🙂 I am assembling more PRs as well, no rush

danielfinke added a commit to danielfinke/amazon-neptune-for-graphql that referenced this pull request Jan 30, 2025
Wrap the JSON in single quotes instead of backticks to reduce the escape
sequence surface area. Escape any single quotes in the stringified JSON.
This handles single and double quotes and backticks in the input
schema's scalar, field/type/etc. descriptions.

- add test `Case03.02` to check output for schema with string delimiters
- update other `outputReference` resolvers
- add templates to lint check
  - fix resulting ESLint errors in templates
- exclude output from ESLint rules (hide errors for those files when
  using editor integrations)

copied changes from aws#61:

- fix query result tests that were sensitive to object key order (use
  `toEqual` instead of strict JSON string equality)
- also check output file size of `.cjs` resolver in test `Case01.02`
- fix running test `Case01.04` (use `.cjs` extension)

closes aws#65
danielfinke added a commit to danielfinke/amazon-neptune-for-graphql that referenced this pull request Jan 31, 2025
Callers can now pass in GraphQL ASTs directly. Saves a redundant
print/parse in `resolveGraphDBQueryFromAppSyncEvent`.

For backwards-compatibility, callers *can* continue to pass in a query
string.

- update `outputReference` resolvers
- update tests to pass in ASTs instead of strings

see aws#61 (comment)
Callers can now pass in GraphQL ASTs directly. Saves a redundant
print/parse in `resolveGraphDBQueryFromAppSyncEvent`.

For backwards-compatibility, callers *can* continue to pass in a query
string.

- update `outputReference` resolvers
- update tests to pass in ASTs instead of strings

see aws#61 (comment)
@andreachild
Copy link
Contributor

LGTM thanks for your patience and taking feedback into consideration

Copy link
Contributor

@Cole-Greer Cole-Greer left a comment

Choose a reason for hiding this comment

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

Thanks @danielfinke, LGTM

@Cole-Greer Cole-Greer merged commit 3fd9c84 into aws:main Feb 3, 2025
2 checks passed
@danielfinke danielfinke deleted the fix-query-from-appsync-with-an-empty-filter-object branch February 3, 2025 21:22
danielfinke added a commit to danielfinke/amazon-neptune-for-graphql that referenced this pull request Feb 9, 2025
Also fix mistyped return annotation on `resolveGraphDBQuery`, my bad
danielfinke added a commit to danielfinke/amazon-neptune-for-graphql that referenced this pull request Feb 10, 2025
Also fix mistyped return annotation on `resolveGraphDBQuery`, my bad
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.

5 participants