From 3221008f90f41af57acec51b6c25cc93f79f2f18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= Date: Tue, 29 Sep 2026 14:23:36 +0200 Subject: [PATCH] refactor: move the bulk export to its own /api/exports resource /api/travels/{dateCode} accepts an arbitrary string, so /api/travels/export shared a namespace with every date code and stayed reachable only because byCode() carried a negative lookahead ruling that one string out. Nothing in the URL announced the constraint, and getting it wrong mis-routed silently: the first attempt widened the segment to `.+` and swallowed /api/travels/{dateId}/{hotelId}/{dateTo}/availability whole. /api/exports never takes a parameterised segment, the same shape as /api/contingents, so the collision stops existing instead of being guarded. TravelController returns byte-for-byte to its state on master, lookahead included, and the export moves to its own resource controller. The routing test now also asserts that nothing is reintroduced under the date code namespace. Co-Authored-By: Claude Opus 5 --- docs/api-consumer-guide.md | 26 +++++--- src/Controller/Api/ExportController.php | 66 +++++++++++++++++++ src/Controller/Api/TravelController.php | 46 ------------- ...ollerTest.php => ExportControllerTest.php} | 20 ++---- .../Api/TravelControllerRoutingTest.php | 52 +++++++++++---- 5 files changed, 130 insertions(+), 80 deletions(-) create mode 100644 src/Controller/Api/ExportController.php rename tests/Controller/Api/{TravelExportControllerTest.php => ExportControllerTest.php} (86%) diff --git a/docs/api-consumer-guide.md b/docs/api-consumer-guide.md index 9bafcce1..070ad898 100644 --- a/docs/api-consumer-guide.md +++ b/docs/api-consumer-guide.md @@ -12,7 +12,7 @@ A Symfony application exposing a read-mostly JSON API under the `/api` prefix. A - Enabled grants: **client_credentials**, **authorization_code** (PKCE required for public clients), **refresh_token**. Password and implicit grants are disabled. - Access token TTL is **10 minutes** for every grant. Do not hardcode it — read `expires_in` from the token response, it is authoritative and may be tuned. - A token can therefore expire part-way through a long page walk of `GET /api/travels/export`; re-request one on + A token can therefore expire part-way through a long page walk of `GET /api/exports/travels`; re-request one on 401 and resume at the page you were on. At the default `perPage=500` the whole export is a single page today. - Available scopes: `email`, `id`, `profile`, `roles`, `api`. Default scope if none requested: `email`. - Each granted scope maps to a role `ROLE_OAUTH2_` (e.g. scope `api` → `ROLE_OAUTH2_API`). @@ -50,9 +50,9 @@ There is no single error envelope; three shapes occur: - `{"error": "...", "type": "api_error"}` — upstream failures on some endpoints. **Caution:** on `/api/products` and `/api/travels/{dateId}/{hotelId}/{dateTo}/availability` this shape is returned with HTTP **200**, so clients must check for the presence of an `error` key, not only the status code. - `{"success": false, "message": "..."}` — the write endpoints (contact form, newsletter). -Validation failures on endpoints using mapped query strings / payloads (`/api/contingents/*`, `/api/travels/export`, `/api/newsletter-subscriptions`) return **400** with Symfony's constraint-violation body. Missing/invalid token yields **401**; insufficient scope yields **403**. +Validation failures on endpoints using mapped query strings / payloads (`/api/contingents/*`, `/api/exports/*`, `/api/newsletter-subscriptions`) return **400** with Symfony's constraint-violation body. Missing/invalid token yields **401**; insufficient scope yields **403**. -`GET /api/travels/export` deliberately uses real status codes throughout and never the 200-with-an-`error`-key shape: a failure there is a 5xx, so retry logic can key on the status code alone. +`GET /api/exports/travels` deliberately uses real status codes throughout and never the 200-with-an-`error`-key shape: a failure there is a 5xx, so retry logic can key on the status code alone. The HTTP methods listed below are the intended ones. Several read routes do not restrict methods server-side; clients should still use `GET` for them. @@ -139,19 +139,19 @@ This is a full overwrite, not a merge. --- -### Travels +### Bulk exports -#### `GET /api/travels` — scope `api` - -Index/mapping of all known travel dates. Object keyed by numeric travel id, each value `{id, code, label, dateFrom, dateTo, hotels: [...]}`. - -#### `GET /api/travels/export` — scope `api` +#### `GET /api/exports/travels` — scope `api` Bulk export of every travel date × hotel pair with its boarding points resolved, for clients that mirror the disposition locally. One request replaces a walk over `GET /api/travels` plus a per-pair detail call. This is the only paginated endpoint and the only one with a `meta` envelope. +It sits under `/api/exports`, not `/api/travels`, on purpose: `/api/travels/{dateCode}` accepts an +arbitrary string, so an export hung off that collection would share a namespace with every date code. +`/api/exports` never takes a parameterised segment, so there is nothing for it to collide with. + Query parameters: | Parameter | Type | Default | Notes | @@ -238,6 +238,14 @@ rest. `total` is the row count **after** skips, and `totalPages` is `ceil(total under one cache tag, so all three move together on an upstream sync rather than drifting between your calls. It is cached for 3 hours; `generatedAt` is when the cached copy was assembled, not when you requested it. +--- + +### Travels + +#### `GET /api/travels` — scope `api` + +Index/mapping of all known travel dates. Object keyed by numeric travel id, each value `{id, code, label, dateFrom, dateTo, hotels: [...]}`. + #### `GET /api/travels/{dateId}/{hotelId}` — scope `api` Single travel by **numeric** ids (`hotelId` optional). Route matches only when both segments are digits. diff --git a/src/Controller/Api/ExportController.php b/src/Controller/Api/ExportController.php new file mode 100644 index 00000000..0052a892 --- /dev/null +++ b/src/Controller/Api/ExportController.php @@ -0,0 +1,66 @@ +travelExportBuilder->build(); + + $total = count($export->rows); + + return $this->json([ + 'data' => array_slice($export->rows, ($query->page - 1) * $query->perPage, $query->perPage), + 'meta' => [ + 'page' => $query->page, + 'perPage' => $query->perPage, + 'total' => $total, + 'totalPages' => (int) ceil($total / $query->perPage), + 'skipped' => $export->skipped, + 'pickupsWithoutMasterData' => $export->pickupsWithoutMasterData, + 'generatedAt' => $export->generatedAt->format('c'), + ], + ]); + } +} diff --git a/src/Controller/Api/TravelController.php b/src/Controller/Api/TravelController.php index 452097e2..8b5a1128 100644 --- a/src/Controller/Api/TravelController.php +++ b/src/Controller/Api/TravelController.php @@ -6,15 +6,12 @@ use App\BusProNet\ApiClient; use App\BusProNet\Exception\ApiClientException; use App\BusProNet\Model\Travel; use App\BusProNet\Utility\DateCodeUtility; -use App\Model\TravelExportQuery; use App\Service\TravelDataProvider; -use App\Service\TravelExportBuilder; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\HttpFoundation\JsonResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Attribute\MapDateTime; -use Symfony\Component\HttpKernel\Attribute\MapQueryString; use Symfony\Component\Routing\Attribute\Route; use Symfony\Component\Security\Http\Attribute\IsGranted; @@ -25,7 +22,6 @@ class TravelController extends AbstractController public function __construct( private readonly ApiClient $apiClient, private readonly TravelDataProvider $travelDataService, - private readonly TravelExportBuilder $travelExportBuilder, ) { } @@ -37,45 +33,6 @@ class TravelController extends AbstractController return $this->json($mapping); } - /** - * Bulk export of every travel date x hotel pair with its pickups resolved. - * - * Declared before api_travel_single_code on purpose: that route puts no requirement on - * {dateCode}, so /api/travels/export would otherwise resolve to byCode('export'), fail - * the code lookup and answer 404 -- a failure that reads like an endpoint with no data - * rather than a missing route. - * - * The counters in meta describe the whole export, not the page, so they repeat - * unchanged on every page and a consumer can abort after the first one. - */ - #[Route(path: '/travels/export', name: 'api_travel_export', methods: ['GET'])] - public function export( - #[MapQueryString(validationFailedStatusCode: Response::HTTP_BAD_REQUEST)] - TravelExportQuery $query = new TravelExportQuery(), - ): JsonResponse { - // Deliberately not wrapped in a catch that answers 200 with an error body, the way - // hotelAvailability() below does: the consumer's retry logic keys on status codes, - // so a failure here has to surface as a 5xx. - $export = $this->travelExportBuilder->build(); - - $total = count($export->rows); - $totalPages = (int) ceil($total / $query->perPage); - $offset = ($query->page - 1) * $query->perPage; - - return $this->json([ - 'data' => array_slice($export->rows, $offset, $query->perPage), - 'meta' => [ - 'page' => $query->page, - 'perPage' => $query->perPage, - 'total' => $total, - 'totalPages' => $totalPages, - 'skipped' => $export->skipped, - 'pickupsWithoutMasterData' => $export->pickupsWithoutMasterData, - 'generatedAt' => $export->generatedAt->format('c'), - ], - ]); - } - #[Route( path: '/travels/{dateId}/{hotelId}', name: 'api_travel_single_id', @@ -99,9 +56,6 @@ class TravelController extends AbstractController #[Route( path: '/travels/{dateCode}/{hotelCode}', name: 'api_travel_single_code', - // [^/]+ and not .+ : the default segment class must be kept, or dateCode swallows - // slashes and this route shadows api_travel_hotel_availability below it. - requirements: ['dateCode' => '(?!export$)[^/]+'], defaults: ['hotelCode' => null], )] public function byCode(Request $request, string $dateCode, ?string $hotelCode = null): JsonResponse diff --git a/tests/Controller/Api/TravelExportControllerTest.php b/tests/Controller/Api/ExportControllerTest.php similarity index 86% rename from tests/Controller/Api/TravelExportControllerTest.php rename to tests/Controller/Api/ExportControllerTest.php index d6c6d41c..874f24ea 100644 --- a/tests/Controller/Api/TravelExportControllerTest.php +++ b/tests/Controller/Api/ExportControllerTest.php @@ -4,17 +4,15 @@ declare(strict_types=1); namespace App\Tests\Controller\Api; -use App\BusProNet\ApiClient; -use App\Controller\Api\TravelController; +use App\Controller\Api\ExportController; use App\Model\TravelExport; use App\Model\TravelExportQuery; -use App\Service\TravelDataProvider; use App\Service\TravelExportBuilder; use PHPUnit\Framework\TestCase; use Symfony\Component\DependencyInjection\Container; use Symfony\Component\HttpFoundation\Response; -class TravelExportControllerTest extends TestCase +class ExportControllerTest extends TestCase { public function testPaginatesAndReportsWholeExportFigures(): void { @@ -84,7 +82,7 @@ class TravelExportControllerTest extends TestCase /** * A builder failure must reach the consumer as a 5xx, never as the 200-with-an-error-body - * shape hotelAvailability() uses: the consumer's retry logic keys on the status code. + * shape TravelController::hotelAvailability() uses: the retry logic keys on the status code. */ public function testABuilderFailureIsNotDowngradedToATwoHundred(): void { @@ -93,7 +91,7 @@ class TravelExportControllerTest extends TestCase $this->expectException(\RuntimeException::class); - $this->controller($builder)->export(new TravelExportQuery()); + $this->controller($builder)->travels(new TravelExportQuery()); } /** @@ -114,20 +112,16 @@ class TravelExportControllerTest extends TestCase new \DateTimeImmutable('2026-09-29 03:20:00', new \DateTimeZone('Europe/Berlin')), )); - $response = $this->controller($builder)->export($query ?? new TravelExportQuery()); + $response = $this->controller($builder)->travels($query ?? new TravelExportQuery()); self::assertSame(Response::HTTP_OK, $response->getStatusCode()); return json_decode((string) $response->getContent(), true); } - private function controller(TravelExportBuilder $builder): TravelController + private function controller(TravelExportBuilder $builder): ExportController { - $controller = new TravelController( - $this->createStub(ApiClient::class), - $this->createStub(TravelDataProvider::class), - $builder, - ); + $controller = new ExportController($builder); // No 'serializer' service registered, so AbstractController::json() falls back to // JsonResponse -- which is what this endpoint produces in production anyway. diff --git a/tests/Controller/Api/TravelControllerRoutingTest.php b/tests/Controller/Api/TravelControllerRoutingTest.php index 1532a742..ea360a99 100644 --- a/tests/Controller/Api/TravelControllerRoutingTest.php +++ b/tests/Controller/Api/TravelControllerRoutingTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Tests\Controller\Api; +use App\Controller\Api\ExportController; use App\Controller\Api\TravelController; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; @@ -14,11 +15,13 @@ use Symfony\Component\Routing\Route; use Symfony\Component\Routing\RouteCollection; /** - * Guards the ordering and the segment requirements of the /api/travels routes. + * Guards the /api/travels namespace against a sibling route being swallowed by byCode(). * - * Both are load-bearing and neither fails loudly: a path that falls through to the wrong - * route answers 404 with a body that reads like an endpoint holding no data. The collection - * is built by reflecting over the real attributes, so this breaks if the declarations move. + * `/api/travels/{dateCode}` accepts an arbitrary string, so anything else placed under + * `/api/travels/` competes with every date code, and a path falling through to the wrong + * route answers 404 with a body that reads like an endpoint holding no data. The bulk + * export lives at `/api/exports/travels` for exactly this reason. The collection is built + * by reflecting over the real attributes, so this breaks if the declarations change. */ class TravelControllerRoutingTest extends TestCase { @@ -28,9 +31,9 @@ class TravelControllerRoutingTest extends TestCase public static function pathProvider(): array { return [ - 'the export route wins over the code lookup' => ['/api/travels/export', 'api_travel_export'], - // Regression: a dateCode requirement of `.+` instead of `[^/]+` widens the segment - // across slashes, and this four-segment path is then swallowed by byCode(). + 'the export lives outside the travels namespace' => ['/api/exports/travels', 'api_exports_travels'], + // Regression: a dateCode requirement of `.+` instead of the default `[^/]+` widens + // the segment across slashes, and this four-segment path is then swallowed by byCode(). 'availability is not shadowed by the code lookup' => [ '/api/travels/12794/225444/2026-12-30/availability', 'api_travel_hotel_availability', @@ -62,20 +65,47 @@ class TravelControllerRoutingTest extends TestCase ); } + /** + * The export must stay out of `/api/travels/`, where it would be one date code away from + * ambiguity and would need a negative lookahead on byCode() to be reachable at all. + */ + public function testNothingButTravelLookupsLivesUnderTheTravelsNamespace(): void + { + foreach ($this->collection()->all() as $name => $route) { + self::assertStringNotContainsString( + '/api/travels/export', + $route->getPath(), + $name.' must not sit in the date code namespace', + ); + } + } + /** * Reflects the controller's own #[Route] attributes, in declaration order. */ private function collection(): RouteCollection { - $class = new \ReflectionClass(TravelController::class); + $collection = new RouteCollection(); + + foreach ([TravelController::class, ExportController::class] as $controller) { + $this->addRoutes($collection, $controller); + } + + return $collection; + } + + /** + * @param class-string $controller + */ + private function addRoutes(RouteCollection $collection, string $controller): void + { + $class = new \ReflectionClass($controller); $prefix = ''; foreach ($class->getAttributes(RouteAttribute::class) as $attribute) { $prefix = (string) $attribute->newInstance()->path; } - $collection = new RouteCollection(); - foreach ($class->getMethods(\ReflectionMethod::IS_PUBLIC) as $method) { foreach ($method->getAttributes(RouteAttribute::class) as $attribute) { $route = $attribute->newInstance(); @@ -91,7 +121,5 @@ class TravelControllerRoutingTest extends TestCase )); } } - - return $collection; } }