Merge pull request #10031 from weseek/imprv/add-non-empty-string-type

imprv: Add NonEmptyString type

apps/app/src/server/service/config-manager/config-manager.integ.ts

@@ -107,6 +107,55 @@ describe('ConfigManager', () => {
     });
   });
 
+  describe('updateConfig', () => {
+    beforeEach(async() => {
+      await Config.deleteMany({ key: /app.*/ }).exec();
+      await Config.create({ key: 'app:siteUrl', value: JSON.stringify('initial value') });
+    });
+
+    test('updates a single config', async() => {
+      // arrange
+      await configManager.loadConfigs();
+      const config = await Config.findOne({ key: 'app:siteUrl' }).exec();
+      expect(config?.value).toEqual(JSON.stringify('initial value'));
+
+      // act
+      await configManager.updateConfig('app:siteUrl', 'updated value');
+
+      // assert
+      const updatedConfig = await Config.findOne({ key: 'app:siteUrl' }).exec();
+      expect(updatedConfig?.value).toEqual(JSON.stringify('updated value'));
+    });
+
+    test('removes config when value is undefined and removeIfUndefined is true', async() => {
+      // arrange
+      await configManager.loadConfigs();
+      const config = await Config.findOne({ key: 'app:siteUrl' }).exec();
+      expect(config?.value).toEqual(JSON.stringify('initial value'));
+
+      // act
+      await configManager.updateConfig('app:siteUrl', undefined, { removeIfUndefined: true });
+
+      // assert
+      const updatedConfig = await Config.findOne({ key: 'app:siteUrl' }).exec();
+      expect(updatedConfig).toBeNull(); // should be removed
+    });
+
+    test('does not update config when value is undefined and removeIfUndefined is false', async() => {
+      // arrange
+      await configManager.loadConfigs();
+      const config = await Config.findOne({ key: 'app:siteUrl' }).exec();
+      expect(config?.value).toEqual(JSON.stringify('initial value'));
+
+      // act
+      await configManager.updateConfig('app:siteUrl', undefined);
+
+      // assert
+      const updatedConfig = await Config.findOne({ key: 'app:siteUrl' }).exec();
+      expect(updatedConfig?.value).toEqual(JSON.stringify('initial value')); // should remain unchanged
+    });
+  });
+
   describe('updateConfigs', () => {
     beforeEach(async() => {
       await Config.deleteMany({ key: /app.*/ }).exec();
@@ -133,6 +182,44 @@ describe('ConfigManager', () => {
       expect(updatedConfig1?.value).toEqual(JSON.stringify('new value1'));
       expect(updatedConfig2?.value).toEqual(JSON.stringify('aws'));
     });
+
+    test('removes config when value is undefined and removeIfUndefined is true', async() => {
+      // arrange
+      await configManager.loadConfigs();
+      const config1 = await Config.findOne({ key: 'app:siteUrl' }).exec();
+      expect(config1?.value).toEqual(JSON.stringify('value1'));
+
+      // act
+      await configManager.updateConfigs({
+        'app:siteUrl': undefined,
+        'app:fileUploadType': 'aws',
+      }, { removeIfUndefined: true });
+
+      // assert
+      const updatedConfig1 = await Config.findOne({ key: 'app:siteUrl' }).exec();
+      const updatedConfig2 = await Config.findOne({ key: 'app:fileUploadType' }).exec();
+      expect(updatedConfig1).toBeNull(); // should be removed
+      expect(updatedConfig2?.value).toEqual(JSON.stringify('aws'));
+    });
+
+    test('does not update config when value is undefined and removeIfUndefined is false', async() => {
+      // arrange
+      await configManager.loadConfigs();
+      const config1 = await Config.findOne({ key: 'app:siteUrl' }).exec();
+      expect(config1?.value).toEqual(JSON.stringify('value1'));
+
+      // act
+      await configManager.updateConfigs({
+        'app:siteUrl': undefined,
+        'app:fileUploadType': 'aws',
+      });
+
+      // assert
+      const updatedConfig1 = await Config.findOne({ key: 'app:siteUrl' }).exec();
+      const updatedConfig2 = await Config.findOne({ key: 'app:fileUploadType' }).exec();
+      expect(updatedConfig1?.value).toEqual(JSON.stringify('value1')); // should remain unchanged
+      expect(updatedConfig2?.value).toEqual(JSON.stringify('aws'));
+    });
   });
 
   describe('removeConfigs', () => {

apps/app/src/server/service/config-manager/config-manager.spec.ts

@@ -13,6 +13,7 @@ const mocks = vi.hoisted(() => ({
   ConfigMock: {
     updateOne: vi.fn(),
     bulkWrite: vi.fn(),
+    deleteOne: vi.fn(),
   },
 }));
 vi.mock('../../models/config', () => ({
@@ -40,6 +41,9 @@ describe('ConfigManager test', () => {
     let loadConfigsSpy;
     beforeEach(async() => {
       loadConfigsSpy = vi.spyOn(configManager, 'loadConfigs');
+      // Reset mocks
+      mocks.ConfigMock.updateOne.mockClear();
+      mocks.ConfigMock.deleteOne.mockClear();
     });
 
     test('invoke publishUpdateMessage()', async() => {
@@ -70,6 +74,42 @@ describe('ConfigManager test', () => {
       expect(configManager.publishUpdateMessage).not.toHaveBeenCalled();
     });
 
+    test('remove config when value is undefined and removeIfUndefined is true', async() => {
+      // arrange
+      configManager.publishUpdateMessage = vi.fn();
+      vi.spyOn((configManager as unknown as ConfigManagerToGetLoader).configLoader, 'loadFromDB').mockImplementation(vi.fn());
+
+      // act
+      await configManager.updateConfig('app:siteUrl', undefined, { removeIfUndefined: true });
+
+      // assert
+      expect(mocks.ConfigMock.deleteOne).toHaveBeenCalledTimes(1);
+      expect(mocks.ConfigMock.deleteOne).toHaveBeenCalledWith({ key: 'app:siteUrl' });
+      expect(mocks.ConfigMock.updateOne).not.toHaveBeenCalled();
+      expect(loadConfigsSpy).toHaveBeenCalledTimes(1);
+      expect(configManager.publishUpdateMessage).toHaveBeenCalledTimes(1);
+    });
+
+    test('update config with undefined value when removeIfUndefined is false', async() => {
+      // arrange
+      configManager.publishUpdateMessage = vi.fn();
+      vi.spyOn((configManager as unknown as ConfigManagerToGetLoader).configLoader, 'loadFromDB').mockImplementation(vi.fn());
+
+      // act
+      await configManager.updateConfig('app:siteUrl', undefined);
+
+      // assert
+      expect(mocks.ConfigMock.updateOne).toHaveBeenCalledTimes(1);
+      expect(mocks.ConfigMock.updateOne).toHaveBeenCalledWith(
+        { key: 'app:siteUrl' },
+        { value: JSON.stringify(undefined) },
+        { upsert: true },
+      );
+      expect(mocks.ConfigMock.deleteOne).not.toHaveBeenCalled();
+      expect(loadConfigsSpy).toHaveBeenCalledTimes(1);
+      expect(configManager.publishUpdateMessage).toHaveBeenCalledTimes(1);
+    });
+
   });
 
   describe('updateConfigs()', () => {
@@ -77,18 +117,20 @@ describe('ConfigManager test', () => {
     let loadConfigsSpy;
     beforeEach(async() => {
       loadConfigsSpy = vi.spyOn(configManager, 'loadConfigs');
+      // Reset mocks
+      mocks.ConfigMock.bulkWrite.mockClear();
     });
 
     test('invoke publishUpdateMessage()', async() => {
-      // arrenge
+      // arrange
       configManager.publishUpdateMessage = vi.fn();
       vi.spyOn((configManager as unknown as ConfigManagerToGetLoader).configLoader, 'loadFromDB').mockImplementation(vi.fn());
 
       // act
-      await configManager.updateConfig('app:siteUrl', '');
+      await configManager.updateConfigs({ 'app:siteUrl': 'https://example.com' });
 
       // assert
-      // expect(Config.bulkWrite).toHaveBeenCalledTimes(1);
+      expect(mocks.ConfigMock.bulkWrite).toHaveBeenCalledTimes(1);
       expect(loadConfigsSpy).toHaveBeenCalledTimes(1);
       expect(configManager.publishUpdateMessage).toHaveBeenCalledTimes(1);
     });
@@ -102,10 +144,68 @@ describe('ConfigManager test', () => {
       await configManager.updateConfigs({ 'app:siteUrl': '' }, { skipPubsub: true });
 
       // assert
-      // expect(Config.bulkWrite).toHaveBeenCalledTimes(1);
+      expect(mocks.ConfigMock.bulkWrite).toHaveBeenCalledTimes(1);
       expect(loadConfigsSpy).toHaveBeenCalledTimes(1);
       expect(configManager.publishUpdateMessage).not.toHaveBeenCalled();
     });
+
+    test('remove configs when values are undefined and removeIfUndefined is true', async() => {
+      // arrange
+      configManager.publishUpdateMessage = vi.fn();
+      vi.spyOn((configManager as unknown as ConfigManagerToGetLoader).configLoader, 'loadFromDB').mockImplementation(vi.fn());
+
+      // act
+      await configManager.updateConfigs(
+        { 'app:siteUrl': undefined, 'app:title': 'GROWI' },
+        { removeIfUndefined: true },
+      );
+
+      // assert
+      expect(mocks.ConfigMock.bulkWrite).toHaveBeenCalledTimes(1);
+      const operations = mocks.ConfigMock.bulkWrite.mock.calls[0][0];
+      expect(operations).toHaveLength(2);
+      expect(operations[0]).toEqual({ deleteOne: { filter: { key: 'app:siteUrl' } } });
+      expect(operations[1]).toEqual({
+        updateOne: {
+          filter: { key: 'app:title' },
+          update: { value: JSON.stringify('GROWI') },
+          upsert: true,
+        },
+      });
+      expect(loadConfigsSpy).toHaveBeenCalledTimes(1);
+      expect(configManager.publishUpdateMessage).toHaveBeenCalledTimes(1);
+    });
+
+    test('update configs including undefined values when removeIfUndefined is false', async() => {
+      // arrange
+      configManager.publishUpdateMessage = vi.fn();
+      vi.spyOn((configManager as unknown as ConfigManagerToGetLoader).configLoader, 'loadFromDB').mockImplementation(vi.fn());
+
+      // act
+      await configManager.updateConfigs({ 'app:siteUrl': undefined, 'app:title': 'GROWI' });
+
+      // assert
+      expect(mocks.ConfigMock.bulkWrite).toHaveBeenCalledTimes(1);
+      const operations = mocks.ConfigMock.bulkWrite.mock.calls[0][0];
+      expect(operations).toHaveLength(2); // both operations should be included
+      expect(operations[0]).toEqual({
+        updateOne: {
+          filter: { key: 'app:siteUrl' },
+          update: { value: JSON.stringify(undefined) },
+          upsert: true,
+        },
+      });
+      expect(operations[1]).toEqual({
+        updateOne: {
+          filter: { key: 'app:title' },
+          update: { value: JSON.stringify('GROWI') },
+          upsert: true,
+        },
+      });
+      expect(loadConfigsSpy).toHaveBeenCalledTimes(1);
+      expect(configManager.publishUpdateMessage).toHaveBeenCalledTimes(1);
+    });
+
   });
 
   describe('getManagedEnvVars()', () => {

apps/app/src/server/service/config-manager/config-manager.ts

@@ -111,11 +111,17 @@ export class ConfigManager implements IConfigManagerForApp, S2sMessageHandlable
     // Dynamic import to avoid loading database modules too early
     const { Config } = await import('../../models/config');
 
-    await Config.updateOne(
-      { key },
-      { value: JSON.stringify(value) },
-      { upsert: true },
-    );
+    if (options?.removeIfUndefined && value === undefined) {
+      // remove the config if the value is undefined and removeIfUndefined is true
+      await Config.deleteOne({ key });
+    }
+    else {
+      await Config.updateOne(
+        { key },
+        { value: JSON.stringify(value) },
+        { upsert: true },
+      );
+    }
 
     await this.loadConfigs({ source: 'db' });
 
@@ -128,13 +134,19 @@ export class ConfigManager implements IConfigManagerForApp, S2sMessageHandlable
     // Dynamic import to avoid loading database modules too early
     const { Config } = await import('../../models/config');
 
-    const operations = Object.entries(updates).map(([key, value]) => ({
-      updateOne: {
-        filter: { key },
-        update: { value: JSON.stringify(value) },
-        upsert: true,
-      },
-    }));
+    const operations = Object.entries(updates).map(([key, value]) => {
+      return (options?.removeIfUndefined && value === undefined)
+        // remove the config if the value is undefined
+        ? { deleteOne: { filter: { key } } }
+        // update
+        : {
+          updateOne: {
+            filter: { key },
+            update: { value: JSON.stringify(value) },
+            upsert: true,
+          },
+        };
+    });
 
     await Config.bulkWrite(operations);
     await this.loadConfigs({ source: 'db' });

packages/core/src/interfaces/config-manager.ts

@@ -43,7 +43,10 @@ export type RawConfigData<K extends string, V extends Record<K, any>> = Record<K
   definition?: ConfigDefinition<V[K]>;
 }>;
 
-export type UpdateConfigOptions = { skipPubsub?: boolean };
+export type UpdateConfigOptions = {
+  skipPubsub?: boolean;
+  removeIfUndefined?: boolean;
+};
 
 /**
  * Interface for managing configuration values

packages/core/src/interfaces/index.ts

@@ -1,3 +1,4 @@
+export * from './primitive/string';
 export * from './attachment';
 export * from './color-scheme';
 export * from './color-scheme';

packages/core/src/interfaces/primitive/string.spec.ts

@@ -0,0 +1,164 @@
+import { describe, expect, it } from 'vitest';
+
+import {
+  isNonEmptyString,
+  toNonEmptyString,
+  toNonEmptyStringOrUndefined,
+  isNonBlankString,
+  toNonBlankString,
+  toNonBlankStringOrUndefined,
+} from './string';
+
+describe('isNonEmptyString', () => {
+  /* eslint-disable indent */
+  it.each`
+    input         | expected      | description
+    ${'hello'}    | ${true}       | ${'non-empty string'}
+    ${'world'}    | ${true}       | ${'non-empty string'}
+    ${'a'}        | ${true}       | ${'single character'}
+    ${'1'}        | ${true}       | ${'numeric string'}
+    ${' '}        | ${true}       | ${'space character'}
+    ${'   '}      | ${true}       | ${'multiple spaces'}
+    ${''}         | ${false}      | ${'empty string'}
+    ${null}       | ${false}      | ${'null'}
+    ${undefined}  | ${false}      | ${'undefined'}
+  `('should return $expected for $description: $input', ({ input, expected }) => {
+  /* eslint-enable indent */
+    expect(isNonEmptyString(input)).toBe(expected);
+  });
+});
+
+describe('isNonBlankString', () => {
+  /* eslint-disable indent */
+  it.each`
+    input         | expected      | description
+    ${'hello'}    | ${true}       | ${'non-blank string'}
+    ${'world'}    | ${true}       | ${'non-blank string'}
+    ${'a'}        | ${true}       | ${'single character'}
+    ${'1'}        | ${true}       | ${'numeric string'}
+    ${' '}        | ${false}      | ${'space character'}
+    ${'   '}      | ${false}      | ${'multiple spaces'}
+    ${'\t'}       | ${false}      | ${'tab character'}
+    ${'\n'}       | ${false}      | ${'newline character'}
+    ${''}         | ${false}      | ${'empty string'}
+    ${null}       | ${false}      | ${'null'}
+    ${undefined}  | ${false}      | ${'undefined'}
+  `('should return $expected for $description: $input', ({ input, expected }) => {
+  /* eslint-enable indent */
+    expect(isNonBlankString(input)).toBe(expected);
+  });
+});
+
+describe('toNonEmptyStringOrUndefined', () => {
+  /* eslint-disable indent */
+  it.each`
+    input         | expected      | description
+    ${'hello'}    | ${'hello'}    | ${'non-empty string'}
+    ${'world'}    | ${'world'}    | ${'non-empty string'}
+    ${'a'}        | ${'a'}        | ${'single character'}
+    ${'1'}        | ${'1'}        | ${'numeric string'}
+    ${' '}        | ${' '}        | ${'space character'}
+    ${'   '}      | ${'   '}      | ${'multiple spaces'}
+    ${''}         | ${undefined}  | ${'empty string'}
+    ${null}       | ${undefined}  | ${'null'}
+    ${undefined}  | ${undefined}  | ${'undefined'}
+  `('should return $expected for $description: $input', ({ input, expected }) => {
+  /* eslint-enable indent */
+    expect(toNonEmptyStringOrUndefined(input)).toBe(expected);
+  });
+});
+
+describe('toNonBlankStringOrUndefined', () => {
+  /* eslint-disable indent */
+  it.each`
+    input         | expected      | description
+    ${'hello'}    | ${'hello'}    | ${'non-blank string'}
+    ${'world'}    | ${'world'}    | ${'non-blank string'}
+    ${'a'}        | ${'a'}        | ${'single character'}
+    ${'1'}        | ${'1'}        | ${'numeric string'}
+    ${' '}        | ${undefined}  | ${'space character'}
+    ${'   '}      | ${undefined}  | ${'multiple spaces'}
+    ${'\t'}       | ${undefined}  | ${'tab character'}
+    ${'\n'}       | ${undefined}  | ${'newline character'}
+    ${''}         | ${undefined}  | ${'empty string'}
+    ${null}       | ${undefined}  | ${'null'}
+    ${undefined}  | ${undefined}  | ${'undefined'}
+  `('should return $expected for $description: $input', ({ input, expected }) => {
+  /* eslint-enable indent */
+    expect(toNonBlankStringOrUndefined(input)).toBe(expected);
+  });
+});
+
+describe('toNonEmptyString', () => {
+  /* eslint-disable indent */
+  it.each`
+    input         | expected      | description
+    ${'hello'}    | ${'hello'}    | ${'non-empty string'}
+    ${'world'}    | ${'world'}    | ${'non-empty string'}
+    ${'a'}        | ${'a'}        | ${'single character'}
+    ${'1'}        | ${'1'}        | ${'numeric string'}
+    ${' '}        | ${' '}        | ${'space character'}
+    ${'   '}      | ${'   '}      | ${'multiple spaces'}
+  `('should return $expected for valid $description: $input', ({ input, expected }) => {
+  /* eslint-enable indent */
+    expect(toNonEmptyString(input)).toBe(expected);
+  });
+
+  /* eslint-disable indent */
+  it.each`
+    input         | description
+    ${''}         | ${'empty string'}
+    ${null}       | ${'null'}
+    ${undefined}  | ${'undefined'}
+  `('should throw error for invalid $description: $input', ({ input }) => {
+  /* eslint-enable indent */
+    expect(() => toNonEmptyString(input)).toThrow('Expected a non-empty string, but received:');
+  });
+});
+
+describe('toNonBlankString', () => {
+  /* eslint-disable indent */
+  it.each`
+    input         | expected      | description
+    ${'hello'}    | ${'hello'}    | ${'non-blank string'}
+    ${'world'}    | ${'world'}    | ${'non-blank string'}
+    ${'a'}        | ${'a'}        | ${'single character'}
+    ${'1'}        | ${'1'}        | ${'numeric string'}
+  `('should return $expected for valid $description: $input', ({ input, expected }) => {
+  /* eslint-enable indent */
+    expect(toNonBlankString(input)).toBe(expected);
+  });
+
+  /* eslint-disable indent */
+  it.each`
+    input         | description
+    ${' '}        | ${'space character'}
+    ${'   '}      | ${'multiple spaces'}
+    ${'\t'}       | ${'tab character'}
+    ${'\n'}       | ${'newline character'}
+    ${''}         | ${'empty string'}
+    ${null}       | ${'null'}
+    ${undefined}  | ${'undefined'}
+  `('should throw error for invalid $description: $input', ({ input }) => {
+  /* eslint-enable indent */
+    expect(() => toNonBlankString(input)).toThrow('Expected a non-blank string, but received:');
+  });
+});
+
+describe('type safety', () => {
+  it('should maintain type safety with branded types', () => {
+    const validString = 'test';
+
+    const nonEmptyResult = toNonEmptyStringOrUndefined(validString);
+    const nonBlankResult = toNonBlankStringOrUndefined(validString);
+
+    expect(nonEmptyResult).toBe(validString);
+    expect(nonBlankResult).toBe(validString);
+
+    // These types should be different at compile time
+    // but we can't easily test that in runtime
+    if (nonEmptyResult !== undefined && nonBlankResult !== undefined) {
+      expect(nonEmptyResult).toBe(nonBlankResult);
+    }
+  });
+});

packages/core/src/interfaces/primitive/string.ts

@@ -0,0 +1,77 @@
+/**
+ * A branded type representing a string that is guaranteed to be non-empty (length > 0).
+ * This type allows distinguishing non-empty strings from regular strings at compile time.
+ */
+export type NonEmptyString = string & { readonly __brand: unique symbol };
+
+/**
+ * Checks if a value is a non-empty string.
+ * @param value - The value to check
+ * @returns True if the value is a string with length > 0, false otherwise
+ */
+export const isNonEmptyString = (value: string | null | undefined): value is NonEmptyString => {
+  return value != null && value.length > 0;
+};
+
+/**
+ * Converts a string to NonEmptyString type.
+ * @param value - The string to convert
+ * @returns The string as NonEmptyString type
+ * @throws Error if the value is null, undefined, or empty string
+ */
+export const toNonEmptyString = (value: string): NonEmptyString => {
+  // throw Error if the value is null, undefined or empty
+  if (!isNonEmptyString(value)) throw new Error(`Expected a non-empty string, but received: ${value}`);
+  return value;
+};
+
+/**
+ * Converts a string to NonEmptyString type or returns undefined.
+ * @param value - The string to convert
+ * @returns The string as NonEmptyString type, or undefined if the value is null, undefined, or empty
+ */
+export const toNonEmptyStringOrUndefined = (value: string | null | undefined): NonEmptyString | undefined => {
+  // return undefined if the value is null, undefined or empty
+  if (!isNonEmptyString(value)) return undefined;
+  return value;
+};
+
+/**
+ * A branded type representing a string that is guaranteed to be non-blank.
+ * A non-blank string contains at least one non-whitespace character.
+ * This type allows distinguishing non-blank strings from regular strings at compile time.
+ */
+export type NonBlankString = string & { readonly __brand: unique symbol };
+
+/**
+ * Checks if a value is a non-blank string.
+ * A non-blank string is a string that contains at least one non-whitespace character.
+ * @param value - The value to check
+ * @returns True if the value is a string with trimmed length > 0, false otherwise
+ */
+export const isNonBlankString = (value: string | null | undefined): value is NonBlankString => {
+  return value != null && value.trim().length > 0;
+};
+
+/**
+ * Converts a string to NonBlankString type.
+ * @param value - The string to convert
+ * @returns The string as NonBlankString type
+ * @throws Error if the value is null, undefined, empty string, or contains only whitespace characters
+ */
+export const toNonBlankString = (value: string): NonBlankString => {
+  // throw Error if the value is null, undefined or empty
+  if (!isNonBlankString(value)) throw new Error(`Expected a non-blank string, but received: ${value}`);
+  return value;
+};
+
+/**
+ * Converts a string to NonBlankString type or returns undefined.
+ * @param value - The string to convert
+ * @returns The string as NonBlankString type, or undefined if the value is null, undefined, empty, or contains only whitespace characters
+ */
+export const toNonBlankStringOrUndefined = (value: string | null | undefined): NonBlankString | undefined => {
+  // return undefined if the value is null, undefined or blank (empty or whitespace only)
+  if (!isNonBlankString(value)) return undefined;
+  return value;
+};