diff --git a/README.md b/README.md index 10f6e99..6253ec1 100644 --- a/README.md +++ b/README.md @@ -1274,13 +1274,18 @@ a leer ese documento y no este código. Dos cosas lo sujetan: -**Un test de contrato** ([`PublicApiContractTest`](tests/Feature/Api/PublicApiContractTest.php)) -que llama a cada endpoint público con datos sembrados y compara la respuesta con -el esquema del `openapi.yaml`, clave a clave. La regla es asimétrica a -propósito: **todo lo que la API manda tiene que estar documentado** —siempre—, y -lo que el documento promete tiene que llegar en los endpoints de detalle, salvo -lo que está marcado `nullable`, que es como este documento dice «puede no -estar». +**Dos tests de contrato**, uno por mitad de la API, comparten la comparación en +sí ([`VerifiesOpenApiContract`](tests/VerifiesOpenApiContract.php)) porque la +pregunta es la misma dos veces: si la forma que el documento promete es la que +la API manda. Lo que cambia es todo lo que rodea la pregunta, no la +comparación. + +[`PublicApiContractTest`](tests/Feature/Api/PublicApiContractTest.php) llama a +cada endpoint público con datos sembrados y compara la respuesta con el esquema +del `openapi.yaml`, clave a clave. La regla es asimétrica a propósito: **todo lo +que la API manda tiene que estar documentado** —siempre—, y lo que el documento +promete tiene que llegar en los endpoints de detalle, salvo lo que está marcado +`nullable`, que es como este documento dice «puede no estar». Al escribirlo aparecieron cuatro desajustes que llevaban ahí un tiempo: `meta_title` y `meta_description` sin documentar en proyectos y servicios, @@ -1292,6 +1297,19 @@ era la API: el alt vive dentro de la imagen, que es donde lo necesita un `` y dos nombres para el mismo texto es cómo un frontend acaba leyendo el que resulta ser el accidental. +[`AdminApiContractTest`](tests/Feature/Api/AdminApiContractTest.php) hace lo +mismo con `/v1/*`, autenticado como `super_admin` para que un permiso de menos +nunca se confunda con una forma equivocada, y añade una comprobación que la +pública no necesita: el **sobre de paginación** (`links`, `meta`) de cada +listado contra lo documentado, y que `/v1/roles` —el único listado que Laravel +no pagina— no lleve ninguno de los dos. Escribirlo encontró cuatro desajustes +más: `featured_image_alt` sin documentar en absoluto en la respuesta de página +(la petición de escritura sí lo tenía, con otra forma), `permissions` de +usuario documentado solo en prosa en vez de con `nullable: true`, y el sobre de +paginación real de Laravel —que manda `from`, `to`, `path` y un `links` de +primer nivel— documentado con la mitad de esas claves y sin ese `links` en +absoluto. + **Tipos generados** para el otro lado: ```bash @@ -1346,7 +1364,7 @@ php artisan site:maintenance --up ## Tests ```bash -php artisan test # 391 tests, 1121 aserciones +php artisan test # 394 tests, 1128 aserciones ``` En cada push y cada pull request, [`.github/workflows/tests.yml`](.github/workflows/tests.yml) @@ -1569,7 +1587,7 @@ docker compose -f docker-compose.test.yml run --rm test Levanta un Postgres en memoria, corre la suite entera contra él y lo tira. Es la comprobación que convierte «debería funcionar» en «funciona», y conviene hacerla -antes del primer despliegue. **391 tests en verde contra Postgres 17.** +antes del primer despliegue. **394 tests en verde contra Postgres 17.** Usa una etapa propia de la imagen: la de producción se construye con `--no-dev` y ahí no está ni el runner de tests. diff --git a/public/openapi.yaml b/public/openapi.yaml index a2e61e1..0bbb129 100644 --- a/public/openapi.yaml +++ b/public/openapi.yaml @@ -1198,6 +1198,7 @@ paths: data: type: array items: { $ref: "#/components/schemas/User" } + links: { $ref: "#/components/schemas/PaginationLinks" } meta: { $ref: "#/components/schemas/PaginationMeta" } "403": { $ref: "#/components/responses/Forbidden" } @@ -2075,8 +2076,16 @@ components: deleted_at: { type: string, format: date-time, nullable: true } category: { $ref: "#/components/schemas/Category" } author: { $ref: "#/components/schemas/User" } + featured_image_alt: + type: string + nullable: true + description: | + What the image shows, resolved into the language above — unlike + `PageWrite`'s field of the same name, which carries every + language at once. translations: type: object + nullable: true description: Present only when `?with=translations` was passed. additionalProperties: type: object @@ -2128,6 +2137,13 @@ components: pages_count: { type: integer } created_at: { type: string, format: date-time } updated_at: { type: string, format: date-time } + translations: + type: object + nullable: true + description: Present only when `?with=translations` was passed. + additionalProperties: + type: object + additionalProperties: { type: string } CategoryWrite: type: object @@ -2160,6 +2176,7 @@ components: permissions: type: array items: { type: string } + nullable: true description: Effective local permissions. Returned by `/v1/me` only. remote_roles: type: array @@ -2221,18 +2238,45 @@ components: PaginationMeta: type: object + description: | + Laravel's own paginator shape, sent as-is — nothing on this side + trims it down, so this describes exactly what `->paginate()` sends + rather than a curated subset of it. properties: current_page: { type: integer } + from: { type: integer, nullable: true, description: Null on an empty page. } last_page: { type: integer } + links: + type: array + description: One entry per page number shown by a "1 2 3 … 9" control, plus prev/next. + items: + type: object + properties: + url: { type: string, nullable: true } + label: { type: string, description: "Already translated — e.g. «Anterior», not «Previous»." } + page: { type: integer, nullable: true } + active: { type: boolean } + path: { type: string } per_page: { type: integer } + to: { type: integer, nullable: true, description: Null on an empty page. } total: { type: integer } + PaginationLinks: + type: object + description: The four addresses a client actually navigates with, ready to use verbatim. + properties: + first: { type: string } + last: { type: string } + prev: { type: string, nullable: true } + next: { type: string, nullable: true } + PageCollection: type: object properties: data: type: array items: { $ref: "#/components/schemas/Page" } + links: { $ref: "#/components/schemas/PaginationLinks" } meta: { $ref: "#/components/schemas/PaginationMeta" } CategoryCollection: @@ -2241,4 +2285,5 @@ components: data: type: array items: { $ref: "#/components/schemas/Category" } + links: { $ref: "#/components/schemas/PaginationLinks" } meta: { $ref: "#/components/schemas/PaginationMeta" } diff --git a/tests/Feature/Api/AdminApiContractTest.php b/tests/Feature/Api/AdminApiContractTest.php new file mode 100644 index 0000000..692885d --- /dev/null +++ b/tests/Feature/Api/AdminApiContractTest.php @@ -0,0 +1,179 @@ +loadOpenApiSpec(); + + $this->seed(RolePermissionSeeder::class); + + // super_admin, so a permission gap never masquerades as a shape + // mismatch: every request here must reach the resource, not a 403. + $this->caller = User::factory()->create(); + $this->caller->syncRoles(['super_admin']); + + $this->withHeaders($this->apiHeadersFor($this->caller)); + } + + /** + * @return array + */ + public static function paginatedEndpoints(): array + { + return [ + 'pages' => ['path' => '/v1/pages'], + 'categories' => ['path' => '/v1/categories'], + 'users' => ['path' => '/v1/users'], + ]; + } + + #[Test] + public function every_admin_record_answers_in_the_shape_it_documents(): void + { + $page = Page::factory()->create(); + $category = Category::factory()->create(); + + // One request, one documented path — the URL a real caller hits and + // the path key that describes it, side by side rather than derived + // from one another, because guessing the second from the first is + // exactly the kind of cleverness that breaks quietly. + $cases = [ + 'pages' => ['/v1/pages', '/v1/pages'], + 'page' => ["/v1/pages/{$page->id}", '/v1/pages/{page}'], + 'categories' => ['/v1/categories', '/v1/categories'], + 'category' => ["/v1/categories/{$category->id}", '/v1/categories/{category}'], + 'users' => ['/v1/users', '/v1/users'], + 'user' => ["/v1/users/{$this->caller->id}", '/v1/users/{user}'], + 'me' => ['/v1/me', '/v1/me'], + 'roles' => ['/v1/roles', '/v1/roles'], + ]; + + $problems = []; + + foreach ($cases as $name => [$url, $documentedPath]) { + $response = $this->getJson('/api'.$url); + + if ($response->status() !== 200) { + $problems[] = "{$name}: answered {$response->status()}"; + + continue; + } + + $payload = $response->json('data'); + $record = array_is_list((array) $payload) ? ($payload[0] ?? null) : $payload; + + if (! is_array($record)) { + $problems[] = "{$name}: no record to compare against"; + + continue; + } + + $schema = $this->recordSchema($documentedPath); + + foreach (array_diff(array_keys($record), $this->documentedKeys($schema)) as $undocumented) { + $problems[] = "{$name}: sends `{$undocumented}`, which the specification does not describe"; + } + + foreach (array_diff($this->alwaysPresent($schema), array_keys($record)) as $missing) { + $problems[] = "{$name}: documents `{$missing}`, which it does not send"; + } + } + + $this->assertSame([], $problems, "The admin API and its specification disagree:\n".implode("\n", $problems)); + } + + /** + * The pagination envelope every listing here uses — `data`, `links`, + * `meta` — checked the same way the record inside it is: nothing sent + * that is not documented. + * + * Worth its own test rather than folding into the one above: the record + * comparison already proves the endpoints work at all, and a failure here + * points at a completely different place to fix — the collection schema, + * not a `Resource` class. + */ + #[Test] + public function the_pagination_envelope_matches_what_is_documented(): void + { + Page::factory()->create(); + Category::factory()->create(); + + $problems = []; + + foreach (self::paginatedEndpoints() as $name => $endpoint) { + $response = $this->getJson('/api'.$endpoint['path'])->assertOk(); + + $envelope = $this->responseSchema($endpoint['path']); + $documented = $this->documentedKeys($envelope); + + foreach (array_diff(array_keys($response->json()), $documented) as $undocumented) { + $problems[] = "{$name}: envelope sends `{$undocumented}`, which the specification does not describe"; + } + + $metaSchema = $this->resolve($envelope['properties']['meta'] ?? []); + $meta = (array) $response->json('meta'); + + foreach (array_diff(array_keys($meta), $this->documentedKeys($metaSchema)) as $undocumented) { + $problems[] = "{$name}: meta sends `{$undocumented}`, which the specification does not describe"; + } + + foreach (array_diff($this->alwaysPresent($metaSchema), array_keys($meta)) as $missing) { + $problems[] = "{$name}: meta documents `{$missing}`, which it does not send"; + } + } + + $this->assertSame([], $problems, "The pagination envelope and its specification disagree:\n".implode("\n", $problems)); + } + + /** + * `/v1/roles` is the one listing here Laravel never paginates — + * `RoleController::index()` calls `->get()`, not `->paginate()` — so + * proving it carries no stray `meta`/`links` is the other half of the + * envelope test above. + */ + #[Test] + public function the_unpaginated_roles_listing_carries_no_pagination_envelope(): void + { + $response = $this->getJson('/api/v1/roles')->assertOk(); + + $this->assertSame(['data'], array_keys($response->json())); + } +} diff --git a/tests/Feature/Api/PublicApiContractTest.php b/tests/Feature/Api/PublicApiContractTest.php index 82ab685..7eb7c9a 100644 --- a/tests/Feature/Api/PublicApiContractTest.php +++ b/tests/Feature/Api/PublicApiContractTest.php @@ -15,8 +15,8 @@ use App\Models\Technology; use App\Models\Testimonial; use Illuminate\Foundation\Testing\RefreshDatabase; use PHPUnit\Framework\Attributes\Test; -use Symfony\Component\Yaml\Yaml; use Tests\TestCase; +use Tests\VerifiesOpenApiContract; /** * The public API answers with what the specification says it answers with. @@ -34,12 +34,7 @@ use Tests\TestCase; */ class PublicApiContractTest extends TestCase { - use RefreshDatabase; - - /** - * @var array - */ - protected array $spec; + use RefreshDatabase, VerifiesOpenApiContract; protected function setUp(): void { @@ -47,7 +42,7 @@ class PublicApiContractTest extends TestCase $this->withHeaders(['Accept-Language' => 'es']); - $this->spec = (array) Yaml::parseFile(public_path('openapi.yaml')); + $this->loadOpenApiSpec(); } /** @@ -171,14 +166,15 @@ class PublicApiContractTest extends TestCase continue; } - $documented = $this->documentedKeys($endpoint['path']); + $schema = $this->recordSchema($endpoint['path']); + $documented = $this->documentedKeys($schema); foreach (array_diff(array_keys($record), $documented) as $undocumented) { $problems[] = "{$name}: sends `{$undocumented}`, which the specification does not describe"; } if ($endpoint['detail']) { - foreach (array_diff($this->alwaysPresent($endpoint['path']), array_keys($record)) as $missing) { + foreach (array_diff($this->alwaysPresent($schema), array_keys($record)) as $missing) { $problems[] = "{$name}: documents `{$missing}`, which it does not send"; } } @@ -186,90 +182,4 @@ class PublicApiContractTest extends TestCase $this->assertSame([], $problems, "The public API and its specification disagree:\n".implode("\n", $problems)); } - - /** - * The field names the specification promises for one endpoint's records. - * - * @return array - */ - protected function documentedKeys(string $path): array - { - return array_keys($this->recordSchema($path)['properties'] ?? []); - } - - /** - * The schema of a single record, whichever way the endpoint wraps it. - * - * @return array - */ - protected function recordSchema(string $path): array - { - $schema = $this->resolve( - $this->spec['paths'][$path]['get']['responses'][200]['content']['application/json']['schema'] ?? [], - ); - - $data = $this->resolve($schema['properties']['data'] ?? []); - - // A listing documents its records under `items`; a detail endpoint - // describes them directly. - if (($data['type'] ?? null) === 'array') { - $data = $this->resolve($data['items'] ?? []); - } - - return $data; - } - - /** - * The documented fields a detail endpoint must always answer with. - * - * Anything the specification marks `nullable` is left out of this half of - * the check. That is the vocabulary this document already uses for a field - * that may not be there — a member's `role` exists only while they are - * nested under a project, and the schema says so. The other direction - * stays strict for every field either way: whatever the API sends has to - * be described, nullable or not. - * - * @return array - */ - protected function alwaysPresent(string $path): array - { - $record = $this->recordSchema($path); - - return array_keys(array_filter( - (array) ($record['properties'] ?? []), - fn (array $property) => ($property['nullable'] ?? false) !== true, - )); - } - - /** - * Follow `$ref` and flatten `allOf`, which is how the specification says - * "this, plus a couple of fields" without repeating a schema. - * - * @param array $schema - * @return array - */ - protected function resolve(array $schema): array - { - if (isset($schema['$ref'])) { - $name = basename((string) $schema['$ref']); - - return $this->resolve((array) ($this->spec['components']['schemas'][$name] ?? [])); - } - - if (isset($schema['allOf'])) { - $merged = ['type' => 'object', 'properties' => []]; - - foreach ((array) $schema['allOf'] as $part) { - $resolved = $this->resolve((array) $part); - $merged['properties'] = [ - ...$merged['properties'], - ...($resolved['properties'] ?? []), - ]; - } - - return [...$schema, ...$merged]; - } - - return $schema; - } } diff --git a/tests/VerifiesOpenApiContract.php b/tests/VerifiesOpenApiContract.php new file mode 100644 index 0000000..c9fba67 --- /dev/null +++ b/tests/VerifiesOpenApiContract.php @@ -0,0 +1,131 @@ + + */ + protected array $spec; + + protected function loadOpenApiSpec(): void + { + $this->spec = (array) Yaml::parseFile(public_path('openapi.yaml')); + } + + /** + * The field names a schema promises, one level deep — enough for a flat + * comparison against `array_keys()` on a decoded JSON object. + * + * @return array + */ + protected function documentedKeys(array $schema): array + { + return array_keys($this->resolve($schema)['properties'] ?? []); + } + + /** + * The documented fields that must always be present. + * + * Anything the specification marks `nullable` is left out of this half of + * the check — that is the vocabulary this document already uses for a + * field that may not be there, whether because the value itself can be + * null or because it only appears down one particular route. The other + * direction stays strict regardless: whatever is actually sent has to be + * described, nullable or not. + * + * @return array + */ + protected function alwaysPresent(array $schema): array + { + $resolved = $this->resolve($schema); + + return array_keys(array_filter( + (array) ($resolved['properties'] ?? []), + fn (array $property) => ($property['nullable'] ?? false) !== true, + )); + } + + /** + * The schema of one record inside a `{"data": ...}` envelope, whichever + * way the endpoint wraps it — a bare object for a detail endpoint, or the + * `items` of an array for a listing. + * + * @return array + */ + protected function recordSchema(string $path, string $method = 'get', int $status = 200): array + { + $schema = $this->resolve( + $this->spec['paths'][$path][$method]['responses'][$status]['content']['application/json']['schema'] ?? [], + ); + + $data = $this->resolve($schema['properties']['data'] ?? []); + + if (($data['type'] ?? null) === 'array') { + $data = $this->resolve($data['items'] ?? []); + } + + return $data; + } + + /** + * Everything documented for one path's response, `data` included — for + * comparing against the envelope as a whole rather than only the record + * inside it. + * + * @return array + */ + protected function responseSchema(string $path, string $method = 'get', int $status = 200): array + { + return $this->resolve( + $this->spec['paths'][$path][$method]['responses'][$status]['content']['application/json']['schema'] ?? [], + ); + } + + /** + * Follow `$ref` and flatten `allOf`, which is how the specification says + * "this, plus a couple of fields" without repeating a schema. + * + * @param array $schema + * @return array + */ + protected function resolve(array $schema): array + { + if (isset($schema['$ref'])) { + $name = basename((string) $schema['$ref']); + + return $this->resolve((array) ($this->spec['components']['schemas'][$name] ?? [])); + } + + if (isset($schema['allOf'])) { + $merged = ['type' => 'object', 'properties' => []]; + + foreach ((array) $schema['allOf'] as $part) { + $resolved = $this->resolve((array) $part); + $merged['properties'] = [ + ...$merged['properties'], + ...($resolved['properties'] ?? []), + ]; + } + + return [...$schema, ...$merged]; + } + + return $schema; + } +}