refactor(server): Extract the naming of Assets into a strategy

server/e2e/config/testing-asset-storage-strategy.ts

@@ -37,4 +37,8 @@ export class TestingAssetStorageStrategy implements AssetStorageStrategy {
             writable.on('error', reject);
         });
     }
+
+    fileExists(fileName: string): Promise<boolean> {
+        return Promise.resolve(false);
+    }
 }

server/src/config/asset-naming-strategy/asset-naming-strategy.ts

@@ -0,0 +1,25 @@
+/**
+ * The AssetNamingStrategy determines how file names are generated based on the uploaded source file name,
+ * as well as how to handle naming conflicts.
+ */
+export interface AssetNamingStrategy {
+    /**
+     * Given the original file name of the uploaded file, generate a file name to
+     * be stored on the server. Operations like normalization and time-stamping can
+     * be performed in this method.
+     *
+     * The output will be checked for a naming conflict with an existing file. If a conflict
+     * exists, this method will be invoked again with the second argument passed in and a new, unique
+     * file name should then be generated. This process will repeat until a unique file name has
+     * been returned.
+     */
+    generateSourceFileName(originalFileName: string, conflictFileName?: string): string;
+
+    /**
+     * Given the source file name generated in the {@link generateSourceFileName} method, this method
+     * should generate the file name of the preview image.
+     *
+     * The same mechanism of checking for conflicts is used as described above.
+     */
+    generatePreviewFileName(sourceFileName: string, conflictFileName?: string): string;
+}

server/src/config/asset-naming-strategy/default-asset-naming-strategy.spec.ts

@@ -0,0 +1,49 @@
+import { DefaultAssetNamingStrategy } from './default-asset-naming-strategy';
+
+describe('DefaultAssetNamingStrategy', () => {
+    describe('generateSourceFileName()', () => {
+        it('normalizes file names', () => {
+            const strategy = new DefaultAssetNamingStrategy();
+
+            expect(strategy.generateSourceFileName('foo.jpg')).toBe('foo.jpg');
+            expect(strategy.generateSourceFileName('curaçao.jpg')).toBe('curacao.jpg');
+            expect(strategy.generateSourceFileName('dấu hỏi.jpg')).toBe('dau-hoi.jpg');
+        });
+
+        it('increments conflicting file names', () => {
+            const strategy = new DefaultAssetNamingStrategy();
+
+            expect(strategy.generateSourceFileName('foo.jpg', 'foo.jpg')).toBe('foo__02.jpg');
+            expect(strategy.generateSourceFileName('foo.jpg', 'foo__02.jpg')).toBe('foo__03.jpg');
+            expect(strategy.generateSourceFileName('foo.jpg', 'foo__09.jpg')).toBe('foo__10.jpg');
+            expect(strategy.generateSourceFileName('foo.jpg', 'foo__99.jpg')).toBe('foo__100.jpg');
+            expect(strategy.generateSourceFileName('foo.jpg', 'foo__999.jpg')).toBe('foo__1000.jpg');
+        });
+    });
+
+    describe('generatePreviewFileName()', () => {
+        it('adds the preview suffix', () => {
+            const strategy = new DefaultAssetNamingStrategy();
+
+            expect(strategy.generatePreviewFileName('foo.jpg')).toBe('foo__preview.jpg');
+        });
+
+        it('preserves the extension of supported image files', () => {
+            const strategy = new DefaultAssetNamingStrategy();
+
+            expect(strategy.generatePreviewFileName('foo.jpg')).toBe('foo__preview.jpg');
+            expect(strategy.generatePreviewFileName('foo.jpeg')).toBe('foo__preview.jpeg');
+            expect(strategy.generatePreviewFileName('foo.png')).toBe('foo__preview.png');
+            expect(strategy.generatePreviewFileName('foo.webp')).toBe('foo__preview.webp');
+            expect(strategy.generatePreviewFileName('foo.tiff')).toBe('foo__preview.tiff');
+        });
+
+        it('adds a png extension for unsupported images and other files', () => {
+            const strategy = new DefaultAssetNamingStrategy();
+
+            expect(strategy.generatePreviewFileName('foo.svg')).toBe('foo__preview.svg.png');
+            expect(strategy.generatePreviewFileName('foo.gif')).toBe('foo__preview.gif.png');
+            expect(strategy.generatePreviewFileName('foo.pdf')).toBe('foo__preview.pdf.png');
+        });
+    });
+});

