Skip to content

Commit d5cfc8f

Browse files
author
MateuszKolankowski
committed
Improved error handling in DownloadController for file not found scenarios
1 parent b3c2901 commit d5cfc8f

2 files changed

Lines changed: 115 additions & 27 deletions

File tree

‎src/lib/MVC/Symfony/Controller/Content/DownloadController.php‎

Lines changed: 29 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,17 @@
88

99
use Ibexa\Bundle\IO\BinaryStreamResponse;
1010
use Ibexa\Contracts\Core\Repository\ContentService;
11+
use Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException as RepositoryNotFoundException;
1112
use Ibexa\Contracts\Core\Repository\Values\Content\Content;
1213
use Ibexa\Contracts\Core\Repository\Values\Content\Field;
1314
use Ibexa\Core\Base\Exceptions\InvalidArgumentException;
14-
use Ibexa\Core\Base\Exceptions\NotFoundException;
1515
use Ibexa\Core\Helper\TranslationHelper;
1616
use Ibexa\Core\IO\IOServiceInterface;
1717
use Ibexa\Core\MVC\Symfony\Controller\Controller;
1818
use Symfony\Component\HttpFoundation\Request;
1919
use Symfony\Component\HttpFoundation\ResponseHeaderBag;
20+
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
21+
use Throwable;
2022

