Skip to content

Comments

feat: allow customtypes ref in arguments/main#497

Merged
vaisshnavi7 merged 41 commits intomainfrom
feat/allow-customtypes-ref-in-arguments/main
Feb 18, 2025
Merged

feat: allow customtypes ref in arguments/main#497
vaisshnavi7 merged 41 commits intomainfrom
feat/allow-customtypes-ref-in-arguments/main

Conversation

@vaisshnavi7
Copy link
Contributor

@vaisshnavi7 vaisshnavi7 commented Jan 10, 2025

Description of changes: This PR allows the use of CustomType and RefType in arguments for custom operations. The changes span across three main areas, building upon the work done in PRs #402 and #412 :

  1. Updated CustomOperation.ts to accept CustomType, RefType to EnumType & CustomType in CustomArguments.
  2. Modified bridge-types.ts to include CustomType and RefType in relevant interfaces.
  3. Added type tests in CustomOperations.test-d.ts to verify the new functionality.
  4. Improved type resolution logic in ClientCustomOperations.ts for new argument types.
  5. Updated SchemaProcessor.ts to:
  • Handle customType and ref arguments in custom queries, mutations & subscriptions.
  • Generate separate GraphQL input types for non-scalar arguments.
  • Ensure correct referencing of these input types in the generated GraphQL operations.
  • Support nested custom types and combinations of scalar and non-scalar arguments.

Key Implementations:

  • Allow custom types, nested types, and references in query/mutation/subscription arguments.
  • Generate separate input types for non-scalar arguments in GraphQL schema.
  • Correctly reference these input types in operation definitions.
  • Handle both scalar and non-scalar arguments appropriately.
  • Support nested custom types and complex argument structures.

Testing:

  • Added new type tests in CustomOperations.test-d.ts
  • Added Unit Tests in CustomOperations.test.ts:
    • Custom queries, mutations, and subscriptions with custom type arguments
    • Custom queries, mutations, and subscriptions with ref arguments
    • Custom mutations and subscriptions with nested references
    • Updated test snapshots to reflect new schema generation behavior
  • Deployed sample application to verify schema generation and successful deployment

These changes enhance the flexibility of schema definitions by allowing more complex argument structures in custom operations.

This PR replaces #387 and includes the same changes, but with a branch name that matches the required pattern for the preid build process.

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

@changeset-bot
Copy link

changeset-bot bot commented Jan 10, 2025

🦋 Changeset detected

Latest commit: 7bd9d1d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@aws-amplify/data-schema Minor

Not sure what this means? Click here to learn what changesets are.

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

@vaisshnavi7 vaisshnavi7 changed the title Feat/allow customtypes ref in arguments/main Feat: allow customtypes ref in arguments/main Jan 10, 2025
@vaisshnavi7 vaisshnavi7 marked this pull request as ready for review January 10, 2025 20:23
@vaisshnavi7 vaisshnavi7 requested review from a team as code owners January 10, 2025 20:23
stocaaro
stocaaro previously approved these changes Jan 13, 2025
Copy link
Contributor

@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.

Spent an hour exploring this today and it works in the way I was hoping. I think I just negated my own testing ask, and we've reviewed all of this in smaller chunks.

This is a change in API, but I'm not sure what an API review would add here since we have followed the existing pattern consistently.

The SchemaProcessor logic is a bit hard to parse (as discussed elsewhere), but it works and the whole thing needs a refactor more than we need to do the refactor right now.

LGTM.

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.

Overall changes look good!

Seeing 2 things that are off from my perspective:

  1. If we have a nested CustomType in the args like
myMutation: a
  .mutation()
  .arguments({
    loc: a.customType({
      lat: a.float(),
      long: a.float(),
      meta: a.customType({
        timestamp: a.timestamp(),
      }),
    }),
  })
.returns(
  a.customType({
    lat: a.float(),
    long: a.float(),
  }),
),

We get the following GQL output:

type MyMutationReturnType
{
  lat: Float
  long: Float
}

type Mutation {
  myMutation(loc: MyMutationLocInput): MyMutationReturnType 
}

input MyMutationLocInput
{
  lat: Float
  long: Float
  meta: MyMutationLocInputMetaInput
}

MyMutationLocInputMetaInput is referenced in the top-level input, but does not get generated in the output itself.

  1. We're introducing quite a bit of unnecessary additional complexity to SchemaProcessor.ts. I'm seeing quite a bit of prop-drilling and new conditionals and iteration that should not be needed. Given the number of changes to this file and their interdependence, it's difficult to properly communicate this via specific comments, but maybe we can pair on it?
    Ultimately, this part is non-blocking. If the functionality works, we can defer cleaning up the code to a broader refactor task, as Aaron suggested above.