server/src/config/asset-naming-strategy/default-asset-naming-strategy.ts

@@ -0,0 +1,63 @@
+import * as path from 'path';
+import { normalizeString } from 'shared/normalize-string';
+
+import { AssetNamingStrategy } from './asset-naming-strategy';
+
+/**
+ * The default strategy normalizes the file names to remove unwanted characters and
+ * in the case of conflicts, increments a counter suffix.
+ */
+export class DefaultAssetNamingStrategy implements AssetNamingStrategy {
+    private readonly numberingRe = /__(\d+)\.[^.]+$/;
+
+    generateSourceFileName(originalFileName: string, conflictFileName?: string): string {
+        const normalized = normalizeString(originalFileName, '-');
+        if (!conflictFileName) {
+            return normalized;
+        } else {
+            return this.incrementOrdinalSuffix(normalized, conflictFileName);
+        }
+    }
+
+    generatePreviewFileName(sourceFileName: string, conflictFileName?: string): string {
+        const previewSuffix = '__preview';
+        const previewFileName = this.isSupportedImageFormat(sourceFileName)
+            ? this.addSuffix(sourceFileName, previewSuffix)
+            : this.addSuffix(sourceFileName, previewSuffix) + '.png';
+
+        if (!conflictFileName) {
+            return previewFileName;
+        } else {
+            return this.incrementOrdinalSuffix(previewFileName, conflictFileName);
+        }
+    }
+
+    /**
+     * A "supported format" means that the Sharp library can transform it and output the same
+     * file type. Unsupported images and other non-image files will be converted to png.
+     *
+     * See http://sharp.pixelplumbing.com/en/stable/api-output/#tobuffer
+     */
+    private isSupportedImageFormat(fileName: string): boolean {
+        const imageExtensions = ['.jpg', '.jpeg', '.png', '.webp', '.tiff'];
+        const ext = path.extname(fileName);
+        return imageExtensions.includes(ext);
+    }
+
+    private incrementOrdinalSuffix(baseFileName: string, conflictFileName: string): string {
+        const matches = conflictFileName.match(this.numberingRe);
+        const ord = Number(matches && matches[1]) || 1;
+        return this.addOrdinalSuffix(baseFileName, ord + 1);
+    }
+
+    private addOrdinalSuffix(fileName: string, order: number): string {
+        const paddedOrder = order.toString(10).padStart(2, '0');
+        return this.addSuffix(fileName, `__${paddedOrder}`);
+    }
+
+    private addSuffix(fileName: string, suffix: string): string {
+        const ext = path.extname(fileName);
+        const baseName = path.basename(fileName, ext);
+        return `${baseName}${suffix}${ext}`;
+    }
+}

server/src/config/asset-storage-strategy/asset-storage-strategy.ts

