Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,31 @@ export const schema = configure({
"
`;

exports[`Type name conversions should annotate scalar int fields with existing default with \`.default()\` 1`] = `
"/* eslint-disable */
/* THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. */
import { a } from \\"@aws-amplify/data-schema\\";
import { configure } from \\"@aws-amplify/data-schema/internals\\";
import { secret } from \\"@aws-amplify/backend\\";

export const schema = configure({
database: {
identifier: \\"ID1234567890\\",
engine: \\"postgresql\\",
connectionUri: secret(\\"CONN_STR\\")
}
}).schema({
\\"CoffeeQueue\\": a.model({
id: a.integer().default(),
name: a.string(),
orderNumber: a.integer().default()
Comment on lines +262 to +265
Copy link
Contributor Author

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.

}).identifier([
\\"id\\"
])
});
"
`;

exports[`Type name conversions ts schema generator should invoke generate schema 1`] = `
"/* eslint-disable */
/* THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,19 @@ type Story @refersTo(name: \\"stories\\") @model {
"
`;

exports[`generate graphqlSchemaFromSQLSchema creates graphql schema from "postgres" "serial" schema 1`] = `
Copy link
Contributor Author

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

"input AMPLIFY {
engine: String = \\"postgres\\"
}


type SerialTable @refersTo(name: \\" serial_table\\") @model {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

id: Int! @primaryKey
number: Int
}
"
`;

exports[`generate graphqlSchemaFromSQLSchema creates graphql schema from "postgres" "todo" schema 1`] = `
"input AMPLIFY {
engine: String = \\"postgres\\"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -279,3 +279,38 @@ Array [
},
]
`;

exports[`testPostgresStringDataSourceAdapter sets the model field correctly from a schema with a SERIAL field 1`] = `
Copy link
Contributor Author

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.

Array [
Object {
"default": Object {
"kind": "DB_GENERATED",
"value": "nextval('serial_table_id_seq'::regclass)",
},
"length": "",
"name": "id",
"type": Object {
"kind": "NonNull",
"type": Object {
"kind": "Scalar",
"name": "Int",
},
},
},
Object {
"default": Object {
"kind": "DB_GENERATED",
"value": "nextval('serial_table_number_seq'::regclass)",
},
"length": "",
"name": "number",
"type": Object {
"kind": "NonNull",
"type": Object {
"kind": "Scalar",
"name": "Int",
},
},
},
]
`;
Original file line number Diff line number Diff line change
Expand Up @@ -155,5 +155,16 @@ comments,comment_text,NULL,4,varchar,varchar(30),NO,30,NULL,NULL,NULL,NULL`,
"stories",NULL,NULL,"placeholder",NULL,3,"character","bpchar","NO",1,NULL,NULL,NULL
"stories",NULL,NULL,"pub_id",NULL,1,"integer","int4","NO",NULL,"stories_pkey","PRIMARY KEY","pub_id, pub_type"
"stories",NULL,NULL,"pub_type","'S'::bpchar",2,"character","bpchar","NO",1,"stories_pkey","PRIMARY KEY","pub_id, pub_type"`,
/*
* CREATE TABLE serial_table (
* id SERIAL PRIMARY KEY,
* number SERIAL,
* );
*/
serial: `
"table_name","enum_name","enum_values","column_name","column_default","ordinal_position","data_type","udt_name","is_nullable","character_maximum_length","indexname","constraint_type","index_columns"
serial_table,,,id,nextval('serial_table_id_seq'::regclass),1,integer,int4,NO,,serial_table_pkey,PRIMARY KEY,id
serial_table,,,number,nextval('serial_table_number_seq'::regclass),2,integer,int4,NO,,,,
`,
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,108 @@ describe('Type name conversions', () => {
expect(graphqlSchema).toMatchSnapshot();
});

it.each([
Copy link
Contributor Author

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.

{
case: 'string',
field: () => {
const f = new Field('field', { kind: 'Scalar', name: 'String' });
f.default = { kind: 'DB_GENERATED', value: 'A squat grey building of only thirty-four stouries' };
return f;
},
},
{
case: 'Float',
field: () => {
const f = new Field('field', { kind: 'Scalar', name: 'Float' });
f.default = { kind: 'DB_GENERATED', value: 3.14 };
return f;
},
},
{
case: 'List',
field: () => {
const f = new Field('field', { kind: 'List', type: { kind: 'Scalar', name: 'String' } });
f.default = { kind: 'DB_GENERATED', value: false };
return f;
},
},
{
case: 'CustomType',
field: () => {
const f = new Field('field', { kind: 'Custom', name: 'MyCustomType' });
f.default = { kind: 'DB_GENERATED', value: 'I could make of both names nothing longer or more explicit than Pip' };
return f;
},
},
{
case: 'Transformer Generated',
field: () => {
const f = new Field('field', { kind: 'Scalar', name: 'Int' });
f.default = { kind: 'TRANSFORMER_GENERATED', value: 42 };
return f;
},
},
{
case: 'Default Integer Constant',
field: () => {
const f = new Field('field', { kind: 'Scalar', name: 'Int' });
f.default = { kind: 'DB_GENERATED', value: 42 };
return f;
},
},
{
case: 'No default',
field: () => new Field('field', { kind: 'Scalar', name: 'Int' }),
},
])('should not annotate fields with `.default()` where we do not support db generation (case: %case)', (test) => {
const dbschema = new Schema(new Engine('Postgres'));
const model = new Model('User');
model.addField(new Field('id', { kind: 'NonNull', type: { kind: 'Scalar', name: 'String' } }));
model.setPrimaryKey(['id']);

model.addField(test.field());

dbschema.addModel(model);
const config: DataSourceGenerateConfig = {
identifier: 'ID1234567890',
secretNames: {
connectionUri: 'CONN_STR',
},
};

const graphqlSchema = generateTypescriptDataSchema(dbschema, config);
const containsDefault = graphqlSchema.includes('default()');
expect(containsDefault).toBe(false);
});

it('should annotate scalar int fields with existing default with `.default()`', async () => {
const dbschema = new Schema(new Engine('Postgres'));

const model = new Model('CoffeeQueue');

const serialPKField = new Field('id', { kind: 'NonNull', type: { kind: 'Scalar', name: 'Int' } });
serialPKField.default = { kind: 'DB_GENERATED', value: "nextval('coffeequeue_id_seq'::regclass)" };
model.addField(serialPKField);
model.setPrimaryKey(['id']);

model.addField(new Field('name', { kind: 'Scalar', name: 'String' }));

const serialField = new Field('orderNumber', { kind: 'Scalar', name: 'Int' });
serialField.default = { kind: 'DB_GENERATED', value: "nextval('coffeequeue_ordernumber_seq'::regclass)" };
model.addField(serialField);

dbschema.addModel(model);
const config: DataSourceGenerateConfig = {
identifier: 'ID1234567890',
secretNames: {
connectionUri: 'CONN_STR',
},
};

const graphqlSchema = generateTypescriptDataSchema(dbschema, config);
expect(graphqlSchema).toMatchSnapshot();
});

it('schema with database config without vpc should generate typescript data schema with configure', () => {
const dbschema = new Schema(new Engine('MySQL'));
let model = new Model('User');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

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.

Copy link
Contributor Author

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",
    },

});

