Multiple bug fixes

- Handle numeric custom signal values
- Handle empty custom signal map
- Log error instead of throwing when exceeding limits

Author: ashish-kothari

packages/remote-config/src/api.ts

@@ -28,7 +28,7 @@ import {
   RC_CUSTOM_SIGNAL_KEY_MAX_LENGTH,
   RC_CUSTOM_SIGNAL_VALUE_MAX_LENGTH
 } from './constants';
-import { ERROR_FACTORY, ErrorCode, hasErrorCode } from './errors';
+import { ErrorCode, hasErrorCode } from './errors';
 import { RemoteConfig as RemoteConfigImpl } from './remote_config';
 import { Value as ValueImpl } from './value';
 import { LogLevel as FirebaseLogLevel } from '@firebase/logger';
@@ -270,8 +270,8 @@ function getAllKeys(obj1: {} = {}, obj2: {} = {}): string[] {
  *
  * @param remoteConfig - The {@link RemoteConfig} instance.
  * @param customSignals - Map (key, value) of the custom signals to be set for the app instance. If
- * a key already exists, the value is overwritten. Setting the value of a custom signal null unsets
- * the signal. The signals will be persisted locally on the client.
+ * a key already exists, the value is overwritten. Setting the value of a custom signal to null
+ * unsets the signal. The signals will be persisted locally on the client.
  *
  * @public
  */
@@ -280,25 +280,35 @@ export async function setCustomSignals(
   customSignals: CustomSignals
 ): Promise<void> {
   const rc = getModularInstance(remoteConfig) as RemoteConfigImpl;
+  if (Object.keys(customSignals).length === 0) {
+    return;
+  }
+
   // eslint-disable-next-line guard-for-in
   for (const key in customSignals) {
     if (key.length > RC_CUSTOM_SIGNAL_KEY_MAX_LENGTH) {
-      throw ERROR_FACTORY.create(ErrorCode.CUSTOM_SIGNAL_KEY_LENGTH, {
-        key,
-        maxLength: RC_CUSTOM_SIGNAL_KEY_MAX_LENGTH
-      });
+      rc._logger.error(
+        `Custom signal key ${key} is too long, max allowed length is ${RC_CUSTOM_SIGNAL_KEY_MAX_LENGTH}.`
+      );
+      return;
     }
     const value = customSignals[key];
     if (
       typeof value === 'string' &&
       value.length > RC_CUSTOM_SIGNAL_VALUE_MAX_LENGTH
     ) {
-      throw ERROR_FACTORY.create(ErrorCode.CUSTOM_SIGNAL_VALUE_LENGTH, {
-        key,
-        maxLength: RC_CUSTOM_SIGNAL_VALUE_MAX_LENGTH
-      });
+      rc._logger.error(
+        `Value supplied for custom signal ${key} is too long, max allowed length is ${RC_CUSTOM_SIGNAL_VALUE_MAX_LENGTH}.`
+      );
+      return;
     }
   }
 
-  return rc._storageCache.setCustomSignals(customSignals);
+  try {
+    await rc._storageCache.setCustomSignals(customSignals);
+  } catch (error) {
+    rc._logger.error(
+      `Error encountered while setting custom signals: ${error}`
+    );
+  }
 }

packages/remote-config/src/errors.ts

@@ -32,9 +32,7 @@ export const enum ErrorCode {
   FETCH_PARSE = 'fetch-client-parse',
   FETCH_STATUS = 'fetch-status',
   INDEXED_DB_UNAVAILABLE = 'indexed-db-unavailable',
-  CUSTOM_SIGNAL_MAX_ALLOWED_SIGNALS = 'custom-signal-max-allowed-signals',
-  CUSTOM_SIGNAL_KEY_LENGTH = 'custom-signal-key-length',
-  CUSTOM_SIGNAL_VALUE_LENGTH = 'custom-signal-value-length'
+  CUSTOM_SIGNAL_MAX_ALLOWED_SIGNALS = 'custom-signal-max-allowed-signals'
 }
 
 const ERROR_DESCRIPTION_MAP: { readonly [key in ErrorCode]: string } = {
@@ -72,11 +70,7 @@ const ERROR_DESCRIPTION_MAP: { readonly [key in ErrorCode]: string } = {
   [ErrorCode.INDEXED_DB_UNAVAILABLE]:
     'Indexed DB is not supported by current browser',
   [ErrorCode.CUSTOM_SIGNAL_MAX_ALLOWED_SIGNALS]:
-    'Setting more than {$maxSignals} custom signals is not supported.',
-  [ErrorCode.CUSTOM_SIGNAL_KEY_LENGTH]:
-    'Custom signal key {$key} is too long, max allowed length is {$maxLength}.',
-  [ErrorCode.CUSTOM_SIGNAL_VALUE_LENGTH]:
-    'Value supplied for custom signal {$key} is too long, max allowed length is {$maxLength}.'
+    'Setting more than {$maxSignals} custom signals is not supported.'
 };
 
 // Note this is effectively a type system binding a code to params. This approach overlaps with the
@@ -96,8 +90,6 @@ interface ErrorParams {
   [ErrorCode.FETCH_PARSE]: { originalErrorMessage: string };
   [ErrorCode.FETCH_STATUS]: { httpStatus: number };
   [ErrorCode.CUSTOM_SIGNAL_MAX_ALLOWED_SIGNALS]: { maxSignals: number };
-  [ErrorCode.CUSTOM_SIGNAL_KEY_LENGTH]: { key: string; maxLength: number };
-  [ErrorCode.CUSTOM_SIGNAL_VALUE_LENGTH]: { key: string; maxLength: number };
 }
 
 export const ERROR_FACTORY = new ErrorFactory<ErrorCode, ErrorParams>(

packages/remote-config/src/storage/storage.ts

@@ -187,7 +187,7 @@ export class Storage {
     return this.get<CustomSignals>('custom_signals');
   }
 
-  async setCustomSignals(customSignals: CustomSignals): Promise<void> {
+  async setCustomSignals(customSignals: CustomSignals): Promise<CustomSignals> {
     const db = await this.openDbPromise;
     const transaction = db.transaction([APP_NAMESPACE_STORE], 'readwrite');
     const storedSignals = await this.getWithTransaction<CustomSignals>(
@@ -199,24 +199,34 @@ export class Storage {
       ...customSignals
     };
     // Filter out key-value assignments with null values since they are signals being unset
-    const signalsToUpdate = Object.fromEntries(
-      Object.entries(combinedSignals).filter(([_, v]) => v !== null)
+    const updatedSignals = Object.fromEntries(
+      Object.entries(combinedSignals)
+        .filter(([_, v]) => v !== null)
+        .map(([k, v]) => {
+          // Stringify numbers to store a map of string keys and values which can be sent
+          // as-is in a fetch call.
+          if (typeof v === 'number') {
+            return [k, v.toString()];
+          }
+          return [k, v];
+        })
     );
 
     // Throw an error if the number of custom signals to be stored exceeds the limit
     if (
-      Object.keys(signalsToUpdate).length > RC_CUSTOM_SIGNAL_MAX_ALLOWED_SIGNALS
+      Object.keys(updatedSignals).length > RC_CUSTOM_SIGNAL_MAX_ALLOWED_SIGNALS
     ) {
       throw ERROR_FACTORY.create(ErrorCode.CUSTOM_SIGNAL_MAX_ALLOWED_SIGNALS, {
         maxSignals: RC_CUSTOM_SIGNAL_MAX_ALLOWED_SIGNALS
       });
     }
 
-    return this.setWithTransaction<CustomSignals>(
+    await this.setWithTransaction<CustomSignals>(
       'custom_signals',
-      signalsToUpdate,
+      updatedSignals,
       transaction
     );
+    return updatedSignals;
   }
 
   /**

packages/remote-config/src/storage/storage_cache.ts

@@ -111,8 +111,7 @@ export class StorageCache {
     return this.storage.setActiveConfig(activeConfig);
   }
 
-  setCustomSignals(customSignals: CustomSignals): Promise<void> {
-    this.customSignals = customSignals;
-    return this.storage.setCustomSignals(customSignals);
+  async setCustomSignals(customSignals: CustomSignals): Promise<void> {
+    this.customSignals = await this.storage.setCustomSignals(customSignals);
   }
 }

packages/remote-config/test/remote_config.test.ts

@@ -98,6 +98,7 @@ describe('RemoteConfig', () => {
     beforeEach(() => {
       storageCache.setCustomSignals = sinon.stub();
       storage.setCustomSignals = sinon.stub();
+      logger.error = sinon.stub();
     });
 
     it('call storage API to store signals', async () => {
@@ -108,26 +109,30 @@ describe('RemoteConfig', () => {
       });
     });
 
-    it('throws an error when supplied with a custom signal key greater than 250 characters', async () => {
+    it('logs an error when supplied with a custom signal key greater than 250 characters', async () => {
       const longKey = 'a'.repeat(251);
       const customSignals = { [longKey]: 'value' };
 
-      await expect(
-        setCustomSignals(rc, customSignals)
-      ).to.eventually.be.rejectedWith(
-        `Remote Config: Custom signal key ${longKey} is too long, max allowed length is 250.`
-      );
+      await setCustomSignals(rc, customSignals);
+
+      expect(storageCache.setCustomSignals).to.not.have.been.called;
+      expect(logger.error).to.have.been.called;
     });
 
-    it('throws an error when supplied with a custom signal value greater than 500 characters', async () => {
+    it('logs an error when supplied with a custom signal value greater than 500 characters', async () => {
       const longValue = 'a'.repeat(501);
       const customSignals = { 'key': longValue };
 
-      await expect(
-        setCustomSignals(rc, customSignals)
-      ).to.eventually.be.rejectedWith(
-        'Remote Config: Value supplied for custom signal key is too long, max allowed length is 500.'
-      );
+      await setCustomSignals(rc, customSignals);
+
+      expect(storageCache.setCustomSignals).to.not.have.been.called;
+      expect(logger.error).to.have.been.called;
+    });
+
+    it('empty custom signals map does nothing', async () => {
+      await setCustomSignals(rc, {});
+
+      expect(storageCache.setCustomSignals).to.not.have.been.called;
     });
   });
 

packages/remote-config/test/storage/storage.test.ts

@@ -119,13 +119,18 @@ describe('Storage', () => {
   });
 
   it('sets and gets custom signals', async () => {
-    const customSignals = { key: 'value', key1: 'value1' };
+    const customSignals = { key: 'value', key1: 'value1', key2: 1 };
+    const customSignalsInStorage = {
+      key: 'value',
+      key1: 'value1',
+      key2: '1'
+    };
 
     await storage.setCustomSignals(customSignals);
 
     const storedCustomSignals = await storage.getCustomSignals();
 
-    expect(storedCustomSignals).to.deep.eq(customSignals);
+    expect(storedCustomSignals).to.deep.eq(customSignalsInStorage);
   });
 
   it('upserts custom signals when key is present in storage', async () => {

packages/remote-config/test/storage/storage_cache.test.ts

@@ -91,7 +91,9 @@ describe('StorageCache', () => {
     const customSignals = { key: 'value' };
 
     beforeEach(() => {
-      storage.setCustomSignals = sinon.stub().returns(Promise.resolve());
+      storage.setCustomSignals = sinon
+        .stub()
+        .returns(Promise.resolve(customSignals));
     });
 
     it('writes to memory cache', async () => {