fix: Ensure client logger is always wrapped in a safe logger. (#599)

Author: kinyoklion

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,

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,

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);
+});

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 () => {

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;

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),