From 4287c630295c304c7ff8343922926b4830b75cd4 Mon Sep 17 00:00:00 2001 From: Josue Ruiz Date: Thu, 28 Jan 2021 13:05:51 -0800 Subject: [PATCH] feat(graphql-key-transformer): change default to add GSIs when using @key (#5648) --- .../src/feature-flags/featureFlags.ts | 8 ++- .../amplify-e2e-core/src/categories/api.ts | 7 ++- .../api.key.migration.test.ts | 37 +++++++++++- .../src/transform-graphql-schema.ts | 3 +- .../src/KeyTransformer.ts | 3 +- .../src/util/amplifyUtils.ts | 3 + .../src/util/sanity-check.ts | 22 +++++++ .../__tests__/KeyTransformerLocal.e2e.test.ts | 59 +++++++++++++++++-- 8 files changed, 129 insertions(+), 13 deletions(-) diff --git a/packages/amplify-cli-core/src/feature-flags/featureFlags.ts b/packages/amplify-cli-core/src/feature-flags/featureFlags.ts index 6f6800d9b26..e27809eb104 100644 --- a/packages/amplify-cli-core/src/feature-flags/featureFlags.ts +++ b/packages/amplify-cli-core/src/feature-flags/featureFlags.ts @@ -533,6 +533,12 @@ export class FeatureFlags { defaultValueForExistingProjects: false, defaultValueForNewProjects: false, }, + { + name: 'secondaryKeyAsGSI', + type: 'boolean', + defaultValueForExistingProjects: false, + defaultValueForNewProjects: true, + }, ]); this.registerFlag('frontend-ios', [ @@ -568,7 +574,7 @@ export class FeatureFlags { type: 'boolean', defaultValueForExistingProjects: false, defaultValueForNewProjects: true, - }, + } ]); }; } diff --git a/packages/amplify-e2e-core/src/categories/api.ts b/packages/amplify-e2e-core/src/categories/api.ts index 5b15bc63741..293e54433b3 100644 --- a/packages/amplify-e2e-core/src/categories/api.ts +++ b/packages/amplify-e2e-core/src/categories/api.ts @@ -138,9 +138,12 @@ export function addApiWithSchemaAndConflictDetection(cwd: string, schemaFile: st }); } -export function updateApiSchema(cwd: string, projectName: string, schemaName: string) { +export function updateApiSchema(cwd: string, projectName: string, schemaName: string, forceUpdate: boolean = false) { const testSchemaPath = getSchemaPath(schemaName); - const schemaText = fs.readFileSync(testSchemaPath).toString(); + let schemaText = fs.readFileSync(testSchemaPath).toString(); + if (forceUpdate) { + schemaText += ' '; + } updateSchema(cwd, projectName, schemaText); } diff --git a/packages/amplify-migration-tests/src/__tests__/migration_tests/transformer_migration/api.key.migration.test.ts b/packages/amplify-migration-tests/src/__tests__/migration_tests/transformer_migration/api.key.migration.test.ts index 8a75bf830d1..dbb363e69a4 100644 --- a/packages/amplify-migration-tests/src/__tests__/migration_tests/transformer_migration/api.key.migration.test.ts +++ b/packages/amplify-migration-tests/src/__tests__/migration_tests/transformer_migration/api.key.migration.test.ts @@ -1,6 +1,16 @@ -import { initJSProjectWithProfile, deleteProject, amplifyPush, amplifyPushForce } from 'amplify-e2e-core'; -import { addApiWithSchema, apiGqlCompile } from 'amplify-e2e-core'; -import { createNewProjectDir, deleteProjectDir } from 'amplify-e2e-core'; +import { + initJSProjectWithProfile, + deleteProject, + amplifyPush, + amplifyPushForce, + addApiWithSchema, + updateApiSchema, + apiGqlCompile, + createNewProjectDir, + deleteProjectDir, + amplifyPushUpdate, + addFeatureFlag +} from 'amplify-e2e-core'; describe('amplify key force push', () => { let projRoot: string; @@ -24,4 +34,25 @@ describe('amplify key force push', () => { await apiGqlCompile(projRoot, true); await amplifyPushForce(projRoot, true); }); + + it('init project, add lsi key and force push expect error', async () => { + const projectName = 'keyforce'; + const initialSchema = 'migrations_key/initial_schema.graphql'; + // init, add api and push with installed cli + await initJSProjectWithProfile(projRoot, { name: projectName }); + await addApiWithSchema(projRoot, initialSchema); + await amplifyPush(projRoot); + // add feature flag + addFeatureFlag(projRoot, 'graphqltransformer', 'secondaryKeyAsGSI', true); + // forceUpdateSchema + updateApiSchema(projRoot, projectName, initialSchema, true); + // gql-compile and force push with codebase cli + await expect( + amplifyPushUpdate( + projRoot, + /Attempting to remove a local secondary index on the TodoTable table in the Todo stack.*/, + true, + ), + ).rejects.toThrowError(/Attempting to remove a local secondary index on the TodoTable table in the Todo stack.*/); + }); }); diff --git a/packages/amplify-provider-awscloudformation/src/transform-graphql-schema.ts b/packages/amplify-provider-awscloudformation/src/transform-graphql-schema.ts index 39899de41bc..fb24a344b73 100644 --- a/packages/amplify-provider-awscloudformation/src/transform-graphql-schema.ts +++ b/packages/amplify-provider-awscloudformation/src/transform-graphql-schema.ts @@ -31,6 +31,7 @@ import { migrateAPIProject, readProjectConfiguration, buildAPIProject, + TransformConfig, } from 'graphql-transformer-core'; import { print } from 'graphql'; @@ -72,7 +73,7 @@ function getTransformerFactory(context, resourceDir, authConfig?) { transformerList.push(new SearchableModelTransformer()); } - const customTransformersConfig = await readTransformerConfiguration(resourceDir); + const customTransformersConfig: TransformConfig = await readTransformerConfiguration(resourceDir); const customTransformers = (customTransformersConfig && customTransformersConfig.transformers ? customTransformersConfig.transformers : [] diff --git a/packages/graphql-key-transformer/src/KeyTransformer.ts b/packages/graphql-key-transformer/src/KeyTransformer.ts index 1d2c5269a42..6a7a4d3d7a8 100644 --- a/packages/graphql-key-transformer/src/KeyTransformer.ts +++ b/packages/graphql-key-transformer/src/KeyTransformer.ts @@ -646,6 +646,7 @@ export class KeyTransformer extends Transformer { const tableResource = ctx.getResource(tableLogicalID); const primaryKeyDirective = getPrimaryKey(definition); const primaryPartitionKeyName = primaryKeyDirective ? getDirectiveArguments(primaryKeyDirective).fields[0] : 'id'; + const defaultGSI = ctx.featureFlags.getBoolean('secondaryKeyAsGSI', false); if (!tableResource) { throw new InvalidDirectiveError(`The @key directive may only be added to object definitions annotated with @model.`); } else { @@ -656,7 +657,7 @@ export class KeyTransformer extends Transformer { ProjectionType: 'ALL', }), }; - if (primaryPartitionKeyName === ks[0].AttributeName) { + if (primaryPartitionKeyName === ks[0].AttributeName && !defaultGSI) { // This is an LSI. // Add the new secondary index and update the table's attribute definitions. tableResource.Properties.LocalSecondaryIndexes = append( diff --git a/packages/graphql-transformer-core/src/util/amplifyUtils.ts b/packages/graphql-transformer-core/src/util/amplifyUtils.ts index 6dffecc812b..ebde4330ffc 100644 --- a/packages/graphql-transformer-core/src/util/amplifyUtils.ts +++ b/packages/graphql-transformer-core/src/util/amplifyUtils.ts @@ -11,6 +11,7 @@ import { FeatureFlagProvider } from '../FeatureFlags'; import { cantAddAndRemoveGSIAtSameTimeRule, cantAddLSILaterRule, + cantRemoveLSILater, cantEditGSIKeySchemaRule, cantEditKeySchemaRule, cantEditLSIKeySchemaRule, @@ -64,6 +65,7 @@ export async function buildProject(opts: ProjectOptions) { // LSI cantEditKeySchemaRule, cantAddLSILaterRule, + cantRemoveLSILater, cantEditLSIKeySchemaRule, ]; @@ -74,6 +76,7 @@ export async function buildProject(opts: ProjectOptions) { // LSI cantEditKeySchemaRule, cantAddLSILaterRule, + cantRemoveLSILater, cantEditLSIKeySchemaRule, // GSI cantEditGSIKeySchemaRule, diff --git a/packages/graphql-transformer-core/src/util/sanity-check.ts b/packages/graphql-transformer-core/src/util/sanity-check.ts index 4f535bace33..689081346f8 100644 --- a/packages/graphql-transformer-core/src/util/sanity-check.ts +++ b/packages/graphql-transformer-core/src/util/sanity-check.ts @@ -333,6 +333,28 @@ export const cantEditLSIKeySchemaRule = (diff: Diff, currentBuild: DiffableProje } }; +export function cantRemoveLSILater(diff: Diff, currentBuild: DiffableProject, nextBuild: DiffableProject) { + const throwError = (stackName: string, tableName: string): void => { + throw new InvalidMigrationError( + `Attempting to remove a local secondary index on the ${tableName} table in the ${stackName} stack.`, + 'A local secondary index cannot be removed after deployment.', + 'In order to remove the local secondary index you need to delete or rename the table.', + ); + }; + // if removing more than one lsi + if (diff.kind === 'D' && diff.lhs && diff.path.length === 6 && diff.path[5] === 'LocalSecondaryIndexes') { + const tableName = diff.path[3]; + const stackName = path.basename(diff.path[1], '.json'); + throwError(stackName, tableName); + } + // if removing one lsi + if(diff.kind === 'A' && diff.item.kind === 'D' && diff.path.length === 6 && diff.path[5] === 'LocalSecondaryIndexes') { + const tableName = diff.path[3]; + const stackName = path.basename(diff.path[1], '.json'); + throwError(stackName, tableName); + } +} + export const cantHaveMoreThan500ResourcesRule = (diffs: Diff[], currentBuild: DiffableProject, nextBuild: DiffableProject): void => { const stackKeys = Object.keys(nextBuild.stacks); diff --git a/packages/graphql-transformers-e2e-tests/src/__tests__/KeyTransformerLocal.e2e.test.ts b/packages/graphql-transformers-e2e-tests/src/__tests__/KeyTransformerLocal.e2e.test.ts index e2b5b6dfcbc..f1bf02a3f6e 100644 --- a/packages/graphql-transformers-e2e-tests/src/__tests__/KeyTransformerLocal.e2e.test.ts +++ b/packages/graphql-transformers-e2e-tests/src/__tests__/KeyTransformerLocal.e2e.test.ts @@ -1,4 +1,4 @@ -import { GraphQLTransform } from 'graphql-transformer-core'; +import { GraphQLTransform, FeatureFlagProvider } from 'graphql-transformer-core'; import { DynamoDBModelTransformer } from 'graphql-dynamodb-transformer'; import { KeyTransformer } from 'graphql-key-transformer'; import { parse, FieldDefinitionNode, ObjectTypeDefinitionNode, Kind, InputObjectTypeDefinitionNode } from 'graphql'; @@ -315,7 +315,7 @@ test('Test that a secondary @key with a multiple field adds an GSI.', () => { expect(deleteInput.fields).toHaveLength(2); }); -test('Test that a secondary @key with a multiple field adds an LSI.', () => { +test('Test that a secondary @key with a multiple field adds an LSI with GSI FF turned off', () => { const validSchema = ` type Test @model @key(fields: ["email", "createdAt"]) @@ -326,9 +326,15 @@ test('Test that a secondary @key with a multiple field adds an LSI.', () => { } `; - const transformer = new GraphQLTransform({ - transformers: [new DynamoDBModelTransformer(), new KeyTransformer()], - }); + const transformer = new GraphQLTransform({ + transformers: [new DynamoDBModelTransformer(), new KeyTransformer()], + featureFlags: { + getBoolean: (featureName: string, defaultValue: boolean) => { + if (featureName === 'secondaryKeyAsGSI') return false; + return defaultValue || false; + }, + } as unknown as FeatureFlagProvider + }); const out = transformer.transform(validSchema); let tableResource = out.stacks.Test.Resources.TestTable; @@ -352,6 +358,49 @@ test('Test that a secondary @key with a multiple field adds an LSI.', () => { expectArguments(listTestsField, ['email', 'createdAt', 'filter', 'nextToken', 'limit', 'sortDirection']); }); +test('Test that a secondary @key with a multiple field adds an GSI based on enabled feature flag.', () => { + const validSchema = ` + type Test + @model @key(fields: ["email", "createdAt"]) + @key(name: "GSI_Email_UpdatedAt", fields: ["email", "updatedAt"], queryField: "testsByEmailByUpdatedAt") { + email: String! + createdAt: AWSDateTime! + updatedAt: AWSDateTime! + } + `; + + const transformer = new GraphQLTransform({ + transformers: [new DynamoDBModelTransformer(), new KeyTransformer()], + featureFlags: { + getBoolean: (featureName: string, defaultValue: boolean) => { + if (featureName === 'secondaryKeyAsGSI') return true; + return defaultValue || false; + }, + } as unknown as FeatureFlagProvider + }); + + const out = transformer.transform(validSchema); + let tableResource = out.stacks.Test.Resources.TestTable; + expect(tableResource).toBeDefined(); + expect(tableResource.Properties.GlobalSecondaryIndexes[0].KeySchema[0].AttributeName).toEqual('email'); + expect(tableResource.Properties.GlobalSecondaryIndexes[0].KeySchema[0].KeyType).toEqual('HASH'); + expect(tableResource.Properties.GlobalSecondaryIndexes[0].KeySchema[1].AttributeName).toEqual('updatedAt'); + expect(tableResource.Properties.GlobalSecondaryIndexes[0].KeySchema[1].KeyType).toEqual('RANGE'); + expect(tableResource.Properties.AttributeDefinitions.find(ad => ad.AttributeName === 'email').AttributeType).toEqual('S'); + expect(tableResource.Properties.AttributeDefinitions.find(ad => ad.AttributeName === 'updatedAt').AttributeType).toEqual('S'); + expect(tableResource.Properties.AttributeDefinitions.find(ad => ad.AttributeName === 'createdAt').AttributeType).toEqual('S'); + const schema = parse(out.schema); + const queryType = schema.definitions.find((def: any) => def.name && def.name.value === 'Query') as ObjectTypeDefinitionNode; + const queryIndexField = queryType.fields.find(f => f.name && f.name.value === 'testsByEmailByUpdatedAt') as FieldDefinitionNode; + expect(queryIndexField.arguments).toHaveLength(6); + expectArguments(queryIndexField, ['email', 'updatedAt', 'filter', 'nextToken', 'limit', 'sortDirection']); + + // When using a complex primary key args are added to the list field. They are optional and if provided, will use a Query instead of a Scan. + const listTestsField = queryType.fields.find(f => f.name && f.name.value === 'listTests') as FieldDefinitionNode; + expect(listTestsField.arguments).toHaveLength(6); + expectArguments(listTestsField, ['email', 'createdAt', 'filter', 'nextToken', 'limit', 'sortDirection']); +}); + test('Test that a primary @key with complex fields will update the input objects.', () => { const validSchema = ` type Test @model @key(fields: ["email"]) {