feat: allow customtypes ref in arguments/main #497

vaisshnavi7 · 2025-01-10T19:58:17Z

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

Updated CustomOperation.ts to accept CustomType and RefType in CustomArguments.
Modified bridge-types.ts to include CustomType and RefType in relevant interfaces.
Added type tests in CustomOperations.test-d.ts to verify the new functionality.
Improved type resolution logic in ClientCustomOperations.ts for new argument types.
Updated SchemaProcessor.ts to:

Handle customType and ref arguments in custom queries and mutations.
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 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
Implemented client operation tests with mocked runtime behavior in custom-operations.ts for:
- Custom queries with custom type arguments
- Custom queries with ref arguments
- Custom mutations with custom type arguments
- Custom mutations with ref arguments- 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.

Co-authored-by: aws-amplify-bot <[email protected]>

…412)

…-ref-in-arguments

changeset-bot · 2025-01-10T19:58:21Z

🦋 Changeset detected

Latest commit: f1b8e0e

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

stocaaro

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.

stocaaro · 2025-01-13T22:40:46Z

packages/data-schema/__tests__/CustomOperations.test-d.ts

+  it('accepts CustomType in arguments', () => {
+    const operation: CustomOperation<any, "arguments" | "for", "queryCustomOperation"> = a.query().arguments({
+      customArg: a.customType({
+        field1: a.string(),
+        field2: a.integer()
+      })
+    });
+  });


Would it be worth adding some type comparison testing around Schema['fcnCall']['functionHandler']?

It appears to work as I would expect, but looking at this test, it covers the definition, but doesn't test the resulting types.

In my example app, I leveraged the types for the handler definition like:

export const handler: Schema['fcnCall']['functionHandler'] = async (x) => { return { todoCount: (await client.models.Todo.list()).data.length, x: JSON.stringify(x.arguments), } } // Picking into the type to inspect the arg types type T = Parameters<Schema['fcnCall']['functionHandler']>[0]['arguments']['arg1']

packages/integration-tests/__tests__/defined-behavior/2-expected-use/custom-operations.ts

iartemiev

Overall changes look good!

Seeing 2 things that are off from my perspective:

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.

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.

stocaaro

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

packages/data-schema/__tests__/CustomOperations.test-d.ts

stocaaro

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.

stocaaro · 2025-01-16T16:12:45Z

packages/data-schema/src/SchemaProcessor.ts

+  const customOperationTypes = generateCustomOperationTypes(customOperations);
+
+  const schemaComponents = [
+    ...Array.from(enumTypes),


Looking over the snapshot tests, there's a bit of noise as the enum types move before the other definitions. Semantically, I think it makes sense that they would be defined first, but it doesn't appear to have made a difference in the graphql definitions.

Why did this change include the reordering?

As a pure refactor, could we make this change in separately to remove some of the less related diff from this change?

The reordering was an unintended side effect of implementing support for custom types and refs in arguments. To handle these new argument types, separate processing for different schema components was introduced. This led to enums being collected and added independently, resulting in their reordering relative to other definitions.

I've made adjustments to minimize this reordering. Many snapshots now match the original order, but some differences persist, particularly with types having specific directives. These changes don't affect the GraphQL definitions functionally.

I don't mind the reordering. I think it makes sense that things get accumulated and a logical order is applied when rendering the graphql document which is inconsistent with the order from before. Its a no-effect refactor that makes a good isolated change. Something easy to commit ahead of the effective change.

stocaaro · 2025-01-16T16:20:26Z

packages/data-schema/__tests__/CustomOperations.test.ts

@@ -1401,6 +1399,28 @@ describe('custom operations + custom type auth inheritance', () => {
    );
  });