2123
class DownloadController extends Controller
2224
{
@@ -48,15 +50,15 @@ public function downloadBinaryFileByIdAction(Request $request, int $contentId, i
4850
$versionNo = $request->query->has('version') ? $request->query->getInt('version') : null;
4951
$language = $request->query->has('inLanguage') ? $request->query->get('inLanguage') : null;
5052

51-
$content = $this->contentService->loadContent(
52-
$contentId,
53-
$language !== null ? [$language] : null,
54-
$versionNo,
55-
);
5653
try {
54+
$content = $this->contentService->loadContent(
55+
$contentId,
56+
$language !== null ? [$language] : null,
57+
$versionNo,
58+
);
5759
$field = $this->findFieldInContent($fieldId, $content);
58-
} catch (InvalidArgumentException $e) {
59-
throw new NotFoundException('File', $fieldId);
60+
} catch (RepositoryNotFoundException | InvalidArgumentException $e) {
61+
throw $this->createFileNotFoundException($e);
6062
}
6163

6264
return $this->downloadBinaryFileAction($contentId, $field->fieldDefIdentifier, $field->value->fileName, $request);
@@ -90,18 +92,22 @@ protected function findFieldInContent(int $fieldId, Content $content): Field
9092
*/
9193
public function downloadBinaryFileAction(int $contentId, string $fieldIdentifier, string $filename, Request $request): BinaryStreamResponse
9294
{
93-
if ($request->query->has('version')) {
94-
$version = (int) $request->query->get('version');
95-
if ($version <= 0) {
96-
throw new NotFoundException('File', $filename);
95+
try {
96+
if ($request->query->has('version')) {
97+
$version = (int) $request->query->get('version');
98+
if ($version <= 0) {
99+
throw $this->createFileNotFoundException();
100+
}
101+
$content = $this->contentService->loadContent($contentId, null, $version);
102+
} else {
103+
$content = $this->contentService->loadContent($contentId);
97104
}
98-
$content = $this->contentService->loadContent($contentId, null, $version);
99-
} else {
100-
$content = $this->contentService->loadContent($contentId);
105+
} catch (RepositoryNotFoundException $e) {
106+
throw $this->createFileNotFoundException($e);
101107
}
102108

103109
if ($content->contentInfo->isTrashed()) {
104-
throw new NotFoundException('File', $filename);
110+
throw $this->createFileNotFoundException();
105111
}
106112

107113
$field = $this->translationHelper->getTranslatedField(
@@ -110,14 +116,11 @@ public function downloadBinaryFileAction(int $contentId, string $fieldIdentifier
110116
$request->query->has('inLanguage') ? $request->query->get('inLanguage') : null
111117
);
112118
if (!$field instanceof Field) {
113-
throw new InvalidArgumentException(
114-
'$fieldIdentifier',
115-
"'{$fieldIdentifier}' field not present on content #{$content->contentInfo->id} '{$content->contentInfo->name}'"
116-
);
119+
throw $this->createFileNotFoundException();
117120
}
118121

119122
if ($field->value->fileName !== $filename) {
120-
throw new NotFoundException('File', $filename);
123+
throw $this->createFileNotFoundException();
121124
}
122125

123126
$response = new BinaryStreamResponse($this->ioService->loadBinaryFile($field->value->id), $this->ioService);
@@ -129,6 +132,11 @@ public function downloadBinaryFileAction(int $contentId, string $fieldIdentifier
129132

130133
return $response;
131134
}
135+
136+
private function createFileNotFoundException(?Throwable $previous = null): NotFoundHttpException
137+
{
138+
return new NotFoundHttpException('File not found', $previous);
139+
}
132140
}
133141

134142
class_alias(DownloadController::class, 'eZ\Publish\Core\MVC\Symfony\Controller\Content\DownloadController');

‎tests/lib/MVC/Symfony/Controller/Controller/Content/DownloadControllerTest.php‎

Lines changed: 86 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
use Ibexa\Contracts\Core\Repository\ContentService;
1313
use Ibexa\Contracts\Core\Repository\Values\Content\ContentInfo;
1414
use Ibexa\Contracts\Core\Repository\Values\Content\Field;
15-
use Ibexa\Core\Base\Exceptions\NotFoundException;
15+
use Ibexa\Core\Base\Exceptions\NotFoundException as BaseNotFoundException;
1616
use Ibexa\Core\FieldType\BinaryFile\Value as BinaryFileValue;
1717
use Ibexa\Core\Helper\TranslationHelper;
1818
use Ibexa\Core\IO\IOServiceInterface;
@@ -22,6 +22,7 @@
2222
use Ibexa\Core\Repository\Values\Content\VersionInfo;
2323
use PHPUnit\Framework\TestCase;
2424
use Symfony\Component\HttpFoundation\Request;
25+
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
2526

2627
/**
2728
* @covers \Ibexa\Core\MVC\Symfony\Controller\Content\DownloadController
@@ -104,8 +105,76 @@ public function testDownloadBinaryFileActionReturnsNotFoundWhenFilenameDoesNotMa
104105
->expects($this->never())
105106
->method('loadBinaryFile');
106107

107-
$this->expectException(NotFoundException::class);
108-
$this->createController()->downloadBinaryFileAction(42, 'file', 'SomeRandomText.txt', $request);
108+
$this->assertFileNotFound(function () use ($request): void {
109+
$this->createController()->downloadBinaryFileAction(42, 'file', 'SomeRandomText.txt', $request);
110+
});
111+
}
112+
113+
public function testDownloadBinaryFileActionReturnsNotFoundWhenFieldIdentifierDoesNotMatch(): void
114+
{
115+
$content = $this->createContent(393, 'New file');
116+
$request = new Request(['inLanguage' => 'eng-GB']);
117+
118+
$this->contentService
119+
->expects(self::once())
120+
->method('loadContent')
121+
->with(393)
122+
->willReturn($content);
123+
$this->translationHelper
124+
->expects(self::once())
125+
->method('getTranslatedField')
126+
->with($content, 'file5', 'eng-GB')
127+
->willReturn(null);
128+
$this->ioService
129+
->expects($this->never())
130+
->method('loadBinaryFile');
131+
132+
$this->assertFileNotFound(function () use ($request): void {
133+
$this->createController()->downloadBinaryFileAction(393, 'file5', 'snorelax_snooze.png', $request);
134+
});
135+
}
136+
137+
public function testDownloadBinaryFileActionReturnsNotFoundWhenContentDoesNotExist(): void
138+
{
139+
$request = new Request(['inLanguage' => 'eng-GB']);
140+
141+
$this->contentService
142+
->expects(self::once())
143+
->method('loadContent')
144+
->with(393)
145+
->willThrowException(new BaseNotFoundException('Content', 393));
146+
$this->translationHelper
147+
->expects($this->never())
148+
->method('getTranslatedField');
149+
$this->ioService
150+
->expects($this->never())
151+
->method('loadBinaryFile');
152+
153+
$this->assertFileNotFound(function () use ($request): void {
154+
$this->createController()->downloadBinaryFileAction(393, 'file', 'snorelax_snooze.png', $request);
155+
});
156+
}
157+
158+
public function testDownloadBinaryFileByIdActionReturnsNotFoundWhenFieldIdDoesNotMatch(): void
159+
{
160+
$content = $this->createContent();
161+
$request = new Request();
162+
163+
$this->contentService
164+
->expects(self::once())
165+
->method('loadContent')
166+
->with(42, null, null)
167+
->willReturn($content);
168+
$this->translationHelper
169+
->expects($this->never())
170+
->method('getTranslatedField');
171+
$this->ioService
172+
->expects($this->never())
173+
->method('loadBinaryFile');
174+
175+
$this->assertFileNotFound(function () use ($request): void {
176+
$this->createController()->downloadBinaryFileByIdAction($request, 42, 123);
177+
});
109178
}
110179

111180
private function createController(): DownloadController
@@ -117,11 +186,22 @@ private function createController(): DownloadController
117186
);
118187
}
119188

120-
private function createContent(): Content
189+
private function assertFileNotFound(callable $callback): void
190+
{
191+
try {
192+
$callback();
193+
self::fail(sprintf('Expected %s to be thrown.', NotFoundHttpException::class));
194+
} catch (NotFoundHttpException $e) {
195+
self::assertSame('File not found', $e->getMessage());
196+
}
197+
}
198+
199+
private function createContent(int $contentId = 42, string $contentName = 'Test content'): Content
121200
{
122201
return new Content([
123202
'internalFields' => [
124203
new Field([
204+
'id' => 7,
125205
'fieldDefIdentifier' => 'file',
126206
'languageCode' => 'eng-GB',
127207
'value' => new BinaryFileValue([
@@ -132,9 +212,9 @@ private function createContent(): Content
132212
],
133213
'versionInfo' => new VersionInfo([
134214
'contentInfo' => new ContentInfo([
135-
'id' => 42,
215+
'id' => $contentId,
136216
'mainLanguageCode' => 'eng-GB',
137-
'name' => 'Test content',
217+
'name' => $contentName,
138218
'status' => ContentInfo::STATUS_PUBLISHED,
139219
]),
140220
]),

0 commit comments

Comments
 (0)