diff --git a/ROADMAP.md b/ROADMAP.md index ad2afb3..e5fa869 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -69,6 +69,35 @@ and `php -l` gates, before anything ships. 400 instead of 422), and the error-bag-to-schema mapping is application semantics the spec does not encode. The generator contributes the typed half (error component schemas already generate Data classes); the docs guide `guides/validation-errors` holds the bootstrap renderer recipe. + **Extended, not re-litigated, by the inlined `ApiError` throwable (issue #168):** a `final` + exception, inlined into the consumer's own `\Support` namespace exactly like `RespondsWithStatus`, + that carries any generated Data class (or other Responsable/Arrayable/JsonSerializable value) plus + an HTTP status and self-renders through Laravel's `render(Request): Response` exception-handler + hook (no `bootstrap/app.php` registration needed). It is a schema-agnostic CARRIER, not a + renderer: it never inspects or maps a spec's error shape itself (that mapping is still the + documented recipe's job), so this decision's core stance is unchanged. It solves a narrower, + different problem than #79: the generated abstract controller method's return type is always the + operation's success DTO (by design, error responses are never inspected for typing), so a concrete + controller that must answer a spec-declared error status previously had to hand-roll a helper that + RETURNS a JsonResponse, which does not satisfy the success return type. Throwing (never returning) + satisfies any return type, so `ApiError::notFound($errorData)` and its sibling named-status + factories give that throw an ergonomic, typed home without inventing a new generated renderer. + **Further extended by the generated `Errors` factory layer (issue #168):** the ApiError + carrier is now complemented by a GENERATED per-operation factory, `Errors` (one static + method per concrete 4xx/5xx error response whose JSON schema resolves to a named component object; v1 + scope, inline-object, non-object, unresolvable, and default/wildcard error slots are documented + residuals). An operation that DOES get a factory warns once per declared error slot it could not + turn into a method; an operation with NO qualifying error slot generates no factory and stays + silent (so specs whose error bodies are entirely non-objects or `default` catch-alls are not + flooded with warnings). + `throw GetPetByIdErrors::notFound(message: '...');` is now the PRIMARY, RECOMMENDED pattern for a + spec-declared error whose operation has a generated factory; `ApiError`'s own named factories and + general constructor remain a documented escape hatch for anything a generated factory does not + cover (a cross-cutting error the spec does not declare per-operation, an operation whose error + responses do not qualify for flattening, or a status the spec's per-operation responses map omits). + Neither layer maps Laravel's error bag into a spec shape or inspects a spec's error schema on the + developer's behalf beyond flattening an ALREADY-NAMED schema's own constructor; decision #11's core + stance (no generated renderer) remains unchanged by either layer. 12. **Config diet: the generator is opinionated about style.** New options must be environment facts the generator cannot know (paths, middleware names, FQCNs) or correctness escape hatches, never style preferences. Style is the generator's job. The #93 (`--group-by-tag` / diff --git a/composer-require-checker.json b/composer-require-checker.json index d99eb5d..e618ab8 100644 --- a/composer-require-checker.json +++ b/composer-require-checker.json @@ -3,11 +3,14 @@ "config", "config_path", "base_path", + "response", "Illuminate\\Console\\Command", "Illuminate\\Support\\ServiceProvider", "Illuminate\\Support\\Arr", "Illuminate\\Contracts\\Validation\\ValidationRule", "Illuminate\\Contracts\\Validation\\DataAwareRule", + "Illuminate\\Contracts\\Support\\Arrayable", + "Illuminate\\Contracts\\Support\\Responsable", "Illuminate\\Http\\Request", "Symfony\\Component\\HttpFoundation\\Response" ] diff --git a/docs/src/content/docs/guides/runtime-coupling.mdx b/docs/src/content/docs/guides/runtime-coupling.mdx index 4e179fb..34175b5 100644 --- a/docs/src/content/docs/guides/runtime-coupling.mdx +++ b/docs/src/content/docs/guides/runtime-coupling.mdx @@ -1,6 +1,6 @@ --- title: Runtime coupling of generated code -description: Generated code used to import eight support classes from the generator package, making it a runtime dependency. The 1.0.0 question, keep that dependency or make generation self-contained, is decided and shipped, Option B (inline into the consumer's output), with the analysis and tradeoffs that led there. +description: Generated code used to import nine support classes from the generator package, making it a runtime dependency. The 1.0.0 question, keep that dependency or make generation self-contained, is decided and shipped, Option B (inline into the consumer's output), with the analysis and tradeoffs that led there. --- import { Aside } from '@astrojs/starlight/components' @@ -21,6 +21,7 @@ The generated Data classes stand alone in your repo. When a class needs a rule o - `App\Data\Support\MapObjectTransformer` (every `additionalProperties` map property, via `#[WithTransformer(...)]`) - `App\Data\Support\NoUnknownPropertiesRule` (the default `additionalProperties: false` enforcement) - `App\Data\Support\RespondsWithStatus` (the route middleware that enforces non-200 declared success status codes) +- `App\Data\Support\ApiError` (the self-rendering throwable for a spec-declared error response) (The `App\Data` prefix mirrors whatever Data namespace you configured: the support namespace is always the Data namespace plus a `\Support` suffix.) @@ -32,14 +33,14 @@ The result is the headline this decision delivers: **`codewithagents/openapi-lar The project's stated philosophy is [you own the output](/philosophy): readable PHP in your repo that keeps working even if you stop using the generator. The runtime coupling (now removed) dented that in two ways: -- **It was not fully owned.** Eight classes that your generated `rules()` depended on lived in `vendor/`, outside the code you committed and review in diffs. +- **It was not fully owned.** Nine classes that your generated code depended on lived in `vendor/`, outside the code you committed and review in diffs. - **A `composer update` could change runtime behavior under committed code.** Because the support classes were versioned with the generator, upgrading the generator could change how your already-generated, already-committed classes validate and serialize, without you regenerating anything. That was the exact silent-change surface the [versioning policy](/guides/versioning-policy) pins down. Inlining closes it: a rule changes only when you regenerate and review the diff. -`spatie/laravel-data` itself is a genuine runtime dependency under **every** option below, that is unavoidable and expected, the generated classes *are* laravel-data classes. The decision below concerned only the eight `openapi-laravel`-owned support classes, which now live in your own `Support` namespace. +`spatie/laravel-data` itself is a genuine runtime dependency under **every** option below, that is unavoidable and expected, the generated classes *are* laravel-data classes. The decision below concerned only the nine `openapi-laravel`-owned support classes, which now live in your own `Support` namespace. ## The 1.0.0 question -This had to settle **before** 1.0.0, because the import lines are part of the frozen output format. Moving the namespace later (for example from `CodeWithAgents\OpenApiLaravel\Support\...` to `App\Data\Support\...`) is a breaking output change, exactly what the 1.0.0 freeze is meant to prevent. The discriminator-aware cast ([#38](https://github.com/codewithagents/openapi-laravel/issues/38)) has since **shipped using spatie's native `PropertyMorphableData`** (an abstract morphable base plus `morph()`), so it added **no** new runtime support class. The inlined set under Option B is therefore the eight classes above (including `NoUnknownPropertiesRule`, emitted whenever a closed object is present, which is now the default), not a growing list driven by #38. +This had to settle **before** 1.0.0, because the import lines are part of the frozen output format. Moving the namespace later (for example from `CodeWithAgents\OpenApiLaravel\Support\...` to `App\Data\Support\...`) is a breaking output change, exactly what the 1.0.0 freeze is meant to prevent. The discriminator-aware cast ([#38](https://github.com/codewithagents/openapi-laravel/issues/38)) has since **shipped using spatie's native `PropertyMorphableData`** (an abstract morphable base plus `morph()`), so it added **no** new runtime support class. The inlined set under Option B is therefore the nine classes above (including `NoUnknownPropertiesRule`, emitted whenever a closed object is present, which is now the default), not a growing list driven by #38. ## Option A: keep the runtime dependency (status quo) @@ -56,7 +57,7 @@ Generated code keeps importing the support classes from the generator package. ` - **Generated code is not self-contained.** "Stop using the generator and your code still works" is false: remove the package and the classes that import `Support\...` break. - **Silent runtime-behavior changes on upgrade**, as above. This is the strongest argument against A. -- **The generator is a heavier dependency than it needs to be.** Consumers pull the whole generator (parser, emitter, symfony/yaml) into production just to get eight small runtime classes. +- **The generator is a heavier dependency than it needs to be.** Consumers pull the whole generator (parser, emitter, symfony/yaml) into production just to get nine small runtime classes. ## Option B: inline the support classes into the consumer's output (adopted and shipped) @@ -76,7 +77,7 @@ The generator emits the referenced support classes into the consumer's own names ## Option C: a tiny frozen runtime package -Split the eight support classes into a separate, minimal, semver-frozen package (for example `codewithagents/openapi-laravel-runtime`) that changes essentially never. The generator depends on it; generated code imports from it; consumers `require` only the tiny runtime, not the whole generator. +Split the nine support classes into a separate, minimal, semver-frozen package (for example `codewithagents/openapi-laravel-runtime`) that changes essentially never. The generator depends on it; generated code imports from it; consumers `require` only the tiny runtime, not the whole generator. **Pros** @@ -114,6 +115,6 @@ Option C was the reasonable fallback if duplication had been judged unacceptable **Option B is implemented.** The generator inlines the referenced support classes into the consumer's own namespace (the Data namespace plus a `\Support` suffix, for example `App\Data\Support\...`), making generated output fully self-contained with no runtime dependency on the generator. - **Status:** done. The support classes are emitted into `/Support/`, imported by the generated Data classes from there, and drift-checked byte-for-byte by `openapi:check`. Only the classes a spec references are emitted. - - **No growing set from [#38](https://github.com/codewithagents/openapi-laravel/issues/38):** the discriminator-aware cast shipped on spatie's native `PropertyMorphableData`, adding zero support classes. The inlined set is the eight listed above (including `NoUnknownPropertiesRule`, emitted whenever a closed object is present, which is now the default). + - **No growing set from [#38](https://github.com/codewithagents/openapi-laravel/issues/38):** the discriminator-aware cast shipped on spatie's native `PropertyMorphableData`, adding zero support classes. The inlined set is the nine listed above (including `NoUnknownPropertiesRule`, emitted whenever a closed object is present, which is now the default). - **It resolves [#41 (versioning policy)](/guides/versioning-policy):** now that they are inlined, the support classes fall under the **output** surface and are governed by the same major-bump rule as the rest of the generated code. diff --git a/docs/src/content/docs/guides/server-scaffold.mdx b/docs/src/content/docs/guides/server-scaffold.mdx index 31f1e79..d499f46 100644 --- a/docs/src/content/docs/guides/server-scaffold.mdx +++ b/docs/src/content/docs/guides/server-scaffold.mdx @@ -357,6 +357,28 @@ final class PetController extends AbstractPetController } ``` +Every abstract method is typed to the operation's success DTO, so a concrete method that needs to +answer a spec-declared **error** status throws instead of returning (a `throw` never reaches the +`return`, so the success type stays satisfied). For an operation whose spec declares a +named-component object error response, the generator emits a `Errors` factory with one +status-named method per qualifying error status, so `show()` can answer the spec's own 404 shape +rather than let a generic lookup decide it: + +```php +public function show(int $petId): PetData +{ + return $this->petService->find($petId) + ?? throw GetPetByIdErrors::notFound(message: "Pet {$petId} not found."); +} +``` + +The status lives in the method name, the error DTO is built and flattened for you, and the thrown +value self-renders with no `bootstrap/app.php` wiring. For a status a generated factory does not +cover, the `App\Data\Support\ApiError` carrier is directly available +(`throw ApiError::forbidden($body)` or `new ApiError($body, $status)`). See +[Throwing other error statuses](/guides/validation-errors#throwing-other-error-statuses) for the +full treatment. + ### Generated routes file ```php diff --git a/docs/src/content/docs/guides/stability.mdx b/docs/src/content/docs/guides/stability.mdx index a6d5aba..8495dab 100644 --- a/docs/src/content/docs/guides/stability.mdx +++ b/docs/src/content/docs/guides/stability.mdx @@ -19,9 +19,9 @@ release produces byte-identical PHP: the same class and property names, the same same type hints and docblocks, the same file set, the same `#[MapName]` attributes. The [drift check](/guides/drift-check) (`openapi:check`) enforces this byte-for-byte in CI. -**2. The support-class namespace and import lines.** The eight support classes the generator inlines +**2. The support-class namespace and import lines.** The nine support classes the generator inlines (`MultipleOfRule`, `Rfc3339DateTimeRule`, `Rfc3339TimeRule`, `Iso8601DurationRule`, `HostnameRule`, -`MapObjectTransformer`, `NoUnknownPropertiesRule`, `RespondsWithStatus`) land at a fixed location +`MapObjectTransformer`, `NoUnknownPropertiesRule`, `RespondsWithStatus`, `ApiError`) land at a fixed location relative to your configured Data namespace, for example `App\Data\Support\MultipleOfRule`. Their import lines are part of the committed output, so moving them would be an output-shape change covered by the same rule. The namespace is not changing after `1.0.0`. diff --git a/docs/src/content/docs/guides/validation-errors.mdx b/docs/src/content/docs/guides/validation-errors.mdx index b7ef6f6..0ce47f8 100644 --- a/docs/src/content/docs/guides/validation-errors.mdx +++ b/docs/src/content/docs/guides/validation-errors.mdx @@ -191,6 +191,116 @@ Why the pieces are what they are: status separately; the explicit `response()->json(...)` form keeps the status visible. +## Throwing other error statuses + +The recipe above reshapes the framework's **own** 422, the one Laravel raises when a spec-derived +`rules()` check fails before your code runs. A different case is a spec-declared error your *own* +code decides to answer: a 404 when a lookup misses, a 409 on a conflicting write. The generated +abstract method is typed to the operation's **success** DTO (error responses are never inspected for +typing), so a hand-rolled helper that `return`s a `JsonResponse` will not satisfy that return type. +Throwing does: a `throw` never reaches the `return`, so the declared success type stays honored no +matter which error path a method takes. + +For every operation whose spec declares a named-component object error response, the generator emits +a per-operation factory, `Errors`, with one status-named static method per declared +error. Throw it directly: + +```php +public function show(int $petId): PetData +{ + return $this->store->pet($petId) + ?? throw GetPetByIdErrors::notFound(message: "Pet {$petId} not found."); +} +``` + +The factory is generated alongside the operation's other Data classes: + +```php +namespace App\Data\Pets; + +use App\Data\Support\ApiError; + +final class GetPetByIdErrors +{ + public static function notFound(string $message): ApiError + { + return new ApiError(new PetErrorData(message: $message), 404); + } +} +``` + +Three things fall out of that shape, and each removes a way to get the error wrong: + +- **The status is written once, in the method name.** `notFound` *is* the 404; there is no status + literal at the throw site to drift from the spec. The name comes from the spec's declared status + key (`400` becomes `badRequest`, `404` becomes `notFound`, `409` becomes `conflict`, `422` becomes + `unprocessable`, and so on). +- **The error DTO is never hand-rolled.** The factory constructs `PetErrorData` for you and flattens + its constructor into named parameters, so the call site passes `message:` directly with no + `new PetErrorData(...)` wrapper. A nested or collection field keeps its sub-DTO array typing, + exactly as the constructor declares it. +- **No registration.** `ApiError` self-renders through Laravel's `render(Request): Response` + exception-handler hook, so the thrown value becomes the spec's declared error body at the spec's + declared status with **no** `bootstrap/app.php` closure. (Contrast the 422 recipe above: that one + needs a closure because Laravel's validation machinery throws `ValidationException` itself, before + any of your code runs.) + +A single error schema shared across several statuses (a 400 and a 404 both pointing at `PetError`) +produces one method per status, each forwarding into the same DTO, so the call site stays +status-precise while the body shape stays single-sourced. + +### The ApiError escape hatch + +`Errors` is the pattern to reach for first. When no generated method fits, the same +`ApiError` carrier the factory forwards into is available directly, with named-status factories +(`badRequest`, `unauthorized`, `forbidden`, `notFound`, `conflict`, `unprocessable`, +`tooManyRequests`, `serverError`) and a general `new ApiError($body, $status)` constructor for any +other code: + +```php +use App\Data\Support\ApiError; + +throw ApiError::forbidden($errorData); // a named status +throw new ApiError($errorData, 451); // any other code +``` + +Three cases call for the escape hatch rather than a generated factory: + +- **An operation whose error responses do not qualify.** A v1 factory method is emitted only for a + declared error whose schema resolves to a **named component object**. An operation whose error + slot is an inline object schema (a documented residual for a fast-follow), a non-object shape, an + unresolvable `$ref`, or a `default`/`4XX`/`5XX` wildcard (which names no single concrete status to + throw) gets no generated method; throw `ApiError` with the Data class (or any + `Responsable`/`Arrayable`/`JsonSerializable` value) you build yourself. +- **A cross-cutting error not tied to one operation.** A global 401 or 403 enforced in a shared base + controller or in middleware is not part of any single operation's declared responses, so no + per-operation factory covers it. `ApiError::unauthorized(...)` gives that throw the same typed, + ergonomic home. +- **A status the spec's per-operation responses omit.** Spec authors routinely leave a 401 or 403 + undeclared even when the API genuinely enforces it. The status has no generated method to reach + for, so `ApiError` fills the gap without waiting on a spec edit. + +The status names match the factory's exactly (`notFound` is a 404 whether it comes from +`GetPetByIdErrors::notFound(...)` or the carrier's own `ApiError::notFound(...)`), so moving between +the two layers is a mechanical change, never a semantic one. + +The generator is deliberate about which gaps it surfaces. When an operation **does** get a factory, +it warns once per declared error slot that did not become a method (an inline-object schema, a +non-object shape, an unresolvable `$ref`, or a `default`/`4XX`/`5XX` wildcard), so an incomplete +factory is visible at generation time. An operation with **no** qualifying error slot at all +generates no factory and stays **silent**: a warning per non-object error body would flood specs +whose error responses are entirely non-objects or `default` catch-alls (a common shape in the wild), +which is noise, not information the developer needs. So it is not the case that every unsupported +error response warns, only the ones on operations that already earned a factory. + + + ## Testing the shape The renderer is application code, so prove it in your suite. The strongest assertion is the @@ -232,9 +342,13 @@ contract level, so the shape cannot drift in either direction. - **The renderer is yours.** It is written once in `bootstrap/app.php` and survives regeneration untouched; the generator never writes into your bootstrap file. Regenerating after a spec change updates the Data classes the renderer is built from, which is exactly the coupling you want. -- **Other error statuses follow the same pattern.** A 404 or 409 body declared in the spec also - generates its Data class; register additional `render` closures (for `NotFoundHttpException`, - your domain exceptions, and so on) and build their bodies the same way. +- **Other error statuses: throw the generated factory.** A 404 or 409 body declared in the spec + also generates its Data class, and for a named-component object schema the generator emits a + `Errors::notFound(...)`-style factory that wraps it in a self-rendering `ApiError` with + no `bootstrap/app.php` closure at all. Reach for that first (see + [Throwing other error statuses](#throwing-other-error-statuses)); the `ApiError` carrier covers + anything a generated factory does not, and a `render` closure per exception type stays available + when you want the framework-level hook instead. ## Related pages diff --git a/docs/src/content/docs/guides/versioning-policy.mdx b/docs/src/content/docs/guides/versioning-policy.mdx index 31092f1..e9a57a9 100644 --- a/docs/src/content/docs/guides/versioning-policy.mdx +++ b/docs/src/content/docs/guides/versioning-policy.mdx @@ -32,7 +32,7 @@ Under this policy, a **major** version bump (post-1.0.0) is required for any of: - A change to the **generated output for an unchanged spec** that alters its shape: different class or property names, a changed `rules()` result, a different type hint, a moved or renamed file, a changed `#[MapName]` or attribute. Pure formatting churn counts too, because the [drift check](/guides/drift-check) compares byte-for-byte. - A change to **validation behavior**: a payload that the generated `rules()` previously accepted is now rejected, or vice versa. This is the most consequential break, because it can start rejecting real production traffic. -- A change to the **support classes** (`MultipleOfRule`, `Rfc3339DateTimeRule`, `Rfc3339TimeRule`, `Iso8601DurationRule`, `HostnameRule`, `MapObjectTransformer`, and `NoUnknownPropertiesRule`) that changes their validation or serialization behavior. Where these classes live, and therefore which surface governs them, is settled by [#40 (runtime coupling)](/guides/runtime-coupling): **Option B is implemented**, the classes are inlined into the consumer's own output (the Data namespace plus a `\Support` suffix). They are now part of the **output surface**, owned, committed, and drift-checked, so a change to their behavior is an output-shape change under the same major-bump rule above. The previous silent-change surface (a `composer update` swapping their behavior under committed code) is closed: the support classes live in your repo, and a rule changes only when you regenerate and review the diff. +- A change to the **support classes** (`MultipleOfRule`, `Rfc3339DateTimeRule`, `Rfc3339TimeRule`, `Iso8601DurationRule`, `HostnameRule`, `MapObjectTransformer`, `NoUnknownPropertiesRule`, `RespondsWithStatus`, and `ApiError`) that changes their validation or serialization behavior. Where these classes live, and therefore which surface governs them, is settled by [#40 (runtime coupling)](/guides/runtime-coupling): **Option B is implemented**, the classes are inlined into the consumer's own output (the Data namespace plus a `\Support` suffix). They are now part of the **output surface**, owned, committed, and drift-checked, so a change to their behavior is an output-shape change under the same major-bump rule above. The previous silent-change surface (a `composer update` swapping their behavior under committed code) is closed: the support classes live in your repo, and a rule changes only when you regenerate and review the diff. **Tool-surface breaks (the classic part):** diff --git a/e2e/backend/.gitignore b/e2e/backend/.gitignore index 3271194..8b288e4 100644 --- a/e2e/backend/.gitignore +++ b/e2e/backend/.gitignore @@ -30,3 +30,4 @@ Thumbs.db /app/Data/ /app/Http/Controllers/Api/Abstract*Controller.php /routes/api.generated.php +/openapi-laravel.unsupported.json diff --git a/e2e/backend/app/Http/Controllers/Api/PetController.php b/e2e/backend/app/Http/Controllers/Api/PetController.php index b84d09d..2341bdc 100644 --- a/e2e/backend/app/Http/Controllers/Api/PetController.php +++ b/e2e/backend/app/Http/Controllers/Api/PetController.php @@ -7,6 +7,7 @@ use App\Data\ApiResponseData; use App\Data\Pet\FindPetsByStatusQueryData; use App\Data\Pet\FindPetsByTagsQueryData; +use App\Data\Pet\GetPetByIdErrors; use App\Data\Pet\PetData; use App\Data\Pet\PetWritableData; use App\Data\Pet\UpdatePetWithFormQueryData; @@ -82,7 +83,13 @@ public function show(int $petId): PetData $pet = $this->store->findPet($petId); if ($pet === null) { - throw new NotFoundHttpException("Pet {$petId} not found."); + // Answer the spec-declared 404 (getPetById -> PetNotFoundError) with + // the GENERATED throwable factory. It wraps a PetNotFoundErrorData in + // an ApiError at status 404 and self-renders, so this stays a single + // throw and never clashes with the PetData success return type. The + // other NotFoundHttpException sites below are left as-is (a documented + // residual): their operations declare no object error schema to flatten. + throw GetPetByIdErrors::notFound(message: "Pet {$petId} not found."); } return $pet; diff --git a/e2e/e2e-tests/tests/petstore.spec.ts b/e2e/e2e-tests/tests/petstore.spec.ts index 7281a9a..960951e 100644 --- a/e2e/e2e-tests/tests/petstore.spec.ts +++ b/e2e/e2e-tests/tests/petstore.spec.ts @@ -732,6 +732,37 @@ test('DELETE pet returns a 204 No Content with an empty body', async ({ request expect(again.status()).toBe(404); }); +// --------------------------------------------------------------------------- +// Scenario 10b: generated GetPetByIdErrors factory drives the 404 body shape +// +// The spec now gives getPetById's 404 an object error schema +// (#/components/schemas/PetNotFoundError = { message: string }). The generator +// therefore emits App\Data\Pet\GetPetByIdErrors with a notFound(string $message) +// factory that wraps a PetNotFoundErrorData in an ApiError at status 404 and +// self-renders. The concrete PetController::show() throws that generated factory +// instead of an ad hoc NotFoundHttpException, so the spec's declared error schema +// now drives the runtime 404 body shape. This asserts both the status and the +// { message: string } shape over real HTTP. +// --------------------------------------------------------------------------- + +test('GET /pet/{petId} for a missing id 404s with the generated PetNotFoundError { message } shape', async ({ request }) => { + // An id far above any seed or created pet, so the store never has it. + const missingId = 987654321; + + const res = await request.get(`${API_BASE}/pet/${missingId}`, { + headers: { Accept: 'application/json' }, + }); + expect(res.status(), `expected 404 for a missing pet, got ${res.status()}: ${await res.text()}`).toBe(404); + + const body = await res.json(); + // The body IS the spec-declared PetNotFoundError schema: a single `message` + // string, rendered by the GENERATED GetPetByIdErrors::notFound() factory via + // ApiError. The factory sets the message from the throw site, so it carries the + // requested id, distinguishing this generated path from a bare framework 404. + expect(typeof body.message).toBe('string'); + expect(body.message).toContain(String(missingId)); +}); + // --------------------------------------------------------------------------- // Scenario 11: X-Total-Count response header (#114, a DOCUMENTED RESIDUAL) // diff --git a/e2e/spec/petstore.yaml b/e2e/spec/petstore.yaml index 37a21e5..bb4e4c4 100644 --- a/e2e/spec/petstore.yaml +++ b/e2e/spec/petstore.yaml @@ -238,6 +238,10 @@ paths: description: Invalid ID supplied '404': description: Pet not found + content: + application/json: + schema: + $ref: '#/components/schemas/PetNotFoundError' default: description: Unexpected error security: @@ -1997,6 +2001,13 @@ components: type: string xml: name: '##default' + PetNotFoundError: + type: object + required: + - message + properties: + message: + type: string LabNumeric: type: object required: diff --git a/qodana.yaml b/qodana.yaml index a8ef816..e471420 100644 --- a/qodana.yaml +++ b/qodana.yaml @@ -54,10 +54,9 @@ exclude: # The remaining `(string) ...` casts the emitter keeps are load-bearing, not # redundant: `(string) $name` restores the string-key invariant after PHP # coerces a numeric-string array key (e.g. "200") to int at runtime (the same - # rationale as PhpCastIsUnnecessaryInspection above), and - # `(string) json_encode(...)` collapses json_encode's `string|false` return to - # a string so the `%s` sprintf argument is well-typed. Both guard real - # behaviour, so this inspection is a false positive here. + # rationale as PhpCastIsUnnecessaryInspection above). It guards real behaviour, + # so this inspection is a false positive here. (A `json_encode(...) ?: $raw` + # fallback, not a cast, now feeds the two dropped-pattern warnings.) - name: PhpUnnecessaryStringCast # PhpSameParameterValue fires on the framework-free CLI's general-purpose # helpers `repeatedOption()` (StandaloneApplication) and `resolveRepeatable()` @@ -78,6 +77,15 @@ exclude: - name: PhpInternalEntityUsed paths: - bin/openapi-laravel + # ApiError::render(Request $request): Response (#168) takes $request unused: + # the signature IS Laravel's exception-handler render() contract (the + # framework calls render($request) via method_exists on a thrown exception, + # before it even checks Responsable), so the parameter is required by that + # contract even though the response is built entirely from the carried body + # and status. Dropping it would break the zero-registration self-render. + - name: PhpUnusedParameterInspection + paths: + - src/Support/ApiError.php # The Clover report fed to Qodana (#89) comes from the FAST suite only # (--exclude-group=slow), so the slow corpus gate (ReaderCorpusBaselineTest # runs the parser/emitter over all 135 specs) never contributes coverage to diff --git a/src/Console/GenerationPlanner.php b/src/Console/GenerationPlanner.php index 549c3ba..77f1105 100644 --- a/src/Console/GenerationPlanner.php +++ b/src/Console/GenerationPlanner.php @@ -154,12 +154,41 @@ public function plan(GenerationRequest $request): GenerationPlan ); } + // The per-operation `Errors` throwable-factory classes live + // next to the model Data classes (same namespace, same directory, same + // drift-checked CATEGORY_DATA bucket). Unlike the query/body/response + // siblings above (always planned, even a model-only run), these exist + // only to be thrown FROM a concrete controller, so they get their own + // controllers-only gate, parallel to (and co-occurring with) the + // ApiError support-class veto below. + if ($request->controllers) { + foreach ($generator->errorFactoryFiles() as $operationFile) { + $files[] = new PlannedFile( + $target.'/'.$operationFile->filename(), + $operationFile->code, + PlannedFile::CATEGORY_DATA, + ); + } + } + // Inline the runtime support classes the generated Data files reference // into the consumer's own `/Support/` directory (issue #40), so // generated output is self-contained with no runtime dependency on the // generator package. Only the classes this spec used are emitted, and // they are owned, drift-checked output like the Data classes themselves. - foreach ($generator->supportFiles() as $supportFile) { + // + // ApiError is vetoed when controllers are disabled (--no-controllers): + // the collector marks it purely from the spec's response shapes (only + // when an Errors factory is actually emitted), with no + // visibility into the controllers flag. An ApiError with no abstract + // controller to throw it from is dead weight, so the planner (the one + // layer both generate and check share, and the only layer that knows + // $request->controllers) drops it here, alongside its factory classes. + $supportFiles = $generator->supportFiles(); + if (! $request->controllers) { + unset($supportFiles['ApiError']); + } + foreach ($supportFiles as $supportFile) { $files[] = new PlannedFile( $target.'/Support/'.$supportFile->filename(), $supportFile->code, diff --git a/src/Emitter/EnumEmitter.php b/src/Emitter/EnumEmitter.php index 06acbf4..e3c30e1 100644 --- a/src/Emitter/EnumEmitter.php +++ b/src/Emitter/EnumEmitter.php @@ -60,12 +60,15 @@ public function emitEnum(string $className, SchemaNode $schema): void } /** + * The enum is int-backed only when EVERY value can round-trip as an int + * without losing information; otherwise it is string-backed. + * * @param list $values */ private function enumBacking(array $values): string { foreach ($values as $value) { - if (! is_int($value) && ! (is_string($value) && $value !== '' && strspn($value, '0123456789') === strlen($value))) { + if (! $this->isIntBackable($value)) { return 'string'; } } @@ -73,6 +76,33 @@ private function enumBacking(array $values): string return 'int'; } + /** + * Whether a value can back a native PHP int enum without corruption. A + * native int always can. A string can ONLY when it is an unsigned-digit + * string that ALSO round-trips through int unchanged (issue #145). + * + * The old check accepted any unsigned-digit string, which silently + * corrupted a leading-zero wire value: `"01"` was emitted as + * `case Value1 = 1`, so the spec value `"01"` no longer round-tripped (a + * consumer sending `"01"` never matched), and a sibling `"1"` collapsed to + * the SAME `1`, producing a fatal "Duplicate value in enum" PHP error. The + * added `(string) (int) $value === $value` round-trip test rejects every + * non-canonical decimal form (`"01"`, `"040000"`, `"00"`) so such an enum + * falls back to a faithful string backing instead. The unsigned-digit gate + * is kept so a signed string like `"-1"` stays string-backed exactly as + * before, leaving the backing decision for existing specs unchanged. + */ + private function isIntBackable(string|int $value): bool + { + if (is_int($value)) { + return true; + } + + return $value !== '' + && strspn($value, '0123456789') === strlen($value) + && (string) (int) $value === $value; + } + private function enumCaseName(string|int $value, string $backing): string { if ($backing === 'int') { diff --git a/src/Emitter/ErrorFactorySynthesizer.php b/src/Emitter/ErrorFactorySynthesizer.php new file mode 100644 index 0000000..6fde679 --- /dev/null +++ b/src/Emitter/ErrorFactorySynthesizer.php @@ -0,0 +1,235 @@ +Errors` throwable-factory class: + * one static method per spec-declared error response whose JSON schema + * resolves to a NAMED-COMPONENT object (v1 scope). Each method is named by its + * HTTP status (`badRequest`/`notFound`/... via the derived 4xx+5xx table), + * flattens the target error Data class's constructor into named parameters + * (mirroring that constructor exactly, nested/array fields keep their sub-DTO + * array typing), and forwards into `new ApiError(new Data(...), )`. + * A concrete controller then answers a spec error with one throw: + * + * throw GetPetByIdErrors::notFound(message: 'No such pet.'); + * + * Deliberately lightweight: it needs ONLY {@see GenerationState}, because the + * flattened parameter model for every already-emitted Data class was captured + * ONCE during ModelGenerator::emitData()'s original pass (see + * {@see GenerationState::$constructorParams}). This class never calls the type + * resolver or the emission pipeline again, so it cannot double-emit an inline + * nested class the way a naive re-derivation would. + * + * @internal + */ +final class ErrorFactorySynthesizer +{ + /** + * HTTP status -> factory method name, mechanically camelCased from + * Symfony's HttpFoundation `Response::HTTP_*` constants, with the same + * deliberate deviations the shipped `Support\ApiError` carrier's own named + * factories use so a status covered by both layers reads identically + * (422 -> `unprocessable`, not `unprocessableEntity`; 500 -> `serverError`, + * not `internalServerError`). Injective by construction (Symfony's own + * UPPER_SNAKE suffixes are distinct), so two different statuses in one + * operation can never derive the same method name. A concrete 4xx/5xx code + * absent from this table falls back to `status` (e.g. `status480`). + * + * @var array + */ + private const STATUS_NAMES = [ + 400 => 'badRequest', 401 => 'unauthorized', 402 => 'paymentRequired', + 403 => 'forbidden', 404 => 'notFound', 405 => 'methodNotAllowed', + 406 => 'notAcceptable', 407 => 'proxyAuthenticationRequired', + 408 => 'requestTimeout', 409 => 'conflict', 410 => 'gone', + 411 => 'lengthRequired', 412 => 'preconditionFailed', + 413 => 'payloadTooLarge', 414 => 'uriTooLong', + 415 => 'unsupportedMediaType', 416 => 'rangeNotSatisfiable', + 417 => 'expectationFailed', 418 => 'imATeapot', + 421 => 'misdirectedRequest', 422 => 'unprocessable', 423 => 'locked', + 424 => 'failedDependency', 425 => 'tooEarly', 426 => 'upgradeRequired', + 428 => 'preconditionRequired', 429 => 'tooManyRequests', + 431 => 'requestHeaderFieldsTooLarge', + 451 => 'unavailableForLegalReasons', 500 => 'serverError', + 501 => 'notImplemented', 502 => 'badGateway', + 503 => 'serviceUnavailable', 504 => 'gatewayTimeout', + 505 => 'httpVersionNotSupported', 506 => 'variantAlsoNegotiates', + 507 => 'insufficientStorage', 508 => 'loopDetected', + 510 => 'notExtended', 511 => 'networkAuthenticationRequired', + ]; + + public function __construct( + private readonly GenerationState $state, + ) {} + + /** + * Emit the `Errors` factory class for one operation's qualifying + * error slots (already classified by the collector, sorted by status). The + * target Data class of every slot is guaranteed to carry a captured + * constructor model, so this method only READS the stored model and never + * re-runs emission. Returns the reserved class name, or null when there are + * no slots (defensive: the collector never calls with an empty list). + * + * @param string $baseName StudlyCaps operation context (the same operationId-or-fallback the body/response classes use) + * @param string $operationLabel "GET /pets/{petId}", for the class docblock + * @param ?string $tag the operation's first tag (or the 'Untagged' fallback), so the grouped layout (issue #93) places the factory in its operation's tag group; ignored in the flat layout + * @param list $slots qualifying error slots in ascending status order + * @return string|null the reserved factory class name, or null when there are no slots + */ + public function generate(string $baseName, string $operationLabel, ?string $tag, array $slots): ?string + { + if ($slots === []) { + return null; + } + + $className = $this->state->names->reserve($baseName.'Errors'); + $this->state->fileGroups[$className] = $tag !== null ? TagGroups::forTag($tag) : null; + + // ApiError is the carrier every method forwards into; importing it here + // marks it used, so it is inlined into the consumer's Support namespace + // exactly when a factory class is emitted (the unified trigger). + $imports = [$this->state->supportImport('ApiError')]; + $refs = []; + $methods = []; + + foreach ($slots as $slot) { + $dataClass = $slot['dataClass']; + $params = $this->state->constructorParams[$dataClass] ?? []; + + $refs[] = $dataClass; + foreach ($params as $param) { + foreach ($param['type']->classRefs as $ref) { + $refs[] = $ref; + } + } + + $methods[] = $this->renderMethod($this->methodNameFor($slot['status']), $dataClass, $slot['status'], $params); + } + + // Same-group references stay short-name-only; a cross-group Data class + // (or an enum/Data class named in a parameter's docblock type) is + // imported from its real namespace, exactly like every other emitter. + $imports = $this->state->withCrossGroupImports($className, $imports, array_values(array_unique($refs))); + + $this->state->errorFactoryFiles[$className] = new GeneratedFile( + $className, + $this->renderClass($className, $operationLabel, $imports, $methods), + $this->state->fileGroups[$className] ?? null, + ); + + return $className; + } + + private function methodNameFor(int $status): string + { + return self::STATUS_NAMES[$status] ?? 'status'.$status; + } + + /** + * Render one static factory method: the flattened signature, an optional + * PHPDoc line for any parameter carrying a richer generic (an array of a + * sub-DTO, an object union), and the single forwarding return. + * + * @param list $params + */ + private function renderMethod(string $methodName, string $dataClass, int $status, array $params): string + { + $signatureParts = []; + $argParts = []; + $docLines = []; + + foreach ($params as $param) { + $signatureParts[] = $this->paramSignature($param); + $argParts[] = $param['phpName'].': $'.$param['phpName']; + + if ($param['type']->docType !== null) { + $docLines[] = '@param '.$param['type']->docType.' $'.$param['phpName']; + } + } + + $doc = ''; + if ($docLines !== []) { + $doc = " /**\n".implode("\n", array_map(static fn (string $line): string => ' * '.$line, $docLines))."\n */\n"; + } + + return $doc + .' public static function '.$methodName.'('.implode(', ', $signatureParts).'): ApiError'."\n" + ." {\n" + .' return new ApiError(new '.$dataClass.'('.implode(', ', $argParts).'), '.$status.');'."\n" + .' }'; + } + + /** + * One flattened parameter declaration, mirroring the target Data class's + * own constructor parameter exactly (required -> the type's nullable-aware + * declaration, a scalar default -> the seeded literal, otherwise the + * optional `?T = null` form) so a caller passes the same values the Data + * class would accept. + * + * @param array{wireName: string, phpName: string, type: ResolvedType, required: bool, default: ?string} $param + */ + private function paramSignature(array $param): string + { + $type = $param['type']; + + if ($param['required']) { + return $type->declaration().' $'.$param['phpName']; + } + + if ($param['default'] !== null) { + $declaration = $type->nullable ? $this->optionalDeclaration($type) : $type->declaration; + + return $declaration.' $'.$param['phpName'].' = '.$param['default']; + } + + return $this->optionalDeclaration($type).' $'.$param['phpName'].' = null'; + } + + /** + * The optional (defaulting-to-null) declaration of a type, matching + * {@see ClassRenderer}'s own rule: `mixed` already includes null, a genuine + * multi-member union spells null as a trailing `|null` member (PHP forbids + * `?A|B`), and every single type uses the `?T` shorthand. + */ + private function optionalDeclaration(ResolvedType $type): string + { + if ($type->declaration === 'mixed') { + return 'mixed'; + } + + if ($type->isMultiMemberUnion()) { + return str_ends_with($type->declaration, '|null') ? $type->declaration : $type->declaration.'|null'; + } + + return '?'.$type->declaration; + } + + /** + * Assemble the final class source: the header, the ApiError (and any + * cross-group) imports, an explanatory docblock naming the operation, and + * the static factory methods. + * + * @param list $imports + * @param list $methods already-rendered method bodies + */ + private function renderClass(string $className, string $operationLabel, array $imports, array $methods): string + { + $useBlock = implode("\n", array_map(static fn (string $fqcn): string => 'use '.$fqcn.';', $imports)); + + $docLines = [ + 'Throwable factories for the spec-declared error responses of '.PhpLiteral::docblockSafe($operationLabel).'.', + '', + 'Each method builds the operation\'s declared error Data class and wraps it in', + 'an ApiError at the response\'s HTTP status, so a concrete controller answers a', + 'spec error with one throw and never breaks the success return type.', + ]; + $docBlock = "/**\n".implode("\n", array_map(static fn (string $line): string => $line === '' ? ' *' : ' * '.$line, $docLines))."\n */\n"; + + $header = "state->namespaceFor($className).";\n\n".$useBlock."\n\n".$docBlock.'final class '.$className; + + return $header."\n{\n".implode("\n\n", $methods)."\n}\n"; + } +} diff --git a/src/Emitter/GenerationState.php b/src/Emitter/GenerationState.php index d64e71b..cea055c 100644 --- a/src/Emitter/GenerationState.php +++ b/src/Emitter/GenerationState.php @@ -171,6 +171,36 @@ final class GenerationState */ public array $responseFiles = []; + /** + * Per-operation error-factory classes (`Errors`), emitted on + * demand AFTER generate() ran during the server-scaffold collection, keyed + * by class name. Each carries one static factory method per spec-declared + * error response whose JSON schema resolves to a named-component object, so + * a concrete controller answers a spec error with one throw. Kept apart + * from $responseFiles so the throwable-factory surface stays auditable as + * its own layer; the planner collects them into the same drift-checked data + * output (CATEGORY_DATA), gated on controllers being generated. + * + * @var array + */ + public array $errorFactoryFiles = []; + + /** + * The captured constructor-parameter model of every emitted Data class, + * keyed by class name, in constructor-parameter order (required params + * first, then optional, mirroring the emitted constructor). Populated by + * ModelGenerator::emitData() from values it already computes for the + * property-rendering path, for EVERY plain Data class (not just error + * targets, since emitData() cannot know in advance which classes a later + * operation-collection pass will flatten). The error-factory synthesizer + * READS this stored model to flatten a Data class's constructor into named + * factory parameters, so it never re-invokes the type resolver or the + * emission pipeline (which would double-emit an inline nested class). + * + * @var array> + */ + public array $constructorParams = []; + /** * Non-fatal diagnostics gathered during a generate() run, keyed by the * warning text so the same finding (re-seen across the read/write variants of diff --git a/src/Emitter/ModelGenerator.php b/src/Emitter/ModelGenerator.php index 92edade..950c65f 100644 --- a/src/Emitter/ModelGenerator.php +++ b/src/Emitter/ModelGenerator.php @@ -93,6 +93,14 @@ final class ModelGenerator */ private RequestDataSynthesizer $bodies; + /** + * Synthesizes the per-operation `Errors` throwable-factory + * classes for the current run; recreated together with the state. Reads + * the constructor model captured during emitData(), so it never re-enters + * the emission pipeline. + */ + private ErrorFactorySynthesizer $errorFactories; + public function __construct( private readonly GeneratorOptions $options = new GeneratorOptions, ) { @@ -119,6 +127,7 @@ private function wireCollaborators(): void $this->emitData(...), $this->hasReadWriteFlags(...), ); + $this->errorFactories = new ErrorFactorySynthesizer($this->state); } /** @@ -565,6 +574,52 @@ public function responseFiles(): array return $files; } + /** + * The captured constructor-parameter model of a generated Data class, in + * constructor order, or null when no such class was emitted. The server + * scaffold consults this to confirm a named-component error schema resolved + * to a concrete, flattenable Data class (a discriminated-union base or + * variant carries no captured model, so it returns null and is skipped). + * + * @return list|null + */ + public function constructorParamsFor(string $className): ?array + { + return $this->state->constructorParams[$className] ?? null; + } + + /** + * Emit the per-operation `Errors` throwable-factory class for an + * operation's qualifying error slots (a named-component object error + * response per slot, already classified by the collector). Must be called + * AFTER generate(); see {@see ErrorFactorySynthesizer::generate()} for the + * full contract. Kept on the generator so the server scaffold keeps one + * entry point into the model pipeline, exactly like generateInlineResponseData(). + * + * @param list $slots qualifying error slots in ascending status order + * @return string|null the reserved factory class name, or null when there are no slots + */ + public function generateOperationErrors(string $baseName, string $operationLabel, ?string $tag, array $slots): ?string + { + return $this->errorFactories->generate($baseName, $operationLabel, $tag, $slots); + } + + /** + * The per-operation error-factory classes emitted since the last generate() + * run, keyed and ordered by class name. A dedicated bucket mirroring + * responseFiles(); the planner collects them into the same CATEGORY_DATA + * output, gated on controllers being generated. + * + * @return array + */ + public function errorFactoryFiles(): array + { + $files = $this->state->errorFactoryFiles; + ksort($files); + + return $files; + } + /** * @return array */ @@ -636,6 +691,13 @@ private function emitData(string $className, SchemaNode $schema, int $depth, str $paramsRequired = []; $paramsOptional = []; + // The captured constructor-parameter model, split required/optional + // exactly like $paramsRequired/$paramsOptional so it merges in the same + // order (issue: error-factory flattening). The error-factory + // synthesizer reads this instead of re-resolving the schema, which + // would double-emit an inline nested class. + $constructorParamsRequired = []; + $constructorParamsOptional = []; $rules = []; $usesRule = false; @@ -696,10 +758,17 @@ private function emitData(string $className, SchemaNode $schema, int $depth, str $rendered = $this->renderer->renderProperty($wireName, $propertyName, $type, $isRequired, $default, SchemaFacts::deprecationTag($propertySchema)); + // Capture the same (wireName, phpName, type, required, default) + // tuple the rendered property is built from, once, for the + // error-factory synthesizer to flatten later (see above). + $captured = ['wireName' => $wireName, 'phpName' => $propertyName, 'type' => $type, 'required' => $isRequired, 'default' => $default[0] ?? null]; + if ($isRequired) { $paramsRequired[] = $rendered; + $constructorParamsRequired[] = $captured; } else { $paramsOptional[] = $rendered; + $constructorParamsOptional[] = $captured; } // Validation rules are keyed by the wire (mapped input) name. The @@ -739,6 +808,14 @@ private function emitData(string $className, SchemaNode $schema, int $depth, str } $params = array_merge($paramsRequired, $paramsOptional); + + // Store the captured constructor model in the SAME order $params is + // merged, keyed by class name, for every emitted class (an error + // factory can only flatten a class that carries this entry, which is + // how a discriminated-union base/variant is naturally excluded: those + // are emitted through their own paths and never populate this bucket). + $this->state->constructorParams[$className] = array_merge($constructorParamsRequired, $constructorParamsOptional); + $imports = $this->renderer->collectImports($params, $usesRule, $rules); // An empty class body (no properties, no rules) compiles fine but diff --git a/src/Emitter/PhpLiteral.php b/src/Emitter/PhpLiteral.php index 2ed4b3c..16f724f 100644 --- a/src/Emitter/PhpLiteral.php +++ b/src/Emitter/PhpLiteral.php @@ -36,7 +36,68 @@ public static function scalarLiteral(string|int|float|bool $value): string public static function numberLiteral(int|float $value): string { - return (string) $value; + $rendered = (string) $value; + + // A small- or large-magnitude float stringifies in scientific notation + // (`(string) 1e-7` is `"1.0E-7"`). That form breaks a Laravel rule-string + // parameter (`min:1.0E-7`, `multipleOf:1.0E-7`), where the validator + // reads the literal text and the `E` is not understood as an exponent, + // and it makes a generated PHP default needlessly opaque. The exponent is + // expanded into plain fixed-decimal notation, preserving the exact digits + // the cast produced (issue #148). A non-scientific value is returned + // untouched, so every normal-range number is byte-identical to before. + if (stripos($rendered, 'e') === false) { + return $rendered; + } + + return self::expandScientific($rendered); + } + + /** + * Expand a scientific-notation decimal string (`"1.0E-7"`, `"-2.5E+20"`) + * into plain fixed-decimal notation (`"0.0000001"`, `"-250000000000000000000"`) + * by shifting the decimal point per the exponent. The mantissa digits are + * preserved verbatim; only the radix point moves, so the rendered value is + * exactly the one the `(string)` cast produced, just without the exponent. + */ + private static function expandScientific(string $rendered): string + { + $sign = ''; + if ($rendered !== '' && ($rendered[0] === '-' || $rendered[0] === '+')) { + $sign = $rendered[0] === '-' ? '-' : ''; + $rendered = substr($rendered, 1); + } + + [$mantissa, $exponentPart] = preg_split('/[eE]/', $rendered, 2) ?: [$rendered, '0']; + $exponent = (int) $exponentPart; + + $dot = strpos($mantissa, '.'); + if ($dot === false) { + $intDigits = $mantissa; + $fracDigits = ''; + } else { + $intDigits = substr($mantissa, 0, $dot); + $fracDigits = substr($mantissa, $dot + 1); + } + + $digits = $intDigits.$fracDigits; + // Where the radix point lands, measured from the left of $digits, after + // applying the exponent shift. + $pointPosition = strlen($intDigits) + $exponent; + + if ($pointPosition <= 0) { + $result = '0.'.str_repeat('0', -$pointPosition).$digits; + } elseif ($pointPosition >= strlen($digits)) { + $result = $digits.str_repeat('0', $pointPosition - strlen($digits)); + } else { + $result = substr($digits, 0, $pointPosition).'.'.substr($digits, $pointPosition); + } + + if (str_contains($result, '.')) { + $result = rtrim(rtrim($result, '0'), '.'); + } + + return $result === '' ? '0' : $sign.$result; } /** diff --git a/src/Emitter/RulesBuilder.php b/src/Emitter/RulesBuilder.php index 09dbe96..5180727 100644 --- a/src/Emitter/RulesBuilder.php +++ b/src/Emitter/RulesBuilder.php @@ -972,7 +972,28 @@ private function regexRule(string $pattern): ?string return null; } - return "'regex:".PhpLiteral::escapeSingleQuoted($this->delimitedPattern($pattern))."'"; + $delimited = $this->delimitedPattern($pattern); + + // The spec `pattern` is ECMA-262 and untrusted input; PHP's `regex:` + // rule compiles it as PCRE. An ECMA-valid-but-PCRE-invalid (or simply + // broken) pattern compiled by Laravel's preg_match raises an + // UNCATCHABLE compile error on every request to the field, a runtime + // 500/DoS in the consumer's app. So the pattern is probed exactly like + // the patternProperties patterns in closedObjectRule(); if it does not + // compile, the regex rule is dropped (the field keeps its other rules) + // and the skip is surfaced as a build warning. + if (! $this->compilesAsPcre($delimited)) { + $this->state->warnings[sprintf( + 'A string schema declares a `pattern` that is not valid PCRE (%s); the `regex:` rule is dropped ' + .'so the generated app never raises an uncatchable preg_match compile error at runtime. ' + .'The field keeps its other validation rules.', + json_encode($pattern) ?: $pattern, + )] = true; + + return null; + } + + return "'regex:".PhpLiteral::escapeSingleQuoted($delimited)."'"; } /** @@ -1069,7 +1090,7 @@ public function closedObjectRule(string $schemaName, array $wireNames, SchemaNod .'closed-object enforcement (additionalProperties: false) is skipped for this schema ' .'so spec-legal keys are never falsely rejected.', $schemaName, - (string) json_encode($pattern), + json_encode($pattern) ?: $pattern, )] = true; return null; diff --git a/src/Emitter/Server/OperationCollector.php b/src/Emitter/Server/OperationCollector.php index 43117d7..531861c 100644 --- a/src/Emitter/Server/OperationCollector.php +++ b/src/Emitter/Server/OperationCollector.php @@ -406,6 +406,19 @@ private function describe(string $path, string $method, OperationNode $operation $this->models?->markSupportClassUsed('RespondsWithStatus'); } + // A spec-declared error response whose JSON schema resolves to a + // named-component object gets a throwable `Errors` factory + // class a concrete controller can throw (never returned, so the + // success return type stays satisfied). The ApiError carrier the + // factory forwards into is inlined only when at least one such factory + // class is actually emitted (the unified trigger): this is the single + // mark point, mirroring RespondsWithStatus above; the --no-controllers + // veto lives one layer up, in GenerationPlanner. + $errorsClass = $this->operationErrorFactory($operation, $label, $bodyBaseName); + if ($errorsClass !== null) { + $this->models?->markSupportClassUsed('ApiError'); + } + return new OperationDescriptor( httpMethod: $method, path: $path, @@ -1836,6 +1849,156 @@ private function resolveComponentResponse(ReferenceNode $response, string $statu return [$resolved, $componentName]; } + /** + * Emit the per-operation `Errors` throwable-factory class for + * every spec-declared error response whose JSON schema resolves to a + * NAMED-COMPONENT object (v1 scope), or null when the operation declares no + * such response (in which case the ApiError carrier is not marked either). + * + * A qualifying slot is a CONCRETE 4xx/5xx status (the `default` key and the + * `4XX`/`5XX` range wildcards are omitted in v1: none names one status to + * throw) whose `application/json` schema is a `$ref` to a registered Data + * class that carries a captured constructor model (a discriminated-union + * base or variant does not, so it is skipped). An unresolvable response + * `$ref` is silently ignored: the factory is an ergonomics layer, not a + * correctness surface, so it never warns the way the success path does. + * + * A CONCRETE error slot that does NOT qualify (an inline object schema, + * deferred to a fast-follow; a non-object schema; an unresolvable schema + * `$ref`) is warned about, but ONLY when the operation actually gets a + * factory: an operation with no qualifying slot produces no class and no + * warning, so the overwhelmingly common "error body is not a named + * component object" case stays quiet across the corpus. + * + * @param string $label "GET /pets/{petId}", for warning messages and the class docblock + * @param string $baseName StudlyCaps operation context (the same operationId-or-fallback the body/response classes use) + */ + private function operationErrorFactory(OperationNode $operation, string $label, string $baseName): ?string + { + if ($this->models === null) { + return null; + } + + $responses = $operation->responses; + if (! $responses instanceof ResponsesNode) { + return null; + } + + /** @var list $slots */ + $slots = []; + /** @var list $skipped */ + $skipped = []; + + foreach ($responses->responses as $status => $response) { + $status = (string) $status; + + // v1: only concrete 4xx/5xx codes get a factory method (400-599). + if (preg_match('~^[45][0-9][0-9]$~', $status) !== 1) { + // `default` and the 4XX/5XX range wildcards ARE error responses, + // but name no single concrete status to throw, so v1 defers + // them: record them as skipped so a factory-getting operation + // warns for them too, exactly like the inline-object/non-object + // slots below (the "warn-and-skip" contract for deferred slots). + // 1xx/2xx/3xx (concrete or wildcard) are not error responses and + // never warn. + if ($status === 'default' || $status === '4XX' || $status === '5XX') { + $skipped[] = ['status' => $status, 'reason' => 'default and 4XX/5XX wildcard error responses are not generated in this version; throw ApiError directly for them']; + } + + continue; + } + + // Resolve a #/components/responses/ $ref silently. + if ($response instanceof ReferenceNode) { + $componentName = SchemaPointer::componentName($response->pointer(), 'responses'); + $response = $componentName !== null ? ($this->componentResponses[$componentName] ?? null) : null; + } + + if (! $response instanceof ResponseNode) { + continue; + } + + $schema = $this->jsonSchema($response->content); + + if ($schema instanceof ReferenceNode) { + $name = SchemaPointer::refName($schema->pointer()); + if ($name !== null && isset($this->registry[$name]) && $this->registry[$name]['kind'] === 'data') { + $dataClass = $this->registry[$name]['dataClass']; + // The target must be a concrete, flattenable Data class: a + // discriminated-union base (abstract) or variant carries no + // captured constructor model and cannot be forwarded to. + if ($this->models->constructorParamsFor($dataClass) !== null) { + $slots[] = ['status' => (int) $status, 'dataClass' => $dataClass]; + + continue; + } + } + } + + $skipped[] = ['status' => $status, 'reason' => $this->errorSlotSkipReason($schema)]; + } + + // No concrete 4xx/5xx object error slot: no factory is emitted, and the + // operation stays SILENT by design, even if it declared default/wildcard + // or non-object error responses. Warning per non-object error body would + // flood the corpus (e.g. Stripe declares a `default` error on hundreds of + // operations); the warn-and-skip diagnostics fire ONLY once the operation + // actually gets a factory (see the loop below), where they tell the user + // which of its error responses that factory did not cover. + if ($slots === []) { + return null; + } + + // Deterministic order: ascending status. + usort($slots, static fn (array $a, array $b): int => $a['status'] <=> $b['status']); + + $class = $this->models->generateOperationErrors($baseName, $label, $this->firstTag($operation), $slots); + + // The operation got a factory, so surface each error slot that did NOT + // become a throwable method (mirroring the body/response fallbacks). + foreach ($skipped as $skip) { + $this->warnings[sprintf( + 'Operation %s: the %s error response did not get a throwable factory method (%s).', + $label, + $skip['status'], + $skip['reason'], + )] = true; + } + + return $class; + } + + /** + * Why a concrete error slot did not qualify for a factory method, for the + * warning text. An inline object schema is a documented v1 deferral; a + * schema $ref that resolves to a registered object Data class but carries + * no flattenable constructor model is a discriminated-union base/variant + * (emitted through its own path, never captured); any other $ref does not + * resolve to a generated object Data class at all. + */ + private function errorSlotSkipReason(SchemaNode|ReferenceNode|null $schema): string + { + if ($schema === null) { + return 'it declares no application/json schema'; + } + + if ($schema instanceof ReferenceNode) { + // A qualifying object component never reaches here (it becomes a + // slot), so a $ref that IS a registered Data-class component can + // only be one with no captured constructor: a discriminated-union + // base (abstract) or a variant (forwards a discriminator to its + // parent), neither of which a static factory can build. + $name = SchemaPointer::refName($schema->pointer()); + if ($name !== null && isset($this->registry[$name]) && $this->registry[$name]['kind'] === 'data') { + return 'its schema resolves to a discriminated-union base or variant, which has no flattenable constructor'; + } + + return 'its schema $ref does not resolve to a generated object Data class'; + } + + return 'its schema is inline; only named-component object error schemas are supported in this version'; + } + /** * Find the schema of the first content entry whose media type the predicate * accepts (and that carries a schema). The three media-type lookups below diff --git a/src/Parser/OpenApiReader.php b/src/Parser/OpenApiReader.php index 4c326cc..b958e8f 100644 --- a/src/Parser/OpenApiReader.php +++ b/src/Parser/OpenApiReader.php @@ -1253,15 +1253,32 @@ private function boolOrNull(mixed $value): ?bool * A number-valued keyword: int and float pass through, a strictly-numeric * string is coerced (issue #32), anything else is null (routed to `extra` * by the caller). + * + * A non-finite float (INF or NAN) is rejected as if absent (issue #151). + * The spec is untrusted: JSON `1e400` decodes to INF and YAML `.inf`/`.nan` + * yield non-finite floats, which would otherwise reach a numeric keyword + * and emit a degenerate rule, e.g. `max:NAN` rejects EVERY value (an + * availability bug planted by spec input) and `min:INF` / `MultipleOfRule(INF)` + * are nonsensical. Returning null here routes the raw value to `extra`, the + * same graceful-ignored path an absent or non-numeric keyword already takes. */ private function numberValue(mixed $value): int|float|null { - if (is_int($value) || is_float($value)) { + if (is_int($value)) { return $value; } + if (is_float($value)) { + return is_finite($value) ? $value : null; + } + if (is_string($value) && is_numeric($value)) { - return $this->numericFromString($value); + // A numeric string can also overflow to INF (e.g. "1e400"), so the + // coerced result is checked the same way: a non-finite float is + // rejected as if absent, an int is always finite. + $coerced = $this->numericFromString($value); + + return is_int($coerced) || is_finite($coerced) ? $coerced : null; } return null; diff --git a/src/Support/ApiError.php b/src/Support/ApiError.php new file mode 100644 index 0000000..b38c2f0 --- /dev/null +++ b/src/Support/ApiError.php @@ -0,0 +1,133 @@ + 'No such pet.'])); + * + * For a status without a named factory, the general constructor stays + * available: `throw new ApiError($body, 451);`. + * + * ApiError is schema-agnostic by design (issue #79 stands: the generator does + * not generate a renderer that maps Laravel's error bag into a spec shape). + * It is a typed CARRIER only; the caller supplies the already-generated Data + * object (or any Responsable/Arrayable/JsonSerializable value) that matches + * the spec's declared error schema. + */ +final class ApiError extends RuntimeException +{ + /** + * @param Arrayable|JsonSerializable|Responsable $body + */ + public function __construct( + public readonly Arrayable|JsonSerializable|Responsable $body, + public readonly int $status, + ) { + parent::__construct(sprintf('API error: HTTP %d.', $status)); + } + + /** + * @param Arrayable|JsonSerializable|Responsable $body + */ + public static function badRequest(Arrayable|JsonSerializable|Responsable $body): self + { + return new self($body, 400); + } + + /** + * @param Arrayable|JsonSerializable|Responsable $body + */ + public static function unauthorized(Arrayable|JsonSerializable|Responsable $body): self + { + return new self($body, 401); + } + + /** + * @param Arrayable|JsonSerializable|Responsable $body + */ + public static function forbidden(Arrayable|JsonSerializable|Responsable $body): self + { + return new self($body, 403); + } + + /** + * @param Arrayable|JsonSerializable|Responsable $body + */ + public static function notFound(Arrayable|JsonSerializable|Responsable $body): self + { + return new self($body, 404); + } + + /** + * @param Arrayable|JsonSerializable|Responsable $body + */ + public static function conflict(Arrayable|JsonSerializable|Responsable $body): self + { + return new self($body, 409); + } + + /** + * @param Arrayable|JsonSerializable|Responsable $body + */ + public static function unprocessable(Arrayable|JsonSerializable|Responsable $body): self + { + return new self($body, 422); + } + + /** + * @param Arrayable|JsonSerializable|Responsable $body + */ + public static function tooManyRequests(Arrayable|JsonSerializable|Responsable $body): self + { + return new self($body, 429); + } + + /** + * @param Arrayable|JsonSerializable|Responsable $body + */ + public static function serverError(Arrayable|JsonSerializable|Responsable $body): self + { + return new self($body, 500); + } + + /** + * Laravel calls this automatically on any thrown exception that defines + * it (no bootstrap/app.php registration needed): the response IS the + * spec's declared error body, at the spec's declared status. + */ + public function render(Request $request): Response + { + return response()->json($this->body, $this->status); + } +} diff --git a/tests/Conformance/ConformanceGoldenTest.php b/tests/Conformance/ConformanceGoldenTest.php index e5bd9cd..657afdc 100644 --- a/tests/Conformance/ConformanceGoldenTest.php +++ b/tests/Conformance/ConformanceGoldenTest.php @@ -772,6 +772,29 @@ function conformanceSupport(): array ->toContain('use App\Data\Misc\DuplicateOpResponseData_2;'); }); +// --- Per-operation error-response factories (Errors) ------------- + +it('synthesizes an Errors factory for a concrete named-component error response', function () { + [, $generator] = conformance31Server(); + + // getWidget declares a concrete 404 -> ErrorObject (a named component + // object): a GetWidgetErrors factory flattens ErrorObjectData's + // constructor and forwards into ApiError at 404. The `default` slot is + // deliberately omitted in v1 (no single concrete status to throw). + $files = $generator->errorFactoryFiles(); + expect($files)->toHaveKey('GetWidgetErrors'); + + expect($files['GetWidgetErrors']->code) + ->toContain('final class GetWidgetErrors') + ->toContain('use App\Data\Support\ApiError;') + ->toContain('public static function notFound(int $code, string $message): ApiError') + ->toContain('return new ApiError(new ErrorObjectData(code: $code, message: $message), 404);') + ->not->toContain('function unexpected'); + + // Emitting a factory is exactly what inlines the ApiError carrier. + expect($generator->supportFiles())->toHaveKey('ApiError'); +}); + // --- Non-JSON responses typed as the base Response (#117/#118) -------------- it('types non-JSON-only responses as the base Symfony Response in the abstract controllers (#117/#118)', function () { diff --git a/tests/Corpus/GeneratedOutputPhpstanTest.php b/tests/Corpus/GeneratedOutputPhpstanTest.php index c1c8337..3cab067 100644 --- a/tests/Corpus/GeneratedOutputPhpstanTest.php +++ b/tests/Corpus/GeneratedOutputPhpstanTest.php @@ -87,7 +87,7 @@ // in, exactly like the planner, so they are analysed alongside the // model classes. Stripe in particular emits hundreds of them. (new OperationCollector(new ServerOptions, $generator->registry(), null, $generator))->collect($document); - $files = array_merge($files, $generator->queryFiles(), $generator->bodyFiles(), $generator->responseFiles()); + $files = array_merge($files, $generator->queryFiles(), $generator->bodyFiles(), $generator->responseFiles(), $generator->errorFactoryFiles()); // The inlined runtime support classes (issue #40) are owned output and // the Data classes import them, so analyse them too: both to prove the // support code is itself PHPStan-max-clean in the consumer namespace, and diff --git a/tests/Corpus/GeneratedOutputPintTest.php b/tests/Corpus/GeneratedOutputPintTest.php index 3c4d5ab..3d1812c 100644 --- a/tests/Corpus/GeneratedOutputPintTest.php +++ b/tests/Corpus/GeneratedOutputPintTest.php @@ -38,13 +38,16 @@ $queryFiles = $generator->queryFiles(); $bodyFiles = $generator->bodyFiles(); $responseFiles = $generator->responseFiles(); + // The per-operation error-factory classes (`Errors`) are owned + // output too, so they must be born Pint-clean like every other class. + $errorFactoryFiles = $generator->errorFactoryFiles(); // The inlined runtime support classes (issue #40) are owned, drift-checked // output too, so they must be born Pint-clean exactly like the Data classes. // Collected AFTER the query and body classes so their rule references count. $supportFiles = $generator->supportFiles(); expect(count($files))->toBeGreaterThan(0, "spec generated no files: {$path}"); - $files = array_merge(array_values($files), array_values($queryFiles), array_values($bodyFiles), array_values($responseFiles)); + $files = array_merge(array_values($files), array_values($queryFiles), array_values($bodyFiles), array_values($responseFiles), array_values($errorFactoryFiles)); $dir = sys_get_temp_dir().'/openapi-laravel-pint-'.bin2hex(random_bytes(6)); expect(mkdir($dir, 0700, true) || is_dir($dir))->toBeTrue("could not create temp dir {$dir}"); diff --git a/tests/Corpus/ReaderCorpusBaselineTest.php b/tests/Corpus/ReaderCorpusBaselineTest.php index b2b07f1..60292ee 100644 --- a/tests/Corpus/ReaderCorpusBaselineTest.php +++ b/tests/Corpus/ReaderCorpusBaselineTest.php @@ -1368,6 +1368,104 @@ 'zuora.json' => 'int32/int64 fields gain format-derived min/max range rules', ]; +/* + * INTENTIONAL post-freeze rebaseline (uncompilable-pattern rules fix, commit + * c670c09, bundled into this branch): the two specs in + * READER_BASELINE_REBASELINED_RULES_UNCOMPILABLE_PATTERN carry a post-fix hash, + * because they declare a `pattern` PHP's PCRE cannot compile (AWS IAM and + * SendGrid use JSON-Schema `\uXXXX` escapes, which PCRE spells `\x{XXXX}`). + * v0.11.0 emitted the pattern verbatim as a `regex:#...#` rule, which throws at + * runtime on the first validation; the fix drops the uncompilable rule instead + * of emitting a broken one. The ONLY divergence in each hash is the removed + * `regex:` rule on the affected string fields. The three sibling fixes bundled + * in the same commit range (float fixed-decimal rendering, enum int-back, and + * non-finite numeric keywords) touch NO corpus spec, so they need no rebaseline + * entry. sendgrid ALSO emits #168 error-factory output, so its hash carries + * both changes; every spec outside the rebaseline lists stays the frozen + * v0.11.0 freeze, byte for byte. + */ + +/** + * Specs whose frozen hash was deliberately updated to the post-fix output (an + * uncompilable `\uXXXX`-escape `pattern` drops its broken `regex:` rule instead + * of emitting one, commit c670c09), keyed by spec basename. The per-spec test + * below still compares against the JSON baseline, which now holds these specs' + * post-fix hashes; the coverage test pins that every listed name exists on disk + * and in the baseline, so the list cannot rot. + * + * @var array + */ +const READER_BASELINE_REBASELINED_RULES_UNCOMPILABLE_PATTERN = [ + 'aws_iam.json' => 'uncompilable \uXXXX-escape query-param patterns drop their broken regex: rules', + 'sendgrid.json' => 'an uncompilable \uXXXX-escape pattern drops its broken regex: rule (hash also carries the #168 error-factory output)', +]; + +/* + * INTENTIONAL post-freeze rebaseline (#168): the thirty-two specs in + * READER_BASELINE_REBASELINED_168 carry a post-#168 hash, because they declare + * at least one error response (a concrete 4xx/5xx status) whose JSON schema + * resolves to a NAMED-COMPONENT object. v0.11.0 generated the error component's + * Data class but nothing that throws it; #168 adds, per such operation, a + * `Errors` throwable-factory class (one static method per qualifying + * error status, forwarding into the ApiError carrier) AND inlines the ApiError + * support class into the consumer's Support namespace (the unified trigger: + * ApiError is inlined iff at least one factory is emitted). The divergence in + * each hash is exactly the new factory file(s), the added Support/ApiError.php, + * and, for an operation that DID get a factory, one warn-and-skip line per error + * slot it could not cover (an inline-object, non-object, unresolvable, or + * default/4XX/5XX wildcard slot); an operation with no qualifying error slot + * generates no factory and no warning. No existing Data class, controller, or + * routes file moves (responseType() is untouched by the feature). A spec also + * present in an earlier rebaseline list accumulates the changes; every spec + * outside the rebaseline lists stays the frozen v0.11.0 freeze, byte for byte. + */ + +/** + * Specs whose frozen hash was deliberately updated to the post-#168 output + * (generated Errors factory classes plus the inlined ApiError + * support class for spec-declared named-component object error responses), + * keyed by spec basename, with the number of factory classes for auditability. + * The per-spec test below still compares against the JSON baseline, which now + * holds these specs' post-#168 hashes; the coverage test pins that every listed + * name exists on disk and in the baseline, so the list cannot rot. + * + * @var array + */ +const READER_BASELINE_REBASELINED_168 = [ + '1password-connect.yaml' => '12 Errors factory classes plus the inlined ApiError support class', + 'ably_control.json' => '22 Errors factory classes plus the inlined ApiError support class', + 'adyen-checkout.yaml' => '20 Errors factory classes plus the inlined ApiError support class', + 'adyen-legal-entity.yaml' => '33 Errors factory classes plus the inlined ApiError support class', + 'airflow.json' => '71 Errors factory classes plus the inlined ApiError support class', + 'amadeus.json' => '2 Errors factory classes plus the inlined ApiError support class (hash also carries 2 default-slot warn-and-skip lines)', + 'apple_appstore.json' => '252 Errors factory classes plus the inlined ApiError support class', + 'asana.json' => '166 Errors factory classes plus the inlined ApiError support class', + 'aws_apigateway.json' => '120 Errors factory classes plus the inlined ApiError support class', + 'aws_cognito.json' => '101 Errors factory classes plus the inlined ApiError support class', + 'aws_dynamodb.json' => '52 Errors factory classes plus the inlined ApiError support class', + 'aws_lambda.json' => '66 Errors factory classes plus the inlined ApiError support class', + 'bitbucket.json' => '251 Errors factory classes plus the inlined ApiError support class', + 'box.json' => '160 Errors factory classes plus the inlined ApiError support class (hash also carries 160 default-slot warn-and-skip lines)', + 'clevercloud.json' => '1 Errors factory class plus the inlined ApiError support class', + 'dnd5e.json' => '1 Errors factory class plus the inlined ApiError support class', + 'docker.json' => '96 Errors factory classes plus the inlined ApiError support class', + 'dracoon.json' => '280 Errors factory classes plus the inlined ApiError support class', + 'elevenlabs.json' => '18 Errors factory classes plus the inlined ApiError support class', + 'github.json' => '479 Errors factory classes plus the inlined ApiError support class', + 'here_positioning.json' => '1 Errors factory class plus the inlined ApiError support class', + 'jira.json' => '70 Errors factory classes plus the inlined ApiError support class', + 'klarna.json' => '1 Errors factory class plus the inlined ApiError support class', + 'openai.yaml' => '14 Errors factory classes plus the inlined ApiError support class', + 'redocly-museum.yaml' => '8 Errors factory classes plus the inlined ApiError support class', + 'sendgrid.json' => '127 Errors factory classes plus the inlined ApiError support class (hash also carries the uncompilable-pattern rules fix)', + 'soundcloud.json' => '55 Errors factory classes plus the inlined ApiError support class', + 'spotify.yaml' => '3 Errors factory classes plus the inlined ApiError support class', + 'vimeo.json' => '226 Errors factory classes plus the inlined ApiError support class', + 'webflow.json' => '81 Errors factory classes plus the inlined ApiError support class', + 'xero.json' => '96 Errors factory classes plus the inlined ApiError support class', + 'zuora.json' => '140 Errors factory classes plus the inlined ApiError support class', +]; + /** * Corpus specs added AFTER the v0.11.0 baseline freeze (#104 T8: the OpenAPI * 3.2 fixtures). The frozen baseline cannot contain them by definition, so @@ -1438,11 +1536,13 @@ // Every spec rebaselined for #110, #116, #120, #122, #124, #125, #126, // #113, #121, #129, #30, #130, the transitive nested-readOnly write split, - // #132 (delimited arrays), #131 (deepObject object query parameters), or the - // deprecated controller docblocks must still exist on disk and carry a hash - // in the baseline (it is an update, not an exemption): a renamed or deleted - // spec would make the documented rebaseline lists rot silently. - foreach ([...array_keys(READER_BASELINE_REBASELINED_110), ...array_keys(READER_BASELINE_REBASELINED_116), ...array_keys(READER_BASELINE_REBASELINED_120), ...array_keys(READER_BASELINE_REBASELINED_122), ...array_keys(READER_BASELINE_REBASELINED_124), ...array_keys(READER_BASELINE_REBASELINED_125), ...array_keys(READER_BASELINE_REBASELINED_126), ...array_keys(READER_BASELINE_REBASELINED_113), ...array_keys(READER_BASELINE_REBASELINED_121), ...array_keys(READER_BASELINE_REBASELINED_129), ...array_keys(READER_BASELINE_REBASELINED_129_INLINE_RESPONSES), ...array_keys(READER_BASELINE_REBASELINED_30), ...array_keys(READER_BASELINE_REBASELINED_NESTED_READONLY), ...array_keys(READER_BASELINE_REBASELINED_130), ...array_keys(READER_BASELINE_REBASELINED_DELIMITED_ARRAYS), ...array_keys(READER_BASELINE_REBASELINED_DEEPOBJECT), ...array_keys(READER_BASELINE_REBASELINED_SELF_STATIC), ...array_keys(READER_BASELINE_REBASELINED_DEPRECATED_CONTROLLER_DOCBLOCKS), ...array_keys(READER_BASELINE_REBASELINED_INT_FORMATS)] as $spec) { + // #132 (delimited arrays), #131 (deepObject object query parameters), the + // deprecated controller docblocks, int32/int64 formats, the uncompilable + // `\uXXXX`-pattern rules fix, or #168 (generated Errors factory + // classes) must still exist on disk and carry a hash in the baseline (it is + // an update, not an exemption): a renamed or deleted spec would make the + // documented rebaseline lists rot silently. + foreach ([...array_keys(READER_BASELINE_REBASELINED_110), ...array_keys(READER_BASELINE_REBASELINED_116), ...array_keys(READER_BASELINE_REBASELINED_120), ...array_keys(READER_BASELINE_REBASELINED_122), ...array_keys(READER_BASELINE_REBASELINED_124), ...array_keys(READER_BASELINE_REBASELINED_125), ...array_keys(READER_BASELINE_REBASELINED_126), ...array_keys(READER_BASELINE_REBASELINED_113), ...array_keys(READER_BASELINE_REBASELINED_121), ...array_keys(READER_BASELINE_REBASELINED_129), ...array_keys(READER_BASELINE_REBASELINED_129_INLINE_RESPONSES), ...array_keys(READER_BASELINE_REBASELINED_30), ...array_keys(READER_BASELINE_REBASELINED_NESTED_READONLY), ...array_keys(READER_BASELINE_REBASELINED_130), ...array_keys(READER_BASELINE_REBASELINED_DELIMITED_ARRAYS), ...array_keys(READER_BASELINE_REBASELINED_DEEPOBJECT), ...array_keys(READER_BASELINE_REBASELINED_SELF_STATIC), ...array_keys(READER_BASELINE_REBASELINED_DEPRECATED_CONTROLLER_DOCBLOCKS), ...array_keys(READER_BASELINE_REBASELINED_INT_FORMATS), ...array_keys(READER_BASELINE_REBASELINED_RULES_UNCOMPILABLE_PATTERN), ...array_keys(READER_BASELINE_REBASELINED_168)] as $spec) { expect($specs)->toContain($spec) ->and($baseline)->toHaveKey($spec); } @@ -1497,6 +1597,7 @@ function readerBaselinePipeline(string $path): array ...array_values($generator->queryFiles()), ...array_values($generator->bodyFiles()), ...array_values($generator->responseFiles()), + ...array_values($generator->errorFactoryFiles()), ...array_values($controllers), $routes, ] as $file) { diff --git a/tests/Feature/Console/CheckCommandTest.php b/tests/Feature/Console/CheckCommandTest.php index 9b6694d..232ddbe 100644 --- a/tests/Feature/Console/CheckCommandTest.php +++ b/tests/Feature/Console/CheckCommandTest.php @@ -4,6 +4,7 @@ $customerSpec = fn (): string => __DIR__.'/../../Fixtures/emitter/customer.json'; $serverSpec = fn (): string => __DIR__.'/../../Fixtures/server/petstore.yaml'; +$apiErrorSpec = fn (): string => __DIR__.'/../../Fixtures/server/api-error.yaml'; $tempOut = fn (): string => sys_get_temp_dir().'/oal_check_'.uniqid(); it('reports in sync after generating into the output', function () use ($customerSpec, $tempOut) { @@ -177,6 +178,40 @@ ->assertExitCode(1); }); +it('drift-checks the generated Errors factory classes like any other owned file', function () use ($apiErrorSpec, $tempOut) { + $out = $tempOut(); + + config()->set('openapi-laravel.controllers.path', $out.'/Http/Controllers/Api'); + config()->set('openapi-laravel.controllers.namespace', 'App\\Http\\Controllers\\Api'); + config()->set('openapi-laravel.routes.path', $out.'/routes/api.generated.php'); + + $this->artisan('openapi:generate', [ + '--spec' => $apiErrorSpec(), + '--output' => $out, + ])->assertSuccessful(); + + // The factory class is written as a CATEGORY_DATA file in its tag group. + $factory = $out.'/Pets/GetPetByIdErrors.php'; + expect(is_file($factory))->toBeTrue(); + + // Right after generation the whole set, factory included, is in sync. + $this->artisan('openapi:check', [ + '--spec' => $apiErrorSpec(), + '--output' => $out, + ])->assertExitCode(0); + + // Tamper the factory file: check must detect the drift (generate/check + // share the planner, so the factory files flow through both in lockstep). + file_put_contents($factory, file_get_contents($factory).' '); + + $this->artisan('openapi:check', [ + '--spec' => $apiErrorSpec(), + '--output' => $out, + ]) + ->expectsOutputToContain('[changed] '.$factory) + ->assertExitCode(1); +}); + it('stays in lockstep with generate when routes.middleware and routes.prefix are configured (#71)', function () use ($serverSpec, $tempOut) { $out = $tempOut(); $routesOut = $out.'/routes/api.generated.php'; diff --git a/tests/Feature/Emitter/ApiErrorRoundTripTest.php b/tests/Feature/Emitter/ApiErrorRoundTripTest.php new file mode 100644 index 0000000..82ec143 --- /dev/null +++ b/tests/Feature/Emitter/ApiErrorRoundTripTest.php @@ -0,0 +1,149 @@ +Errors` + * factories: generate the full scaffold from api-error.yaml, load the emitted + * classes (Data classes, the inlined ApiError support class, the factory + * classes, the abstract controller) into the booted app, register the GENERATED + * routes, implement the abstract controller with plain returns plus error + * throws, and drive real HTTP requests through the REAL Laravel exception + * handler with zero bootstrap/app.php registration. + * + * Proves the point of the whole feature: a concrete method typed to return the + * success DTO answers a spec-declared error by THROWING (never a `return`), so + * the declared return type stays satisfied, and the thrown ApiError renders the + * exact spec error body at the exact status. Both the generated factory + * (`GetPetByIdErrors::notFound(...)`) and the direct carrier + * (`ApiError::notFound(...)`) paths, a shared-schema factory (badRequest AND + * notFound both forwarding to PetErrorData), and the untouched success path. + */ +beforeEach(function () { + static $routesPath = null; + + if ($routesPath === null) { + $dir = sys_get_temp_dir().'/oal_apierror_roundtrip_'.getmypid(); + if (! is_dir($dir)) { + mkdir($dir, 0777, true); + } + + $document = (new SpecParser)->parseFileToDocument(__DIR__.'/../../Fixtures/server/api-error.yaml'); + $generator = new ModelGenerator; + $modelFiles = $generator->generate($document); + $options = new ServerOptions; + $descriptors = (new OperationCollector($options, $generator->registry(), null, $generator))->collect($document); + $controllers = (new ControllerGenerator($options))->generate($descriptors); + $routes = (new RouteGenerator($options))->generate($descriptors); + + loadGeneratedFiles($dir, [ + ...array_values($modelFiles), + ...array_values($generator->pathFiles()), + ...array_values($generator->errorFactoryFiles()), + ]); + loadGeneratedFiles($dir.'/Support', array_values($generator->supportFiles())); + loadGeneratedFiles($dir.'/Controllers', array_values($controllers)); + + $concrete = <<<'PHP' + 7, 'name' => $pet->name]); + } + + public function show(int $petId): PetData + { + // The generated factory: one call, no manual status, no manual wrapper. + if ($petId === 999) { + throw GetPetByIdErrors::notFound(message: 'No pet 999.'); + } + + // The ApiError escape hatch: build the Data class by hand and throw it. + if ($petId === 998) { + throw ApiError::notFound(PetErrorData::from(['message' => 'No pet 998.'])); + } + + return PetData::from(['id' => $petId, 'name' => 'Rex']); + } + + public function update(PetData $pet, int $petId): PetData + { + // Shared schema across two statuses: both forward to PetErrorData. + if ($petId === 400) { + throw UpdatePetErrors::badRequest(message: 'Bad update.'); + } + if ($petId === 999) { + throw UpdatePetErrors::notFound(message: 'No pet.'); + } + + return PetData::from(['id' => $petId, 'name' => $pet->name]); + } + } + PHP; + file_put_contents($dir.'/ConcreteControllers.php', $concrete); + require_once $dir.'/ConcreteControllers.php'; + + $routesPath = $dir.'/'.$routes->filename(); + file_put_contents($routesPath, $routes->code); + } + + require $routesPath; +}); + +it('answers a spec error thrown through the generated factory with the exact schema body at 404', function () { + // The declared return type stays PetData throughout: the 404 branch throws, + // never returns, so Laravel's real exception handler renders the ApiError. + $this->getJson('/pets/999') + ->assertStatus(404) + ->assertExactJson(['message' => 'No pet 999.']); +}); + +it('answers a spec error thrown through the ApiError escape hatch too', function () { + $this->getJson('/pets/998') + ->assertStatus(404) + ->assertExactJson(['message' => 'No pet 998.']); +}); + +it('leaves the success path completely unaffected', function () { + $this->getJson('/pets/7') + ->assertOk() + ->assertJsonPath('id', 7) + ->assertJsonPath('name', 'Rex'); +}); + +it('round-trips both statuses of a shared-schema factory (badRequest and notFound to one Data class)', function () { + $this->putJson('/pets/400', ['id' => 400, 'name' => 'X']) + ->assertStatus(400) + ->assertExactJson(['message' => 'Bad update.']); + + $this->putJson('/pets/999', ['id' => 999, 'name' => 'X']) + ->assertStatus(404) + ->assertExactJson(['message' => 'No pet.']); +}); + +it('does not touch the declared success status of a create operation (201)', function () { + $this->postJson('/pets', ['id' => 1, 'name' => 'Bella']) + ->assertStatus(201) + ->assertJsonPath('id', 7) + ->assertJsonPath('name', 'Bella'); +}); diff --git a/tests/Feature/Support/ApiErrorRenderTest.php b/tests/Feature/Support/ApiErrorRenderTest.php new file mode 100644 index 0000000..5e65b3a --- /dev/null +++ b/tests/Feature/Support/ApiErrorRenderTest.php @@ -0,0 +1,58 @@ + $data + */ + public function __construct(private array $data) {} + + /** + * @return array + */ + public function jsonSerialize(): array + { + return $this->data; + } +} + +it('renders the carried body at the documented status for each named factory', function (string $factory, int $status) { + $body = new ApiErrorRenderFakeBody(['message' => 'boom', 'code' => $status]); + + /** @var ApiError $error */ + $error = ApiError::{$factory}($body); + $response = $error->render(Request::create('/x')); + + expect($response)->toBeInstanceOf(JsonResponse::class) + ->and($response->getStatusCode())->toBe($status) + ->and(json_decode((string) $response->getContent(), true))->toBe(['message' => 'boom', 'code' => $status]); +})->with([ + 'badRequest' => ['badRequest', 400], + 'unauthorized' => ['unauthorized', 401], + 'forbidden' => ['forbidden', 403], + 'notFound' => ['notFound', 404], + 'conflict' => ['conflict', 409], + 'unprocessable' => ['unprocessable', 422], + 'tooManyRequests' => ['tooManyRequests', 429], + 'serverError' => ['serverError', 500], +]); + +it('renders an arbitrary status from the general constructor', function () { + $response = (new ApiError(new ApiErrorRenderFakeBody(['detail' => 'nope']), 451))->render(Request::create('/x')); + + expect($response)->toBeInstanceOf(JsonResponse::class) + ->and($response->getStatusCode())->toBe(451) + ->and(json_decode((string) $response->getContent(), true))->toBe(['detail' => 'nope']); +}); diff --git a/tests/Fixtures/conformance/conformance-3.1.yaml b/tests/Fixtures/conformance/conformance-3.1.yaml index d95bcfd..4da906a 100644 --- a/tests/Fixtures/conformance/conformance-3.1.yaml +++ b/tests/Fixtures/conformance/conformance-3.1.yaml @@ -77,8 +77,14 @@ paths: application/json: schema: $ref: '#/components/schemas/Widget' - default: # default response - description: Unexpected error + '404': # concrete error response: exercises the + description: Widget not found # generated Errors factory + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorObject' + default: # default response (v1 omits a factory method + description: Unexpected error # for it: no single concrete status) content: application/json: schema: diff --git a/tests/Fixtures/corpus-baseline-v0.11.0.json b/tests/Fixtures/corpus-baseline-v0.11.0.json index 92c6412..f89f07a 100644 --- a/tests/Fixtures/corpus-baseline-v0.11.0.json +++ b/tests/Fixtures/corpus-baseline-v0.11.0.json @@ -1,21 +1,21 @@ { - "1password-connect.yaml": "2e94f4817462244d0c39b03d1254fdb9e4d6a1c53d4a3f2e8b9282ec98b0ca24", + "1password-connect.yaml": "ecb701e9f155f93d83c71973403757d56257e7fac1a01250086811db40f79109", "ably.json": "8d06970c47325e0de83419bad80e6bc0bcfbf5f215950dcaceaeced3c4845eca", - "ably_control.json": "389e2bef69aaa0bdac774d7a0e4f9b14bdf22ddd0ec2228e7d75767f44322bae", - "adyen-checkout.yaml": "3323dc1c16b9d86c2c55e2acc35746bda0c2ad0a104d73488dbe89f63fa63354", - "adyen-legal-entity.yaml": "42ef86ae524c365fb04ed7482d5ca26d26d2312aa93dac18c70fbf89b11d3f34", - "airflow.json": "e52f6ee128478579eeccc92dd70f04d39f0474e53a6172daa065b5abf430776f", - "amadeus.json": "eeacf988b7575f43190b7683fd369f12c6f4edfc9e03eb041db7ea3e5f951a5c", + "ably_control.json": "48d16b038d56467715a7e9ceeccc73ae2fa511b2a30bbbec5018cda19dcaafef", + "adyen-checkout.yaml": "91eb8ff4f81c8e6c399c1dde0ff6179871b9480e538364c13c31f98e08c54ec3", + "adyen-legal-entity.yaml": "51904db801bf412074f694c813a3e89f6bfcac832e2ed9e6db31380979393c34", + "airflow.json": "0f90826641c139ef8227a772faf4d30c52639144a9e3c034c596010e50960436", + "amadeus.json": "4167ee953de257983df84a50151569cba02367a9c47bb678f283a60488f6af73", "apisguru.json": "2d5125564c7dd85dd59dce9daff022f11430a54ea3dae5112c4378885513ec12", - "apple_appstore.json": "5e09ad705c889a0544743e9df34658e4988873dccd8bf8ed898d6a99be4058c2", + "apple_appstore.json": "d782bd05a1d87b26a54aed48141b663ef3396a87cfda4f0b5cf80a1bf3065ee8", "appwrite.json": "09577e64d7287b6ef779b996b6b8b414e6759fdb0fde890430f87b655d701fe1", - "asana.json": "b82dedabd07b440a0b8a04e576eac103460a3127c61b850f97f9b39738d4e92f", - "aws_apigateway.json": "31558e8685a2dcb92823be8ba693836aaa7d248a49031cc2160e21acc01c10a9", + "asana.json": "edad44701230ce479bdc406b03e58be83aa89057678fe7afe5f475446690ab16", + "aws_apigateway.json": "e40ab4a19a3ece403d4432d63bcdf173bacdc34bc95ae56492dc4fad68aceeef", "aws_cloudformation.json": "2384b16b8e267f56a11dc5985bb4b235a58ed08d254e78833b87921f9d2ff739", - "aws_cognito.json": "93faf457f56ef7ac6cc4fc9e093ae485eccf11628281c63c1c28a85f40f538cb", - "aws_dynamodb.json": "730d67c16140c1e25c31823e5588cb9eaccf87f7e07d321af717f809323063ab", - "aws_iam.json": "1ec58a51fa667c068bf27f72d90c46d62f2e89ea23e32ef3e05efe423e424076", - "aws_lambda.json": "20613ea1b0c0ef17d04f69f1545179ecee228059659482766343e71434ae6d37", + "aws_cognito.json": "c4ea77ff75b599c0eaddcec82127a5949c9a14f7e3adf9ae30da832f5f81559b", + "aws_dynamodb.json": "14114b2845616f50892be94d25cd3163556fcb3529966b27705168ee1404a1e4", + "aws_iam.json": "d0967478c005a321d70093144a7ae5a04671559dd9808916cd863823b8cb04d3", + "aws_lambda.json": "f07a2904cac0f41943976ec50a60796f0a251257ba6c26504720e3caa6405a1c", "aws_rds.json": "6acafc98f5efd3e5d400bf8d6dcb3343be23efe952e17add411fe7510a6477d4", "aws_s3.json": "fe56a114cc6490370352b8028fe3386483d3a64057327c7b9c56f297ece49e3b", "aws_sns.json": "556eec616ba7e253c74241a9dfb9680860d40d6ba10d955337c44a31d249b9f0", @@ -24,15 +24,15 @@ "bbc.json": "92605bca080fde343aa0fda8f76de083f6d364e00269b0a32c9113b3e4909b0d", "bigdatacloud.json": "1ff53b6e4d90cddb5ac0c44546a9368b9f4efad2fe8187e6c59435e1c2972280", "bikewise.json": "ebacd2bb2ddcdbadd690c9a2ea73cedfad5c8c52b4d53d70e525dde650a5890d", - "bitbucket.json": "012282997505f2f95193ed5249f53427f0eb0ecbe7ff7902abc221eb18e05360", - "box.json": "7f89fd9ab8d851d7cab08d285c8a8dc5b074d16e5b5f7fd3d25f4b9aba1f0112", + "bitbucket.json": "3c48ac94041c06d5ed496b9df9fc29124ee1936e48ca3d380f83b34cfb526041", + "box.json": "8dff4869ce7c2588380bfd8460e96c11cb1e5672522c9b997fc244f35d020594", "braze.json": "9a71106e801be815d034fa7a2686218b4c023f73e0729462f6378f6017e99d5f", "brex.json": "63e6de6ee2b70b855f07cb0cba7a7ff06e4670de78eec9ca066d0b25f7e99c51", "bungie.json": "3829eb4afec5374f088053c5beb4c305a4c272970f765e3abaaf26a5d426d821", "bunq.json": "506c94276360af38310f805ae58ba6995a09f02bffbb0430e95c6b7d1376403b", "canada_holidays.json": "34c19b08c0a20ece1fce8c623dacea3548190ed238215a8e09d188ff48b55cad", "circleci.json": "051c29d94f930115240785553d591475646d46af7ce05be04fb804e6de3288f5", - "clevercloud.json": "3c0af5bb9caec22c127ca1f4aaa29b52f76aae0c91dd129e39fc49ed37dba170", + "clevercloud.json": "6242f770ec1eacffee58c3776ed28591ed530959f20bd9a86beed53ff63e42f6", "clicksend.json": "022584af297d9e08772f2b4e3ed626310be7654b71c03fa18de78b2d581da002", "clickup.json": "c01f2c4c2dd154f5a7d6728cc50312d484bdd746445e5511586e75f6115bd06b", "codat_accounting.json": "64d6fb804e098b9a6497c61474d54204d7a32c6e5da728575910c0f57eefe44d", @@ -41,18 +41,18 @@ "devto.json": "f8ea4c1e86e11a9cb16e93ac33450bbcd8912e2cfd55b690ccc48e8badb86dd4", "digitalocean.json": "d57afde51597981a26bd0d8edb704adfb77ecfeb79df228bafd33f7221bc0055", "discourse.json": "006932c02117a0da0569b58caf30e285d5f47b7a54a93f0bc9ea7ce26d0b19d8", - "dnd5e.json": "04e3a9752a4dd83e34d78b7e7153bf53c07ce63ce75146199538344d98677fd0", - "docker.json": "4fc12fed314e68ac283bf11e976f71139f83a629f8e93358656eb3d891babd87", + "dnd5e.json": "68aa4f001c5136fcec44097523115a60ddda6b07895ca4b63acaac9a90d09791", + "docker.json": "b1c9650c65dbb58a69a9ed05d0fe778ea22f0ee29350a69af464531ad00eabe1", "docusign.json": "79cb7065505c52983972f84d65570bd5e89e179093c8db20e8be7720323e5403", - "dracoon.json": "c805cfc1315840fc28262ac8ce95c59c75726cf77f3708e4d67cb2193af75536", + "dracoon.json": "ab98046afbfaa902efbe55f199c4d83ca176cfba505814c6c755e8319ee6c691", "ebay_fulfillment.json": "44c17dc2099ea07bc6f40d492cfdbdf8930c552a3e7a07bdfa76b06f86731426", "ebay_marketing.json": "8e33dc4a784dc906f01f2c6f96e90006ca4ca12518093407111f852403328a20", - "elevenlabs.json": "7604c6ac2b766a704724e1f6d96a40765f301ad68793c1c3ac81bef7fea1187d", + "elevenlabs.json": "00a245fd13e2686fca5d18a14e2f1684793e9a5ca6d7dfb742627bf7a89faebe", "exchangerate.json": "e423074045c279af42d2ca60eb2c0b9798dbab09f919c1768bafe1b0b2946674", "flickr.json": "a8330d212bbda6a1908cb17fa7ce3091fed97911ef169402a32813958ca2a530", "gettyimages.json": "8189748242e2e7fed1fc556d1a7abbf972d0030f3ca90261eee2953b004be449", "giphy.json": "b70e7464a229360228366b0e3cb46b574df18def6af6f703d5803375bc2e39c9", - "github.json": "a2a1d42e68c3813e89615de2e6523fa5856fd7f261a666056a70b2a27a4f6bd4", + "github.json": "f6b344e1b537757d7df6bfc7b303a3477c3d9fdaa6825d8c10b3082deffecb37", "google_bigquery.json": "7169282debce4eb4d3d4e7b41c4c1d6f8e5093fadedac11aaeb5bec213783602", "google_calendar.json": "9e87b8cf4fa1088f0cc1b09ed0c4d8eb1f9f3c5df49b1cd53d83fbfccb2e9e58", "google_cloudrun.json": "a200f8dc9477d3c1b1e5b3ebef9846e0e78509f86652acf27071dc88f59a8407", @@ -70,11 +70,11 @@ "google_translate.json": "81ad75a23810159ac80e3d954a1097dfcf97d82bfd8f4324a4517c5a44b2f317", "google_tts.json": "2c08dc0d66b26715ac074eae41823d9b095c98961000b38f2c543d4e5428dfef", "google_vision.json": "14ea7e6a72f44c979ad6590df6693e27c7a760c17426a6a7ea19a1a9342101e5", - "here_positioning.json": "b29a6c283898d28760cd5bbe9199dfd15b152efcaeadca7d441a702c3f0fc35d", + "here_positioning.json": "9bf40c3c8580fee85030ef13049d4791c5164034c88ec200f672ae4e384685fc", "here_tracking.json": "d24c402762e253bdb1b331b358dafb754185b5827b8a76d48a49440a391041a0", "ipgeo.json": "d79a7ed96c6cd6881230b44d2756ce1a6d2df9073a27996254d32acdfa479606", - "jira.json": "bdaa3af7881ebb05ff53711bd17eb88a6b6a5aaa12ddc61184ae815ae42f1d42", - "klarna.json": "35f41718d71a74923ee2bc66223fc4f9f32ede00cc5584016f572dccec013ee8", + "jira.json": "465f989f4ecb7271d7e9b0aad585b4431be17341cbcada052208e09429bc58d8", + "klarna.json": "6339abcbd10731f2a3b788eb5fbfd195ccf78ec8b662a065f00dcef8c8905b55", "linode.json": "a805736d2e72338fae40fe2ba950504cb936855873420a39ab67fdf36ee0f4d9", "lufthansa.json": "e021bdcf53bacb046a34bf7e1ba735b15a7e455c5f26ddd7fa7014ce103d90a9", "medium.json": "c8d9eddf75bde31ea390b62c38580e988dee41200049a3d358b0936d7602eb30", @@ -83,24 +83,24 @@ "nytimes.json": "58f971f50a4a0cb0e21459f746d3331d1d8d3a2df200e67bd63c254d1e090d61", "okta.json": "ac9e57f511299e667b562c0807dfe3ba5662e44549881c68edf7f6a9eae9c618", "open-meteo.yaml": "1dd2676fa2a0c8edec659bef4280bfa1f651189463f5ecd777a6b07a55266dda", - "openai.yaml": "545cd2cb9aa6441c7c28b7fc038dd803ccc06a87c004ceea027907792a785492", + "openai.yaml": "106bc6ad295c45a0b93b757dda7d38bd863821fefe87467b2415ac1f87cbd29d", "openbanking.json": "1ab49e7f2e2d1238bea630e131f1d6a29585708b290a43283c5215a33aee8a0f", "petstore-3.0.yaml": "6a44b6de59610377ce4ec534f543a884a2085fcac7fa634303285c739ab078f0", "pinecone.json": "14f18364c04a9704a565bddc8d7f49c94459f1f31d8b664b385e5914f2dafcd9", "plaid.json": "92e1dd06a27d4b57ccd92a7daaf3afc17594c2aaf93f153fd7ee7130f7d14acc", "postman.json": "3877c442064b415ddfea4cec7f43994087e6f8eee5cc953fb7ee1068db7a7fad", "rawg.json": "4537a738188b73041319c546d0e045172d0af58ebb554e60c45ff4b04030019c", - "redocly-museum.yaml": "117c9d904d27cc7a30db0cc870d89374ff3b3f5f306fd6115dbe84d214b6dac6", + "redocly-museum.yaml": "b398e39cecbb80757e3fb3041df4ec7c304d6b48686fb3c984fae0c527283b48", "resend.json": "02b69917b76a9a911d3dd654e00d90e09160d5416747ad436831ddec638b847e", "reverb.json": "4149c338ed85f31da02cb31ffda07017f90432ce14d97f64c8fc0b1ec86df19a", - "sendgrid.json": "3da5ad648ef7e87b492d803316247d6c7e26d7e442609ee6ecd2c423d5831819", + "sendgrid.json": "47a88edf1f0a81d1b3a42472dc03652279fb2cd6a55763a0051ae06f630cff83", "sentry.json": "0f0a4c5d38405f20d2dab25bce4829edf4a7c34adabaa23d350fb33c19044aa7", "shipstation.json": "c01f2c4c2dd154f5a7d6728cc50312d484bdd746445e5511586e75f6115bd06b", "shutterstock.json": "ce0d39eafb228a796e9b9760730bde2b19598e364b6a5d84793815f4c7131db3", "slack.json": "fb719293ae1d339aed0c6a8abc4f4250492565ed56f379f4799b41172f993866", "snyk.json": "ff7630502d18ef3110635488b56d9ab40f4028c912c9cd3106e6383b1ae8cf9c", - "soundcloud.json": "213eaec519f2970983070dab0e45baa6f14fa0055653b2fcabdf32e4243e8b02", - "spotify.yaml": "8495c816664d879b49d1be455300688685e9e299e30ddcf738c731f0c8bb0e69", + "soundcloud.json": "fe9fcfd149c619e5392667d03b89b5f4cab06ab4d53b6ca50b7ada72cd7ee2a9", + "spotify.yaml": "a5866f1175730393c43c81783b5a21f8727ebfb0b33d40120e1224d7956f407b", "square.json": "25ba01de86497cb7bd886828857a2792f0496c5f0c03885e237e2e7cec8e0df3", "stackexchange.json": "23c70bdcd2ba7b77bc285b6a0d590b2e78ca2cc68f4059cf81d42c34df5acde8", "stripe.json": "68dda0b2216b24f07a951fbfaa955b3affaf1fa4510f31698bbdf1a38600c666", @@ -116,17 +116,17 @@ "twilio_video.json": "41c5644c73908bc789605c1489a51e4aeef227ff5fd0c253de39e515866e03b6", "twitter.json": "0e759b5b8d48877a554542898e1816e7ed90daa3d08d015e88fe9dc68e32a994", "vercel.json": "776dae0679d1e1fff44a605ec4098cf4d85ad0fc687dcf85a5931d4813f0f2e3", - "vimeo.json": "b7ff46ed87062c8afb54ac304fe9c85a3fbcc424a8da7692198cb8fb624f1fc3", + "vimeo.json": "c9b02f10ea9e3d8f562d1d9b941cd38c5a02c0f7ab7aa32dc7bf69e683bca742", "vonage.json": "2ff263de6d34fb15620810ecf4e203210e20c37ad5578787b052a82637494269", "wayback.json": "a3f7c2a37022fd5471ac23bcabbc9605ee873752e94b518bc7b88a6243c1a045", "weather_visual.json": "b822ba86865ab9145216dff6317cec2460f2037a7fd14116c796241ce1521357", - "webflow.json": "9197e0620ec572103dc3f5d933223095eb053139dcaa0c73bf92ab850e2adeef", + "webflow.json": "345b2557772b52120de3e1a92f86b8b73b4f684a574ca3a1b18a8264baac976b", "wolfram.json": "2ae0df6603992293185e5cb898af2b523125cae632fd8cd08565870182f90aaa", "wordnik.json": "9e524efeea0cb7d3f6fcbbd3f61da4ccdb403d107eab04acf373c44a885e65b8", "worldtime.json": "9529fe0b1dd9137d8dcd00bc9f7bbc634999d14de9064ac64e35c45a4c2bfbc0", - "xero.json": "7488788caef5e5bfdc9597f34cab944eceb3d03112c5c14c6c43599031b1b421", + "xero.json": "339a8a81c4339144e222c7f1d34e4f2dba695ddad848e8f23e5387de7b0a9949", "xero_assets.json": "5a2ee47426d950ccf3d556ce303fca8d9db8d1f29bee6344788d941851ec5455", "youtube.json": "8c5c1cae6d26c453e37eab271b01eb1a96c5561898e0da89a23c7ad039983b58", "zoom.json": "4458e1d27cb695fd1f85f2e40895e2a650bae67661abb4c538006af01c5a6916", - "zuora.json": "27c6625fa732113f84d711745ca7ae15b3b527d914a30ca489dc1f699719baa8" + "zuora.json": "e7fc214da5e9db7b2bc929599aa27dd6be0fc1083eb26f4994007c72336607f4" } diff --git a/tests/Fixtures/server/api-error.yaml b/tests/Fixtures/server/api-error.yaml new file mode 100644 index 0000000..df87295 --- /dev/null +++ b/tests/Fixtures/server/api-error.yaml @@ -0,0 +1,122 @@ +openapi: 3.0.3 +info: + title: API Error Demo + version: 1.0.0 +paths: + /pets/{petId}: + get: + tags: + - pets + operationId: getPetById + parameters: + - name: petId + in: path + required: true + schema: + type: integer + responses: + '200': + description: The pet. + content: + application/json: + schema: + $ref: '#/components/schemas/Pet' + '404': + description: Pet not found. + content: + application/json: + schema: + $ref: '#/components/schemas/PetError' + put: + tags: + - pets + operationId: updatePet + parameters: + - name: petId + in: path + required: true + schema: + type: integer + requestBody: + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/Pet' + responses: + '200': + description: The updated pet. + content: + application/json: + schema: + $ref: '#/components/schemas/Pet' + '400': + description: The update was rejected. + content: + application/json: + schema: + $ref: '#/components/schemas/PetError' + '404': + description: Pet not found. + content: + application/json: + schema: + $ref: '#/components/schemas/PetError' + /pets: + post: + tags: + - pets + operationId: createPet + requestBody: + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/Pet' + responses: + '201': + description: The created pet. + content: + application/json: + schema: + $ref: '#/components/schemas/Pet' + '400': + description: The request was malformed. + content: + application/json: + schema: + $ref: '#/components/schemas/BadRequestProblem' + '404': + description: A referenced resource does not exist. + content: + application/json: + schema: + $ref: '#/components/schemas/NotFoundProblem' +components: + schemas: + Pet: + type: object + required: [id, name] + properties: + id: + type: integer + name: + type: string + PetError: + type: object + required: [message] + properties: + message: + type: string + BadRequestProblem: + type: object + required: [reason] + properties: + reason: + type: string + NotFoundProblem: + type: object + required: [resource] + properties: + resource: + type: string diff --git a/tests/Unit/Console/GenerationPlannerTest.php b/tests/Unit/Console/GenerationPlannerTest.php index 697d220..f89460e 100644 --- a/tests/Unit/Console/GenerationPlannerTest.php +++ b/tests/Unit/Console/GenerationPlannerTest.php @@ -100,6 +100,67 @@ ->and($plan->filesByCategory(PlannedFile::CATEGORY_ROUTES))->toBe([]); }); +it('plans the ApiError support class and the Errors CATEGORY_DATA files when controllers are enabled', function () use ($tempOut) { + $out = $tempOut(); + $spec = __DIR__.'/../../Fixtures/server/api-error.yaml'; + $plan = (new GenerationPlanner)->plan(new GenerationRequest( + spec: $spec, + output: $out, + namespace: 'App\\Data', + suffix: 'Data', + maxDepth: 64, + maxBytes: null, + controllers: true, + controllerPath: $out.'/Http', + controllerNamespace: 'App\\Http\\Controllers\\Api', + routes: true, + routesPath: $out.'/routes/api.generated.php', + )); + + $support = array_map(static fn (PlannedFile $f): string => $f->path, $plan->filesByCategory(PlannedFile::CATEGORY_SUPPORT)); + $data = array_map(static fn (PlannedFile $f): string => $f->path, $plan->filesByCategory(PlannedFile::CATEGORY_DATA)); + + // The Errors factories are CATEGORY_DATA, placed in their tag + // group (Pets) exactly like the QueryData/ResponseData siblings. + expect($support)->toContain($out.'/Support/ApiError.php') + ->and($data)->toContain($out.'/Pets/GetPetByIdErrors.php') + ->and($data)->toContain($out.'/Pets/UpdatePetErrors.php') + ->and($data)->toContain($out.'/Pets/CreatePetErrors.php'); +}); + +it('vetoes both the ApiError support class and the Errors files under --no-controllers, keeping the Data classes', function () use ($tempOut) { + $out = $tempOut(); + $spec = __DIR__.'/../../Fixtures/server/api-error.yaml'; + + // controllers: false, routes: true proves the veto keys off controllers, + // not routes: an Errors class exists only to be thrown from a + // concrete controller, and ApiError only to carry that throw. + $plan = (new GenerationPlanner)->plan(new GenerationRequest( + spec: $spec, + output: $out, + namespace: 'App\\Data', + suffix: 'Data', + maxDepth: 64, + maxBytes: null, + controllers: false, + controllerPath: $out.'/Http', + controllerNamespace: 'App\\Http\\Controllers\\Api', + routes: true, + routesPath: $out.'/routes/api.generated.php', + )); + + $support = array_map(static fn (PlannedFile $f): string => $f->path, $plan->filesByCategory(PlannedFile::CATEGORY_SUPPORT)); + $data = array_map(static fn (PlannedFile $f): string => $f->path, $plan->filesByCategory(PlannedFile::CATEGORY_DATA)); + + // ApiError and every factory drop together; the underlying error Data + // classes are ordinary model output and stay (surgical veto). + expect($support)->not->toContain($out.'/Support/ApiError.php') + ->and($data)->not->toContain($out.'/Pets/GetPetByIdErrors.php') + ->and($data)->not->toContain($out.'/Pets/UpdatePetErrors.php') + ->and($data)->not->toContain($out.'/Pets/CreatePetErrors.php') + ->and($data)->toContain($out.'/Pets/PetErrorData.php'); +}); + it('plans byte-identical Data files with and without the server scaffold (lockstep)', function () use ($querySpec, $tempOut, $request) { $out = $tempOut(); $planner = new GenerationPlanner; diff --git a/tests/Unit/Emitter/EnumBackingTest.php b/tests/Unit/Emitter/EnumBackingTest.php new file mode 100644 index 0000000..968deab --- /dev/null +++ b/tests/Unit/Emitter/EnumBackingTest.php @@ -0,0 +1,87 @@ + $enum + * @return array + */ +function generateBackedEnum(array $enum): array +{ + $document = [ + 'openapi' => '3.0.3', + 'info' => ['title' => 'Test', 'version' => '1.0.0'], + 'paths' => new stdClass, + 'components' => ['schemas' => ['Code' => ['type' => 'string', 'enum' => $enum]]], + ]; + + $spec = (new OpenApiReader)->read($document); + expect($spec)->toBeInstanceOf(OpenApiDocument::class); + + return (new ModelGenerator)->generate($spec); +} + +it('int-backs an enum whose every value is a canonical int string', function () { + $code = generateBackedEnum(['0', '1', '42'])['Code']->code; + + expect($code)->toContain('enum Code: int') + ->and($code)->toContain('case Value0 = 0;') + ->and($code)->toContain('case Value1 = 1;') + ->and($code)->toContain('case Value42 = 42;'); +}); + +it('string-backs an enum with a leading-zero value instead of corrupting it to an int (#145)', function () { + // "01" is not the canonical decimal form of 1: int-backing would emit + // `case ... = 1`, silently rewriting the wire value "01" to 1. + $code = generateBackedEnum(['01', '02', '03'])['Code']->code; + + expect($code)->toContain('enum Code: string') + ->and($code)->toContain("'01'") + ->and($code)->toContain("'02'") + ->and($code)->toContain("'03'") + // No int corruption: the bare-int literal `= 1;` must never appear. + ->and($code)->not->toContain('= 1;') + ->and($code)->not->toContain('enum Code: int'); +}); + +it('string-backs (not duplicate-int) an enum mixing "01" and "1" so it never fatals (#145)', function () { + // Under the old all-digits rule both "01" and "1" int-backed to 1, emitting + // two `case ... = 1;` lines: a fatal "Duplicate value in enum". String + // backing keeps them distinct. + $code = generateBackedEnum(['1', '01'])['Code']->code; + + expect($code)->toContain('enum Code: string') + ->and($code)->toContain("'1'") + ->and($code)->toContain("'01'") + // The two cases carry distinct string literals, not a duplicated int 1. + ->and(substr_count($code, '= 1;'))->toBe(0); +}); + +it('string-backs a multi-digit leading-zero value like a git file mode (#145)', function () { + $code = generateBackedEnum(['100644', '040000', '120000'])['Code']->code; + + expect($code)->toContain('enum Code: string') + ->and($code)->toContain("'040000'") + // "040000" must not collapse to its int form 40000. + ->and($code)->not->toContain('= 40000;'); +}); + +it('keeps a signed-int string string-backed, unchanged from prior behaviour (#145)', function () { + // The unsigned-digit gate is retained, so a "-1" member stays string-backed + // exactly as before: no widening of int-backing to signed forms. + $code = generateBackedEnum(['0', '1', '-1'])['Code']->code; + + expect($code)->toContain('enum Code: string') + ->and($code)->toContain("'-1'"); +}); diff --git a/tests/Unit/Emitter/ErrorFactorySynthesizerTest.php b/tests/Unit/Emitter/ErrorFactorySynthesizerTest.php new file mode 100644 index 0000000..016b77c --- /dev/null +++ b/tests/Unit/Emitter/ErrorFactorySynthesizerTest.php @@ -0,0 +1,304 @@ +Errors` factory synthesizer, + * wired exactly like the planner (generate() then collect() with the generator + * wired in) and inspecting $generator->errorFactoryFiles(). + */ + +/** + * Generate + collect an in-memory document, returning the wired generator and + * collector so the factory files, support set, and warnings can all be read. + * + * @param array $document + * @return array{0: ModelGenerator, 1: OperationCollector} + */ +function factoriesFor(array $document): array +{ + $spec = (new OpenApiReader)->read($document); + $generator = new ModelGenerator; + $generator->generate($spec); + $collector = new OperationCollector(new ServerOptions, $generator->registry(), null, $generator); + $collector->collect($spec); + + return [$generator, $collector]; +} + +/** + * A one-operation error document: a GET whose declared error responses are + * $status => componentSchema, over a shared component pool. + * + * @param array $errors status code => component schema name + * @param array> $schemas component name => schema + * @return array + */ +function errorDoc(array $errors, array $schemas, string $tag = 'things'): array +{ + $responses = ['200' => ['description' => 'ok', 'content' => ['application/json' => ['schema' => ['$ref' => '#/components/schemas/Ok']]]]]; + foreach ($errors as $status => $component) { + $responses[(string) $status] = ['description' => 'e', 'content' => ['application/json' => ['schema' => ['$ref' => '#/components/schemas/'.$component]]]]; + } + + return [ + 'openapi' => '3.0.3', + 'info' => ['title' => 'E', 'version' => '1.0.0'], + 'paths' => ['/things' => ['get' => ['tags' => [$tag], 'operationId' => 'getThing', 'responses' => $responses]]], + 'components' => ['schemas' => ['Ok' => ['type' => 'object', 'properties' => ['id' => ['type' => 'integer']]]] + $schemas], + ]; +} + +$problem = ['type' => 'object', 'required' => ['message'], 'properties' => ['message' => ['type' => 'string']]]; + +it('emits one static factory method per named-component object error slot', function () use ($problem) { + [$generator] = factoriesFor(errorDoc(['404' => 'Problem'], ['Problem' => $problem])); + + $files = $generator->errorFactoryFiles(); + expect($files)->toHaveKey('GetThingErrors'); + + $code = $files['GetThingErrors']->code; + expect($code)->toContain('final class GetThingErrors') + ->and($code)->toContain('use App\Data\Support\ApiError;') + ->and($code)->toContain('public static function notFound(string $message): ApiError') + ->and($code)->toContain('return new ApiError(new ProblemData(message: $message), 404);'); + + // ApiError is inlined because a factory class was actually emitted. + expect($generator->supportFiles())->toHaveKey('ApiError'); +}); + +it('emits two independent methods when one schema is shared across two statuses', function () use ($problem) { + [$generator] = factoriesFor(errorDoc(['400' => 'Problem', '404' => 'Problem'], ['Problem' => $problem])); + + $code = $generator->errorFactoryFiles()['GetThingErrors']->code; + + // Both methods forward to the SAME Data class at their OWN status. + expect($code)->toContain('public static function badRequest(string $message): ApiError') + ->and($code)->toContain('return new ApiError(new ProblemData(message: $message), 400);') + ->and($code)->toContain('public static function notFound(string $message): ApiError') + ->and($code)->toContain('return new ApiError(new ProblemData(message: $message), 404);'); +}); + +it('forwards to a distinct Data class per status when the schemas differ', function () { + [$generator] = factoriesFor(errorDoc( + ['400' => 'BadRequestProblem', '404' => 'NotFoundProblem'], + [ + 'BadRequestProblem' => ['type' => 'object', 'required' => ['reason'], 'properties' => ['reason' => ['type' => 'string']]], + 'NotFoundProblem' => ['type' => 'object', 'required' => ['resource'], 'properties' => ['resource' => ['type' => 'string']]], + ], + )); + + $code = $generator->errorFactoryFiles()['GetThingErrors']->code; + + expect($code)->toContain('return new ApiError(new BadRequestProblemData(reason: $reason), 400);') + ->and($code)->toContain('return new ApiError(new NotFoundProblemData(resource: $resource), 404);'); +}); + +it('flattens an array-of-DTO property to a bare array parameter with an @param docblock and no DataCollectionOf', function () { + [$generator] = factoriesFor(errorDoc( + ['422' => 'ValidationProblem'], + [ + 'Violation' => ['type' => 'object', 'required' => ['field'], 'properties' => ['field' => ['type' => 'string']]], + 'ValidationProblem' => [ + 'type' => 'object', + 'required' => ['message'], + 'properties' => [ + 'message' => ['type' => 'string'], + 'violations' => ['type' => 'array', 'items' => ['$ref' => '#/components/schemas/Violation']], + ], + ], + ], + )); + + $code = $generator->errorFactoryFiles()['GetThingErrors']->code; + + expect($code)->toContain('@param array $violations') + ->and($code)->toContain('public static function unprocessable(string $message, ?array $violations = null): ApiError') + ->and($code)->toContain('return new ApiError(new ValidationProblemData(message: $message, violations: $violations), 422);') + ->and($code)->not->toContain('DataCollectionOf'); +}); + +it('emits methods for the qualifying slots only and warns per skipped slot (partially-qualifying operation)', function () use ($problem) { + $document = errorDoc(['404' => 'Problem'], ['Problem' => $problem]); + // Add a non-qualifying inline-object 422 slot alongside the qualifying 404. + $document['paths']['/things']['get']['responses']['422'] = [ + 'description' => 'inline', + 'content' => ['application/json' => ['schema' => ['type' => 'object', 'properties' => ['msg' => ['type' => 'string']]]]], + ]; + + [$generator, $collector] = factoriesFor($document); + + $code = $generator->errorFactoryFiles()['GetThingErrors']->code; + expect($code)->toContain('public static function notFound(') + ->and($code)->not->toContain('unprocessable'); + + expect(implode("\n", $collector->warnings())) + ->toContain('Operation GET /things: the 422 error response did not get a throwable factory method'); +}); + +it('skips a discriminated-union error slot while still emitting the sibling plain-object method', function () use ($problem) { + // A named-component oneOf+discriminator schema is a Data class in the + // registry (an abstract morphable base), but it carries no captured + // constructor model, so it cannot be flattened into a factory method: the + // 400 slot is skipped (with a discriminated-specific warning) while the + // plain-object 404 slot still gets its notFound() method. This pins the + // constructorParamsFor() guard against a base/variant double-emission. + [$generator, $collector] = factoriesFor(errorDoc( + ['400' => 'ErrorUnion', '404' => 'Problem'], + [ + 'Problem' => $problem, + 'ErrorUnion' => [ + 'oneOf' => [ + ['$ref' => '#/components/schemas/CatError'], + ['$ref' => '#/components/schemas/DogError'], + ], + 'discriminator' => [ + 'propertyName' => 'kind', + 'mapping' => ['cat' => '#/components/schemas/CatError', 'dog' => '#/components/schemas/DogError'], + ], + ], + 'CatError' => ['type' => 'object', 'required' => ['kind', 'meow'], 'properties' => ['kind' => ['type' => 'string'], 'meow' => ['type' => 'string']]], + 'DogError' => ['type' => 'object', 'required' => ['kind', 'bark'], 'properties' => ['kind' => ['type' => 'string'], 'bark' => ['type' => 'string']]], + ], + )); + + $files = $generator->errorFactoryFiles(); + expect($files)->toHaveKey('GetThingErrors'); + + $code = $files['GetThingErrors']->code; + expect($code)->toContain('public static function notFound(string $message): ApiError') + ->and($code)->toContain('return new ApiError(new ProblemData(message: $message), 404);') + ->and($code)->not->toContain('badRequest') + ->and($code)->not->toContain('ErrorUnion'); + + expect(implode("\n", $collector->warnings()))->toContain( + 'Operation GET /things: the 400 error response did not get a throwable factory method (its schema resolves to a discriminated-union base or variant, which has no flattenable constructor).', + ); +}); + +it('flattens the READ variant of an error target: keeps plain and readOnly params, drops writeOnly', function () { + // Error responses are server OUTPUT, so the factory targets the READ + // variant: a readOnly property stays (it is returned to the client), a + // writeOnly property is dropped (it is only ever client input). + [$generator] = factoriesFor(errorDoc( + ['404' => 'ProblemRW'], + [ + 'ProblemRW' => [ + 'type' => 'object', + 'required' => ['detail'], + 'properties' => [ + 'detail' => ['type' => 'string'], + 'traceId' => ['type' => 'string', 'readOnly' => true], + 'debugToken' => ['type' => 'string', 'writeOnly' => true], + ], + ], + ], + )); + + $code = $generator->errorFactoryFiles()['GetThingErrors']->code; + + expect($code)->toContain('public static function notFound(string $detail, ?string $traceId = null): ApiError') + ->and($code)->toContain('return new ApiError(new ProblemRWData(detail: $detail, traceId: $traceId), 404);') + ->and($code)->not->toContain('debugToken'); +}); + +it('emits no factory and does not mark ApiError when no slot qualifies', function () { + // A scalar (non-object) error schema, and an unresolvable inline object only. + [$generator] = factoriesFor(errorDoc( + ['404' => 'ScalarError'], + ['ScalarError' => ['type' => 'string']], + )); + + expect($generator->errorFactoryFiles())->toBe([]) + ->and($generator->supportFiles())->not->toHaveKey('ApiError'); +}); + +it('does not mark ApiError when the only error slot is an inline object (deferred in v1)', function () { + $document = errorDoc([], []); + $document['paths']['/things']['get']['responses']['404'] = [ + 'description' => 'inline', + 'content' => ['application/json' => ['schema' => ['type' => 'object', 'properties' => ['msg' => ['type' => 'string']]]]], + ]; + + [$generator] = factoriesFor($document); + + expect($generator->errorFactoryFiles())->toBe([]) + ->and($generator->supportFiles())->not->toHaveKey('ApiError'); +}); + +it('never emits a factory method for a 1xx or 3xx response, even with an object schema', function () use ($problem) { + [$generator] = factoriesFor(errorDoc(['100' => 'Problem', '304' => 'Problem'], ['Problem' => $problem])); + + expect($generator->errorFactoryFiles())->toBe([]); +}); + +it('omits the default and 4XX/5XX wildcard slots entirely in v1, and stays silent when they are the ONLY error slots', function () use ($problem) { + $document = errorDoc([], ['Problem' => $problem]); + $document['paths']['/things']['get']['responses']['default'] = ['description' => 'd', 'content' => ['application/json' => ['schema' => ['$ref' => '#/components/schemas/Problem']]]]; + $document['paths']['/things']['get']['responses']['4XX'] = ['description' => 'w', 'content' => ['application/json' => ['schema' => ['$ref' => '#/components/schemas/Problem']]]]; + + [$generator, $collector] = factoriesFor($document); + + // No concrete 4xx/5xx slot, so no factory at all (default/wildcard omitted), + // no ApiError inlined, and NO warning (a factory-less operation is silent, + // to avoid flooding specs whose error bodies are entirely default/wildcard). + expect($generator->errorFactoryFiles())->toBe([]) + ->and($generator->supportFiles())->not->toHaveKey('ApiError') + ->and(implode("\n", $collector->warnings()))->not->toContain('did not get a throwable factory method'); +}); + +it('warns for a deferred default and 4XX wildcard slot on an operation that DOES get a factory', function () use ($problem) { + $document = errorDoc(['404' => 'Problem'], ['Problem' => $problem]); + $document['paths']['/things']['get']['responses']['default'] = ['description' => 'd', 'content' => ['application/json' => ['schema' => ['$ref' => '#/components/schemas/Problem']]]]; + $document['paths']['/things']['get']['responses']['4XX'] = ['description' => 'w', 'content' => ['application/json' => ['schema' => ['$ref' => '#/components/schemas/Problem']]]]; + + [$generator, $collector] = factoriesFor($document); + + // The concrete 404 gets a notFound() method; the default and 4XX wildcard + // slots are deferred in v1, and because the operation DID get a factory they + // are warned (consistent with the inline-object/non-object skip warnings). + $code = $generator->errorFactoryFiles()['GetThingErrors']->code; + expect($code)->toContain('public static function notFound('); + + $warnings = implode("\n", $collector->warnings()); + expect($warnings) + ->toContain('Operation GET /things: the default error response did not get a throwable factory method') + ->toContain('Operation GET /things: the 4XX error response did not get a throwable factory method'); +}); + +it('falls back to statusNNN for a concrete code not in the status-name table', function () use ($problem) { + [$generator] = factoriesFor(errorDoc(['480' => 'Problem'], ['Problem' => $problem])); + + $code = $generator->errorFactoryFiles()['GetThingErrors']->code; + expect($code)->toContain('public static function status480(string $message): ApiError') + ->and($code)->toContain('return new ApiError(new ProblemData(message: $message), 480);'); +}); + +it('reuses the committed api-error.yaml fixture: base, shared, and distinct operations', function () { + $spec = (new SpecParser)->parseFileToDocument(__DIR__.'/../../Fixtures/server/api-error.yaml'); + $generator = new ModelGenerator; + $generator->generate($spec); + (new OperationCollector(new ServerOptions, $generator->registry(), null, $generator))->collect($spec); + + $files = $generator->errorFactoryFiles(); + + expect(array_keys($files))->toBe(['CreatePetErrors', 'GetPetByIdErrors', 'UpdatePetErrors']) + ->and($files)->each->toBeInstanceOf(GeneratedFile::class); + + // Shared schema across statuses: badRequest and notFound both -> PetErrorData. + expect($files['UpdatePetErrors']->code) + ->toContain('return new ApiError(new PetErrorData(message: $message), 400);') + ->toContain('return new ApiError(new PetErrorData(message: $message), 404);'); + + // Distinct schema per status. + expect($files['CreatePetErrors']->code) + ->toContain('return new ApiError(new BadRequestProblemData(reason: $reason), 400);') + ->toContain('return new ApiError(new NotFoundProblemData(resource: $resource), 404);'); +}); diff --git a/tests/Unit/Emitter/ModelGeneratorTest.php b/tests/Unit/Emitter/ModelGeneratorTest.php index 0197722..114ffad 100644 --- a/tests/Unit/Emitter/ModelGeneratorTest.php +++ b/tests/Unit/Emitter/ModelGeneratorTest.php @@ -3,7 +3,9 @@ declare(strict_types=1); use CodeWithAgents\OpenApiLaravel\Emitter\GeneratedFile; +use CodeWithAgents\OpenApiLaravel\Emitter\GeneratorOptions; use CodeWithAgents\OpenApiLaravel\Emitter\ModelGenerator; +use CodeWithAgents\OpenApiLaravel\Parser\OpenApiReader; use CodeWithAgents\OpenApiLaravel\Parser\SpecParser; /** @@ -16,6 +18,33 @@ function generateCustomer(): array return (new ModelGenerator)->generate($doc); } +/** + * Generate a single-string-property schema whose property carries the given + * `pattern`, returning [generated code, build warnings] for the regex-rule + * compile-probe tests (#150). + * + * @return array{string, list} + */ +function generatePatternedSchema(string $pattern): array +{ + $document = [ + 'openapi' => '3.0.3', + 'info' => ['title' => 'Test', 'version' => '1.0.0'], + 'paths' => new stdClass, + 'components' => ['schemas' => [ + 'Patterned' => [ + 'type' => 'object', + 'properties' => ['code' => ['type' => 'string', 'pattern' => $pattern]], + ], + ]], + ]; + + $generator = new ModelGenerator(new GeneratorOptions); + $code = $generator->generate((new OpenApiReader)->read($document))['PatternedData']->code; + + return [$code, $generator->warnings()]; +} + it('emits one class per schema plus nested objects', function () { expect(array_keys(generateCustomer())) ->toBe(['CustomerAddressData', 'CustomerData', 'CustomerStatus', 'TagData']); @@ -83,3 +112,28 @@ function generateCustomer(): array expect($combined)->toMatchSnapshot(); }); + +it('drops an uncompilable spec pattern instead of emitting a broken regex rule (#150)', function () { + // An ECMA-valid-but-PCRE-invalid (here syntactically broken) pattern + // embedded verbatim into a `regex:` rule makes Laravel's preg_match raise + // an uncatchable compile error on every request, a runtime 500/DoS. So the + // rule is dropped, the field keeps its other rules, and a warning surfaces. + [$code, $warnings] = generatePatternedSchema('('); + + expect($code) + ->not->toContain('regex:') + ->and($code)->toContain("'code' => ['sometimes', 'string'],") + ->and($warnings)->toContain( + 'A string schema declares a `pattern` that is not valid PCRE ("("); the `regex:` rule is dropped ' + .'so the generated app never raises an uncatchable preg_match compile error at runtime. ' + .'The field keeps its other validation rules.', + ); +}); + +it('still emits the regex rule for a valid spec pattern (#150 over-drop guard)', function () { + [$code, $warnings] = generatePatternedSchema('^[A-Z]{3}$'); + + expect($code) + ->toContain("'code' => ['sometimes', 'string', 'regex:#^[A-Z]{3}\$#'],") + ->and($warnings)->toBe([]); +}); diff --git a/tests/Unit/Emitter/PhpLiteralTest.php b/tests/Unit/Emitter/PhpLiteralTest.php new file mode 100644 index 0000000..82e2493 --- /dev/null +++ b/tests/Unit/Emitter/PhpLiteralTest.php @@ -0,0 +1,53 @@ +toBe('0') + ->and(PhpLiteral::numberLiteral(42))->toBe('42') + ->and(PhpLiteral::numberLiteral(-7))->toBe('-7'); +}); + +it('leaves a normal-range float untouched (no drift)', function () { + expect(PhpLiteral::numberLiteral(0.5))->toBe('0.5') + ->and(PhpLiteral::numberLiteral(1.5))->toBe('1.5') + ->and(PhpLiteral::numberLiteral(0.1))->toBe('0.1') + ->and(PhpLiteral::numberLiteral(0.0001))->toBe('0.0001') + ->and(PhpLiteral::numberLiteral(2.0))->toBe('2') + ->and(PhpLiteral::numberLiteral(300000000.0))->toBe('300000000'); +}); + +it('expands a small-magnitude float out of scientific notation (#148)', function () { + expect(PhpLiteral::numberLiteral(1e-7))->toBe('0.0000001') + ->and(PhpLiteral::numberLiteral(1.5e-5))->toBe('0.000015') + ->and(PhpLiteral::numberLiteral(1.23e-10))->toBe('0.000000000123') + ->and(PhpLiteral::numberLiteral(-2.5e-8))->toBe('-0.000000025'); +}); + +it('expands a large-magnitude float out of scientific notation (#148)', function () { + expect(PhpLiteral::numberLiteral(1e20))->toBe('100000000000000000000') + ->and(PhpLiteral::numberLiteral(1e15))->toBe('1000000000000000'); +}); + +it('never emits the letter E for any of a range of magnitudes (#148)', function () { + foreach ([1e-7, 1e-12, 1.5e-9, 1e20, 9.999e22, -3.3e-8, 5e-300] as $value) { + expect(PhpLiteral::numberLiteral($value))->not->toContain('E') + ->and(PhpLiteral::numberLiteral($value))->not->toContain('e'); + } +}); + +it('renders a value that parses back to the original float (#148)', function () { + foreach ([1e-7, 1.5e-5, 1.23e-10, -2.5e-8, 1e20, 1e15, 0.5, 0.0001, 42.0] as $value) { + expect((float) PhpLiteral::numberLiteral($value))->toBe((float) $value); + } +}); diff --git a/tests/Unit/Emitter/Server/OperationCollectorTest.php b/tests/Unit/Emitter/Server/OperationCollectorTest.php index a2e5823..26d7df3 100644 --- a/tests/Unit/Emitter/Server/OperationCollectorTest.php +++ b/tests/Unit/Emitter/Server/OperationCollectorTest.php @@ -2,6 +2,7 @@ declare(strict_types=1); +use CodeWithAgents\OpenApiLaravel\Emitter\GeneratedFile; use CodeWithAgents\OpenApiLaravel\Emitter\ModelGenerator; use CodeWithAgents\OpenApiLaravel\Emitter\Server\ControllerGenerator; use CodeWithAgents\OpenApiLaravel\Emitter\Server\OperationCollector; @@ -207,6 +208,80 @@ function descriptorFor(array $descriptors, string $method, string $path): Operat expect($generator->supportFiles())->not->toHaveKey('RespondsWithStatus'); }); +/** + * A single-operation document whose GET declares a 200 plus the given error + * responses, over a small component pool, for the ApiError mark-used cases. + * + * @param array> $errorResponses status => response object + * @param array> $schemas extra component schemas + */ +function apiErrorDocument(array $errorResponses, array $schemas = []): OpenApiDocument +{ + $document = [ + 'openapi' => '3.0.3', + 'info' => ['title' => 'Test', 'version' => '1.0.0'], + 'paths' => [ + '/things' => [ + 'get' => [ + 'tags' => ['things'], + 'operationId' => 'getThing', + 'responses' => ['200' => ['description' => 'ok']] + $errorResponses, + ], + ], + ], + 'components' => ['schemas' => ['ErrorData' => ['type' => 'object', 'required' => ['message'], 'properties' => ['message' => ['type' => 'string']]]] + $schemas], + ]; + + $spec = (new OpenApiReader)->read($document); + expect($spec)->toBeInstanceOf(OpenApiDocument::class); + + return $spec; +} + +/** + * @return array + */ +function apiErrorSupport(OpenApiDocument $spec): array +{ + $generator = new ModelGenerator; + $generator->generate($spec); + (new OperationCollector(new ServerOptions, $generator->registry(), null, $generator))->collect($spec); + + return $generator->supportFiles(); +} + +it('marks the ApiError support class when an error response is a named-component object', function () { + $spec = apiErrorDocument([ + '404' => ['description' => 'nf', 'content' => ['application/json' => ['schema' => ['$ref' => '#/components/schemas/ErrorData']]]], + ]); + + $support = apiErrorSupport($spec); + expect($support)->toHaveKey('ApiError') + ->and($support['ApiError']->code)->toContain('namespace App\Data\Support;') + ->and($support['ApiError']->code)->toContain('final class ApiError'); +}); + +it('does not mark ApiError when there is no error response at all', function () { + expect(apiErrorSupport(apiErrorDocument([])))->not->toHaveKey('ApiError'); +}); + +it('does not mark ApiError when the error response schema is a non-object component', function () { + $spec = apiErrorDocument( + ['404' => ['description' => 'nf', 'content' => ['application/json' => ['schema' => ['$ref' => '#/components/schemas/ScalarError']]]]], + ['ScalarError' => ['type' => 'string']], + ); + + expect(apiErrorSupport($spec))->not->toHaveKey('ApiError'); +}); + +it('does not mark ApiError when the only error slot is an inline object (deferred in v1)', function () { + $spec = apiErrorDocument([ + '404' => ['description' => 'nf', 'content' => ['application/json' => ['schema' => ['type' => 'object', 'properties' => ['message' => ['type' => 'string']]]]]], + ]); + + expect(apiErrorSupport($spec))->not->toHaveKey('ApiError'); +}); + it('orders descriptors by path then by a fixed HTTP-method order', function () { $descriptors = collectPetstore(); diff --git a/tests/Unit/Emitter/ValidationConstraintsTest.php b/tests/Unit/Emitter/ValidationConstraintsTest.php index 173d727..0d205e7 100644 --- a/tests/Unit/Emitter/ValidationConstraintsTest.php +++ b/tests/Unit/Emitter/ValidationConstraintsTest.php @@ -243,6 +243,27 @@ function generateConstraintSchemas(array $schemas, string $openapi = '3.0.3'): a ->and($code)->toContain('use App\Data\Support\MultipleOfRule;'); }); +it('renders a tiny float bound and multipleOf in fixed decimal, never scientific notation (#148)', function () { + // multipleOf/minimum of 1e-7 would stringify as "1.0E-7": a broken + // `min:1.0E-7` rule parameter and an opaque MultipleOfRule argument. Both + // must come out as plain decimal. + $files = generateConstraintSchemas([ + 'Holder' => [ + 'type' => 'object', + 'properties' => [ + 'n' => ['type' => 'number', 'minimum' => 0.0000001, 'multipleOf' => 0.0000001], + ], + ], + ]); + + $code = $files['HolderData']->code; + + expect($code)->toContain("'min:0.0000001'") + ->and($code)->toContain('new MultipleOfRule(0.0000001)') + ->and($code)->not->toContain('E-7') + ->and($code)->not->toContain('1.0E'); +}); + // FIX 5: uniqueItems. it('adds distinct to the field.* item rules for uniqueItems', function () { diff --git a/tests/Unit/Security/HostileSpecTest.php b/tests/Unit/Security/HostileSpecTest.php index 90f73af..bcad4d6 100644 --- a/tests/Unit/Security/HostileSpecTest.php +++ b/tests/Unit/Security/HostileSpecTest.php @@ -9,8 +9,34 @@ use CodeWithAgents\OpenApiLaravel\Emitter\Server\OperationCollector; use CodeWithAgents\OpenApiLaravel\Emitter\Server\RouteGenerator; use CodeWithAgents\OpenApiLaravel\Emitter\Server\ServerOptions; +use CodeWithAgents\OpenApiLaravel\Parser\OpenApiReader; use CodeWithAgents\OpenApiLaravel\Parser\SpecParser; +/** + * Generate the Data class for a single `number` property carrying the given + * numeric-keyword constraints, returning its emitted PHP source. The schema is + * built in memory so a native non-finite float (INF / NAN) can be injected the + * way the parser would receive it from a hostile spec (issue #151). + * + * @param array $constraints + */ +function generateNumberConstraint(array $constraints): string +{ + $document = (new OpenApiReader)->read([ + 'openapi' => '3.1.0', + 'info' => ['title' => 'Test', 'version' => '1.0.0'], + 'paths' => new stdClass, + 'components' => ['schemas' => [ + 'Measure' => [ + 'type' => 'object', + 'properties' => ['value' => ['type' => 'number', ...$constraints]], + ], + ]], + ], 'hostile-number.json'); + + return (new ModelGenerator)->generate($document)['MeasureData']->code; +} + /** * The OpenAPI spec is untrusted input. The generator writes PHP source that the * host then loads and executes, so any spec-derived value that reaches a class @@ -197,3 +223,68 @@ function generateHostileFiles(): array expect(OptionValidator::identifier('--suffix', 'Dto'))->toBe('Dto'); expect(OptionValidator::identifier('--suffix', '', allowEmpty: true))->toBe(''); }); + +it('gracefully ignores a non-finite maximum (NAN) instead of emitting max:NAN (#151)', function () { + // A spec `maximum: NAN` (JSON 1e400 decodes to INF, YAML `.nan` to NAN) + // would otherwise emit `max:NAN`, which rejects EVERY value: an + // availability bug planted by spec input. The bound must be dropped. + $code = generateNumberConstraint(['maximum' => NAN]); + + expect($code) + ->not->toContain('max:') + ->and($code)->not->toContain('NAN'); +}); + +it('gracefully ignores a non-finite minimum (INF) instead of emitting min:INF (#151)', function () { + $code = generateNumberConstraint(['minimum' => INF]); + + expect($code) + ->not->toContain('min:') + ->and($code)->not->toContain('INF'); +}); + +it('gracefully ignores a non-finite multipleOf (INF) instead of emitting MultipleOfRule(INF) (#151)', function () { + $code = generateNumberConstraint(['multipleOf' => INF]); + + expect($code) + ->not->toContain('MultipleOfRule') + ->and($code)->not->toContain('INF'); +}); + +it('still emits a finite bound unchanged (#151 over-drop guard)', function () { + $code = generateNumberConstraint(['maximum' => 10, 'minimum' => 1, 'multipleOf' => 2]); + + expect($code) + ->toContain('max:10') + ->toContain('min:1') + ->toContain('MultipleOfRule(2)'); +}); + +it('gracefully ignores a numeric-string that overflows to INF ("1e400") instead of emitting a rule (#151)', function () { + // The spec is untrusted: an overflowing numeric string coerces to INF just + // like a native non-finite float, so `maximum: "1e400"` must be dropped, not + // emitted as `max:INF`. + $code = generateNumberConstraint(['maximum' => '1e400']); + + expect($code) + ->not->toContain('max:') + ->and($code)->not->toContain('INF'); +}); + +it('gracefully ignores a non-finite exclusiveMinimum (INF) instead of emitting gt:INF (#151)', function () { + // The is_finite() guard sits at the one chokepoint feeding all five numeric + // keywords, so the 3.1 numeric exclusive bounds are covered too: no gt:INF. + $code = generateNumberConstraint(['exclusiveMinimum' => INF]); + + expect($code) + ->not->toContain('gt:') + ->and($code)->not->toContain('INF'); +}); + +it('gracefully ignores a non-finite exclusiveMaximum (NAN) instead of emitting lt:NAN (#151)', function () { + $code = generateNumberConstraint(['exclusiveMaximum' => NAN]); + + expect($code) + ->not->toContain('lt:') + ->and($code)->not->toContain('NAN'); +}); diff --git a/tests/Unit/Support/ApiErrorTest.php b/tests/Unit/Support/ApiErrorTest.php new file mode 100644 index 0000000..a037d56 --- /dev/null +++ b/tests/Unit/Support/ApiErrorTest.php @@ -0,0 +1,65 @@ +Errors` factories forward into, and a + * documented escape hatch in its own right). + * + * Pure PHP, no container: render() needs the response() helper and lives in the + * Feature-level ApiErrorRenderTest. Here we only assert the value semantics: the + * named factories set the documented status and store the exact body, the + * general constructor covers any other status, and it is a Throwable. + */ +final class ApiErrorUnitFakeBody implements JsonSerializable +{ + /** + * @param array $data + */ + public function __construct(private array $data) {} + + /** + * @return array + */ + public function jsonSerialize(): array + { + return $this->data; + } +} + +it('sets the documented status and stores the exact body for each named factory', function (string $factory, int $status) { + $body = new ApiErrorUnitFakeBody(['message' => 'x']); + + /** @var ApiError $error */ + $error = ApiError::{$factory}($body); + + expect($error->status)->toBe($status) + ->and($error->body)->toBe($body); +})->with([ + 'badRequest' => ['badRequest', 400], + 'unauthorized' => ['unauthorized', 401], + 'forbidden' => ['forbidden', 403], + 'notFound' => ['notFound', 404], + 'conflict' => ['conflict', 409], + 'unprocessable' => ['unprocessable', 422], + 'tooManyRequests' => ['tooManyRequests', 429], + 'serverError' => ['serverError', 500], +]); + +it('accepts an arbitrary status through the general constructor', function () { + $body = new ApiErrorUnitFakeBody(['message' => 'gone for legal reasons']); + $error = new ApiError($body, 451); + + expect($error->status)->toBe(451) + ->and($error->body)->toBe($body); +}); + +it('is a RuntimeException, and therefore a Throwable', function () { + $error = ApiError::notFound(new ApiErrorUnitFakeBody([])); + + expect($error)->toBeInstanceOf(RuntimeException::class) + ->and($error)->toBeInstanceOf(Throwable::class); +});