fix: replace only missing uploads with dummy document

src/Service/Upload/UploadHandler.php

@@ -164,24 +164,40 @@ class UploadHandler
     }
 
     /**
-     * Returns the upload file path, substituting the stand-in document while the
-     * upload_replacement feature is on. Use this for read-only access (downloads, email
-     * attachments); getUploadFilepath() stays the real location and is what writes target.
+     * Returns the upload file path, falling back to the stand-in document when the real file is
+     * missing and the upload_replacement feature is on.
+     *
+     * Environments other than prod run on anonymized copies of the production database but never
+     * carry over the uploaded files, so most rows point at a file that is not there. The stand-in
+     * fills exactly that gap: a file that is present - one uploaded while testing - is always
+     * served as it is, which is what keeps the upload flow exercisable.
+     *
+     * Use this for read-only access (downloads, email attachments); getUploadFilepath() stays the
+     * real location and is what writes target.
      */
     public function getResolvedUploadFilepath(Upload $upload): string
     {
+        $originalPath = $this->getUploadFilepath($upload);
+
+        if (true === file_exists($originalPath)) {
+            return $originalPath;
+        }
+
         if (false === $this->featureManager->isActive('upload_replacement')) {
-            return $this->getUploadFilepath($upload);
+            return $originalPath;
         }
 
         $replacementPath = sprintf('%s/%s', $this->projectDir, $this->uploadReplacementFile);
 
         if (false === file_exists($replacementPath)) {
-            // Falling back to the real document here is what made the old behaviour opaque: the
-            // feature would look active and quietly serve the genuine file instead.
             throw new \RuntimeException('Upload replacement file not found at '.$replacementPath);
         }
 
+        $this->logger->info('Serving upload replacement', [
+            'file_id' => $upload->getId(),
+            'missing_path' => $originalPath,
+        ]);
+
         return $replacementPath;
     }
 

tests/Service/Upload/UploadHandlerTest.php

