Merge pull request #9890 from weseek/fix/config-loader

fix: ConfigLoader.loadFromDB for JSON parsing error handling

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

@@ -0,0 +1,82 @@
+import type { RawConfigData } from '@growi/core/dist/interfaces';
+
+import type { ConfigKey, ConfigValues } from './config-definition';
+import { ConfigLoader } from './config-loader';
+
+const mockExec = vi.fn();
+const mockFind = vi.fn().mockReturnValue({ exec: mockExec });
+
+// Mock the Config model
+vi.mock('../../models/config', () => ({
+  Config: {
+    find: mockFind,
+  },
+}));
+
+describe('ConfigLoader', () => {
+  let configLoader: ConfigLoader;
+
+  beforeEach(async() => {
+    configLoader = new ConfigLoader();
+    vi.clearAllMocks();
+  });
+
+  describe('loadFromDB', () => {
+    describe('when doc.value is empty string', () => {
+      beforeEach(() => {
+        const mockDocs = [
+          { key: 'app:referrerPolicy' as ConfigKey, value: '' },
+        ];
+        mockExec.mockResolvedValue(mockDocs);
+      });
+
+      it('should return null for value', async() => {
+        const config: RawConfigData<ConfigKey, ConfigValues> = await configLoader.loadFromDB();
+        expect(config['app:referrerPolicy'].value).toBe(null);
+      });
+    });
+
+    describe('when doc.value is invalid JSON', () => {
+      beforeEach(() => {
+        const mockDocs = [
+          { key: 'app:referrerPolicy' as ConfigKey, value: '{invalid:json' },
+        ];
+        mockExec.mockResolvedValue(mockDocs);
+      });
+
+      it('should return null for value', async() => {
+        const config: RawConfigData<ConfigKey, ConfigValues> = await configLoader.loadFromDB();
+        expect(config['app:referrerPolicy'].value).toBe(null);
+      });
+    });
+
+    describe('when doc.value is valid JSON', () => {
+      const validJson = { key: 'value' };
+      beforeEach(() => {
+        const mockDocs = [
+          { key: 'app:referrerPolicy' as ConfigKey, value: JSON.stringify(validJson) },
+        ];
+        mockExec.mockResolvedValue(mockDocs);
+      });
+
+      it('should return parsed value', async() => {
+        const config: RawConfigData<ConfigKey, ConfigValues> = await configLoader.loadFromDB();
+        expect(config['app:referrerPolicy'].value).toEqual(validJson);
+      });
+    });
+
+    describe('when doc.value is null', () => {
+      beforeEach(() => {
+        const mockDocs = [
+          { key: 'app:referrerPolicy' as ConfigKey, value: null },
+        ];
+        mockExec.mockResolvedValue(mockDocs);
+      });
+
+      it('should return null for value', async() => {
+        const config: RawConfigData<ConfigKey, ConfigValues> = await configLoader.loadFromDB();
+        expect(config['app:referrerPolicy'].value).toBe(null);
+      });
+    });
+  });
+});

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

@@ -44,7 +44,14 @@ export class ConfigLoader implements IConfigLoader<ConfigKey, ConfigValues> {
     for (const doc of docs) {
       dbConfig[doc.key as ConfigKey] = {
         definition: (doc.key in CONFIG_DEFINITIONS) ? CONFIG_DEFINITIONS[doc.key as ConfigKey] : undefined,
-        value: doc.value != null ? JSON.parse(doc.value) : null,
+        value: doc.value != null ? (() => {
+          try {
+            return JSON.parse(doc.value);
+          }
+          catch {
+            return null;
+          }
+        })() : null,
       };
     }
 

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

@@ -1,9 +1,14 @@
+import type { RawConfigData } from '@growi/core/dist/interfaces';
 import { mock } from 'vitest-mock-extended';
 
 import type { S2sMessagingService } from '../s2s-messaging/base';
 
+import type { ConfigKey, ConfigValues } from './config-definition';
 import { configManager } from './config-manager';
 
+// Test helper type for setting configs
+type TestConfigData = RawConfigData<ConfigKey, ConfigValues>;
+
 const mocks = vi.hoisted(() => ({
   ConfigMock: {
     updateOne: vi.fn(),
@@ -104,7 +109,6 @@ describe('ConfigManager test', () => {
   });
 
   describe('getManagedEnvVars()', () => {
-
     beforeAll(async() => {
       process.env.AUTO_INSTALL_ADMIN_USERNAME = 'admin';
       process.env.AUTO_INSTALL_ADMIN_PASSWORD = 'password';
@@ -129,7 +133,64 @@ describe('ConfigManager test', () => {
       expect(result.AUTO_INSTALL_ADMIN_USERNAME).toEqual('admin');
       expect(result.AUTO_INSTALL_ADMIN_PASSWORD).toEqual('***');
     });
+  });
+
+  describe('getConfig()', () => {
+    // Helper function to set configs with proper typing
+    const setTestConfigs = (dbConfig: Partial<TestConfigData>, envConfig: Partial<TestConfigData>): void => {
+      Object.defineProperties(configManager, {
+        dbConfig: { value: dbConfig, configurable: true },
+        envConfig: { value: envConfig, configurable: true },
+      });
+    };
+
+    beforeEach(async() => {
+      // Reset configs before each test using properly typed empty objects
+      setTestConfigs({}, {});
+    });
 
+    test('should fallback to env value when dbConfig[key] exists but its value is undefined', async() => {
+      // Prepare test data that simulates the issue with proper typing
+      const dbConfig: Partial<TestConfigData> = {
+        'app:title': { value: undefined },
+      };
+      const envConfig: Partial<TestConfigData> = {
+        'app:title': { value: 'GROWI' },
+      };
+      setTestConfigs(dbConfig, envConfig);
+
+      // Act
+      const result = configManager.getConfig('app:title');
+
+      // Assert - Should return env value since db value is undefined
+      expect(result).toBe('GROWI');
+    });
+
+    test('should handle various edge case scenarios correctly', async() => {
+      // Setup multiple test scenarios with proper typing
+      const dbConfig: Partial<TestConfigData> = {
+        'app:title': { value: undefined }, // db value is explicitly undefined
+        'app:siteUrl': { value: undefined }, // another undefined value
+        'app:fileUpload': undefined, // db config entry itself is undefined
+        'app:fileUploadType': { value: 'gridfs' }, // db has valid value
+      };
+      const envConfig: Partial<TestConfigData> = {
+        'app:title': { value: 'GROWI' },
+        'app:siteUrl': { value: 'https://example.com' },
+        'app:fileUpload': { value: true },
+        'app:fileUploadType': { value: 'aws' },
+        // Add control flags for env vars
+        'env:useOnlyEnvVars:app:siteUrl': { value: false },
+        'env:useOnlyEnvVars:app:fileUploadType': { value: false },
+      };
+      setTestConfigs(dbConfig, envConfig);
+
+      // Test each scenario
+      expect(configManager.getConfig('app:title')).toBe('GROWI'); // Should fallback to env when db value is undefined
+      expect(configManager.getConfig('app:siteUrl')).toBe('https://example.com'); // Should fallback to env when db value is undefined
+      expect(configManager.getConfig('app:fileUpload')).toBe(true); // Should fallback to env when db config is undefined
+      expect(configManager.getConfig('app:fileUploadType')).toBe('gridfs'); // Should use db value when valid
+    });
   });
 
 });