From 175ab5bede1228393eb83cf4352839ee955bfaad Mon Sep 17 00:00:00 2001 From: Edward Foyle Date: Tue, 18 Oct 2022 15:03:53 -0700 Subject: [PATCH 1/6] fix: prevent removal of analytics and auth when being depended on --- .../src/commands/analytics/remove.ts | 45 ++--- .../src/plugin-client-api-notifications.ts | 3 +- .../src/__tests__/commands/remove.test.ts | 189 ++++++++---------- .../src/commands/auth/remove.ts | 42 ++-- .../src/errors/amplify-exception.ts | 3 +- 5 files changed, 140 insertions(+), 142 deletions(-) diff --git a/packages/amplify-category-analytics/src/commands/analytics/remove.ts b/packages/amplify-category-analytics/src/commands/analytics/remove.ts index 8d2713aee1c..96e818e9df2 100644 --- a/packages/amplify-category-analytics/src/commands/analytics/remove.ts +++ b/packages/amplify-category-analytics/src/commands/analytics/remove.ts @@ -1,30 +1,12 @@ import { - $TSContext, AmplifyCategories, amplifyFaultWithTroubleshootingLink, + $TSContext, AmplifyError, } from 'amplify-cli-core'; -import { printer } from 'amplify-prompts'; -import { checkResourceInUseByNotifications, invokeNotificationsAPIRecursiveRemoveApp } from '../../plugin-client-api-notifications'; +import { checkResourceInUseByNotifications } from '../../plugin-client-api-notifications'; const subcommand = 'remove'; const category = 'analytics'; -/** - * Check if Notifications is using the given Analytics resource. - * note:- TBD: This function will be replaced by a generic pre-remove hook handler in the future. - * The eventual goal is to remove all explicit binding in the code between categories and abstract them out - * by role (capabilities, providerCategory and subscriberCategory ). - */ -const removeResourceDependencies = async (context:$TSContext, resourceName: string): Promise => { - const isResourceInUse = await checkResourceInUseByNotifications(context, resourceName); - if (isResourceInUse) { - // Pinpoint App is in use by Notifications. - printer.warn(`Disabling all notifications on ${resourceName}`); - const result = await invokeNotificationsAPIRecursiveRemoveApp(context, resourceName); - if (!result.status) { - throw amplifyFaultWithTroubleshootingLink('ResourceRemoveFault', { - message: result.reasonMsg || `Failed to remove ${resourceName} from ${AmplifyCategories.NOTIFICATIONS} category`, - }); - } - } -}; + +export const name = subcommand; /** * Analytics remove resource handler. @@ -35,8 +17,19 @@ export const run = async (context: $TSContext): Promise => { const { amplify, parameters } = context; const resourceName = parameters.first; - await amplify.removeResource(context, category, resourceName, { headless: false }); - return removeResourceDependencies(context, resourceName); -}; + const blockRemovalOfInUseAnalytics = async (selectedAnalyticsResource: string): Promise => { + const isResourceInUse = await checkResourceInUseByNotifications(context, selectedAnalyticsResource); + if (isResourceInUse) { + throw new AmplifyError('ResourceInUseError', { + message: `Analytics resource ${selectedAnalyticsResource} is being used by the notifications category and cannot be removed`, + resolution: `Run 'amplify remove notifications', then retry removing analytics`, + }); + } + }; -export const name = subcommand; + // check if the CLI input analytics name is in use by notification + await blockRemovalOfInUseAnalytics(resourceName); + + // remove resource with a resourceName callback that will block removal if selecting an analytics resource that notifications depends on + await amplify.removeResource(context, category, resourceName, { headless: false }, blockRemovalOfInUseAnalytics); +}; diff --git a/packages/amplify-category-analytics/src/plugin-client-api-notifications.ts b/packages/amplify-category-analytics/src/plugin-client-api-notifications.ts index eeac63411d4..19c0a43173d 100644 --- a/packages/amplify-category-analytics/src/plugin-client-api-notifications.ts +++ b/packages/amplify-category-analytics/src/plugin-client-api-notifications.ts @@ -29,7 +29,8 @@ export const invokeNotificationsAPIRecursiveRemoveApp = async ( * @param resourceName Pinpoint resource name * @returns true if Pinpoint resource is in use */ -export const checkResourceInUseByNotifications = async (context: $TSContext, resourceName: string): Promise => { +export const checkResourceInUseByNotifications = async (context: $TSContext, resourceName?: string): Promise => { + if (!resourceName) return false; const notificationsResource = await invokeNotificationsAPIGetResource(context); return (notificationsResource?.resourceName === resourceName); }; diff --git a/packages/amplify-category-auth/src/__tests__/commands/remove.test.ts b/packages/amplify-category-auth/src/__tests__/commands/remove.test.ts index 63ee0520cae..d6dfabc3a2f 100644 --- a/packages/amplify-category-auth/src/__tests__/commands/remove.test.ts +++ b/packages/amplify-category-auth/src/__tests__/commands/remove.test.ts @@ -1,130 +1,115 @@ +import { stateManager, $TSContext, AmplifyError } from 'amplify-cli-core'; +import { printer } from 'amplify-prompts'; import * as remove from '../../commands/auth/remove'; import { messages } from '../../provider-utils/awscloudformation/assets/string-maps'; -import { $TSContext } from 'amplify-cli-core'; -import { printer } from 'amplify-prompts'; -jest.mock('amplify-prompts', () => ({ - printer: { - info: jest.fn(), - }, -})); +jest.mock('amplify-prompts'); -const saveCLIInputPayload_mock = jest.fn(); +const saveCLIInputPayloadMock = jest.fn(); jest.mock('fs-extra'); -jest.mock('../../provider-utils/awscloudformation/auth-inputs-manager/auth-input-state', () => { - return { - AuthInputState: jest.fn().mockImplementation(() => { - return { - isCLIInputsValid: jest.fn(), - getCLIInputPayload: jest.fn().mockImplementation(() => ({ - cognitoConfig: { - userPoolGroupList: ['admin'], - }, - })), - saveCLIInputPayload: saveCLIInputPayload_mock, - }; - }), - }; -}); +jest.mock('../../provider-utils/awscloudformation/auth-inputs-manager/auth-input-state', () => ({ + AuthInputState: jest.fn().mockImplementation(() => ({ + isCLIInputsValid: jest.fn(), + getCLIInputPayload: jest.fn().mockImplementation(() => ({ + cognitoConfig: { + userPoolGroupList: ['admin'], + }, + })), + saveCLIInputPayload: saveCLIInputPayloadMock, + })), +})); -jest.mock('amplify-cli-core', () => ({ - AmplifySupportedService: { - AUTH: 'Cognito', - COGNITOUSERPOOLGROUPS: 'Cognito-UserPool-Groups', +jest.mock('amplify-cli-core'); + +const stateManagerMock = stateManager as jest.Mocked; +stateManagerMock.getMeta.mockReturnValue({ + api: { + mockResource1: {}, }, - stateManager: { - getMeta: jest - .fn() - .mockReturnValueOnce({ - analytics: { - mockResource1: {}, - }, - api: { - mockResource1: {}, - }, - function: { - mockResource1: {}, - }, - storage: { - mockResource1: {}, - }, - auth: { - mockResource1: { - service: 'Cognito', - }, - }, - }) - .mockReturnValueOnce({ - auth: { - mockResource1: { - service: 'Cognito', - }, - mockResource2: { - service: 'Cognito-UserPool-Groups', - }, - }, - }) - .mockReturnValueOnce({ - analytics: { - mockResource1: {}, - }, - api: { - mockResource1: {}, - }, - function: { - mockResource1: {}, - }, - storage: { - mockResource1: {}, - }, - auth: { - mockResource1: { - service: 'Cognito', - }, - }, - }), + function: { + mockResource1: {}, }, -})); + storage: { + mockResource1: {}, + }, + auth: { + mockResource1: { + service: 'Cognito', + }, + }, +}); -const removeResource_mock = jest - .fn() - .mockResolvedValue({ - service: 'Cognito', - }) - .mockResolvedValueOnce({ - service: 'Cognito', - }) - .mockResolvedValueOnce({ - service: 'Cognito-UserPool-Groups', - }); +const AmplifyErrorMock = AmplifyError as jest.MockedClass; +AmplifyErrorMock.mockImplementation(() => (new Error('test error') as unknown) as any); + +const removeResourceMock = jest.fn().mockResolvedValue({ + service: 'Cognito', +}); const warningString = messages.dependenciesExists; const mockContext = { amplify: { - removeResource: removeResource_mock, + removeResource: removeResourceMock, }, parameters: { first: 'mockFirst', }, }; -const context_stub_typed = mockContext as unknown as $TSContext; +const ContextStubTyped = (mockContext as unknown) as $TSContext; + +beforeEach(() => jest.clearAllMocks()); test(`remove method should detect existing categories metadata and display warning with no userPoolGroup`, async () => { - await remove.run(context_stub_typed); - expect(printer.info).toBeCalledWith(warningString); - expect(context_stub_typed.amplify.removeResource).toBeCalled(); + await remove.run(ContextStubTyped); + expect(printer.warn).toBeCalledWith(warningString); + expect(ContextStubTyped.amplify.removeResource).toBeCalled(); }); test(`remove method should not display warning when there is no dependency with no userPoolGroup`, async () => { - jest.clearAllMocks(); - await remove.run(context_stub_typed); - expect(printer.info).not.toBeCalledWith(warningString); - expect(context_stub_typed.amplify.removeResource).toBeCalled(); + stateManagerMock.getMeta.mockReturnValueOnce({ + auth: { + mockResource1: { + service: 'Cognito', + }, + mockResource2: { + service: 'Cognito-UserPool-Groups', + }, + }, + }); + await remove.run(ContextStubTyped); + expect(printer.warn).not.toBeCalledWith(warningString); + expect(ContextStubTyped.amplify.removeResource).toBeCalled(); }); -test(`remove method should still be called even when warning displayed for existing category resource and remmoves userPool group`, async () => { - await remove.run(context_stub_typed); - expect(saveCLIInputPayload_mock).toBeCalledWith({ cognitoConfig: { userPoolGroupList: [] } }); +test(`remove called when warning displayed for existing category resource and removes userPool group`, async () => { + removeResourceMock.mockResolvedValueOnce({ + service: 'Cognito-UserPool-Groups', + }); + await remove.run(ContextStubTyped); + expect(saveCLIInputPayloadMock).toBeCalledWith({ cognitoConfig: { userPoolGroupList: [] } }); +}); + +test('remove throws error if project has analytics', async () => { + stateManagerMock.getMeta.mockReturnValueOnce({ + analytics: { + mockAnalytics1: {}, + }, + api: { + mockResource1: {}, + }, + function: { + mockResource1: {}, + }, + storage: { + mockResource1: {}, + }, + auth: { + mockResource1: { + service: 'Cognito', + }, + }, + }); + await expect(() => remove.run(ContextStubTyped)).rejects.toThrowErrorMatchingInlineSnapshot(`"test error"`); }); diff --git a/packages/amplify-category-auth/src/commands/auth/remove.ts b/packages/amplify-category-auth/src/commands/auth/remove.ts index c1fdcca34ba..91d2599b9e4 100644 --- a/packages/amplify-category-auth/src/commands/auth/remove.ts +++ b/packages/amplify-category-auth/src/commands/auth/remove.ts @@ -1,23 +1,30 @@ -export const name = 'remove'; -const category = 'auth'; -import { $TSContext, AmplifySupportedService, stateManager } from 'amplify-cli-core'; +import { + $TSContext, $TSMeta, AmplifyCategories, AmplifyError, AmplifySupportedService, stateManager, +} from 'amplify-cli-core'; import { printer } from 'amplify-prompts'; import { messages } from '../../provider-utils/awscloudformation/assets/string-maps'; import { AuthInputState } from '../../provider-utils/awscloudformation/auth-inputs-manager/auth-input-state'; -export const run = async (context: $TSContext) => { +export const name = 'remove'; +const category = 'auth'; + +/** + * Entry point for remove auth + */ +export const run = async (context: $TSContext): Promise => { const { amplify, parameters } = context; const resourceName = parameters.first; const meta = stateManager.getMeta(); - const dependentResources = Object.keys(meta).some(e => { - return ['analytics', 'api', 'storage', 'function'].includes(e) && Object.keys(meta[e]).length > 0; - }); - if (dependentResources) { - printer.info(messages.dependenciesExists); + + throwErrorIfProjectHasAnalytics(meta); + + const hasPossiblyDependentResources = Object.keys(meta) + .some(categoryName => ['api', 'storage', 'function'].includes(categoryName) && Object.keys(meta[categoryName]).length > 0); + if (hasPossiblyDependentResources) { + printer.warn(messages.dependenciesExists); } - const authResourceName = Object.keys(meta.auth).filter(resourceKey => { - return meta.auth[resourceKey].service === AmplifySupportedService.COGNITO; - }); + + const authResourceName = Object.keys(meta.auth).filter(resourceKey => meta.auth[resourceKey].service === AmplifySupportedService.COGNITO); try { const resource = await amplify.removeResource(context, category, resourceName); @@ -35,3 +42,14 @@ export const run = async (context: $TSContext) => { process.exitCode = 1; } }; + +const throwErrorIfProjectHasAnalytics = (meta: $TSMeta): void => { + const analyticsCategoryMeta = meta[AmplifyCategories.ANALYTICS]; + if (!analyticsCategoryMeta) return; + const analyticsResourceNames = Object.keys(analyticsCategoryMeta); + if (analyticsResourceNames.length === 0) return; + throw new AmplifyError('ResourceInUseError', { + message: 'Auth cannot be removed because the analytics category depends on it', + resolution: 'Run `amplify remove analytics` first, then retry removing auth', + }); +}; diff --git a/packages/amplify-cli-core/src/errors/amplify-exception.ts b/packages/amplify-cli-core/src/errors/amplify-exception.ts index 0618831bb55..cc4e98d9fed 100644 --- a/packages/amplify-cli-core/src/errors/amplify-exception.ts +++ b/packages/amplify-cli-core/src/errors/amplify-exception.ts @@ -141,8 +141,9 @@ export type AmplifyErrorType = | 'PushResourcesError' | 'RegionNotAvailableError' | 'RemoveNotificationAppError' - | 'ResourceNotReadyError' | 'ResourceAlreadyExistsError' + | 'ResourceInUseError' + | 'ResourceNotReadyError' | 'StackNotFoundError' | 'StackStateError' | 'UserInputError'; From a9840d58c53bcb6a6b35e0a83d39d2102627874c Mon Sep 17 00:00:00 2001 From: Edward Foyle Date: Tue, 18 Oct 2022 15:18:23 -0700 Subject: [PATCH 2/6] test: add analytics test --- .../amplify-category-analytics/package.json | 21 ++++++++++++++-- .../commands/analytics/remove.test.ts | 25 +++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) create mode 100644 packages/amplify-category-analytics/src/__tests__/commands/analytics/remove.test.ts diff --git a/packages/amplify-category-analytics/package.json b/packages/amplify-category-analytics/package.json index c3d82bb85a3..d19a0b63a95 100644 --- a/packages/amplify-category-analytics/package.json +++ b/packages/amplify-category-analytics/package.json @@ -17,7 +17,8 @@ "scripts": { "build": "tsc", "clean": "rimraf lib tsconfig.tsbuildinfo node_modules", - "watch": "tsc --watch" + "watch": "tsc --watch", + "test": "jest" }, "dependencies": { "@aws-amplify/amplify-environment-parameters": "1.1.3", @@ -26,5 +27,21 @@ "fs-extra": "^8.1.0", "inquirer": "^7.3.3", "uuid": "^8.3.2" + }, + "jest": { + "collectCoverage": true, + "transform": { + "^.+\\.tsx?$": "ts-jest" + }, + "testURL": "http://localhost", + "testRegex": "((\\.|/)(test|spec))\\.(jsx?|tsx?)$", + "moduleFileExtensions": [ + "ts", + "tsx", + "js", + "jsx", + "json", + "node" + ] } -} \ No newline at end of file +} diff --git a/packages/amplify-category-analytics/src/__tests__/commands/analytics/remove.test.ts b/packages/amplify-category-analytics/src/__tests__/commands/analytics/remove.test.ts new file mode 100644 index 00000000000..021400f4599 --- /dev/null +++ b/packages/amplify-category-analytics/src/__tests__/commands/analytics/remove.test.ts @@ -0,0 +1,25 @@ +import { $TSContext } from 'amplify-cli-core'; +import { run as runRemove } from '../../../commands/analytics/remove'; +import { checkResourceInUseByNotifications } from '../../../plugin-client-api-notifications'; + +jest.mock('../../../plugin-client-api-notifications'); + +const checkResourceInUseByNotificationsMock = checkResourceInUseByNotifications as jest.MockedFunction< + typeof checkResourceInUseByNotifications +>; + +checkResourceInUseByNotificationsMock.mockResolvedValue(true); + +describe('remove analytics handler', () => { + it('throws error if notifications exists', async () => { + const stubContext = ({ + amplify: {}, + parameters: { + first: 'testing', + }, + } as unknown) as $TSContext; + await expect(() => runRemove(stubContext)).rejects.toThrowErrorMatchingInlineSnapshot( + `"Analytics resource testing is being used by the notifications category and cannot be removed"`, + ); + }); +}); From a16fe05226ac4c918b882bbde2020d0198e437b2 Mon Sep 17 00:00:00 2001 From: Edward Foyle Date: Tue, 18 Oct 2022 16:21:24 -0700 Subject: [PATCH 3/6] chore: ensure resourceName defined upstream --- .../src/__tests__/commands/analytics/remove.test.ts | 6 +++++- .../src/commands/analytics/remove.ts | 3 --- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/amplify-category-analytics/src/__tests__/commands/analytics/remove.test.ts b/packages/amplify-category-analytics/src/__tests__/commands/analytics/remove.test.ts index 021400f4599..6aaa6cc8388 100644 --- a/packages/amplify-category-analytics/src/__tests__/commands/analytics/remove.test.ts +++ b/packages/amplify-category-analytics/src/__tests__/commands/analytics/remove.test.ts @@ -13,7 +13,11 @@ checkResourceInUseByNotificationsMock.mockResolvedValue(true); describe('remove analytics handler', () => { it('throws error if notifications exists', async () => { const stubContext = ({ - amplify: {}, + amplify: { + removeResource: jest.fn().mockImplementation(async (__context, __category, resourceName, __config, callback) => { + await callback(resourceName); + }), + }, parameters: { first: 'testing', }, diff --git a/packages/amplify-category-analytics/src/commands/analytics/remove.ts b/packages/amplify-category-analytics/src/commands/analytics/remove.ts index 96e818e9df2..0bfb6acba4f 100644 --- a/packages/amplify-category-analytics/src/commands/analytics/remove.ts +++ b/packages/amplify-category-analytics/src/commands/analytics/remove.ts @@ -27,9 +27,6 @@ export const run = async (context: $TSContext): Promise => { } }; - // check if the CLI input analytics name is in use by notification - await blockRemovalOfInUseAnalytics(resourceName); - // remove resource with a resourceName callback that will block removal if selecting an analytics resource that notifications depends on await amplify.removeResource(context, category, resourceName, { headless: false }, blockRemovalOfInUseAnalytics); }; From 80452db041c4d91d88a2aeb0adadfe8f6d961064 Mon Sep 17 00:00:00 2001 From: Edward Foyle Date: Tue, 18 Oct 2022 16:32:17 -0700 Subject: [PATCH 4/6] Update packages/amplify-category-analytics/package.json Co-authored-by: John Hockett --- packages/amplify-category-analytics/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/amplify-category-analytics/package.json b/packages/amplify-category-analytics/package.json index d19a0b63a95..b8da39a9f7b 100644 --- a/packages/amplify-category-analytics/package.json +++ b/packages/amplify-category-analytics/package.json @@ -18,7 +18,7 @@ "build": "tsc", "clean": "rimraf lib tsconfig.tsbuildinfo node_modules", "watch": "tsc --watch", - "test": "jest" + "test": "jest --logHeapUsage" }, "dependencies": { "@aws-amplify/amplify-environment-parameters": "1.1.3", From 7ea8f7686edb0fe066f82f505a63b0fb0a7dc330 Mon Sep 17 00:00:00 2001 From: Edward Foyle Date: Tue, 18 Oct 2022 17:01:16 -0700 Subject: [PATCH 5/6] chore: forgot to save a file before committing --- .../src/plugin-client-api-notifications.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/amplify-category-analytics/src/plugin-client-api-notifications.ts b/packages/amplify-category-analytics/src/plugin-client-api-notifications.ts index 19c0a43173d..8c53599bd79 100644 --- a/packages/amplify-category-analytics/src/plugin-client-api-notifications.ts +++ b/packages/amplify-category-analytics/src/plugin-client-api-notifications.ts @@ -29,9 +29,9 @@ export const invokeNotificationsAPIRecursiveRemoveApp = async ( * @param resourceName Pinpoint resource name * @returns true if Pinpoint resource is in use */ -export const checkResourceInUseByNotifications = async (context: $TSContext, resourceName?: string): Promise => { - if (!resourceName) return false; +export const checkResourceInUseByNotifications = async (context: $TSContext, resourceName: string): Promise => { const notificationsResource = await invokeNotificationsAPIGetResource(context); + if (!notificationsResource?.resourceName) return false; return (notificationsResource?.resourceName === resourceName); }; From f1391505b565a56d14d759d0325bb5b75bd8878f Mon Sep 17 00:00:00 2001 From: Edward Foyle Date: Tue, 18 Oct 2022 17:14:23 -0700 Subject: [PATCH 6/6] chore: rename method --- .../src/commands/analytics/remove.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/amplify-category-analytics/src/commands/analytics/remove.ts b/packages/amplify-category-analytics/src/commands/analytics/remove.ts index 0bfb6acba4f..7c920484b22 100644 --- a/packages/amplify-category-analytics/src/commands/analytics/remove.ts +++ b/packages/amplify-category-analytics/src/commands/analytics/remove.ts @@ -17,7 +17,7 @@ export const run = async (context: $TSContext): Promise => { const { amplify, parameters } = context; const resourceName = parameters.first; - const blockRemovalOfInUseAnalytics = async (selectedAnalyticsResource: string): Promise => { + const throwIfUsedByNotifications = async (selectedAnalyticsResource: string): Promise => { const isResourceInUse = await checkResourceInUseByNotifications(context, selectedAnalyticsResource); if (isResourceInUse) { throw new AmplifyError('ResourceInUseError', { @@ -28,5 +28,5 @@ export const run = async (context: $TSContext): Promise => { }; // remove resource with a resourceName callback that will block removal if selecting an analytics resource that notifications depends on - await amplify.removeResource(context, category, resourceName, { headless: false }, blockRemovalOfInUseAnalytics); + await amplify.removeResource(context, category, resourceName, { headless: false }, throwIfUsedByNotifications); };