Skip to content

refactor(parameters): replace EnvironmentVariablesService class with helper functions in Parameters #4168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+84 −135
Open

refactor(parameters): replace EnvironmentVariablesService class with helper functions in Parameters #4168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions packages/commons/src/envUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,15 @@ const getXRayTraceIdFromEnv = (): string | undefined => {
return xRayTraceData?.Root;
};

/**
* Helper function to determine if a value is considered thruthy.
*
* @param value The value to check for truthiness.
*/
const isValueTrue = (value: string): boolean => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to add this one, we can use the existing getBooleanFromEnv above with extendedParsing set to true

return truthyValues.has(value.toLowerCase());
};

export {
getStringFromEnv,
getNumberFromEnv,
Expand All @@ -310,4 +319,5 @@ export {
getXrayTraceDataFromEnv,
isRequestXRaySampled,
getXRayTraceIdFromEnv,
isValueTrue,
};
28 changes: 28 additions & 0 deletions packages/commons/tests/unit/envUtils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
getXRayTraceIdFromEnv,
isDevMode,
isRequestXRaySampled,
isValueTrue,
} from '../../src/envUtils.js';

describe('Functions: envUtils', () => {
Expand Down Expand Up @@ -330,4 +331,31 @@ describe('Functions: envUtils', () => {
expect(value).toEqual(false);
});
});

describe('Function: isValueTrue', () => {
const valuesToTest: Array<Array<string | boolean>> = [
['1', true],
['y', true],
['yes', true],
['t', true],
['TRUE', true],
['on', true],
['', false],
['false', false],
['fasle', false],
['somethingsilly', false],
['0', false],
];

it.each(valuesToTest)(
'takes string "%s" and returns %s',
(input, output) => {
// Prepare
// Act
const value = isValueTrue(input as string);
// Assess
expect(value).toBe(output);
}
);
});
});
3 changes: 2 additions & 1 deletion packages/parameters/src/appconfig/AppConfigProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { getServiceName } from '@aws-lambda-powertools/commons/utils/env';
import type { StartConfigurationSessionCommandInput } from '@aws-sdk/client-appconfigdata';
import {
AppConfigDataClient,
Expand Down Expand Up @@ -206,7 +207,7 @@ class AppConfigProvider extends BaseProvider {
});

