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 <[email protected]>
This commit is contained in:
1 parent
b27639c7fa
commit
3221008f90
5 files changed
+130
-80
No files matched your search
@@ -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_<SCOPE_UPPERCASE>` (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.
|
||||
|
||||
@@ -0,0 +1,66 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Controller\Api;
|
||||
|
||||
use App\Model\TravelExportQuery;
|
||||
use App\Service\TravelExportBuilder;
|
||||
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
|
||||
use Symfony\Component\HttpFoundation\JsonResponse;
|
||||
use Symfony\Component\HttpFoundation\Response;
|
||||
use Symfony\Component\HttpKernel\Attribute\MapQueryString;
|
||||
use Symfony\Component\Routing\Attribute\Route;
|
||||
use Symfony\Component\Security\Http\Attribute\IsGranted;
|
||||
|
||||
/**
|
||||
* Bulk exports, for consumers that mirror our data rather than query it per row.
|
||||
*
|
||||
* Deliberately its own resource rather than a verb hung off the collection it exports.
|
||||
* `/api/travels/{dateCode}` accepts an arbitrary string, so an `/api/travels/export` would
|
||||
* share a namespace with every date code and could only be kept apart from one by a
|
||||
* negative lookahead on the sibling route -- a constraint nothing in the URL announces and
|
||||
* which silently mis-routes when it is wrong. Under `/api/exports` the collision does not
|
||||
* exist: this prefix never takes a parameterised segment, exactly like `/api/contingents`.
|
||||
*/
|
||||
#[Route('/api/exports')]
|
||||
#[IsGranted('ROLE_OAUTH2_API')]
|
||||
class ExportController extends AbstractController
|
||||
{
|
||||
public function __construct(
|
||||
private readonly TravelExportBuilder $travelExportBuilder,
|
||||
) {
|
||||
}
|
||||
|
||||
/**
|
||||
* Every travel date x hotel pair with its pickups resolved, paginated.
|
||||
*
|
||||
* 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', name: 'api_exports_travels', methods: ['GET'])]
|
||||
public function travels(
|
||||
#[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
|
||||
// TravelController::hotelAvailability() 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);
|
||||
|
||||
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'),
|
||||
],
|
||||
]);
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
+7
-13
@@ -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.
|
||||
@@ -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;
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user