fix: handle Enums correctly on Typescript data schema generation #2870

tejas2008 · 2024-09-12T17:43:59Z

Description of changes

Update Schema generator package to add handling of enums globally
Inside the model use ref() to define those enums
Added unit test to handle various combination of enum possibilities for Postgresql and mysql

This change would only affect an existing customer only if they re import the schema which is highly unlikely so we have determined this change to be a non breaking change.

CDK / CloudFormation Parameters Changed

Issue #, if available

#2596

Description of how you validated changes

Updated a current test snapshot
Added more unit tests to validate proper enum handling generating snapshots and passing tests.

Checklist

PR description included
yarn test passes
Tests are changed or added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

… to fix undefined field

...fy-graphql-schema-generator/src/__tests__/__snapshots__/generate-ts-data-schema.test.ts.snap

phani-srikar · 2024-09-18T18:56:10Z

packages/amplify-graphql-schema-generator/src/schema-representation/schema.ts

@@ -15,6 +15,7 @@ export class Schema {
 }

 public addModel(model: Model): void {
+ console.log(this.engine);


nit: remove this logging

Sure I will remove this and add a commit

phani-srikar · 2024-09-18T19:03:08Z

packages/amplify-graphql-schema-generator/src/ts-schema-generator/helpers.ts

+
+ const combinedEnums = nullEnums.concat(notNullEnums).flat() // making 1 D array
+
+ if (engine === 'Postgres') { // for postgresql


For future: Instead of introducing these engine based conditionals that bloats up this method, we could move these to separate methods inside an engine specific controller.

Sure I will look into it.

@phani-srikar I have updated the implementation to make it less bloated in the current task as well and more strongly typed.

palpatim · 2024-09-18T19:04:00Z

packages/amplify-graphql-schema-generator/src/schema-representation/schema.ts

@@ -15,6 +15,7 @@ export class Schema {
 }

 public addModel(model: Model): void {
+ console.log(this.engine);


Is this leftover from dev/debugging? Any reason a customer needs to see this?

Yes its a leftover might have missed it I will remove it

palpatim · 2024-09-18T19:04:47Z

packages/amplify-graphql-schema-generator/src/ts-schema-generator/generate-ts-schema.ts

+
+
+
+


Run yarn prettier --write . from the top-level project dir to fix this and other linting errors before submitting.

palpatim · 2024-09-18T19:07:52Z

packages/amplify-graphql-schema-generator/src/ts-schema-generator/helpers.ts

- ),
- ],
- );
+ if(engine === 'Postgres'){


Looking at the code without context, it looks like we don't handle enum types for MySQL engine. Since we clearly do, let's add a comment explaining the logic about why we're doing this.

palpatim · 2024-09-18T19:10:21Z

packages/amplify-graphql-schema-generator/src/ts-schema-generator/helpers.ts

+ * @returns Typescript data schema type in TS Node format
+ */
+const createEnums = (type: EnumType, engine: string, modelName: string, enumName: string): ts.Node => {
+ if (engine === 'Postgres'){


nit: since engine is a string rather than a strongly typed value, let's make the engine handling in this method explicit and fail for unsupported engine types. That'll protect against typos in calling code. Alternately, use a strongly typed engine type to ensure we are operating on a list of values known at compile time.

palpatim · 2024-09-18T19:12:25Z

packages/amplify-graphql-schema-generator/src/ts-schema-generator/helpers.ts

 const tsSchema = ts.factory.createCallExpression(
 ts.factory.createPropertyAccessExpression(
 createConfigureExpression(schema, config),
- ts.factory.createIdentifier(TYPESCRIPT_DATA_SCHEMA_CONSTANTS.SCHEMA_METHOD),
+ ts.factory.createIdentifier(TYPESCRIPT_DATA_SCHEMA_CONSTANTS.SCHEMA_METHOD), // create a.schema()


prefer to remove dead code rather than leaving it commented

Suggested change

ts.factory.createIdentifier(TYPESCRIPT_DATA_SCHEMA_CONSTANTS.SCHEMA_METHOD), // create a.schema()

ts.factory.createIdentifier(TYPESCRIPT_DATA_SCHEMA_CONSTANTS.SCHEMA_METHOD),

It isn't a dead code its a comment, but its not very necessary since it is existing code so will probably remove it

palpatim · 2024-09-18T19:13:27Z

packages/amplify-graphql-schema-generator/src/ts-schema-generator/helpers.ts

@@ -147,19 +188,92 @@ export const createSchema = (schema: Schema, config?: DataSourceGenerateConfig):
 throw new Error('No valid tables found. Make sure at least one table has a primary key.');
 }

+ const engine = schema.getEngine().type;
+
+ const nullEnums = schema // find null enums in all models


naming nit: I think you mean "nullable enum fields" or rather than "null" enums? (The latter implies a concrete value assignment, as opposed to a field type constraint)

I will update the naming to have more clear variable names to avoid confusion. NullEnumFields and RequiredEnumFields makes sense I will go ahead with these naming conventions to provide more readability and understanding

palpatim · 2024-09-18T19:14:00Z

packages/amplify-graphql-schema-generator/src/ts-schema-generator/helpers.ts

+ }
+ }));
+
+ const notNullEnums = schema // find Non null enums in all models


