Bundle configuration to automatically include ProblemDetails in the OpenAPI schema#16
Merged
Bundle configuration to automatically include ProblemDetails in the OpenAPI schema#16
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughAdds RFC 7807 Problem Details support: new OpenAPI models (ProblemDetails, Violation, ProblemDetailsInvalidRequestBody), a Nelmio spec file, a DI config subtree Changes
Sequence Diagram(s)sequenceDiagram
participant ContainerBuilder
participant StixxExt as StixxOpenApiCommandExtension
participant Yaml as Yaml
participant Nelmio as NelmioApiDocExtension
participant ServiceContainer
ContainerBuilder->>StixxExt: prepend(ContainerBuilder)
activate StixxExt
StixxExt->>StixxExt: read config `openapi.problem_details`
alt problem_details enabled
StixxExt->>Yaml: load nelmio_problem_details.yaml
Yaml-->>StixxExt: parsed spec
StixxExt->>Nelmio: prepend model/response definitions (ProblemDetails, Violation, ProblemDetailsInvalidRequestBody)
Nelmio-->>ServiceContainer: register components/schemas/responses
else disabled
StixxExt-->>ServiceContainer: skip loading problem details
end
StixxExt-->>ContainerBuilder: prepend complete
deactivate StixxExt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/Functional/ProblemDetailsConfigurationTest.php (1)
24-33: Add explicit// Actsections to keep AAA consistent.All three tests perform an action step, but it is not explicitly labeled. Please add
// Actcomments before spec generation in each method.♻️ Example adjustment pattern
// Arrange $this->bootKernel(); $container = self::getContainer(); + // Act /** `@var` ApiDocGenerator $generator */ $generator = $container->get('nelmio_api_doc.generator.default'); /** `@var` array{components?: array{responses?: array<string, mixed>, schemas?: array<string, mixed>}} $spec */ $spec = json_decode($generator->generate()->toJson(), true);As per coding guidelines, "Tests must follow the AAA (Arrange, Act, Assert) pattern with explicit comments."
Also applies to: 57-71, 81-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/ProblemDetailsConfigurationTest.php` around lines 24 - 33, Add an explicit "// Act" comment before the step that generates the API spec in each test method: place the comment immediately above the call that assigns $spec (the line using $generator->generate()->toJson()) so the test clearly follows Arrange, Act, Assert; update the three occurrences around the $generator variable and $spec assignment in ProblemDetailsConfigurationTest (also at the other referenced blocks) to match this pattern.tests/Functional/Resources/specifications/openapi.json (1)
194-199: Consider boundingviolationswithmaxItems.Adding an upper bound improves contract safety and satisfies the static-analysis concern about unbounded arrays.
♻️ Suggested hardening
"violations": { "type": "array", + "maxItems": 100, "items": { "$ref": "#/components/schemas/Violation" } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/Resources/specifications/openapi.json` around lines 194 - 199, The "violations" schema defines an unbounded array (property name "violations" with items referencing "#/components/schemas/Violation"); add a sensible upper bound by introducing a "maxItems" property on that schema (e.g., maxItems: 100 or another project-appropriate limit) to prevent unbounded arrays and satisfy static analysis; keep the existing "items" reference and only add the "maxItems" field within the same "violations" object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Model/ProblemDetails.php`:
- Around line 31-33: The OA\Property on the ProblemDetails::$type property uses
a non-standard format 'url'; update the annotation for the property (the
#[OA\Property(...)] attached to public string $type = 'about:blank') to use
format: 'uri-reference' instead, so the OpenAPI schema matches RFC3986 and
OpenAPI/JSON Schema validators; keep the rest of the attribute
(description/default) unchanged.
In `@src/Resources/specifications/nelmio_problem_details.yaml`:
- Around line 34-36: The example for invalidRequestBody in the Nelmio problem
details YAML uses a violations array that doesn't conform to the declared
Violation shape; update the invalidRequestBody example so each entry in
violations includes the required fields propertyPath and code (e.g., add a
propertyPath string indicating the JSON pointer or field and a code
string/identifier matching your Violation codes), ensuring the violations array
elements match the Violation schema contract and examples for fields like
message remain unchanged.
In `@tests/Functional/Resources/specifications/openapi.json`:
- Around line 188-234: The OpenAPI schema ProblemDetailsInvalidRequestBody lists
violations as a required property but the example omits it; update the example
to include a valid "violations" array (matching the referenced
components/schemas/Violation) so the example satisfies the required field, or
alternatively remove "violations" from the schema's required array; modify the
ProblemDetailsInvalidRequestBody "example" to include at least one Violation
object (or adjust the required list) to restore consistency.
---
Nitpick comments:
In `@tests/Functional/ProblemDetailsConfigurationTest.php`:
- Around line 24-33: Add an explicit "// Act" comment before the step that
generates the API spec in each test method: place the comment immediately above
the call that assigns $spec (the line using $generator->generate()->toJson()) so
the test clearly follows Arrange, Act, Assert; update the three occurrences
around the $generator variable and $spec assignment in
ProblemDetailsConfigurationTest (also at the other referenced blocks) to match
this pattern.
In `@tests/Functional/Resources/specifications/openapi.json`:
- Around line 194-199: The "violations" schema defines an unbounded array
(property name "violations" with items referencing
"#/components/schemas/Violation"); add a sensible upper bound by introducing a
"maxItems" property on that schema (e.g., maxItems: 100 or another
project-appropriate limit) to prevent unbounded arrays and satisfy static
analysis; keep the existing "items" reference and only add the "maxItems" field
within the same "violations" object.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/DependencyInjection/Configuration.phpsrc/DependencyInjection/StixxOpenApiCommandExtension.phpsrc/Model/ProblemDetails.phpsrc/Model/ProblemDetailsInvalidRequestBody.phpsrc/Model/Violation.phpsrc/Resources/specifications/nelmio_problem_details.yamltests/Functional/App/Command/CreateBookCommand.phptests/Functional/App/Command/DeleteBookCommand.phptests/Functional/App/Command/UpdateBookCommand.phptests/Functional/ProblemDetailsConfigurationTest.phptests/Functional/Resources/config/disable_problem_details.phptests/Functional/Resources/config/override_nelmio.phptests/Functional/Resources/specifications/openapi.jsontests/Unit/DependencyInjection/ConfigurationTest.php
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/Functional/Resources/specifications/openapi.json (1)
1-6: Security definitions missing in OpenAPI spec (test fixture).Static analysis flags that global security rules are not defined (
securityfield is absent). Since this is a test fixture, this is likely acceptable, but if this spec is also used as documentation reference, consider adding security definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/Resources/specifications/openapi.json` around lines 1 - 6, The OpenAPI test fixture is missing global security definitions (no "security" field) which causes static analysis warnings; add a top-level "security" array and corresponding "components.securitySchemes" entries to the JSON so global security rules are defined—identify the root "openapi" object and the "info" object in the file and insert a "security" property plus a "components" object with named schemes (e.g., bearerAuth or apiKey) to satisfy validators and document expected auth for the spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Functional/ProblemDetailsConfigurationTest.php`:
- Around line 68-72: The test captures the previous error handler with
$prevErrorHandler = set_error_handler(static fn () => true) but then tries to
restore it by calling set_error_handler($prevErrorHandler), which fails when
$prevErrorHandler is null; replace that restore logic with
restore_error_handler() so the original handler is properly restored after
calling $generator->generate()->toJson() (keep references to set_error_handler,
$prevErrorHandler, and restore_error_handler when updating the test).
- Around line 93-97: The test currently saves and restores the previous error
handler using $prevErrorHandler = set_error_handler(static fn () => true); ...
set_error_handler($prevErrorHandler); — replace this pattern by calling
restore_error_handler() after generating/decoding the spec (remove the manual
$prevErrorHandler save/restore), i.e. install the temporary handler with
set_error_handler(...) and then restore it with restore_error_handler()
immediately after $generator->generate()->toJson() is decoded to $spec.
---
Nitpick comments:
In `@tests/Functional/Resources/specifications/openapi.json`:
- Around line 1-6: The OpenAPI test fixture is missing global security
definitions (no "security" field) which causes static analysis warnings; add a
top-level "security" array and corresponding "components.securitySchemes"
entries to the JSON so global security rules are defined—identify the root
"openapi" object and the "info" object in the file and insert a "security"
property plus a "components" object with named schemes (e.g., bearerAuth or
apiKey) to satisfy validators and document expected auth for the spec.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Model/ProblemDetails.phpsrc/Resources/specifications/nelmio_problem_details.yamltests/Functional/ProblemDetailsConfigurationTest.phptests/Functional/Resources/specifications/openapi.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Resources/specifications/nelmio_problem_details.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/Functional/ProblemDetailsConfigurationTest.php (2)
22-33: Inconsistent error handler usage across tests.The first test (
testProblemDetailsAreLoadedByDefault) doesn't useset_error_handler/restore_error_handler, while the other two tests do. If the suppression is needed due to warnings from the generator, consider applying it consistently across all tests for uniformity.Also applies to: 56-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/ProblemDetailsConfigurationTest.php` around lines 22 - 33, The test testProblemDetailsAreLoadedByDefault is missing the error-suppression wrapper used in the other tests; surround the bootKernel/getContainer/generator->generate()->toJson() call in that test with the same set_error_handler(fn()=>true) before invoking the generator and restore_error_handler() afterwards so all tests consistently suppress the generator warnings (use the same pattern as in the other tests that call set_error_handler and restore_error_handler).
74-79: Weak assertion may hide unexpected behavior.The conditional
if (isset($spec['components']['responses']))means the test silently passes ifcomponentsorresponsesis missing entirely. This could mask unexpected side effects from disabling problem details.Consider a more explicit assertion:
♻️ Suggested improvement
// Assert $this->assertIsArray($spec); - if (isset($spec['components']['responses'])) { - $this->assertArrayNotHasKey('InvalidRequestProblemDetailsResponse', $spec['components']['responses']); - } + // Verify responses section exists but specific key is absent + $responses = $spec['components']['responses'] ?? []; + $this->assertArrayNotHasKey('InvalidRequestProblemDetailsResponse', $responses);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/ProblemDetailsConfigurationTest.php` around lines 74 - 79, The test currently skips checks when components or responses are missing by using if (isset($spec['components']['responses'])); instead assert explicitly that $spec contains 'components' and that $spec['components'] contains 'responses' (e.g., assertArrayHasKey on 'components' and on 'responses') and then assertArrayNotHasKey('InvalidRequestProblemDetailsResponse', $spec['components']['responses']) so the test fails if the structure is missing and will catch unintended removal of components/responses or the forbidden response entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/Functional/ProblemDetailsConfigurationTest.php`:
- Around line 22-33: The test testProblemDetailsAreLoadedByDefault is missing
the error-suppression wrapper used in the other tests; surround the
bootKernel/getContainer/generator->generate()->toJson() call in that test with
the same set_error_handler(fn()=>true) before invoking the generator and
restore_error_handler() afterwards so all tests consistently suppress the
generator warnings (use the same pattern as in the other tests that call
set_error_handler and restore_error_handler).
- Around line 74-79: The test currently skips checks when components or
responses are missing by using if (isset($spec['components']['responses']));
instead assert explicitly that $spec contains 'components' and that
$spec['components'] contains 'responses' (e.g., assertArrayHasKey on
'components' and on 'responses') and then
assertArrayNotHasKey('InvalidRequestProblemDetailsResponse',
$spec['components']['responses']) so the test fails if the structure is missing
and will catch unintended removal of components/responses or the forbidden
response entry.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Model/ProblemDetailsInvalidRequestBody.phptests/Functional/ProblemDetailsConfigurationTest.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores