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; } }