it('sets the correct models from a news schema', () => {
const adapter = new PostgresStringDataSourceAdapter(schemas.postgres.news);
expect(adapter.getModels()).toMatchSnapshot();
Expand Down
6 changes: 3 additions & 3 deletions packages/amplify-graphql-schema-generator/src/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ import { DocumentNode } from 'graphql';
import { Schema, Engine } from './schema-representation';
import { generateGraphQLSchema } from './schema-generator';
import { constructRDSGlobalAmplifyInput } from './input';
import { MySQLStringDataSourceAdapter, PostgresStringDataSourceAdapter } from './datasource-adapter';
import { MySQLStringDataSourceAdapter, PostgresStringDataSourceAdapter, StringDataSourceAdapter } from './datasource-adapter';

const buildSchemaFromString = (stringSchema: string, engineType: ImportedRDSType): Schema => {
let schema;
let adapter;
let schema: Schema;
let adapter: StringDataSourceAdapter;
switch (engineType) {
case ImportedRDSType.MYSQL:
adapter = new MySQLStringDataSourceAdapter(stringSchema);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import ts from 'typescript';
import { TYPESCRIPT_DATA_SCHEMA_CONSTANTS } from 'graphql-transformer-common';
import { VpcConfig } from '@aws-amplify/graphql-transformer-interfaces';
import { DBEngineType, Field, FieldType, Model, Schema } from '../schema-representation';
import { DBEngineType, Field, Model, Schema } from '../schema-representation';

const GQL_TYPESCRIPT_DATA_SCHEMA_TYPE_MAP = {
string: 'string',
Expand All @@ -27,32 +27,46 @@ const GQL_TYPESCRIPT_DATA_SCHEMA_TYPE_MAP = {
* @returns Typescript data schema property in TS Node format
*/
const createProperty = (field: Field): ts.Node => {
const typeExpression = createDataType(field.type);
const typeExpression = createDataType(field);
return ts.factory.createPropertyAssignment(ts.factory.createIdentifier(field.name), typeExpression as ts.Expression);
};

/**
* Creates a typescript data schema type from internal SQL schema representation
* Example typescript data schema type output: `a.string().required()`
* @param type SQL IR field type
* @param field SQL IR field
* @returns Typescript data schema type in TS Node format
*/
const createDataType = (type: FieldType): ts.Node => {
if (type.kind === 'Scalar') {
const createDataType = (field: Field): ts.Node => {
const sequenceRegex = /^nextval\(.+::regclass\)$/;
Copy link
Contributor Author

@p5quared p5quared Oct 14, 2024

Choose a reason for hiding this comment

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

Regex Explain

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.

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

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

Can we find documentation that this is the canonical way to determine whether a field is serial? In the past we've relied on experimentally-verified "rules" that turned out to be incomplete, so I'd love to get ahead of that this time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added main reply with my research/argument later in thread. I don't believe there is a truly canonical way to identify serial but we can identify if a column is backed by a sequence.

if (field.default?.kind === 'DB_GENERATED' && sequenceRegex.test(field.default.value.toString())) {
Copy link
Contributor

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

const baseTypeExpression =
field.type.kind === 'NonNull'
? createDataType(new Field(field.name, field.type.type))
: createDataType(new Field(field.name, field.type));

return ts.factory.createCallExpression(
ts.factory.createPropertyAccessExpression(baseTypeExpression as ts.Expression, TYPESCRIPT_DATA_SCHEMA_CONSTANTS.DEFAULT_METHOD),
undefined,
undefined,
);
}

if (field.type.kind === 'Scalar') {
return ts.factory.createCallExpression(
ts.factory.createIdentifier(`${TYPESCRIPT_DATA_SCHEMA_CONSTANTS.REFERENCE_A}.${getTypescriptDataSchemaType(type.name)}`),
ts.factory.createIdentifier(`${TYPESCRIPT_DATA_SCHEMA_CONSTANTS.REFERENCE_A}.${getTypescriptDataSchemaType(field.type.name)}`),
undefined,
undefined,
);
}

if (type.kind === 'Enum') {
if (field.type.kind === 'Enum') {
return ts.factory.createCallExpression(
ts.factory.createIdentifier(`${TYPESCRIPT_DATA_SCHEMA_CONSTANTS.REFERENCE_A}.${TYPESCRIPT_DATA_SCHEMA_CONSTANTS.ENUM_METHOD}`),
undefined,
[
ts.factory.createArrayLiteralExpression(
type.values.map((value) => ts.factory.createStringLiteral(value)),
field.type.values.map((value) => ts.factory.createStringLiteral(value)),
true,
),
],
Expand All @@ -61,7 +75,7 @@ const createDataType = (type: FieldType): ts.Node => {

// We do not import any Database type as 'Custom' type.
// In case if there is a custom type in the IR schema, we will import it as string.
if (type.kind === 'Custom') {
if (field.type.kind === 'Custom') {
return ts.factory.createCallExpression(
ts.factory.createIdentifier(`${TYPESCRIPT_DATA_SCHEMA_CONSTANTS.REFERENCE_A}.${TYPESCRIPT_DATA_SCHEMA_CONSTANTS.STRING_METHOD}`),
undefined,
Expand All @@ -70,9 +84,11 @@ const createDataType = (type: FieldType): ts.Node => {
}

// List or NonNull
const modifier = type.kind === 'List' ? TYPESCRIPT_DATA_SCHEMA_CONSTANTS.ARRAY_METHOD : TYPESCRIPT_DATA_SCHEMA_CONSTANTS.REQUIRED_METHOD;
const modifier =
field.type.kind === 'List' ? TYPESCRIPT_DATA_SCHEMA_CONSTANTS.ARRAY_METHOD : TYPESCRIPT_DATA_SCHEMA_CONSTANTS.REQUIRED_METHOD;
const unwrappedField = new Field(field.name, field.type.type);
return ts.factory.createCallExpression(
ts.factory.createPropertyAccessExpression(createDataType(type.type) as ts.Expression, ts.factory.createIdentifier(modifier)),
ts.factory.createPropertyAccessExpression(createDataType(unwrappedField) as ts.Expression, ts.factory.createIdentifier(modifier)),
undefined,
undefined,
);
Expand Down
1 change: 1 addition & 0 deletions packages/graphql-transformer-common/API.md
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,7 @@ export const TYPESCRIPT_DATA_SCHEMA_CONSTANTS: {
IDENTIFIER_METHOD: string;
ARRAY_METHOD: string;
REQUIRED_METHOD: string;
DEFAULT_METHOD: string;
STRING_METHOD: string;
ENUM_METHOD: string;
REFERENCE_A: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export const TYPESCRIPT_DATA_SCHEMA_CONSTANTS = {
IDENTIFIER_METHOD: 'identifier',
ARRAY_METHOD: 'array',
REQUIRED_METHOD: 'required',
DEFAULT_METHOD: 'default',
STRING_METHOD: 'string',
ENUM_METHOD: 'enum',
REFERENCE_A: 'a',
Expand Down
Loading