From 980e4daaf32864e18f14b1e5e28e308dff0ae94f Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 27 Sep 2024 14:12:06 -0700 Subject: [PATCH] fix: Ensure client logger is always wrapped in a safe logger. (#599) --- .../__tests__/BrowserDataManager.test.ts | 3 -- .../__tests__/MobileDataManager.test.ts | 3 -- .../configuration/Configuration.test.ts | 47 +++++++++++++++++++ .../__tests__/context/addAutoEnv.test.ts | 10 ++-- .../src/configuration/Configuration.ts | 21 ++++++--- .../createDiagnosticsInitConfig.ts | 6 +-- 6 files changed, 69 insertions(+), 21 deletions(-) diff --git a/packages/sdk/browser/__tests__/BrowserDataManager.test.ts b/packages/sdk/browser/__tests__/BrowserDataManager.test.ts index fee37b29b..d5e05ba7c 100644 --- a/packages/sdk/browser/__tests__/BrowserDataManager.test.ts +++ b/packages/sdk/browser/__tests__/BrowserDataManager.test.ts @@ -74,9 +74,6 @@ describe('given a BrowserDataManager with mocked dependencies', () => { }; config = { logger, - baseUri: 'string', - eventsUri: 'string', - streamUri: 'string', maxCachedContexts: 5, capacity: 100, diagnosticRecordingInterval: 1000, diff --git a/packages/sdk/react-native/__tests__/MobileDataManager.test.ts b/packages/sdk/react-native/__tests__/MobileDataManager.test.ts index 1e54ed5b5..a7e044709 100644 --- a/packages/sdk/react-native/__tests__/MobileDataManager.test.ts +++ b/packages/sdk/react-native/__tests__/MobileDataManager.test.ts @@ -63,9 +63,6 @@ describe('given a MobileDataManager with mocked dependencies', () => { }; config = { logger, - baseUri: 'string', - eventsUri: 'string', - streamUri: 'string', maxCachedContexts: 5, capacity: 100, diagnosticRecordingInterval: 1000, diff --git a/packages/shared/sdk-client/__tests__/configuration/Configuration.test.ts b/packages/shared/sdk-client/__tests__/configuration/Configuration.test.ts index f8af0b79c..81a797a7d 100644 --- a/packages/shared/sdk-client/__tests__/configuration/Configuration.test.ts +++ b/packages/shared/sdk-client/__tests__/configuration/Configuration.test.ts @@ -1,4 +1,6 @@ /* eslint-disable no-console */ +import { createSafeLogger } from '@launchdarkly/js-sdk-common'; + import ConfigurationImpl from '../../src/configuration/Configuration'; describe('Configuration', () => { @@ -120,3 +122,48 @@ describe('Configuration', () => { }, ); }); + +it('makes a safe logger', () => { + const badLogger = { + debug: () => { + throw new Error('bad'); + }, + info: () => { + throw new Error('bad'); + }, + warn: () => { + throw new Error('bad'); + }, + error: () => { + throw new Error('bad'); + }, + }; + const config = new ConfigurationImpl({ + logger: badLogger, + }); + + expect(() => config.logger.debug('debug')).not.toThrow(); + expect(() => config.logger.info('info')).not.toThrow(); + expect(() => config.logger.warn('warn')).not.toThrow(); + expect(() => config.logger.error('error')).not.toThrow(); + expect(config.logger).not.toBe(badLogger); +}); + +it('does not wrap already safe loggers', () => { + const logger = createSafeLogger({ + debug: () => { + throw new Error('bad'); + }, + info: () => { + throw new Error('bad'); + }, + warn: () => { + throw new Error('bad'); + }, + error: () => { + throw new Error('bad'); + }, + }); + const config = new ConfigurationImpl({ logger }); + expect(config.logger).toBe(logger); +}); diff --git a/packages/shared/sdk-client/__tests__/context/addAutoEnv.test.ts b/packages/shared/sdk-client/__tests__/context/addAutoEnv.test.ts index e6980d137..b686a3c1b 100644 --- a/packages/shared/sdk-client/__tests__/context/addAutoEnv.test.ts +++ b/packages/shared/sdk-client/__tests__/context/addAutoEnv.test.ts @@ -233,8 +233,8 @@ describe('automatic environment attributes', () => { await addAutoEnv(context, mockPlatform, config); - expect(config.logger.warn).toHaveBeenCalledTimes(1); - expect(config.logger.warn).toHaveBeenCalledWith( + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith( expect.stringMatching(/ld_application.*already exists/), ); }); @@ -251,10 +251,8 @@ describe('automatic environment attributes', () => { await addAutoEnv(context, mockPlatform, config); - expect(config.logger.warn).toHaveBeenCalledTimes(1); - expect(config.logger.warn).toHaveBeenCalledWith( - expect.stringMatching(/ld_device.*already exists/), - ); + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith(expect.stringMatching(/ld_device.*already exists/)); }); test('single context with an attribute called ld_application should have auto env attributes', async () => { diff --git a/packages/shared/sdk-client/src/configuration/Configuration.ts b/packages/shared/sdk-client/src/configuration/Configuration.ts index 1066fdd9e..50ce86f3f 100644 --- a/packages/shared/sdk-client/src/configuration/Configuration.ts +++ b/packages/shared/sdk-client/src/configuration/Configuration.ts @@ -6,6 +6,7 @@ import { LDLogger, NumberWithMinimum, OptionMessages, + SafeLogger, ServiceEndpoints, TypeValidators, } from '@launchdarkly/js-sdk-common'; @@ -21,9 +22,6 @@ export interface LDClientInternalOptions extends internal.LDInternalOptions { export interface Configuration { readonly logger: LDLogger; - readonly baseUri: string; - readonly eventsUri: string; - readonly streamUri: string; readonly maxCachedContexts: number; readonly capacity: number; readonly diagnosticRecordingInterval: number; @@ -61,12 +59,20 @@ const DEFAULT_STREAM: string = 'https://clientstream.launchdarkly.com'; export { DEFAULT_POLLING, DEFAULT_STREAM }; +function ensureSafeLogger(logger?: LDLogger): LDLogger { + if (logger instanceof SafeLogger) { + return logger; + } + // Even if logger is not defined this will produce a valid logger. + return createSafeLogger(logger); +} + export default class ConfigurationImpl implements Configuration { public readonly logger: LDLogger = createSafeLogger(); - public readonly baseUri = DEFAULT_POLLING; - public readonly eventsUri = ServiceEndpoints.DEFAULT_EVENTS; - public readonly streamUri = DEFAULT_STREAM; + private readonly baseUri = DEFAULT_POLLING; + private readonly eventsUri = ServiceEndpoints.DEFAULT_EVENTS; + private readonly streamUri = DEFAULT_STREAM; public readonly maxCachedContexts = 5; @@ -116,6 +122,7 @@ export default class ConfigurationImpl implements Configuration { [index: string]: any; constructor(pristineOptions: LDOptions = {}, internalOptions: LDClientInternalOptions = {}) { + this.logger = ensureSafeLogger(pristineOptions.logger); const errors = this.validateTypesAndNames(pristineOptions); errors.forEach((e: string) => this.logger.warn(e)); @@ -161,6 +168,8 @@ export default class ConfigurationImpl implements Configuration { } else { errors.push(OptionMessages.wrongOptionType(k, validator.getType(), typeof v)); } + } else if (k === 'logger') { + // Logger already assigned. } else { // if an option is explicitly null, coerce to undefined this[k] = v ?? undefined; diff --git a/packages/shared/sdk-client/src/diagnostics/createDiagnosticsInitConfig.ts b/packages/shared/sdk-client/src/diagnostics/createDiagnosticsInitConfig.ts index e3230b05c..da6be4db1 100644 --- a/packages/shared/sdk-client/src/diagnostics/createDiagnosticsInitConfig.ts +++ b/packages/shared/sdk-client/src/diagnostics/createDiagnosticsInitConfig.ts @@ -18,9 +18,9 @@ export type DiagnosticsInitConfig = { bootstrapMode: boolean; }; const createDiagnosticsInitConfig = (config: Configuration): DiagnosticsInitConfig => ({ - customBaseURI: config.baseUri !== DEFAULT_POLLING, - customStreamURI: config.streamUri !== DEFAULT_STREAM, - customEventsURI: config.eventsUri !== ServiceEndpoints.DEFAULT_EVENTS, + customBaseURI: config.serviceEndpoints.polling !== DEFAULT_POLLING, + customStreamURI: config.serviceEndpoints.streaming !== DEFAULT_STREAM, + customEventsURI: config.serviceEndpoints.events !== ServiceEndpoints.DEFAULT_EVENTS, eventsCapacity: config.capacity, eventsFlushIntervalMillis: secondsToMillis(config.flushInterval), reconnectTimeMillis: secondsToMillis(config.streamInitialReconnectDelay),