+  test('inline custom type inherits auth rules from referencing op', () => {


The change description doesn't talk about inherited auth rules. I'm puzzled by the how the test changes in this file related back to the broader change.

Took a closer look here expecting to find arguments snapshot tests for similar cases to the client type tests in the .t.test file. It looks like we have some related tests in the e2e define_bahavior testing. Why are they there vs here?

My thinking is that we would have unit tests covering a many cases and basic behavior testing in the e2e, but I'm open to this being misaligned with guidance from others. Interested to understand.

I have overlooked the connection between the test changes and the broader change in the description. The current tests, including those for inherited auth rules, are covering the functionality for custom types and refs in arguments. The e2e define_behavior tests complement the unit tests here by covering end-to-end scenarios.

stocaaro · 2025-01-16T16:24:54Z

packages/data-schema/src/SchemaProcessor.ts

+      throw new Error(
+        `Invalid field definition for ${fieldName}. DB-generated fields are only supported with PostgreSQL data sources.`,
+      );
    }

    if (isGenerated && !canGenerateFieldType(fieldType)) {
-      throw new Error(`Incompatible field type. Field type ${fieldType} in field ${fieldName} cannot be configured as a DB-generated field.`);
+      throw new Error(
+        `Incompatible field type. Field type ${fieldType} in field ${fieldName} cannot be configured as a DB-generated field.`,
+      );


There's quite a bit of reformatting noise in this change. I'm not opposed to cleaning/standardizing things, but in a change this size it might be better to pull reformatting / no-op refactors out to their own change to keep the change more concise / easy to review.

You're right. I'll change this.

…stomOperations

stocaaro · 2025-01-22T21:35:11Z

packages/data-schema/src/SchemaProcessor.ts

+  const schemaComponents = [
+    ...gqlModels,
+    ...Array.from(enumTypes),


I like that schemaComponents replaces gqlModel, could also be gqlComponents.

Because the method is so long, I think I liked having the components added to the list as they where complete so that I know what to keep paying attention to in this too long method. Could we factor back to pushing onto the list.

I've implemented the suggested changes. The schemaPreprocessor now uses a single gqlComponents array, and components are added to this array as they are processed throughout the method.

stocaaro · 2025-01-22T21:35:47Z

packages/data-schema/src/SchemaProcessor.ts

+        });
+      }
+    } else if (typeDef.type === 'enum') {
+      const enumDefinition = `enum ${typeName} {\n  ${typeDef.values.join('\n  ')}\n}`;


Commenting out this line doesn't break tests. Either we're missing a test or this line isn't doing anything valuable.

stocaaro · 2025-01-22T21:36:40Z

packages/data-schema/src/SchemaProcessor.ts

+        implicitTypes.forEach(([typeName, typeDef]) => {
+          if (isEnumType(typeDef)) {
+            const enumTypeDefinition = `enum ${typeName} {\n  ${typeDef.values.join('\n  ')}\n}`;


I wonder how of this logic can live elsewhere if we are accruing components one by one in the pattern we where using with gqlModels.

I've simplified the implicit type processing logic by removing redundant variables.

As for moving this logic elsewhere, the current placement allows for processing types at the earliest point where we have sufficient context, particularly information about return types from transformCustomOperations The full schema context is necessary to accurately determine whether a type should be an input type or a regular type.

stocaaro · 2025-01-22T21:41:20Z

packages/data-schema/__tests__/CustomOperations.test-d.ts

+        field2: a.integer(),
+      }),
+    });
+  });


I might expect Equal<A,B> to show up here. Could you use type equality instead of setting the type?

type A = CustomOperation< any, 'arguments' | 'for', 'queryCustomOperation' >; const example = a.query().arguments({ customArg: a.customType({ field1: a.string(), field2: a.integer(), }), }); type test = Expect<Equal<A, typeof example>>;

Implemented checks as suggested.

stocaaro · 2025-01-22T21:48:27Z

packages/data-schema/src/SchemaProcessor.ts

@@ -1316,6 +1386,64 @@ const mergeCustomTypeAuthRules = (
  }
 };