const { application, environment } = options;
this.application = application ?? this.envVarsService.getServiceName();
this.application = application ?? getServiceName();
if (!this.application || this.application.trim().length === 0) {
throw new Error(
'Application name is not defined or POWERTOOLS_SERVICE_NAME is not set'
Expand Down
7 changes: 2 additions & 5 deletions packages/parameters/src/base/BaseProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {
isSdkClient,
isString,
} from '@aws-lambda-powertools/commons';
import { EnvironmentVariablesService } from '../config/EnvironmentVariablesService.js';
import { GetParameterError, TransformParameterError } from '../errors.js';
import type {
BaseProviderConstructorOptions,
Expand Down Expand Up @@ -38,7 +37,6 @@ import { transformValue } from './transformValue.js';
* this should be an acceptable tradeoff.
*/
abstract class BaseProvider implements BaseProviderInterface {
public envVarsService: EnvironmentVariablesService;
protected client: unknown;
protected store: Map<string, ExpirableValue>;

Expand All @@ -48,7 +46,6 @@ abstract class BaseProvider implements BaseProviderInterface {
awsSdkV3ClientPrototype,
}: BaseProviderConstructorOptions) {
this.store = new Map();
this.envVarsService = new EnvironmentVariablesService();
if (awsSdkV3Client) {
if (!isSdkClient(awsSdkV3Client) && awsSdkV3ClientPrototype) {
console.warn(
Expand Down Expand Up @@ -96,7 +93,7 @@ abstract class BaseProvider implements BaseProviderInterface {
name: string,
options?: GetOptionsInterface
): Promise<unknown | undefined> {
const configs = new GetOptions(this.envVarsService, options);
const configs = new GetOptions(options);
const key = [name, configs.transform].toString();

if (!configs.forceFetch && !this.hasKeyExpiredInCache(key)) {
Expand Down Expand Up @@ -135,7 +132,7 @@ abstract class BaseProvider implements BaseProviderInterface {
path: string,
options?: GetMultipleOptionsInterface
): Promise<unknown> {
const configs = new GetMultipleOptions(this.envVarsService, options);
const configs = new GetMultipleOptions(options);
const key = [path, configs.transform].toString();

if (!configs.forceFetch && !this.hasKeyExpiredInCache(key)) {
Expand Down
8 changes: 2 additions & 6 deletions packages/parameters/src/base/GetMultipleOptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import type { EnvironmentVariablesService } from '../config/EnvironmentVariablesService.js';
import type { GetMultipleOptionsInterface } from '../types/BaseProvider.js';
import { GetOptions } from './GetOptions.js';

Expand All @@ -13,11 +12,8 @@ class GetMultipleOptions
{
public throwOnTransformError = false;

public constructor(
envVarsService: EnvironmentVariablesService,
options: GetMultipleOptionsInterface = {}
) {
super(envVarsService, options);
public constructor(options: GetMultipleOptionsInterface = {}) {
super(options);

if (options.throwOnTransformError !== undefined) {
this.throwOnTransformError = options.throwOnTransformError;
Expand Down
10 changes: 3 additions & 7 deletions packages/parameters/src/base/GetOptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import type { EnvironmentVariablesService } from '../config/EnvironmentVariablesService.js';
import { DEFAULT_MAX_AGE_SECS } from '../constants.js';
import type {
GetOptionsInterface,
TransformOptions,
} from '../types/BaseProvider.js';
import { getParametersMaxAge } from '../utils/env.js';

/**
* Options for the `get` method.
Expand All @@ -16,15 +16,11 @@ class GetOptions implements GetOptionsInterface {
public sdkOptions?: unknown;
public transform?: TransformOptions;

public constructor(
envVarsService: EnvironmentVariablesService,
options: GetOptionsInterface = {}
) {
public constructor(options: GetOptionsInterface = {}) {
Object.assign(this, options);

if (options.maxAge === undefined) {
this.maxAge =
envVarsService.getParametersMaxAge() ?? DEFAULT_MAX_AGE_SECS;
this.maxAge = getParametersMaxAge() ?? DEFAULT_MAX_AGE_SECS;
}
}
}
Expand Down
43 changes: 0 additions & 43 deletions packages/parameters/src/config/EnvironmentVariablesService.ts
View file
Open in desktop

This file was deleted.

8 changes: 4 additions & 4 deletions packages/parameters/src/ssm/SSMProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { JSONValue } from '@aws-lambda-powertools/commons/types';
import { isValueTrue } from '@aws-lambda-powertools/commons/utils/env';
import type {
GetParameterCommandInput,
GetParametersByPathCommandInput,
Expand Down Expand Up @@ -32,6 +33,7 @@ import type {
SSMSetOptions,
SSMSplitBatchAndDecryptParametersOutputType,
} from '../types/SSMProvider.js';
import { getSSMDecrypt } from '../utils/env.js';

/**
* ## Intro
Expand Down Expand Up @@ -843,10 +845,8 @@ class SSMProvider extends BaseProvider {
if (options?.decrypt !== undefined) return options.decrypt;
if (sdkOptions?.WithDecryption !== undefined)
return sdkOptions.WithDecryption;
if (this.envVarsService.getSSMDecrypt() !== '') {
return this.envVarsService.isValueTrue(
this.envVarsService.getSSMDecrypt()
);
if (getSSMDecrypt() !== '') {
return isValueTrue(getSSMDecrypt());
}

return undefined;
Expand Down
33 changes: 33 additions & 0 deletions packages/parameters/src/utils/env.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import {
getNumberFromEnv,
getStringFromEnv,
} from '@aws-lambda-powertools/commons/utils/env';

/**
* It returns the value of the POWERTOOLS_PARAMETERS_MAX_AGE environment variable.
*
* @returns {number|undefined}
*/
const getParametersMaxAge = (): number | undefined => {
try {
return getNumberFromEnv({
key: 'POWERTOOLS_PARAMETERS_MAX_AGE',
});
} catch (error) {
return undefined;
}
Comment on lines +16 to +18

Check notice

Code scanning / SonarCloud

Exceptions should not be ignored Low

Handle this exception or don't catch it at all. See more on SonarQube Cloud
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is correct, we probably just want this to fail if the environment variable is not read properly.

Copy link
Author

@JonkaSusaki JonkaSusaki Jul 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we need to catch it, since the getNumberFromEnv throws an error at packages/commons/src/envUtils.ts at line 110.

When removing the try/catch block from packages/parameters/src/utils/env.ts, the AppConfigProvider class can't instantiate the packages/parameters/src/base/GetOptions.ts in case the env does not exists.

Instead of setting the maxAge to DEFAULT_MAX_AGE_SECS, everything just breaks because of that thrown error.

What do you think? Perhaps I could add a console.warn with the error in the catch block to avoid this sonarqube error.

Copy link
Contributor

Choose a reason for hiding this comment

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

ts in case the env does not exists.

Ah yes, that's a good point. Another possibility is to return the default value as defined in the constants file:

import { DEFAULT_MAX_AGE_SECS } from '../constants.js';

// ...

  } catch (error) {
    return DEFAULT_MAX_AGE_SECS;
  }

I'd like to hear @dreamorosi thoughts first though.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, I think it'd be sufficient to do this:

return getNumberFromEnv({
  key: 'POWERTOOLS_PARAMETERS_MAX_AGE',
  // will return `undefined` and not throw if the env var is not set or empty
  defaultValue: undefined,
})

It's ok to return undefined here because in the place where we're calling it (here) we're already doing this:

this.maxAge = getParametersMaxAge() ?? DEFAULT_MAX_AGE_SECS;

Since this getParametersMaxAge is called only in this place, I'd even consider/suggest to call the getNumberFromEnv there directly, so we can save a file and reduce cognitive load.

};

/**
* It returns the value of the POWERTOOLS_PARAMETERS_SSM_DECRYPT environment variable.
*
* @returns {string}
*/
const getSSMDecrypt = (): string => {
return getStringFromEnv({
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest replacing this getStringFromEnv with a call to getBooleanFromEnv with extendedParsing set to true & defaultValue to false.

This would change the return value to a boolean and we could simplify further the call in the SSMProvider class, since this is used in a if/else statement.

Also like discussed in the other comment above, I'd consider/suggest moving the getBooleanFromEnv directly in the SSMProvider so we can remove this file entirely.

key: 'POWERTOOLS_PARAMETERS_SSM_DECRYPT',
defaultValue: '',
});
};

export { getParametersMaxAge, getSSMDecrypt };
69 changes: 0 additions & 69 deletions packages/parameters/tests/unit/EnvironmentVariablesService.test.ts
View file
Open in desktop

This file was deleted.

Loading