fix(document): address PR review feedback on thumbnail generation

ClemRz · ClemRz · commit fa125a227ca4 · 2026-07-07T00:38:11.000-06:00
- Add setCredentialsForTest() to FileService for proper test injection
- Rewrite thumbnail integration tests to call real create/delete paths
- Use real toFile converter in property tests instead of local copy
- Add comment documenting orphaned-blob edge case in ThumbnailService
- Add no-console to eslint-disable in backfill script
diff --git a/api/services/FileService.js b/api/services/FileService.js
@@ -86,6 +86,26 @@ module.exports = {
 
   isCredentials: !!credentials,
 
+  /**
+   * Test-only: Retrieve the current module-level credentials object.
+   * Used by tests to save original credentials before injecting mocks.
+   * @returns {object|null}
+   */
+  getCredentialsForTest() {
+    return credentials;
+  },
+
+  /**
+   * Test-only: Override the module-level credentials object.
+   * Allows integration tests to inject a mock blob client without
+   * reimplementing create/delete. Call with `null` to restore original.
+   * @param {object|null} mockCredentials
+   */
+  setCredentialsForTest(mockCredentials) {
+    credentials = mockCredentials;
+    this.isCredentials = !!mockCredentials;
+  },
+
   document: {
     getUrl(path) {
       // The documents container allow anonymous access
diff --git a/api/services/ThumbnailService.js b/api/services/ThumbnailService.js
@@ -84,15 +84,15 @@ module.exports = {
   /**
    * Generate all applicable thumbnail variants for an image.
    * Uses an all-or-nothing approach: generates all buffers first, then uploads.
-   * If any step fails, returns all nulls.
+   * If any step fails, returns all nulls with hadError: true.
    *
    * @param {Buffer} imageBuffer - The original image file buffer
    * @param {string} originalPath - The blob path of the original
    * @param {object} blobClient - Azure container client for uploading
-   * @returns {Promise<{small: string|null, medium: string|null, large: string|null}>}
+   * @returns {Promise<{small: string|null, medium: string|null, large: string|null, hadError: boolean}>}
    */
   async generate(imageBuffer, originalPath, blobClient) {
-    const result = { small: null, medium: null, large: null };
+    const result = { small: null, medium: null, large: null, hadError: false };
 
     try {
       const metadata = await sharp(imageBuffer).metadata();
@@ -118,6 +118,12 @@ module.exports = {
       );
 
       // Upload all variants
+      // NOTE: If a subset of uploads succeeds before one fails, the successful
+      // uploads become orphaned blobs in Azure (the catch block returns all-null,
+      // so the DB never stores those paths and FileService.document.delete won't
+      // clean them up). This is a known trade-off: orphaned blobs are rare and
+      // low-cost compared to the complexity of per-variant rollback. A periodic
+      // storage cleanup job could address this if it becomes a concern.
       await Promise.all(
         buffers.map(async ({ path: blobPath, buffer }) => {
           const blockBlobClient = blobClient.getBlockBlobClient(blobPath);
@@ -133,7 +139,7 @@ module.exports = {
       });
     } catch (err) {
       sails.log.error('ThumbnailService.generate failed:', err);
-      return { small: null, medium: null, large: null };
+      return { small: null, medium: null, large: null, hadError: true };
     }
 
     return result;
diff --git a/scripts/backfill-thumbnails.js b/scripts/backfill-thumbnails.js
@@ -1,6 +1,6 @@
 #!/usr/bin/env node
 
-/* eslint-disable no-await-in-loop, no-loop-func, global-require */
+/* eslint-disable no-await-in-loop, no-loop-func, global-require, no-console */
 
 /**
  * Backfill Thumbnails Script
@@ -163,6 +163,11 @@ async function run() {
                 thumbnailLarge: thumbnailPaths.large,
               });
               succeeded += 1;
+            } else if (thumbnailPaths.hadError) {
+              console.error(
+                `WARNING: file ${file.id} failed thumbnail generation (not just too small)`
+              );
+              failed += 1;
             } else {
               skipped += 1;
             }
diff --git a/test/integration/1_services/FileService.thumbnails.test.js b/test/integration/1_services/FileService.thumbnails.test.js
@@ -2,155 +2,55 @@ const should = require('should');
 const sinon = require('sinon');
 const sharp = require('sharp');
 const FileService = require('../../../api/services/FileService');
-const ThumbnailService = require('../../../api/services/ThumbnailService');
 
 describe('FileService.document - Thumbnail Integration', () => {
-  // Store original methods for restoration
-  const originalCreate = FileService.document.create;
-  const originalDelete = FileService.document.delete;
-  const originalIsCredentials = FileService.isCredentials;
-
-  // Mock blob client
   let mockUploadData;
   let mockDeleteBlob;
-  let uploadedBlobs;
   let mockContainerClient;
+  let originalIsCredentials;
+  let originalCredentials;
 
   before(() => {
+    // Save original state to restore later
+    originalIsCredentials = FileService.isCredentials;
+    originalCredentials = FileService.getCredentialsForTest();
+
     mockUploadData = sinon.stub().resolves();
     mockDeleteBlob = sinon.stub().resolves();
-    uploadedBlobs = {};
 
     mockContainerClient = {
       getBlockBlobClient: (blobPath) => ({
-        uploadData: async (buffer, options) => {
-          uploadedBlobs[blobPath] = { buffer, options };
-          return mockUploadData(buffer, options);
-        },
-        delete: async (options) => {
-          mockDeleteBlob(blobPath, options);
-        },
+        uploadData: async (buffer, options) =>
+          mockUploadData(blobPath, buffer, options),
+        delete: async (options) => mockDeleteBlob(blobPath, options),
       }),
     };
 
-    // Patch create to use mock credentials
-    FileService.document.create = async function (
-      file,
-      idDocument,
-      fetchResult = false,
-      isValidated = true
-    ) {
-      const name = file.originalname;
-      const pathName = `${Math.random()
-        .toString()
-        .replace(/0\./, '')}-${name.replace(/ /, '_')}`;
-      const lastDot = name.lastIndexOf('.');
-      if (lastDot <= 0 || lastDot === name.length - 1) {
-        const err = new Error(FileService.INVALID_NAME);
-        err.fileName = name;
-        throw err;
-      }
-      const extension = name.slice(lastDot + 1).toLowerCase();
-
-      const foundFormat = await TFileFormat.find({ extension }).limit(1);
-      if (foundFormat.length === 0) {
-        const err = new Error(FileService.INVALID_FORMAT);
-        err.fileName = name;
-        throw err;
-      }
-
-      // Upload original
-      const blockBlobClient = mockContainerClient.getBlockBlobClient(pathName);
-      await blockBlobClient.uploadData(file.buffer, {
-        blobHTTPHeaders: { blobContentType: foundFormat[0].mimeType },
-      });
-
-      // Thumbnail generation (matches actual FileService logic)
-      let thumbnailPaths = { small: null, medium: null, large: null };
-      const fileMimeType = foundFormat[0].mimeType;
-      if (ThumbnailService.isProcessable(fileMimeType)) {
-        try {
-          thumbnailPaths = await ThumbnailService.generate(
-            file.buffer,
-            pathName,
-            mockContainerClient
-          );
-        } catch (err) {
-          sails.log.error('Thumbnail generation failed:', err);
-        }
-      }
-
-      const param = {
-        dateInscription: new Date(),
-        fileName: name,
-        document: idDocument,
-        fileFormat: foundFormat[0].id,
-        path: pathName,
-        isValidated,
-        thumbnailSmall: thumbnailPaths.small,
-        thumbnailMedium: thumbnailPaths.medium,
-        thumbnailLarge: thumbnailPaths.large,
-      };
-      if (fetchResult) {
-        return TFile.create(param).fetch();
-      }
-      return TFile.create(param);
-    };
-
-    // Patch delete to use mock credentials
-    FileService.document.delete = async function (file) {
-      const destroyedRecord = await TFile.destroyOne(file.id);
-      const blockBlobClient = mockContainerClient.getBlockBlobClient(
-        destroyedRecord.path
-      );
-      await blockBlobClient.delete({ deleteSnapshots: 'include' });
-
-      // Clean up thumbnail blobs
-      const thumbnailPaths = [
-        destroyedRecord.thumbnailSmall,
-        destroyedRecord.thumbnailMedium,
-        destroyedRecord.thumbnailLarge,
-      ].filter(Boolean);
-      await Promise.all(
-        thumbnailPaths.map(async (thumbPath) => {
-          try {
-            const thumbBlobClient =
-              mockContainerClient.getBlockBlobClient(thumbPath);
-            await thumbBlobClient.delete({ deleteSnapshots: 'include' });
-          } catch (err) {
-            sails.log.error(
-              `Failed to delete thumbnail blob ${thumbPath}:`,
-              err
-            );
-          }
-        })
-      );
-
-      return destroyedRecord;
-    };
-
-    FileService.isCredentials = true;
+    // Inject mock credentials so the real create/delete code paths run
+    FileService.setCredentialsForTest({
+      documentsBlobClient: mockContainerClient,
+    });
   });
 
   beforeEach(() => {
     mockUploadData.resetHistory();
     mockDeleteBlob.resetHistory();
-    uploadedBlobs = {};
   });
 
   after(() => {
-    FileService.document.create = originalCreate;
-    FileService.document.delete = originalDelete;
+    // Restore original credentials state
+    FileService.setCredentialsForTest(originalCredentials);
     FileService.isCredentials = originalIsCredentials;
     sinon.restore();
   });
 
   describe('create() with image file', () => {
     let createdFile;
 
-    after(async () => {
+    afterEach(async () => {
       if (createdFile) {
         await TFile.destroyOne(createdFile.id);
+        createdFile = null;
       }
     });
 
@@ -183,8 +83,7 @@ describe('FileService.document - Thumbnail Integration', () => {
       should(createdFile.thumbnailMedium).startWith('thumbnails/medium/');
       should(createdFile.thumbnailMedium).endWith('.webp');
 
-      // Large should be null — original is only 2000px, not > 1920
-      // Actually 2000 > 1920, so large IS generated
+      // 2000 > 1920, so large IS generated
       should(createdFile.thumbnailLarge).be.a.String();
       should(createdFile.thumbnailLarge).startWith('thumbnails/large/');
       should(createdFile.thumbnailLarge).endWith('.webp');
@@ -208,13 +107,11 @@ describe('FileService.document - Thumbnail Integration', () => {
         size: imageBuffer.length,
       };
 
-      const result = await FileService.document.create(file, 1, true);
-
-      should(result.thumbnailSmall).be.a.String();
-      should(result.thumbnailMedium).be.a.String();
-      should(result.thumbnailLarge).be.null();
+      createdFile = await FileService.document.create(file, 1, true);
 
-      await TFile.destroyOne(result.id);
+      should(createdFile.thumbnailSmall).be.a.String();
+      should(createdFile.thumbnailMedium).be.a.String();
+      should(createdFile.thumbnailLarge).be.null();
     });
   });
 
@@ -254,9 +151,9 @@ describe('FileService.document - Thumbnail Integration', () => {
 
   describe('create() with thumbnail generation failure', () => {
     it('should still succeed with null thumbnails when sharp fails', async () => {
-      // Stub ThumbnailService.generate to throw
+      // Stub the global ThumbnailService.generate (Sails auto-globalized)
       const generateStub = sinon
-        .stub(ThumbnailService, 'generate')
+        .stub(global.ThumbnailService, 'generate')
         .rejects(new Error('sharp exploded'));
       const logStub = sinon.stub(sails.log, 'error');
 
diff --git a/test/integration/1_services/GeoLocNetworks.property.test.js b/test/integration/1_services/GeoLocNetworks.property.test.js
@@ -57,8 +57,7 @@ const networkRowsArbitrary = fc
 
 describe('GeoLocService formatNetworks - Property Tests', () => {
   describe('Property 1: every network has more than one entrance', () => {
-    it('should produce networks each with at least 2 entrances', function () {
-      this.timeout(10000);
+    it('should produce networks each with at least 2 entrances', () => {
       fc.assert(
         fc.property(networkRowsArbitrary, (rows) => {
           const networks = formatNetworks(rows);
@@ -68,12 +67,11 @@ describe('GeoLocService formatNetworks - Property Tests', () => {
         }),
         { numRuns: 100 }
       );
-    });
+    }).timeout(10000);
   });
 
   describe('Property 2: centroid equals arithmetic mean of entrance coordinates', () => {
-    it('should have centroid matching average of entrance lat/lng', function () {
-      this.timeout(10000);
+    it('should have centroid matching average of entrance lat/lng', () => {
       fc.assert(
         fc.property(networkRowsArbitrary, (rows) => {
           const networks = formatNetworks(rows);
@@ -90,12 +88,11 @@ describe('GeoLocService formatNetworks - Property Tests', () => {
         }),
         { numRuns: 100 }
       );
-    });
+    }).timeout(10000);
   });
 
   describe('Property 6: backwards-compatible output shape', () => {
-    it('should always include id, name, longitude, latitude, entrances', function () {
-      this.timeout(10000);
+    it('should always include id, name, longitude, latitude, entrances', () => {
       fc.assert(
         fc.property(networkRowsArbitrary, (rows) => {
           const networks = formatNetworks(rows);
@@ -114,6 +111,6 @@ describe('GeoLocService formatNetworks - Property Tests', () => {
         }),
         { numRuns: 100 }
       );
-    });
+    }).timeout(10000);
   });
 });
diff --git a/test/integration/1_services/ThumbnailService.property.test.js b/test/integration/1_services/ThumbnailService.property.test.js
diff --git a/test/integration/1_services/ThumbnailService.test.js b/test/integration/1_services/ThumbnailService.test.js
