diff --git a/app/Http/Controllers/v1/ListController.php b/app/Http/Controllers/v1/ListController.php index bcccbec..a773d4c 100644 --- a/app/Http/Controllers/v1/ListController.php +++ b/app/Http/Controllers/v1/ListController.php @@ -7,9 +7,11 @@ use Illuminate\Http\Request; use Illuminate\Http\Response; use Illuminate\Support\Facades\DB; +use Illuminate\Log\Logger; use Laravel\Lumen\Routing\Controller; use Illuminate\Database\Query\Builder; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; +use Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException; class ListController extends Controller { @@ -28,6 +30,16 @@ class ListController extends Controller */ protected $idFilterName = 'material_ids'; + /** + * @var \Illuminate\Log\Logger + */ + protected $log; + + public function __construct(Logger $log) + { + $this->log = $log; + } + /** * @return mixed[] */ @@ -64,8 +76,12 @@ protected function getItems(Request $request, string $listId): array // the URL. $query->where(function ($query) use ($itemIds) { foreach ($itemIds as $itemId) { - $item = ListItem::createFromString($itemId); - $this->idQuery($query, $item, true); + try { + $item = ListItem::createFromString($itemId); + $this->idQuery($query, $item, true); + } catch (\InvalidArgumentException $e) { + throw new UnprocessableEntityHttpException($e->getMessage()); + } } }); } @@ -75,8 +91,12 @@ protected function getItems(Request $request, string $listId): array return $items->map(function (\stdClass $item_data) { $id = $item_data->material ?? $item_data->collection; - return ListItem::createFromString($id); - })->toArray(); + try { + return ListItem::createFromString($id); + } catch (\InvalidArgumentException $e) { + $this->log->error('Unable to process stored item id: ' . $e->getMessage()); + } + })->filter()->values()->toArray(); } public function hasItem(Request $request, string $listId, ListItem $item): Response diff --git a/app/ListItem.php b/app/ListItem.php index d863cc9..4dea7e7 100644 --- a/app/ListItem.php +++ b/app/ListItem.php @@ -2,8 +2,6 @@ namespace App; -use Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException; - final class ListItem { const DEFAULT_LIST_ID = 'default'; @@ -38,7 +36,7 @@ public static function createFromString(string $parameter): self $itemList = new static(); if (!preg_match('/(work-of:)?(\d+)-(\w+):(\w+)/', urldecode($parameter), $matches)) { - throw new UnprocessableEntityHttpException('Invalid pid: ' . $parameter); + throw new \InvalidArgumentException('Invalid pid: ' . $parameter); } [$itemList->fullId, $workOf, $itemList->agency, $itemList->base, $itemList->id] = $matches; diff --git a/app/Providers/RouteBindingServiceProvider.php b/app/Providers/RouteBindingServiceProvider.php index 9b24a8d..ddfa732 100644 --- a/app/Providers/RouteBindingServiceProvider.php +++ b/app/Providers/RouteBindingServiceProvider.php @@ -5,6 +5,7 @@ use App\ListItem; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use mmghv\LumenRouteBinding\RouteBindingServiceProvider as BaseServiceProvider; +use Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException; class RouteBindingServiceProvider extends BaseServiceProvider { @@ -24,7 +25,11 @@ public function boot() }); $binder->bind('item', function ($value): ListItem { - return ListItem::createFromString($value); + try { + return ListItem::createFromString($value); + } catch (\InvalidArgumentException $e) { + throw new UnprocessableEntityHttpException($e->getMessage()); + } }); } } diff --git a/infrastructure/docker/nginx/docker-entrypoint.sh b/infrastructure/docker/nginx/docker-entrypoint.sh index 128606d..978fa9f 100644 --- a/infrastructure/docker/nginx/docker-entrypoint.sh +++ b/infrastructure/docker/nginx/docker-entrypoint.sh @@ -20,11 +20,11 @@ if [ "$1" = "nginx" -o "$1" = "nginx-debug" ]; then case "$f" in *.envsh) if [ -x "$f" ]; then - echo >&3 "$0: Sourcing $f"; + entrypoint_log "$0: Sourcing $f"; . "$f" else # warn on shell scripts without exec bit - echo >&3 "$0: Ignoring $f, not executable"; + entrypoint_log "$0: Ignoring $f, not executable"; fi ;; *.sh) diff --git a/tests/features/get_list.feature b/tests/features/get_list.feature index 07732ba..4c00874 100644 --- a/tests/features/get_list.feature +++ b/tests/features/get_list.feature @@ -97,3 +97,20 @@ Feature: Fetching list | 123-katalog:3 | When checking if "321-basis:2" is on the list Then the system should return success + + Scenario: Invalid list items are not returned as a part of the list + Given a known user + And they have the following items on the list: + | material | + | 123-basis:1 | + # This item is invalid, and should not be part of the response. + | banana | + | 123-basis:2 | + Then fetching the list should return: + | material | + # The order of the list is intentionally reversed. The last item in the precondition should be the first in the + # response as items are returned with the most recently added first. + | 123-basis:2 | + | 123-basis:1 | + And the system should return success +