@@ -31,6 +31,12 @@ export interface AssetStorageStrategy {
      */
     readFileToStream(identifier: string): Promise<Stream>;
 
+    /**
+     * Check whether a file with the given name already exists. Used to avoid
+     * naming conflicts before saving the file.
+     */
+    fileExists(fileName: string): Promise<boolean>;
+
     /**
      * Convert an identifier as generated by the writeFile... methods into an absolute
      * url (if it is not already in that form). If no conversion step is needed

server/src/config/asset-storage-strategy/no-asset-storage-strategy.ts

@@ -30,4 +30,8 @@ export class NoAssetStorageStrategy implements AssetStorageStrategy {
     toAbsoluteUrl(request: Request, identifier: string): string {
         throw new I18nError(errorMessage);
     }
+
+    fileExists(fileName: string): Promise<boolean> {
+        throw new I18nError(errorMessage);
+    }
 }

server/src/config/config.service.mock.ts

@@ -13,6 +13,7 @@ export class MockConfigService implements MockClass<ConfigService> {
     jwtSecret = 'secret';
     defaultLanguageCode: jest.Mock<any>;
     entityIdStrategy = new MockIdStrategy();
+    assetNamingStrategy = {} as any;
     assetStorageStrategy = {} as any;
     assetPreviewStrategy = {} as any;
     uploadMaxFileSize = 1024;

server/src/config/config.service.ts

@@ -7,6 +7,7 @@ import { ConnectionOptions } from 'typeorm';
 
 import { ReadOnlyRequired } from '../common/types/common-types';
 
+import { AssetNamingStrategy } from './asset-naming-strategy/asset-naming-strategy';
 import { AssetPreviewStrategy } from './asset-preview-strategy/asset-preview-strategy';
 import { AssetStorageStrategy } from './asset-storage-strategy/asset-storage-strategy';
 import { EntityIdStrategy } from './entity-id-strategy/entity-id-strategy';
@@ -59,6 +60,10 @@ export class ConfigService implements VendureConfig {
         return this.activeConfig.entityIdStrategy;
     }
 
+    get assetNamingStrategy(): AssetNamingStrategy {
+        return this.activeConfig.assetNamingStrategy;
+    }
+
     get assetStorageStrategy(): AssetStorageStrategy {
         return this.activeConfig.assetStorageStrategy;
     }

server/src/config/vendure-config.ts

@@ -7,6 +7,8 @@ import { ConnectionOptions } from 'typeorm';
 
 import { ReadOnlyRequired } from '../common/types/common-types';
 
+import { AssetNamingStrategy } from './asset-naming-strategy/asset-naming-strategy';
+import { DefaultAssetNamingStrategy } from './asset-naming-strategy/default-asset-naming-strategy';
 import { AssetPreviewStrategy } from './asset-preview-strategy/asset-preview-strategy';
 import { NoAssetPreviewStrategy } from './asset-preview-strategy/no-asset-preview-strategy';
 import { AssetStorageStrategy } from './asset-storage-strategy/asset-storage-strategy';
@@ -60,6 +62,10 @@ export interface VendureConfig {
      * strategy.
      */
     entityIdStrategy?: EntityIdStrategy<any>;
+    /**
+     * Defines how asset files and preview images are named before being saved.
+     */
+    assetNamingStrategy?: AssetNamingStrategy;
     /**
      * Defines the strategy used for storing uploaded binary files. By default files are
      * persisted to the local file system.
@@ -102,6 +108,7 @@ const defaultConfig: ReadOnlyRequired<VendureConfig> = {
     jwtSecret: 'secret',
     apiPath: API_PATH,
     entityIdStrategy: new AutoIncrementIdStrategy(),
+    assetNamingStrategy: new DefaultAssetNamingStrategy(),
     assetStorageStrategy: new NoAssetStorageStrategy(),
     assetPreviewStrategy: new NoAssetPreviewStrategy(),
     dbConnectionOptions: {

server/src/plugin/default-asset-server/default-asset-storage-strategy.ts

@@ -30,6 +30,14 @@ export class DefaultAssetStorageStrategy implements AssetStorageStrategy {
         return this.filePathToIdentifier(filePath);
     }
 
+    fileExists(fileName: string): Promise<boolean> {
+        return new Promise(resolve => {
+            fs.access(this.identifierToFilePath(fileName), fs.constants.F_OK, err => {
+                resolve(!err);
+            });
+        });
+    }
+
     readFileToBuffer(identifier: string): Promise<Buffer> {
         return fs.readFile(this.identifierToFilePath(identifier));
     }

server/src/service/asset.service.ts

@@ -1,8 +1,6 @@
 import { Injectable } from '@nestjs/common';
 import { InjectConnection } from '@nestjs/typeorm';
-import * as path from 'path';
-import { AssetType, CreateAssetInput } from 'shared/generated-types';
-import { normalizeString } from 'shared/normalize-string';
+import { CreateAssetInput } from 'shared/generated-types';
 import { ID, PaginatedList } from 'shared/shared-types';
 import { Connection } from 'typeorm';
 
@@ -37,48 +35,51 @@ export class AssetService {
     async create(input: CreateAssetInput): Promise<Asset> {
         const { stream, filename, mimetype, encoding } = await input.file;
         const { assetPreviewStrategy, assetStorageStrategy } = this.configService;
-        const normalizedFileName = this.normalizeFileName(filename);
+        const sourceFileName = await this.getSourceFileName(filename);
+        const previewFileName = await this.getPreviewFileName(sourceFileName);
 
-        const sourceFileName = await assetStorageStrategy.writeFileFromStream(normalizedFileName, stream);
-        const sourceFile = await assetStorageStrategy.readFileToBuffer(sourceFileName);
+        const sourceFileIdentifier = await assetStorageStrategy.writeFileFromStream(sourceFileName, stream);
+        const sourceFile = await assetStorageStrategy.readFileToBuffer(sourceFileIdentifier);
         const preview = await assetPreviewStrategy.generatePreviewImage(mimetype, sourceFile);
-        const previewFileName = await assetStorageStrategy.writeFileFromBuffer(
-            this.getPreviewFileName(mimetype, normalizedFileName),
+        const previewFileIdentifier = await assetStorageStrategy.writeFileFromBuffer(
+            previewFileName,
             preview,
         );
 
         const asset = new Asset({
             type: getAssetType(mimetype),
-            name: filename,
+            name: sourceFileName,
             fileSize: sourceFile.byteLength,
             mimeType: mimetype,
-            source: sourceFileName,
-            preview: previewFileName,
+            source: sourceFileIdentifier,
+            preview: previewFileIdentifier,
         });
         return this.connection.manager.save(asset);
     }
 
-    private normalizeFileName(fileName: string): string {
-        const normalized = normalizeString(fileName, '-');
-        const randomPart = Math.random()
-            .toString(8)
-            .substr(2, 8);
-        return this.addSuffix(normalized, `-${randomPart}`);
+    private async getSourceFileName(fileName: string): Promise<string> {
+        const { assetNamingStrategy } = this.configService;
+        return this.generateUniqueName(fileName, (name, conflict) =>
+            assetNamingStrategy.generateSourceFileName(name, conflict),
+        );
     }
 
-    private getPreviewFileName(mimeType: string, sourceFileName: string): string {
-        const previewSuffix = '__preview';
-        switch (getAssetType(mimeType)) {
-            case AssetType.IMAGE:
-                return this.addSuffix(sourceFileName, previewSuffix);
-            default:
-                return this.addSuffix(`${sourceFileName}.png`, previewSuffix);
-        }
+    private async getPreviewFileName(fileName: string): Promise<string> {
+        const { assetNamingStrategy } = this.configService;
+        return this.generateUniqueName(fileName, (name, conflict) =>
+            assetNamingStrategy.generatePreviewFileName(name, conflict),
+        );
     }
 
-    private addSuffix(fileName: string, suffix: string): string {
-        const ext = path.extname(fileName);
-        const baseName = path.basename(fileName, ext);
-        return `${baseName}${suffix}${ext}`;
+    private async generateUniqueName(
+        inputFileName: string,
+        generateNameFn: (fileName: string, conflictName?: string) => string,
+    ): Promise<string> {
+        const { assetStorageStrategy } = this.configService;
+        let outputFileName: string | undefined;
+        do {
+            outputFileName = generateNameFn(inputFileName, outputFileName);
+        } while (await assetStorageStrategy.fileExists(outputFileName));
+        return outputFileName;
     }
 }

server/src/service/product.service.spec.ts

@@ -8,6 +8,7 @@ import { ProductTranslation } from '../entity/product/product-translation.entity
 import { Product } from '../entity/product/product.entity';
 import { MockConnection } from '../testing/connection.mock';
 
+import { AssetService } from './asset.service';
 import { ChannelService } from './channel.service';
 import { MockTranslationUpdaterService } from './helpers/translation-updater.mock';
 import { TranslationUpdaterService } from './helpers/translation-updater.service';
@@ -25,6 +26,7 @@ describe('ProductService', () => {
                 { provide: TranslationUpdaterService, useClass: MockTranslationUpdaterService },
                 { provide: Connection, useClass: MockConnection },
                 { provide: ChannelService, useClass: MockChannelService },
+                { provide: AssetService, useValue: {} },
             ],
         }).compile();