@vaisshnavi7 vaisshnavi7 changed the title Feat: allow customtypes ref in arguments/main feat: allow customtypes ref in arguments/main Jan 15, 2025
Copy link
Contributor

@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.

One comment. I got pulled away while reviewing this today. Its at the top of my list for tomorrow morning.

Copy link
Contributor

@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.

This change is a bit large and hard to parse. Can we break some of the refactor changes out and get them merged ahead of the focused deliverable?

Thinking of:

  • White space changes
  • graphql render order changes
  • passing auth down (maybe this can or can't be broken out)

Basically anything that isn't clearly pitched in the description.

There are enough moving parts here that anything we can do to focus/narrow the PR will help me get confidence we're doing the right thing. I would like to refactor the SchemaProcessor into something easier to work around, but that should follow this change, not block it.

Copy link
Contributor

@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.

The most recent changes didn't come with a test to cover the problems being fixed. My inclination is to test other patterns the keep the enums from being duplicated, but the first step is to get this fix tested so we know we're not reintroducing solved issues.

Copy link
Contributor

@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.

Made a review pass with comments. It looks like some of the Input name changes are still in process, so I'll hold off on doing further manual testing for those changes.

The SchemaProcessor refactor is a huge improvement, thank you for investing the time in that!

*/
functionHandler: AppSyncResolverHandler<
CustomOpArguments<Op>,
CustomOpArguments<Op, RefBag>,

This comment was marked as resolved.

stocaaro

This comment was marked as off-topic.

stocaaro
stocaaro previously approved these changes Feb 17, 2025
Copy link
Contributor

@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.

LGTM

Copy link
Contributor

@atierian atierian left a comment

Choose a reason for hiding this comment

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

Looks great! A few comments:

  1. It looks like we're not handling the Schema['opName']['functionHandler'] type updates for custom types. For example:
const s = a.schema({
  Bar: a.customType({
    baz: a.integer(),
  }),
  myQuery: a.query()
    .arguments({
      wrapper: a.customType({
        bar: a.ref('Bar')
      })
    })
    .handler(a.handler.function('myFunc'))
    .returns(a.string()),
})
  .authorization((allow) => allow.publicApiKey())

type Schema = ClientSchema<typeof s>;
type ActualHandler = Prettify<Schema['myQuery']['functionHandler']>;

The ActualHandler has the type definition:

type ActualHandler = (event: AppSyncResolverEvent<ShallowPretty<{} & {
    wrapper?: ShallowPretty<{} & {
        bar?: any;
    }> | null | undefined;
}>, Record<string, any> | null>, context: Context, callback: Callback<Nullable<string>>) => void | Promise<Nullable<string>>

From a brief check, it looks like this one-liner would fix this.

diff --git a/packages/data-schema/src/ClientSchema/Core/ClientCustomOperations.ts b/packages/data-schema/src/ClientSchema/Core/ClientCustomOperations.ts
index aa19659..f96d87e 100644
--- a/packages/data-schema/src/ClientSchema/Core/ClientCustomOperations.ts
+++ b/packages/data-schema/src/ClientSchema/Core/ClientCustomOperations.ts
@@ -38,7 +38,7 @@ export interface ClientCustomOperation<
    * ```
    */
   functionHandler: AppSyncResolverHandler<
-    CustomOpArguments<Op>,
+    CustomOpArguments<Op, RefBag>,
     // If the final handler is an async function, the Schema['fieldname']['functionhandler']
     // should have a return type of `void`. This only applies to `functionHandler` and not
     // `returnType` because `returnType` determines the type returned by the mutation / query

Let's address this and add applicable tests. See CustomOperations.test-d.ts for examples testing the functionHandler type.

  1. Subscription support

Are we intentionally supporting custom type arguments for subscriptions? From the PR description and test cases, it appears that this support is limited to queries and mutations. However this schema is valid with the changes:

const s = a.schema({
  Foo: a.customType({
    bar: a.integer(),
  }),
  mySub: a.subscription()
    .arguments({
      foo: a.ref('Foo')
    })
    .handler(a.handler.function('myFunc'))
    .for(a.ref('myMutation')),
  
  myMutation: a.mutation()
    // ...
});

@atierian atierian dismissed iartemiev’s stale review February 18, 2025 19:43

requested changes resolved

@vaisshnavi7 vaisshnavi7 merged commit bdec9ee into main Feb 18, 2025
19 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.

Custom queries/mutations lack schema support

5 participants