+function generateInputTypes(


If we control the "InputTypes" naming that I think is introduced here, could we change it to "ArgumentTypes". Is there an important difference between arguments and inputs that I'm not seeing? If not, I think it would be better to stick to one name and I think arguments is the customer facing name.

stocaaro · 2025-01-22T21:49:52Z

packages/data-schema/src/SchemaProcessor.ts

+      const scalarDefinition = `scalar ${typeName}`;
+      generatedTypes.add(scalarDefinition);


I could see removing the extra local variable and performing this on one line for cases like this.

… array

vaisshnavi7 and others added 9 commits November 7, 2024 17:37

feat(custom-operations): allow CustomType and RefType in arguments

e7f97fa

Refactor type tests for CustomType and RefType

0e0561e

Merge branch 'main' into feat/allow-customtype-ref-in-arguments

1a54cde

Merge branch 'main' into feat/allow-customtype-ref-in-arguments

ecf2ec2

feat: add support for custom, nested, and ref type arguments (#402)

a0872b9

Co-authored-by: aws-amplify-bot <[email protected]>

Merge branch 'main' into feat/allow-customtype-ref-in-arguments

2f7e948

Merge branch 'main' into feat/allow-customtype-ref-in-arguments

45074a3

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

3517643

…412)

Merge remote-tracking branch 'origin/main' into feat/allow-customtype…

2e81a9d

…-ref-in-arguments

vaisshnavi7 changed the title ~~Feat/allow customtypes ref in arguments/main~~ Feat: allow customtypes ref in arguments/main Jan 10, 2025

vaisshnavi7 marked this pull request as ready for review January 10, 2025 20:23

vaisshnavi7 requested review from a team as code owners January 10, 2025 20:23

iartemiev mentioned this pull request Jan 13, 2025

feat: allow CustomType and RefType in arguments #387

Closed

stocaaro previously approved these changes Jan 13, 2025

View reviewed changes

iartemiev requested changes Jan 14, 2025

View reviewed changes

Fix: Generate nested CustomTypes in schema output

c54ed9b

vaisshnavi7 dismissed stocaaro’s stale review via c54ed9b January 15, 2025 04:20

vaisshnavi7 changed the title ~~Feat: allow customtypes ref in arguments/main~~ feat: allow customtypes ref in arguments/main Jan 15, 2025

vaisshnavi7 requested review from iartemiev and stocaaro January 15, 2025 04:31

vaisshnavi7 and others added 2 commits January 14, 2025 20:40

Merge branch 'main' into feat/allow-customtypes-ref-in-arguments/main

a7dd990

refactor: Add tests for nested custom types and mixed refs

cc50e20

stocaaro reviewed Jan 15, 2025

View reviewed changes

packages/data-schema/__tests__/CustomOperations.test-d.ts Show resolved Hide resolved

vaisshnavi7 and others added 2 commits January 15, 2025 21:45

Add test for RefType with nested CustomType in complex structures

fff0b5d

Merge branch 'main' into feat/allow-customtypes-ref-in-arguments/main

f4b9171

stocaaro reviewed Jan 16, 2025

View reviewed changes

fix: Adjust schema component ordering to minimize enum reordering

949e6ee

Revert unnecessary changes for custom types and refs

cdb02d1

vaisshnavi7 mentioned this pull request Jan 21, 2025

style: Code formatting in SchemaProcessor and CustomOperations tests #504

Merged

vaisshnavi7 added 2 commits January 21, 2025 11:31

Merge branch 'main' into feat/allow-customtypes-ref-in-arguments/main

2f8c281

Add unit tests for custom operations with custom types and refs in Cu…

6b82830

…stomOperations

stocaaro reviewed Jan 22, 2025

View reviewed changes

vaisshnavi7 added 2 commits January 22, 2025 18:41

Refactor schemaPreprocessor Consolidate schema components into single…

f1b8e0e

… array

Improve tests for CustomType and RefType arguments in custom operations

acaa7d4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow customtypes ref in arguments/main #497

feat: allow customtypes ref in arguments/main #497

vaisshnavi7 commented Jan 10, 2025

changeset-bot bot commented Jan 10, 2025 •

edited

Loading

stocaaro left a comment

stocaaro Jan 13, 2025

iartemiev left a comment •

edited

Loading

stocaaro left a comment

stocaaro left a comment

stocaaro Jan 16, 2025

vaisshnavi7 Jan 17, 2025

stocaaro Jan 17, 2025

stocaaro Jan 16, 2025

vaisshnavi7 Jan 17, 2025

stocaaro Jan 16, 2025

vaisshnavi7 Jan 16, 2025 •

edited

Loading

stocaaro Jan 22, 2025

vaisshnavi7 Jan 23, 2025

stocaaro Jan 22, 2025

stocaaro Jan 22, 2025

vaisshnavi7 Jan 23, 2025

stocaaro Jan 22, 2025

vaisshnavi7 Jan 23, 2025

stocaaro Jan 22, 2025

stocaaro Jan 22, 2025

		const scalarDefinition = `scalar ${typeName}`;
		generatedTypes.add(scalarDefinition);

feat: allow customtypes ref in arguments/main #497

Are you sure you want to change the base?

feat: allow customtypes ref in arguments/main #497

Conversation

vaisshnavi7 commented Jan 10, 2025

changeset-bot bot commented Jan 10, 2025 • edited Loading

🦋 Changeset detected

stocaaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iartemiev left a comment • edited Loading

Choose a reason for hiding this comment

stocaaro left a comment

Choose a reason for hiding this comment

stocaaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaisshnavi7 Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Jan 10, 2025 •

edited

Loading

iartemiev left a comment •

edited

Loading

vaisshnavi7 Jan 16, 2025 •

edited

Loading