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

feat: Custom type and ref arg handling in SchemaProcessor with tests #412

Conversation

vaisshnavi7
Copy link
Contributor

@vaisshnavi7 vaisshnavi7 commented Dec 11, 2024

Problem:
-Custom type and ref arguments in custom queries and mutations were not properly handled in the SchemaProcessor, leading to incorrect GraphQL schema generation.
-Several integration tests in custom-operations.ts were previously skipped due to schema validation issues related to custom type argument features.

Related PR
-This PR addresses the skipped tests and completes the implementation started in PR #402.

Changes:

  • Updated SchemaProcessor.ts to generate GraphQL input types for custom types and ref arguments.
  • Modified the processing of custom operations to use these generated input types.
  • Implemented generateInputTypes function to create GraphQL input types for custom structures.
  • Expanded and fixed integration tests in custom-operations.ts:
    • Re-enabled previously skipped tests
    • Added new test for schema.transform() to ensure correct schema generation
    • Added new tests for mocked runtime behavior of client operations with custom type and ref arguments
  • Updated test snapshots to reflect new schema generation behavior
  • Adjusted test cases in CustomOperations.test.ts, client-schema.ts, and ssr-req-client.ts

Validation:

  • Expanded existing integration tests in custom-operations.ts to cover new functionality, including:
    • A new test for schema.transform() to verify correct schema generation
    • New tests for mocked client operations with custom type and ref arguments
  • Re-enabled and fixed previously skipped tests in custom-operations.ts
  • Verified that all tests now pass, including those for custom types and ref arguments
  • Deployed the schema using a sample application to verify:
    • Schema generation matches expectations.
    • Deployment succeeds without errors.

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

Copy link

changeset-bot bot commented Dec 11, 2024

⚠️ No Changeset found

Latest commit: e7493a3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

- Update schema processor to correctly include all custom types
- Adjust test cases in CustomOperations.test.ts, client-schema.ts, and ssr-req-client.ts
- Update corresponding snapshots to reflect change
@vaisshnavi7 vaisshnavi7 marked this pull request as ready for review December 16, 2024 21:07
@vaisshnavi7 vaisshnavi7 requested review from a team as code owners December 16, 2024 21:07
Copy link
Member

@iartemiev iartemiev left a comment

Choose a reason for hiding this comment

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

I'm seeing some unexpected changes in the code and tests/snapshots, as called out in the comments.

Let's baseline on the requirements. When custom query/mutation arguments contain non-scalar values (e.g., custom types or a refs to custom types), we should:

  1. Define a GraphQL Input type from the arguments in the SDL output from schema.transform()
  2. Reference this input type in the query/mutation field argument
  3. The GraphQL Type definition for the custom type should remain in the SDL output

For example, given:

Location: a.customType({
  lat: a.float(),
  long: a.float(),
}),

addLocationToPost: a
  .mutation()
  .arguments({ postId: a.string(), location: a.ref('Location') })
  .handler(a.handler.custom({ entry: './source.js' }))
  .returns(a.boolean())

The GraphQL output should be:

type Location 
{
  lat: Float
  long: Float
}

input AddLocationToPostLocationInput 
{
  lat: Float
  long: Float
}

type Mutation {
  addLocationToPost(postId: string, location: AddLocationToPostLocationInput): Boolean
}

However, when I pull down the PR and run schema.transform() on the same schema , I get the following output:

type Location 
{
  lat: Float
  long: Float
}

type Mutation {
  addLocationToPost(postId: String, location: ID): Boolean
}
  • Input type is not getting generated
  • addLocationToPost field arg location is referencing type ID instead of the Input type

After making the requested changes, I recommend linking @aws-amplify/data-schema into a sample app and actually attempting to deploy some schemas with custom operations containing non-scalar arguments to make sure everything works as expected.

Comment on lines 1364 to -1382
const s = a.schema({
MyQueryReturnType: a.customType({
fieldA: a.string(),
fieldB: a.integer(),
nestedCustomType: a.customType({
nestedA: a.string(),
nestedB: a.string(),
grandChild: a.customType({
grandA: a.string(),
grandB: a.string(),
}),
}),
}),
myQuery: a
.query()
.handler(a.handler.function('myFn'))
.returns(
a.customType({
fieldA: a.string(),
fieldB: a.integer(),
nestedCustomType: a.customType({
nestedA: a.string(),
nestedB: a.string(),
grandChild: a.customType({
grandA: a.string(),
grandB: a.string(),
}),
}),
}),
)

This comment was marked as outdated.

@vaisshnavi7 vaisshnavi7 marked this pull request as draft December 27, 2024 04:48
@vaisshnavi7 vaisshnavi7 marked this pull request as ready for review January 7, 2025 18:32
Copy link
Member

@stocaaro stocaaro left a comment

Choose a reason for hiding this comment

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

How is the unit test branch coverage of the code we're introducing in this change?

packages/data-schema/src/SchemaProcessor.ts Outdated Show resolved Hide resolved
packages/data-schema/src/SchemaProcessor.ts Outdated Show resolved Hide resolved
@@ -258,8 +256,7 @@ type Subscription {
`;

exports[`CustomOperation transform dynamo schema custom subscriptions Custom subscription where .for() resource has a CustomType return type 1`] = `
"type CreateCustomTypePostReturnType @aws_api_key
{
"type CreateCustomTypePostReturnType {
Copy link
Member

Choose a reason for hiding this comment

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

Here and below, I'm seeing that the auth graphql directives (@aws_api_key, @aws_cognito_user_pools, etc.) are now getting dropped from the generated types.

Does this PR actually need to change anything about how return types are handled relative to main? I could be wrong, but AFAIK, we already support custom types and refs to custom types as the return types for custom queries and mutations.

Copy link
Member

@iartemiev iartemiev left a comment

Choose a reason for hiding this comment

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

Thanks for making the requested changes.

Approving merge to the feat branch.

There are a few changes in SchemaProcessor.ts that I'd like to review more closely, but I will defer that until the feat -> main PR is ready.

@vaisshnavi7 vaisshnavi7 merged commit 3517643 into feat/allow-customtype-ref-in-arguments Jan 9, 2025
7 checks passed
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.

3 participants