diff --git a/src/Service/Upload/UploadHandler.php b/src/Service/Upload/UploadHandler.php index 03c29f3..3fa8a1e 100644 --- a/src/Service/Upload/UploadHandler.php +++ b/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; } diff --git a/tests/Service/Upload/UploadHandlerTest.php b/tests/Service/Upload/UploadHandlerTest.php index f0785f4..5975b14 100644 --- a/tests/Service/Upload/UploadHandlerTest.php +++ b/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, ); }