naming nit: I think you mean "required" enums or "non null enum types"?

I will update the naming to have more clear variable names to avoid confusion.

…comments for more clarity

palpatim · 2024-09-20T19:34:04Z

packages/amplify-graphql-schema-generator/src/ts-schema-generator/helpers.ts

@@ -190,33 +195,35 @@ export const createSchema = (schema: Schema, config?: DataSourceGenerateConfig):

 const engine = schema.getEngine().type;

- const nullEnums = schema  // find null enums in all models
+ const nullEnumFields = schema // find null enum fields in all models


nit

Suggested change

const nullEnumFields = schema // find null enum fields in all models

const nullableEnumFields = schema

addressed it

palpatim · 2024-09-20T19:34:43Z

packages/amplify-graphql-schema-generator/src/ts-schema-generator/helpers.ts


-  const notNullEnums = schema  // find Non null enums in all models
+ const RequiredEnumFields = schema // find required enum fields in all models


nit: don't PascalCase non-type, non-Class variables.

Suggested change

const RequiredEnumFields = schema // find required enum fields in all models

const requiredEnumFields = schema

palpatim · 2024-09-20T19:37:06Z

packages/amplify-graphql-schema-generator/src/ts-schema-generator/helpers.ts

+ for postgresql since enums are declared globally to void declaring duplicate enums we
+ declare find unique enums and then pass it to the schema 


I can't tell what you mean with this explanation. Can you reword to clarify? (I mean to say, based on our discussion and reading through the code, I know what you're trying to do--it's the comment wording that is unclear.)

