From f851da40dfccaa34c63c19f57d54c362b2532f85 Mon Sep 17 00:00:00 2001 From: Andrea Amorosi Date: Fri, 20 Oct 2023 11:26:18 +0200 Subject: [PATCH 1/4] chore(parameters): refactor provider constructor to init client as needed --- .../src/appconfig/AppConfigProvider.ts | 19 +++++++------ .../src/dynamodb/DynamoDBProvider.ts | 28 +++++++++++++------ .../parameters/src/secrets/SecretsProvider.ts | 13 +++++---- packages/parameters/src/ssm/SSMProvider.ts | 13 +++++---- 4 files changed, 46 insertions(+), 27 deletions(-) diff --git a/packages/parameters/src/appconfig/AppConfigProvider.ts b/packages/parameters/src/appconfig/AppConfigProvider.ts index 4d7002a243..09906c939f 100644 --- a/packages/parameters/src/appconfig/AppConfigProvider.ts +++ b/packages/parameters/src/appconfig/AppConfigProvider.ts @@ -198,26 +198,29 @@ class AppConfigProvider extends BaseProvider { */ public constructor(options: AppConfigProviderOptions) { super(); - this.client = new AppConfigDataClient(options.clientConfig || {}); - if (options?.awsSdkV3Client) { - if (isSdkClient(options.awsSdkV3Client)) { - this.client = options.awsSdkV3Client; - } else { + + const { clientConfig, awsSdkV3Client, application, environment } = options; + if (awsSdkV3Client) { + if (!isSdkClient(awsSdkV3Client)) { console.warn( 'awsSdkV3Client is not an AWS SDK v3 client, using default client' ); + this.client = new AppConfigDataClient(clientConfig ?? {}); + } else { + this.client = awsSdkV3Client; } + } else { + this.client = new AppConfigDataClient(clientConfig ?? {}); } addUserAgentMiddleware(this.client, 'parameters'); - this.application = - options?.application || this.envVarsService.getServiceName(); + this.application = application || this.envVarsService.getServiceName(); if (!this.application || this.application.trim().length === 0) { throw new Error( 'Application name is not defined or POWERTOOLS_SERVICE_NAME is not set' ); } - this.environment = options.environment; + this.environment = environment; } /** diff --git a/packages/parameters/src/dynamodb/DynamoDBProvider.ts b/packages/parameters/src/dynamodb/DynamoDBProvider.ts index b319a118d4..b86b845387 100644 --- a/packages/parameters/src/dynamodb/DynamoDBProvider.ts +++ b/packages/parameters/src/dynamodb/DynamoDBProvider.ts @@ -253,22 +253,32 @@ class DynamoDBProvider extends BaseProvider { public constructor(config: DynamoDBProviderOptions) { super(); - this.client = new DynamoDBClient(config?.clientConfig || {}); - if (config?.awsSdkV3Client) { - if (isSdkClient(config.awsSdkV3Client)) { - this.client = config.awsSdkV3Client; - } else { + const { + clientConfig, + awsSdkV3Client, + tableName, + keyAttr, + sortAttr, + valueAttr, + } = config; + if (awsSdkV3Client) { + if (!isSdkClient(awsSdkV3Client)) { console.warn( 'awsSdkV3Client is not an AWS SDK v3 client, using default client' ); + this.client = new DynamoDBClient(clientConfig ?? {}); + } else { + this.client = awsSdkV3Client; } + } else { + this.client = new DynamoDBClient(clientConfig ?? {}); } addUserAgentMiddleware(this.client, 'parameters'); - this.tableName = config.tableName; - if (config.keyAttr) this.keyAttr = config.keyAttr; - if (config.sortAttr) this.sortAttr = config.sortAttr; - if (config.valueAttr) this.valueAttr = config.valueAttr; + this.tableName = tableName; + if (keyAttr) this.keyAttr = keyAttr; + if (sortAttr) this.sortAttr = sortAttr; + if (valueAttr) this.valueAttr = valueAttr; } /** diff --git a/packages/parameters/src/secrets/SecretsProvider.ts b/packages/parameters/src/secrets/SecretsProvider.ts index c4cf7899a0..5592ffe951 100644 --- a/packages/parameters/src/secrets/SecretsProvider.ts +++ b/packages/parameters/src/secrets/SecretsProvider.ts @@ -159,15 +159,18 @@ class SecretsProvider extends BaseProvider { public constructor(config?: SecretsProviderOptions) { super(); - this.client = new SecretsManagerClient(config?.clientConfig || {}); - if (config?.awsSdkV3Client) { - if (isSdkClient(config.awsSdkV3Client)) { - this.client = config.awsSdkV3Client; - } else { + const { clientConfig, awsSdkV3Client } = config ?? {}; + if (awsSdkV3Client) { + if (!isSdkClient(awsSdkV3Client)) { console.warn( 'awsSdkV3Client is not an AWS SDK v3 client, using default client' ); + this.client = new SecretsManagerClient(clientConfig ?? {}); + } else { + this.client = awsSdkV3Client; } + } else { + this.client = new SecretsManagerClient(clientConfig ?? {}); } addUserAgentMiddleware(this.client, 'parameters'); } diff --git a/packages/parameters/src/ssm/SSMProvider.ts b/packages/parameters/src/ssm/SSMProvider.ts index c843437333..8739617472 100644 --- a/packages/parameters/src/ssm/SSMProvider.ts +++ b/packages/parameters/src/ssm/SSMProvider.ts @@ -278,15 +278,18 @@ class SSMProvider extends BaseProvider { public constructor(config?: SSMProviderOptions) { super(); - this.client = new SSMClient(config?.clientConfig || {}); - if (config?.awsSdkV3Client) { - if (isSdkClient(config.awsSdkV3Client)) { - this.client = config.awsSdkV3Client; - } else { + const { clientConfig, awsSdkV3Client } = config ?? {}; + if (awsSdkV3Client) { + if (!isSdkClient(awsSdkV3Client)) { console.warn( 'awsSdkV3Client is not an AWS SDK v3 client, using default client' ); + this.client = new SSMClient(clientConfig ?? {}); + } else { + this.client = awsSdkV3Client; } + } else { + this.client = new SSMClient(clientConfig ?? {}); } addUserAgentMiddleware(this.client, 'parameters'); } From 93e48e5e0c365e907e1ff309aac77e81fc244512 Mon Sep 17 00:00:00 2001 From: Andrea Amorosi Date: Fri, 20 Oct 2023 12:27:44 +0200 Subject: [PATCH 2/4] chore(parameters): moved client instrumentation up in baseprovider --- .../src/appconfig/AppConfigProvider.ts | 32 +++++++---------- packages/parameters/src/base/BaseProvider.ts | 26 +++++++++++++- .../src/dynamodb/DynamoDBProvider.ts | 35 +++++-------------- .../parameters/src/secrets/SecretsProvider.ts | 28 ++++----------- packages/parameters/src/ssm/SSMProvider.ts | 26 +++----------- .../tests/unit/BaseProvider.test.ts | 16 +++++++++ 6 files changed, 74 insertions(+), 89 deletions(-) diff --git a/packages/parameters/src/appconfig/AppConfigProvider.ts b/packages/parameters/src/appconfig/AppConfigProvider.ts index 09906c939f..d092c57627 100644 --- a/packages/parameters/src/appconfig/AppConfigProvider.ts +++ b/packages/parameters/src/appconfig/AppConfigProvider.ts @@ -10,10 +10,10 @@ import type { AppConfigGetOptions, AppConfigGetOutput, } from '../types/AppConfigProvider'; -import { +/* import { addUserAgentMiddleware, isSdkClient, -} from '@aws-lambda-powertools/commons'; +} from '@aws-lambda-powertools/commons'; */ /** * ## Intro @@ -185,7 +185,7 @@ import { * For more usage examples, see [our documentation](https://docs.powertools.aws.dev/lambda/typescript/latest/utilities/parameters/). */ class AppConfigProvider extends BaseProvider { - public client: AppConfigDataClient; + public client!: AppConfigDataClient; protected configurationTokenStore = new Map(); protected valueStore = new Map(); private application?: string; @@ -197,24 +197,16 @@ class AppConfigProvider extends BaseProvider { * @param {AppConfigProviderOptions} options - The configuration object. */ public constructor(options: AppConfigProviderOptions) { - super(); - - const { clientConfig, awsSdkV3Client, application, environment } = options; - if (awsSdkV3Client) { - if (!isSdkClient(awsSdkV3Client)) { - console.warn( - 'awsSdkV3Client is not an AWS SDK v3 client, using default client' - ); - this.client = new AppConfigDataClient(clientConfig ?? {}); - } else { - this.client = awsSdkV3Client; - } - } else { - this.client = new AppConfigDataClient(clientConfig ?? {}); - } - addUserAgentMiddleware(this.client, 'parameters'); + super({ + proto: AppConfigDataClient as new ( + config?: unknown + ) => AppConfigDataClient, + clientConfig: options.clientConfig, + awsSdkV3Client: options.awsSdkV3Client, + }); - this.application = application || this.envVarsService.getServiceName(); + const { application, environment } = options; + this.application = application ?? this.envVarsService.getServiceName(); if (!this.application || this.application.trim().length === 0) { throw new Error( 'Application name is not defined or POWERTOOLS_SERVICE_NAME is not set' diff --git a/packages/parameters/src/base/BaseProvider.ts b/packages/parameters/src/base/BaseProvider.ts index a7ada6d5a6..494c44ab03 100644 --- a/packages/parameters/src/base/BaseProvider.ts +++ b/packages/parameters/src/base/BaseProvider.ts @@ -1,6 +1,8 @@ import { + addUserAgentMiddleware, isNullOrUndefined, isRecord, + isSdkClient, isString, } from '@aws-lambda-powertools/commons'; import { GetOptions } from './GetOptions'; @@ -36,11 +38,33 @@ import type { */ abstract class BaseProvider implements BaseProviderInterface { public envVarsService: EnvironmentVariablesService; + protected client: unknown; protected store: Map; - public constructor() { + public constructor({ + awsSdkV3Client, + clientConfig, + proto, + }: { + awsSdkV3Client?: unknown; + clientConfig?: unknown; + proto: new (config?: unknown) => unknown; + }) { this.store = new Map(); this.envVarsService = new EnvironmentVariablesService(); + if (awsSdkV3Client) { + if (!isSdkClient(awsSdkV3Client)) { + console.warn( + 'awsSdkV3Client is not an AWS SDK v3 client, using default client' + ); + this.client = new proto(clientConfig ?? {}); + } else { + this.client = awsSdkV3Client; + } + } else { + this.client = new proto(clientConfig ?? {}); + } + addUserAgentMiddleware(this.client, 'parameters'); } /** diff --git a/packages/parameters/src/dynamodb/DynamoDBProvider.ts b/packages/parameters/src/dynamodb/DynamoDBProvider.ts index b86b845387..cad56e7d6d 100644 --- a/packages/parameters/src/dynamodb/DynamoDBProvider.ts +++ b/packages/parameters/src/dynamodb/DynamoDBProvider.ts @@ -18,10 +18,10 @@ import type { } from '@aws-sdk/client-dynamodb'; import type { PaginationConfiguration } from '@aws-sdk/types'; import type { JSONValue } from '@aws-lambda-powertools/commons'; -import { +/* import { addUserAgentMiddleware, isSdkClient, -} from '@aws-lambda-powertools/commons'; +} from '@aws-lambda-powertools/commons'; */ /** * ## Intro @@ -239,7 +239,7 @@ import { * For more usage examples, see [our documentation](https://docs.powertools.aws.dev/lambda/typescript/latest/utilities/parameters/). */ class DynamoDBProvider extends BaseProvider { - public client: DynamoDBClient; + public client!: DynamoDBClient; protected keyAttr = 'id'; protected sortAttr = 'sk'; protected tableName: string; @@ -251,30 +251,13 @@ class DynamoDBProvider extends BaseProvider { * @param {DynamoDBProviderOptions} config - The configuration object. */ public constructor(config: DynamoDBProviderOptions) { - super(); - - const { - clientConfig, - awsSdkV3Client, - tableName, - keyAttr, - sortAttr, - valueAttr, - } = config; - if (awsSdkV3Client) { - if (!isSdkClient(awsSdkV3Client)) { - console.warn( - 'awsSdkV3Client is not an AWS SDK v3 client, using default client' - ); - this.client = new DynamoDBClient(clientConfig ?? {}); - } else { - this.client = awsSdkV3Client; - } - } else { - this.client = new DynamoDBClient(clientConfig ?? {}); - } - addUserAgentMiddleware(this.client, 'parameters'); + super({ + awsSdkV3Client: config.awsSdkV3Client, + clientConfig: config.clientConfig, + proto: DynamoDBClient as new (config?: unknown) => DynamoDBClient, + }); + const { tableName, keyAttr, sortAttr, valueAttr } = config; this.tableName = tableName; if (keyAttr) this.keyAttr = keyAttr; if (sortAttr) this.sortAttr = sortAttr; diff --git a/packages/parameters/src/secrets/SecretsProvider.ts b/packages/parameters/src/secrets/SecretsProvider.ts index 5592ffe951..6cba452a62 100644 --- a/packages/parameters/src/secrets/SecretsProvider.ts +++ b/packages/parameters/src/secrets/SecretsProvider.ts @@ -9,10 +9,6 @@ import type { SecretsGetOptions, SecretsGetOutput, } from '../types/SecretsProvider'; -import { - addUserAgentMiddleware, - isSdkClient, -} from '@aws-lambda-powertools/commons'; /** * ## Intro @@ -149,7 +145,7 @@ import { * @see https://docs.powertools.aws.dev/lambda/typescript/latest/utilities/parameters/ */ class SecretsProvider extends BaseProvider { - public client: SecretsManagerClient; + public client!: SecretsManagerClient; /** * It initializes the SecretsProvider class. @@ -157,22 +153,12 @@ class SecretsProvider extends BaseProvider { * @param {SecretsProviderOptions} config - The configuration object. */ public constructor(config?: SecretsProviderOptions) { - super(); - - const { clientConfig, awsSdkV3Client } = config ?? {}; - if (awsSdkV3Client) { - if (!isSdkClient(awsSdkV3Client)) { - console.warn( - 'awsSdkV3Client is not an AWS SDK v3 client, using default client' - ); - this.client = new SecretsManagerClient(clientConfig ?? {}); - } else { - this.client = awsSdkV3Client; - } - } else { - this.client = new SecretsManagerClient(clientConfig ?? {}); - } - addUserAgentMiddleware(this.client, 'parameters'); + super({ + proto: SecretsManagerClient as new ( + config?: unknown + ) => SecretsManagerClient, + ...(config ?? {}), + }); } /** diff --git a/packages/parameters/src/ssm/SSMProvider.ts b/packages/parameters/src/ssm/SSMProvider.ts index 8739617472..4e371c0c62 100644 --- a/packages/parameters/src/ssm/SSMProvider.ts +++ b/packages/parameters/src/ssm/SSMProvider.ts @@ -27,10 +27,6 @@ import type { SSMGetParametersByNameFromCacheOutputType, } from '../types/SSMProvider'; import type { PaginationConfiguration } from '@aws-sdk/types'; -import { - addUserAgentMiddleware, - isSdkClient, -} from '@aws-lambda-powertools/commons'; /** * ## Intro @@ -266,7 +262,7 @@ import { * For more usage examples, see [our documentation](https://docs.powertools.aws.dev/lambda/typescript/latest/utilities/parameters/). */ class SSMProvider extends BaseProvider { - public client: SSMClient; + public client!: SSMClient; protected errorsKey = '_errors'; protected maxGetParametersItems = 10; @@ -276,22 +272,10 @@ class SSMProvider extends BaseProvider { * @param {SSMProviderOptions} config - The configuration object. */ public constructor(config?: SSMProviderOptions) { - super(); - - const { clientConfig, awsSdkV3Client } = config ?? {}; - if (awsSdkV3Client) { - if (!isSdkClient(awsSdkV3Client)) { - console.warn( - 'awsSdkV3Client is not an AWS SDK v3 client, using default client' - ); - this.client = new SSMClient(clientConfig ?? {}); - } else { - this.client = awsSdkV3Client; - } - } else { - this.client = new SSMClient(clientConfig ?? {}); - } - addUserAgentMiddleware(this.client, 'parameters'); + super({ + proto: SSMClient as new (config?: unknown) => SSMClient, + ...(config ?? {}), + }); } /** diff --git a/packages/parameters/tests/unit/BaseProvider.test.ts b/packages/parameters/tests/unit/BaseProvider.test.ts index 773c31b23d..845dea18c2 100644 --- a/packages/parameters/tests/unit/BaseProvider.test.ts +++ b/packages/parameters/tests/unit/BaseProvider.test.ts @@ -9,6 +9,10 @@ import { GetParameterError, TransformParameterError } from '../../src/errors'; import { toBase64 } from '@aws-sdk/util-base64-node'; const encoder = new TextEncoder(); +jest.mock('@aws-lambda-powertools/commons', () => ({ + ...jest.requireActual('@aws-lambda-powertools/commons'), + addUserAgentMiddleware: jest.fn(), +})); describe('Class: BaseProvider', () => { afterEach(() => { @@ -16,6 +20,12 @@ describe('Class: BaseProvider', () => { }); class TestProvider extends BaseProvider { + public constructor() { + super({ + proto: class {}, + }); + } + public _add(key: string, value: ExpirableValue): void { this.store.set(key, value); } @@ -582,6 +592,12 @@ describe('Class: BaseProvider', () => { describe('Function: clearCaches', () => { class TestProvider extends BaseProvider { + public constructor() { + super({ + proto: class {}, + }); + } + public _get(_name: string): Promise { throw Error('Not implemented.'); } From 8026e19384c26d669659075f1dbdbb702ff7d4a0 Mon Sep 17 00:00:00 2001 From: Andrea Amorosi Date: Fri, 20 Oct 2023 12:37:58 +0200 Subject: [PATCH 3/4] chore(parameters): fix code smells --- packages/parameters/src/appconfig/AppConfigProvider.ts | 4 ---- packages/parameters/src/dynamodb/DynamoDBProvider.ts | 4 ---- packages/parameters/tests/unit/BaseProvider.test.ts | 8 +++++++- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/parameters/src/appconfig/AppConfigProvider.ts b/packages/parameters/src/appconfig/AppConfigProvider.ts index d092c57627..b5260ee229 100644 --- a/packages/parameters/src/appconfig/AppConfigProvider.ts +++ b/packages/parameters/src/appconfig/AppConfigProvider.ts @@ -10,10 +10,6 @@ import type { AppConfigGetOptions, AppConfigGetOutput, } from '../types/AppConfigProvider'; -/* import { - addUserAgentMiddleware, - isSdkClient, -} from '@aws-lambda-powertools/commons'; */ /** * ## Intro diff --git a/packages/parameters/src/dynamodb/DynamoDBProvider.ts b/packages/parameters/src/dynamodb/DynamoDBProvider.ts index cad56e7d6d..1263229cbe 100644 --- a/packages/parameters/src/dynamodb/DynamoDBProvider.ts +++ b/packages/parameters/src/dynamodb/DynamoDBProvider.ts @@ -18,10 +18,6 @@ import type { } from '@aws-sdk/client-dynamodb'; import type { PaginationConfiguration } from '@aws-sdk/types'; import type { JSONValue } from '@aws-lambda-powertools/commons'; -/* import { - addUserAgentMiddleware, - isSdkClient, -} from '@aws-lambda-powertools/commons'; */ /** * ## Intro diff --git a/packages/parameters/tests/unit/BaseProvider.test.ts b/packages/parameters/tests/unit/BaseProvider.test.ts index 845dea18c2..0bbb04e32a 100644 --- a/packages/parameters/tests/unit/BaseProvider.test.ts +++ b/packages/parameters/tests/unit/BaseProvider.test.ts @@ -22,7 +22,13 @@ describe('Class: BaseProvider', () => { class TestProvider extends BaseProvider { public constructor() { super({ - proto: class {}, + proto: class { + #name = 'TestProvider'; + + public hello(): string { + return this.#name; + } + }, }); } From 2f23aee85da5c8a449cf90909d81b4178895864e Mon Sep 17 00:00:00 2001 From: Andrea Amorosi Date: Fri, 20 Oct 2023 12:41:34 +0200 Subject: [PATCH 4/4] chore(parameters): fix code smells --- .../tests/unit/BaseProvider.test.ts | 86 ++++++++----------- 1 file changed, 34 insertions(+), 52 deletions(-) diff --git a/packages/parameters/tests/unit/BaseProvider.test.ts b/packages/parameters/tests/unit/BaseProvider.test.ts index 0bbb04e32a..355ae5ca27 100644 --- a/packages/parameters/tests/unit/BaseProvider.test.ts +++ b/packages/parameters/tests/unit/BaseProvider.test.ts @@ -14,46 +14,46 @@ jest.mock('@aws-lambda-powertools/commons', () => ({ addUserAgentMiddleware: jest.fn(), })); -describe('Class: BaseProvider', () => { - afterEach(() => { - jest.clearAllMocks(); - }); - - class TestProvider extends BaseProvider { - public constructor() { - super({ - proto: class { - #name = 'TestProvider'; - - public hello(): string { - return this.#name; - } - }, - }); - } +class TestProvider extends BaseProvider { + public constructor() { + super({ + proto: class { + #name = 'TestProvider'; + + public hello(): string { + return this.#name; + } + }, + }); + } - public _add(key: string, value: ExpirableValue): void { - this.store.set(key, value); - } + public _add(key: string, value: ExpirableValue): void { + this.store.set(key, value); + } - public _get(_name: string): Promise { - throw Error('Not implemented.'); - } + public _get(_name: string): Promise { + throw Error('Not implemented.'); + } - public _getKeyTest(key: string): ExpirableValue | undefined { - return this.store.get(key); - } + public _getKeyTest(key: string): ExpirableValue | undefined { + return this.store.get(key); + } - public _getMultiple( - _path: string - ): Promise> { - throw Error('Not implemented.'); - } + public _getMultiple( + _path: string + ): Promise> { + throw Error('Not implemented.'); + } - public _getStoreSize(): number { - return this.store.size; - } + public _getStoreSize(): number { + return this.store.size; } +} + +describe('Class: BaseProvider', () => { + afterEach(() => { + jest.clearAllMocks(); + }); describe('Method: addToCache', () => { test('when called with a value and maxAge equal to 0, it skips the cache entirely', () => { @@ -597,24 +597,6 @@ describe('Class: BaseProvider', () => { }); describe('Function: clearCaches', () => { - class TestProvider extends BaseProvider { - public constructor() { - super({ - proto: class {}, - }); - } - - public _get(_name: string): Promise { - throw Error('Not implemented.'); - } - - public _getMultiple( - _path: string - ): Promise> { - throw Error('Not implemented.'); - } - } - test('when called, it clears all the caches', () => { // Prepare const provider1 = new TestProvider();