refactor(server): cron validation (#13990)

server/src/dtos/system-config.dto.ts

@@ -12,11 +12,8 @@ import {
   IsUrl,
   Max,
   Min,
-  Validate,
   ValidateIf,
   ValidateNested,
-  ValidatorConstraint,
-  ValidatorConstraintInterface,
 } from 'class-validator';
 import { SystemConfig } from 'src/config';
 import { CLIPConfig, DuplicateDetectionConfig, FacialRecognitionConfig } from 'src/dtos/model-config.dto';
@@ -33,14 +30,7 @@ import {
   VideoContainer,
 } from 'src/enum';
 import { ConcurrentQueueName, QueueName } from 'src/interfaces/job.interface';
-import { ValidateBoolean, validateCronExpression } from 'src/validation';
+import { IsCronExpression, ValidateBoolean } from 'src/validation';
-
-@ValidatorConstraint({ name: 'cronValidator' })
-class CronValidator implements ValidatorConstraintInterface {
-  validate(expression: string): boolean {
-    return validateCronExpression(expression);
-  }
-}
 
 const isLibraryScanEnabled = (config: SystemConfigLibraryScanDto) => config.enabled;
 const isOAuthEnabled = (config: SystemConfigOAuthDto) => config.enabled;
@@ -54,7 +44,7 @@ export class DatabaseBackupConfig {
 
   @ValidateIf(isDatabaseBackupEnabled)
   @IsNotEmpty()
-  @Validate(CronValidator, { message: 'Invalid cron expression' })
+  @IsCronExpression()
   @IsString()
   cronExpression!: string;
 
@@ -244,7 +234,7 @@ class SystemConfigLibraryScanDto {
 
   @ValidateIf(isLibraryScanEnabled)
   @IsNotEmpty()
-  @Validate(CronValidator, { message: 'Invalid cron expression' })
+  @IsCronExpression()
   @IsString()
   cronExpression!: string;
 }

server/src/services/backup.service.spec.ts

@@ -88,26 +88,6 @@ describe(BackupService.name, () => {
     });
   });
 
