-
Notifications
You must be signed in to change notification settings - Fork 77
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: generate ts schema from pg serial field #2952
base: main
Are you sure you want to change the base?
Conversation
8d4f420
to
f411e70
Compare
@@ -145,6 +145,19 @@ type Story @refersTo(name: \\"stories\\") @model { | |||
" | |||
`; | |||
|
|||
exports[`generate graphqlSchemaFromSQLSchema creates graphql schema from "postgres" "serial" schema 1`] = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This snapshot is not intentional as we are not intending to support GQL generation here. It is generated automatically to test against anyway from the schemas in __utils__
.
@@ -249,6 +249,108 @@ describe('Type name conversions', () => { | |||
expect(graphqlSchema).toMatchSnapshot(); | |||
}); | |||
|
|||
it.each([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These cases are not exhaustive over every Field
that could exist but cover the most unique cases.
f411e70
to
09e08e8
Compare
09e08e8
to
b793733
Compare
const createDataType = (type: FieldType): ts.Node => { | ||
if (type.kind === 'Scalar') { | ||
const createDataType = (field: Field): ts.Node => { | ||
const sequenceRegex = /^nextval\(.+::regclass\)$/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back and forth on a few approaches to this check but I eventually felt that a quick regex is the simplest surefire way to identify SERIAL
fields. In the future I think this logic could be extracted into something like if (canLetDBGenerate(field))
which evaluates rules isSerial(field) || isUserDefinedFunction(field)
etc... But I felt that might be premature for this implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For more context, here's what a value for column_default
looks like on a serial field when we query the DB schema:
nextval('serial_table_number_seq'::regclass)
.
\\"CoffeeQueue\\": a.model({ | ||
id: a.integer().default(), | ||
name: a.string(), | ||
orderNumber: a.integer().default() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the desired behavior of this PR.
@@ -279,3 +279,38 @@ Array [ | |||
}, | |||
] | |||
`; | |||
|
|||
exports[`testPostgresStringDataSourceAdapter sets the model field correctly from a schema with a SERIAL field 1`] = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behavior is not new but I am codifying it since generation depends on it now.
} | ||
|
||
|
||
type SerialTable @refersTo(name: \\" serial_table\\") @model { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know why we have blank spaces before the table name for refersTo
directive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whitespace in my template string with the field info csv was causing this--resolved.
@@ -133,6 +133,11 @@ describe('testPostgresStringDataSourceAdapter', () => { | |||
expect(adapter.getModels()).toMatchSnapshot(); | |||
}); | |||
|
|||
it('sets the model field correctly from a schema with a SERIAL field', () => { | |||
const adapter = new PostgresStringDataSourceAdapter(schemas.postgres.serial); | |||
expect(adapter.getModels()[0].getFields()).toMatchSnapshot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this validation, can we add a check to make sure getModels contains an element? If there are failures, the current implementation will throw a generic error like trying to access a property of undefined
which could be hard to troubleshoot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified this to match the other tests, e.g. just adapter.getModels()
. I think it's clearer and it also retains info that about the PK:
"primaryKey": Index {
"fields": Array [
"id",
],
"name": "PRIMARY_KEY",
},
if (type.kind === 'Scalar') { | ||
const createDataType = (field: Field): ts.Node => { | ||
const sequenceRegex = /^nextval\(.+::regclass\)$/; | ||
if (field.default?.kind === 'DB_GENERATED' && sequenceRegex.test(field.default.value.toString())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can possible move the check sequenceRegex.test(field.default.value.toString()
to a separate method so that it can reused in the future like isSequenceField(field)
.
the text alignment was causing weird names in snaphot
Description of changes
Update TS schema generator to properly annotate serial fields from Postgres datasources.
e.g.:
myField SERIAL
->myField: a.integer().default()
Explanation:
We already populate
field.default
when importing a DB schema, but don't really do anything with it prior to this change.Now, we check if the field default is
DB_GENERATED
, and the value for the generated field indicates it is aSERIAL
column. If this is the case, we annotate the TS schema field with.default()
.CDK / CloudFormation Parameters Changed
N/A
Issue #, if available
N/A
Description of how you validated changes
See test cases in
packages/amplify-graphql-schema-generator/src/__tests__/generate-ts-data-schema.test.ts
packages/amplify-graphql-schema-generator/src/__tests__/pg-string-datasource-adapter.test.ts
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.