Also, nit: reserve the double-asterisk block comment structure (/** ... */ for docComments (such as we put at the beginning of an exported symbol). Other comments should be delimited with a double-slash (//)

Sure I will re word this and and remove the double-asterisk block structure

Added appropriate comment structure and comments to explain in brief the use case

phani-srikar · 2024-09-20T19:54:18Z

packages/amplify-graphql-schema-generator/src/ts-schema-generator/generate-ts-schema.ts

@@ -1,6 +1,7 @@
 import * as ts from 'typescript';
 import { Schema } from '../schema-representation';
 import { createImportExpression, createSchema, DataSourceGenerateConfig } from './helpers';
+import { LogFormat } from '@aws-sdk/client-lambda';


nit: Are you using this somewhere since there are no other changes in this file.

phani-srikar · 2024-09-24T23:57:38Z

...s/amplify-graphql-schema-generator/src/datasource-adapter/mysql-string-datasource-adapter.ts

 let enumName = enumNamePrefix;
 let counter = 0;
 while (this.enums.has(enumName)) {
- enumName = [enumNamePrefix, counter.toString()].join('_');
+ enumName = toPascalCase([enumNamePrefix, counter.toString()]);


Can you confirm this wouldn't be a breaking change if we change the naming of enums from table_field to TableField?

As per the discussion @palpatim the change could affect customers only if they reimport the schema which is highly unlikely, but we still are waiting for conformation from Josef on the same

…fy-category-api into Handle-enums-sql

p5quared · 2024-10-03T19:04:49Z

packages/amplify-graphql-schema-generator/src/ts-schema-generator/helpers.ts

+ .map((field) => {
+ if (field.type.kind === 'NonNull' && field.type.type.kind === 'Enum') {
+ return createEnums(field.type.type);
+ } else {
+ return undefined;
+ }
+ }),


Is checking field.type.kind and field.type.type.kind redundant because of the filter?

Suggested change

.map((field) => {

if (field.type.kind === 'NonNull' && field.type.type.kind === 'Enum') {

return createEnums(field.type.type);

} else {

return undefined;

}

}),

.map((field) => createEnums(field.type.type))

…y-api into Handle-enums-sql

tejas2008 and others added 6 commits September 12, 2024 10:40

added enums via ref for single models

3e4a3a2

extended implementation to enums for multiple models and added filter…

7bc5b64

… to fix undefined field

added naming in npascal case

6161264

added enum support for postgresql multiple models same enum

07535fa

added unit tests to test working enum handling

3436237

Merge branch 'aws-amplify:main' into Handle-enums-sql

6aed951

tejas2008 changed the title ~~added enums via ref for single models~~ fix: handle Enums correctly on Typescript data schema generation Sep 17, 2024

tejas2008 marked this pull request as ready for review September 17, 2024 22:29

tejas2008 requested a review from a team as a code owner September 17, 2024 22:29

phani-srikar reviewed Sep 18, 2024

View reviewed changes

palpatim reviewed Sep 18, 2024

View reviewed changes

tejas2008 added 3 commits September 19, 2024 16:57

added error handling for unsupported engines removed unused code and …

4030828

…comments for more clarity

variable naming updated for enums

c4f9b4d

fix linting issues

c3d3ee1

palpatim reviewed Sep 20, 2024

View reviewed changes

tejas2008 added 3 commits September 23, 2024 17:06

refactored code for readbility and updated unit tests

8227f72

fixed linting issues

bb0a235

updated one comment and fixed lint issue

5ab00c2

phani-srikar reviewed Sep 24, 2024

View reviewed changes

tejas2008 added 3 commits September 26, 2024 16:14

fixed nit

b064a2d

fixed nit naming

4e52a85

fixed nit naming

2a88fb9

tejas2008 enabled auto-merge September 30, 2024 19:19

lint errors fixed

3734e8f

tejas2008 requested a review from a team as a code owner October 2, 2024 17:55

tejas2008 and others added 5 commits October 2, 2024 11:30

extracted api

94b0767

Merge branch 'main' into Handle-enums-sql

c383ec5

extracted api transformer common

47697f4

Merge branch 'Handle-enums-sql' of https://github.com/tejas2008/ampli…

1b80f3b

…fy-category-api into Handle-enums-sql

helper lint

f11e13c

lint fix

9b04649

p5quared approved these changes Oct 3, 2024

View reviewed changes

Merge branch 'main' of https://github.com/aws-amplify/amplify-categor…

19273c7

…y-api into Handle-enums-sql

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle Enums correctly on Typescript data schema generation #2870

fix: handle Enums correctly on Typescript data schema generation #2870

tejas2008 commented Sep 12, 2024 •

edited

Loading

phani-srikar Sep 18, 2024

tejas2008 Sep 18, 2024

phani-srikar Sep 18, 2024

tejas2008 Sep 18, 2024

tejas2008 Sep 24, 2024

palpatim Sep 18, 2024

tejas2008 Sep 18, 2024

palpatim Sep 18, 2024

palpatim Sep 18, 2024

palpatim Sep 18, 2024

palpatim Sep 18, 2024

tejas2008 Sep 19, 2024

palpatim Sep 18, 2024

tejas2008 Sep 18, 2024

palpatim Sep 18, 2024

tejas2008 Sep 18, 2024

palpatim Sep 20, 2024

tejas2008 Sep 26, 2024

palpatim Sep 20, 2024

tejas2008 Sep 26, 2024

palpatim Sep 20, 2024

tejas2008 Sep 20, 2024

tejas2008 Sep 24, 2024

phani-srikar Sep 20, 2024

tejas2008 Sep 26, 2024

phani-srikar Sep 24, 2024

tejas2008 Sep 26, 2024

p5quared Oct 3, 2024


		const combinedEnums = nullEnums.concat(notNullEnums).flat() // making 1 D array

		if (engine === 'Postgres') { // for postgresql

	ts.factory.createIdentifier(TYPESCRIPT_DATA_SCHEMA_CONSTANTS.SCHEMA_METHOD), // create a.schema()
	ts.factory.createIdentifier(TYPESCRIPT_DATA_SCHEMA_CONSTANTS.SCHEMA_METHOD),

	const nullEnumFields = schema // find null enum fields in all models
	const nullableEnumFields = schema


		const notNullEnums = schema // find Non null enums in all models
		const RequiredEnumFields = schema // find required enum fields in all models

	const RequiredEnumFields = schema // find required enum fields in all models
	const requiredEnumFields = schema

		for postgresql since enums are declared globally to void declaring duplicate enums we
		declare find unique enums and then pass it to the schema

fix: handle Enums correctly on Typescript data schema generation #2870

Are you sure you want to change the base?

fix: handle Enums correctly on Typescript data schema generation #2870

Conversation

tejas2008 commented Sep 12, 2024 • edited Loading

Description of changes

CDK / CloudFormation Parameters Changed

Issue #, if available

Description of how you validated changes

Checklist

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

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

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

tejas2008 commented Sep 12, 2024 •

edited

Loading