-  describe('onConfigValidateEvent', () => {
-    it('should allow a valid cron expression', () => {
-      expect(() =>
-        sut.onConfigValidate({
-          newConfig: { backup: { database: { cronExpression: '0 0 * * *' } } } as SystemConfig,
-          oldConfig: {} as SystemConfig,
-        }),
-      ).not.toThrow(expect.stringContaining('Invalid cron expression'));
-    });
-
-    it('should fail for an invalid cron expression', () => {
-      expect(() =>
-        sut.onConfigValidate({
-          newConfig: { backup: { database: { cronExpression: 'foo' } } } as SystemConfig,
-          oldConfig: {} as SystemConfig,
-        }),
-      ).toThrow(/Invalid cron expression.*/);
-    });
-  });
-
   describe('cleanupDatabaseBackups', () => {
     it('should do nothing if not reached keepLastAmount', async () => {
       systemMock.get.mockResolvedValue(systemConfigStub.backupEnabled);

server/src/services/backup.service.ts

@@ -8,7 +8,6 @@ import { ArgOf } from 'src/interfaces/event.interface';
 import { JobName, JobStatus, QueueName } from 'src/interfaces/job.interface';
 import { BaseService } from 'src/services/base.service';
 import { handlePromiseError } from 'src/utils/misc';
-import { validateCronExpression } from 'src/validation';
 
 @Injectable()
 export class BackupService extends BaseService {
@@ -49,14 +48,6 @@ export class BackupService extends BaseService {
     });
   }
 
-  @OnEvent({ name: 'config.validate' })
-  onConfigValidate({ newConfig }: ArgOf<'config.validate'>) {
-    const { database } = newConfig.backup;
-    if (!validateCronExpression(database.cronExpression)) {
-      throw new Error(`Invalid cron expression ${database.cronExpression}`);
-    }
-  }
-
   async cleanupDatabaseBackups() {
     this.logger.debug(`Database Backup Cleanup Started`);
     const {

server/src/services/library.service.spec.ts

@@ -177,26 +177,6 @@ describe(LibraryService.name, () => {
     });
   });
 
-  describe('onConfigValidateEvent', () => {
-    it('should allow a valid cron expression', () => {
-      expect(() =>
-        sut.onConfigValidate({
-          newConfig: { library: { scan: { cronExpression: '0 0 * * *' } } } as SystemConfig,
-          oldConfig: {} as SystemConfig,
-        }),
-      ).not.toThrow(expect.stringContaining('Invalid cron expression'));
-    });
-
-    it('should fail for an invalid cron expression', () => {
-      expect(() =>
-        sut.onConfigValidate({
-          newConfig: { library: { scan: { cronExpression: 'foo' } } } as SystemConfig,
-          oldConfig: {} as SystemConfig,
-        }),
-      ).toThrow(/Invalid cron expression.*/);
-    });
-  });
-
   describe('handleQueueSyncFiles', () => {
     it('should queue refresh of a new asset', async () => {
       libraryMock.get.mockResolvedValue(libraryStub.externalLibrary1);

server/src/services/library.service.ts

@@ -24,7 +24,6 @@ import { BaseService } from 'src/services/base.service';
 import { mimeTypes } from 'src/utils/mime-types';
 import { handlePromiseError } from 'src/utils/misc';
 import { usePagination } from 'src/utils/pagination';
-import { validateCronExpression } from 'src/validation';
 
 @Injectable()
 export class LibraryService extends BaseService {
@@ -81,14 +80,6 @@ export class LibraryService extends BaseService {
     }
   }
 
-  @OnEvent({ name: 'config.validate' })
-  onConfigValidate({ newConfig }: ArgOf<'config.validate'>) {
-    const { scan } = newConfig.library;
-    if (!validateCronExpression(scan.cronExpression)) {
-      throw new Error(`Invalid cron expression ${scan.cronExpression}`);
-    }
-  }
-
   private async watch(id: string): Promise<boolean> {
     if (!this.watchLibraries) {
       return false;

server/src/services/system-config.service.spec.ts

@@ -261,6 +261,29 @@ describe(SystemConfigService.name, () => {
       });
     });
 
+    it('should accept valid cron expressions', async () => {
+      configMock.getEnv.mockReturnValue(mockEnvData({ configFile: 'immich-config.json' }));
+      systemMock.readFile.mockResolvedValue(JSON.stringify({ library: { scan: { cronExpression: '0 0 * * *' } } }));
+
+      await expect(sut.getSystemConfig()).resolves.toMatchObject({
+        library: {
+          scan: {
+            enabled: true,
+            cronExpression: '0 0 * * *',
+          },
+        },
+      });
+    });
+
+    it('should reject invalid cron expressions', async () => {
+      configMock.getEnv.mockReturnValue(mockEnvData({ configFile: 'immich-config.json' }));
+      systemMock.readFile.mockResolvedValue(JSON.stringify({ library: { scan: { cronExpression: 'foo' } } }));
+
+      await expect(sut.getSystemConfig()).rejects.toThrow(
+        'library.scan.cronExpression has failed the following constraints: cronValidator',
+      );
+    });
+
     it('should log errors with the config file', async () => {
       configMock.getEnv.mockReturnValue(mockEnvData({ configFile: 'immich-config.json' }));
 

server/src/validation.ts

@@ -16,9 +16,12 @@ import {
   IsOptional,
   IsString,
   IsUUID,
+  Validate,
   ValidateBy,
   ValidateIf,
   ValidationOptions,
+  ValidatorConstraint,
+  ValidatorConstraintInterface,
   buildMessage,
   isDateString,
 } from 'class-validator';
@@ -156,16 +159,20 @@ export const ValidateBoolean = (options?: BooleanOptions) => {
   return applyDecorators(...decorators);
 };
 
-export function validateCronExpression(expression: string) {
+@ValidatorConstraint({ name: 'cronValidator' })
-  try {
+class CronValidator implements ValidatorConstraintInterface {
-    new CronJob(expression, () => {});
+  validate(expression: string): boolean {
-  } catch {
+    try {
-    return false;
+      new CronJob(expression, () => {});
+      return true;
+    } catch {
+      return false;
+    }
   }
-
-  return true;
 }
 
+export const IsCronExpression = () => Validate(CronValidator, { message: 'Invalid cron expression' });
+
 type IValue = { value: unknown };
 
 export const toEmail = ({ value }: IValue) => (typeof value === 'string' ? value.toLowerCase() : value);