@@ -12,59 +12,86 @@ use Oneup\UploaderBundle\Uploader\Orphanage\OrphanageManager;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\NullLogger;
+use Symfony\Component\Filesystem\Filesystem;
 use Symfony\Component\HttpFoundation\RequestStack;
 
 /**
- * The stand-in document is gated by the upload_replacement feature and by nothing else - not by
- * the environment, not by whether a path happens to be configured. That, and the fact that a
- * missing stand-in is an error rather than a silent fall back to the genuine file, is what is
- * pinned here.
+ * The stand-in document fills a gap, it never covers a file that is there: environments other
+ * than prod run on anonymized copies of the production database but never carry over the uploaded
+ * files. A file that is present - one uploaded while testing - has to come back untouched, or the
+ * upload flow could not be exercised at all. That, and the fact that the feature is the only
+ * thing gating the fallback, is what is pinned here.
  */
 class UploadHandlerTest extends TestCase
 {
-    private const PROJECT_DIR = '/srv/app';
     private const REPLACEMENT_FILE = 'assets/pdf/chicken.pdf';
+    private const UPLOAD_PATH = 'uploads/invoice/i/invoice-1.pdf';
 
     private FeatureManagerInterface&MockObject $featureManager;
+    private Filesystem $filesystem;
+    private string $projectDir;
 
     protected function setUp(): void
     {
         $this->featureManager = $this->createMock(FeatureManagerInterface::class);
+        $this->filesystem = new Filesystem();
+        $this->projectDir = sprintf('%s/upload-handler-%s', sys_get_temp_dir(), uniqid());
+
+        $this->filesystem->mkdir($this->projectDir.'/'.\dirname(self::REPLACEMENT_FILE));
+        $this->filesystem->dumpFile($this->projectDir.'/'.self::REPLACEMENT_FILE, '%PDF-1.4 stand-in');
     }
 
-    public function testTheRealDocumentIsServedWhileTheFeatureIsOff(): void
+    protected function tearDown(): void
     {
-        $this->featureManager->method('isActive')->with('upload_replacement')->willReturn(false);
+        $this->filesystem->remove($this->projectDir);
+    }
+
+    /**
+     * The point of the whole feature: a document uploaded while testing is served as it is.
+     */
+    public function testAnExistingUploadIsServedEvenWhileTheFeatureIsOn(): void
+    {
+        $this->featureManager->method('isActive')->with('upload_replacement')->willReturn(true);
+        $this->filesystem->dumpFile($this->projectDir.'/'.self::UPLOAD_PATH, '%PDF-1.4 the real one');
 
         $this->assertSame(
-            '/srv/app/uploads/invoice/i/invoice-1.pdf',
+            $this->projectDir.'/'.self::UPLOAD_PATH,
             $this->handler()->getResolvedUploadFilepath($this->upload())
         );
     }
 
-    public function testTheStandInIsServedWhileTheFeatureIsOn(): void
+    public function testAMissingUploadIsReplacedWhileTheFeatureIsOn(): void
     {
         $this->featureManager->method('isActive')->with('upload_replacement')->willReturn(true);
 
-        // the real file of the repository, so the path resolves to something that exists
-        $handler = $this->handler(\dirname(__DIR__, 3), self::REPLACEMENT_FILE);
-
         $this->assertSame(
-            \dirname(__DIR__, 3).'/'.self::REPLACEMENT_FILE,
-            $handler->getResolvedUploadFilepath($this->upload())
+            $this->projectDir.'/'.self::REPLACEMENT_FILE,
+            $this->handler()->getResolvedUploadFilepath($this->upload())
         );
     }
 
-    public function testAMissingStandInFailsInsteadOfServingTheRealDocument(): void
+    /**
+     * Without the feature a missing file stays missing - the callers turn that into their own
+     * error, e.g. the 404 of the download controller.
+     */
+    public function testAMissingUploadIsLeftAloneWhileTheFeatureIsOff(): void
+    {
+        $this->featureManager->method('isActive')->with('upload_replacement')->willReturn(false);
+
+        $this->assertSame(
+            $this->projectDir.'/'.self::UPLOAD_PATH,
+            $this->handler()->getResolvedUploadFilepath($this->upload())
+        );
+    }
+
+    public function testAMissingStandInIsAMisconfigurationAndFails(): void
     {
         $this->featureManager->method('isActive')->with('upload_replacement')->willReturn(true);
 
         $this->expectException(\RuntimeException::class);
-        $this->expectExceptionMessage('/srv/app/assets/pdf/nope.pdf');
+        $this->expectExceptionMessage($this->projectDir.'/assets/pdf/nope.pdf');
 
-        $this->handler(self::PROJECT_DIR, 'assets/pdf/nope.pdf')
-            ->getResolvedUploadFilepath($this->upload())
-        ;
+        $this->handler('assets/pdf/nope.pdf')->getResolvedUploadFilepath($this->upload());
     }
 
     /**
@@ -75,22 +102,20 @@ class UploadHandlerTest extends TestCase
         $this->featureManager->method('isActive')->with('upload_replacement')->willReturn(true);
 
         $this->assertSame(
-            '/srv/app/uploads/invoice/i/invoice-1.pdf',
+            $this->projectDir.'/'.self::UPLOAD_PATH,
             $this->handler()->getUploadFilepath($this->upload())
         );
     }
 
-    private function handler(
-        string $projectDir = self::PROJECT_DIR,
-        string $replacementFile = self::REPLACEMENT_FILE,
-    ): UploadHandler {
+    private function handler(string $replacementFile = self::REPLACEMENT_FILE): UploadHandler
+    {
         return new UploadHandler(
             $this->createMock(RequestStack::class),
             $this->createMock(OrphanageManager::class),
             $this->createMock(Sanitizer::class),
             new NullLogger(),
             $this->featureManager,
-            $projectDir,
+            $this->projectDir,
             $replacementFile,
         );
     }