feat: auto increment support #2883
Conversation
p5quared
force-pushed
the
auto-increment-support
branch
2 times, most recently
from
9922afd
to
c0bc7ff
Compare
...l-default-value-transformer/src/__tests__/amplify-grapphql-default-value-transformer.test.ts
Show resolved
Hide resolved
p5quared
force-pushed
the
auto-increment-support
branch
2 times, most recently
from
7ddef03
to
376fa27
Compare
p5quared
force-pushed
the
auto-increment-support
branch
2 times, most recently
from
29a8c20
to
3896d20
Compare
p5quared
force-pushed
the
auto-increment-support
branch
4 times, most recently
from
5235728
to
9708662
Compare
p5quared
force-pushed
the
auto-increment-support
branch
2 times, most recently
from
9d796c6
to
ead5e20
Compare
packages/amplify-graphql-api-construct-tests/src/__tests__/sql-pg-auto-increment.test.ts
Show resolved
Hide resolved
| `; | ||
|
|
||
| const { projFolderName, region, connectionConfigName, dbController, resourceNames } = options; | ||
| const templatePath = path.resolve(path.join(__dirname, '..', '__tests__', 'backends', 'sql-models')); |
There was a problem hiding this comment.
Now or in the next PR: Can we migrate this test to use the configurable stack so we can (eventually) remove the sql-models stack? Eventually I'd like us to be using just one stack to reduce the number of test fixtures we have to maintain.
There was a problem hiding this comment.
If it's okay I'd like to defer this migration to a separate PR and migrate a number of tests at once.
| describe(`${testBlockDescription} - ${engine}`, () => { | ||
| const amplifyGraphqlSchema = ` | ||
| type CoffeeQueue @model @refersTo(name: "${getRDSTableNamePrefix()}coffee_queue") { | ||
| orderNumber: Int! @primaryKey @default |
There was a problem hiding this comment.
It's worth nothing in a comment here the important characteristics of this tests:
- It's an
Intfield, backed by aSERIALtype in PG - It's a required field, which would normally mean that customers would have to supply a value in the GraphQL mutation input
- It has a
@defaultannotation with no arguments, meaning that the only behavior our transformer should do is to suppress the input requirement.
packages/amplify-graphql-api-construct-tests/src/sql-tests-common/sql-models-auto-increment.ts
Outdated
Show resolved
Hide resolved
| coffeeQueueTableCRUDLHelper.checkGenericError(error?.message); | ||
| } | ||
|
|
||
| const invalidOrderNumber = 99999999; |
There was a problem hiding this comment.
I assume this is invalid because it exceeds the maximum value of a SERIAL field? If so, worth noting that in a comment, in case we start supporting PG versions that have a higher limit.
There was a problem hiding this comment.
Actually it is invalid because it is greater than any existing record's orderNumber. That purpose is definitely not clear since even the number 4 should be invalid. I wrote some comments but then opted to use a more descriptive variable name:
const biggerThanAnyExistingOrderNumber = 99999999;
| transformers: [new ModelTransformer(), new DefaultValueTransformer(), new PrimaryKeyTransformer()], | ||
| dataSourceStrategies: constructDataSourceStrategies(schema, strategy), | ||
| }); | ||
| }).toThrow('The @default directive requires a value property on non Postgres datasources.'); |
There was a problem hiding this comment.
- Can you confirm that a
@default(value: ...)works as expected on a mysql data source? (That is, it passes specified value in the input if the mutation input doesn't include it; and it allows overrides of the default if it IS specified). Best way to confirm that would be a test (possibly an already existing one!) - Do we have a test (possibly an already existing one!) to assert that
@default(value:...)works for postgres?
There was a problem hiding this comment.
The only e2e usage of @default I could find only used DDB. I modified the general sql models test to utilize @default(value) which should verify behavior on both sources.
packages/amplify-graphql-default-value-transformer/src/graphql-default-value-transformer.ts
Outdated
Show resolved
Hide resolved
packages/amplify-graphql-default-value-transformer/src/graphql-default-value-transformer.ts
Outdated
Show resolved
Hide resolved
packages/amplify-graphql-transformer-core/src/utils/model-datasource-strategy-utils.ts
Outdated
Show resolved
Hide resolved
p5quared
force-pushed
the
auto-increment-support
branch
from
57dfce3
to
8f6f81b
Compare
Implements support for auto increment (serial) fields from Postgres datasources. Such fields are denoted by an empty `@default` applied to an `Int` field.
merge changes from fork
Co-authored-by: Tim Schmelter <[email protected]>
p5quared
force-pushed
the
auto-increment-support
branch
from
d4ee97e
to
37c7198
Compare
Description of changes
Transformer changes to support auto increment (
serial) fields on Postgres datasources. Accomplished by making the 'create' input types optional when@defaultis applied.@defaulthas changed, itsvalue: Stringparameter is now optional.default-value-transformerallows novalueon Int fields if the datasource is PostgresCDK / CloudFormation Parameters Changed
n/a
Description of how you validated changes
Unit tests, existing e2e tests,
testTransform. Will write e2e test after implementing changes to Lambda layer.Checklist
yarn testpassesCodebuild Tests
Ran against beta via
yarn split-e2e-tests-beta.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.