fix(schema): allow declaring many summary and description for one endpoint #2944
Conversation
WalkthroughThis pull request introduces several modifications across the Changes
Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/specs/schema/test/integrations/__snapshots__/multi-description-route.integration.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
packages/specs/schema/.barrelsby.json(1 hunks)packages/specs/schema/src/components/open-spec/pathsMapper.ts(1 hunks)packages/specs/schema/src/decorators/operations/operation.ts(3 hunks)packages/specs/schema/src/decorators/operations/publish.spec.ts(2 hunks)packages/specs/schema/src/decorators/operations/route.spec.ts(10 hunks)packages/specs/schema/src/decorators/operations/subscribe.spec.ts(2 hunks)packages/specs/schema/src/domain/JsonMethodStore.spec.ts(3 hunks)packages/specs/schema/src/domain/JsonOperation.ts(3 hunks)packages/specs/schema/src/domain/JsonOperationRoute.spec.ts(4 hunks)packages/specs/schema/src/domain/__fixtures__/inspectOperationsPaths.ts(1 hunks)packages/specs/schema/test/integrations/multi-description-route.integration.spec.ts(1 hunks)
🔇 Additional comments (31)
packages/specs/schema/src/decorators/operations/operation.ts (5)
4-4: New import is appropriate and consistent with recent refactoring.
This import cleanly brings in the refactored JsonMethodPath for greater clarity and encapsulation.
57-57: New protected property for the operation path.
Defining a dedicated protected field helps organize and manage operation path state within this class.
75-76: Simultaneous setting of summary fields promotes consistent metadata.
Calling both this.operationPath?.summary(value) and this.entity.operation.summary(value) aligns path-level and operation-level summaries.
79-81: Parallel assignment of description to both path and operation is consistent.
This dual assignment ensures thorough coverage and consistency of descriptive information at each level.
63-65: Conditional path assignment is a safe approach.
Ensures that an operation path is only created when the path is truthy, reducing risks of invalid references.
✅ Verification successful
Let me verify the conditional path assignment by examining the related code.
Let me check how the path parameter is used in the broader context and if there are any validation requirements.
Based on the gathered information, I can now provide the final response:
Conditional path assignment is indeed a safe and correct approach
The implementation is well-designed because:
- The path check prevents creating unnecessary operation paths when path is falsy
- The
addOperationPathmethod inJsonOperationproperly handles the path by creating a newJsonMethodPathinstance and storing it in a map - The method is called within the
beforeInithook where the path and method are properly extracted from the context
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Let's first look at the implementation of addOperationPath method
ast-grep --pattern 'addOperationPath($_,$_) {
$$$
}'
# Also check for any error handling or validation in the operation class
rg -A 5 "addOperationPath"
Length of output: 1410
Script:
#!/bin/bash
# Check the class and method definition where this code exists
rg -B 10 -A 10 "this.operationPath = this.entity.operation.addOperationPath"
# Look for any path validation or requirements
rg -l "path.*required|required.*path" --type ts
# Check JsonMethodPath constructor
ast-grep --pattern 'class JsonMethodPath {
$$$
}'
Length of output: 2324
packages/specs/schema/src/domain/JsonOperation.ts (3)
11-29: New JsonMethodPath class neatly encapsulates path-specific metadata.
Extending JsonMap and providing typed chaining methods (summary, description) is a clean, object-oriented design.
41-41: OperationPaths map fosters more structured lookup.
Using Map<string, JsonMethodPath> improves clarity and underscores the shift to a path-based model.
217-221: Returning the newly created JsonMethodPath enhances fluency.
Allowing further chaining or manipulation is beneficial. If desired, consider validations on method/path arguments.
packages/specs/schema/src/domain/__fixtures__/inspectOperationsPaths.ts (1)
1-8: Encapsulate operation path retrieval in a dedicated function.
This utility function centralizes the mapping of endpoint.operationPaths, improving test readability and maintainability.
packages/specs/schema/test/integrations/multi-description-route.integration.spec.ts (1)
1-25: Integration test for multiple descriptions is well structured.
Demonstrates how varied descriptions can coexist on multiple routes. The snapshot test approach is appropriate for regression coverage.
packages/specs/schema/src/decorators/operations/publish.spec.ts (2)
4-4: New import of inspectOperationsPaths fosters consistency.
Centralizes logic for reading operation paths, aligning with the refactored testing approach used across the codebase.
Line range hint 19-23: Asserting the inspected operation paths clarifies the test intent.
Replaces direct property access with a cohesive helper function, leading to more robust and maintainable tests.
packages/specs/schema/src/decorators/operations/subscribe.spec.ts (2)
4-4: Good use of the new utility.
Using the dedicated inspectOperationsPaths function keeps the test logic consistent across the codebase.
21-21: Validate operation verb ordering.
The order of returned objects (Subscribe first, Publish second) indicates that appropriate decorators are processed in the desired sequence. This looks correct and aligns with the test intent.
packages/specs/schema/src/components/open-spec/pathsMapper.ts (1)
79-79: Ensure merged properties do not introduce conflicts.
Spreading operationPath.toJSON(options) into the operation object is a logical way to include all properties, but verify naming overlaps (if any) to avoid accidental overwrites.
✅ Verification successful
Let me gather information about the code context to verify potential property conflicts.
Let me gather more context about the operation object and the OperationPath class to understand potential property conflicts.
Let me gather more information about the OperationPath class and its toJSON method.
Let me check one more thing to understand the operation object's structure and the toJSON implementation.
From the code analysis, I can see that the operation object is constructed with a clear precedence order:
- Base properties from
operation(from execMapper) - Properties from
operationPath.toJSON(options) - Explicit overrides for
parametersandoperationId
The spread operation is intentionally structured this way to allow operationPath.toJSON() properties to override the base operation properties, while still ensuring critical properties like parameters and operationId take precedence at the end. This is a deliberate design pattern for merging OpenAPI operation objects.
No conflicts found in property merging strategy.
The code follows a well-defined precedence order where later spreads and explicit property assignments take precedence over earlier ones, which is the intended behavior for OpenAPI operation objects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Let's first look at the implementation of operationPath.toJSON and the surrounding context
ast-grep --pattern 'class $_ {
$$$
toJSON($_) {
$$$
}
$$$
}'
Length of output: 74
Script:
#!/bin/bash
# Let's also see how operationPath is being used in the file
rg -B 5 -A 5 "operationPath\.toJSON\(options\)" packages/specs/schema/src/components/open-spec/pathsMapper.ts
Length of output: 368
Script:
#!/bin/bash
# Let's check for any other similar spread operations in the codebase
rg "\.\.\.operationPath" packages/specs/schema/src/components/open-spec/
Length of output: 180
Script:
#!/bin/bash
# Let's look at the OperationPath class definition
rg -B 10 -A 10 "class OperationPath" packages/specs/schema/src/components/open-spec/
# And see what properties are in the operation object
rg -B 5 -A 5 "operation = {" packages/specs/schema/src/components/open-spec/
Length of output: 255
Script:
#!/bin/bash
# Let's also check for any type definitions that might help us understand the structure
ast-grep --pattern 'interface $_ {
$$$
}'
Length of output: 65655
Script:
#!/bin/bash
# Let's see the full context of the file where this spread operation is used
cat packages/specs/schema/src/components/open-spec/pathsMapper.ts
Length of output: 3644
Script:
#!/bin/bash
# Let's find the OperationPath class definition and its toJSON method
rg -B 5 -A 5 "class .*OperationPath" packages/specs/schema/src/
# Let's also see what properties operation has before the spread
rg -B 5 -A 5 "const operation = " packages/specs/schema/src/components/open-spec/pathsMapper.ts
Length of output: 1315
Script:
#!/bin/bash
# Let's see what properties are being merged
ast-grep --pattern 'operation: {
$$$
}'
Length of output: 43
Script:
#!/bin/bash
# Let's check if there are any other places where operationPath.toJSON is used
rg "operationPath\.toJSON" packages/specs/schema/src/
Length of output: 161
Script:
#!/bin/bash
# Let's find the implementation of execMapper("operation") to see what properties it sets
rg -B 10 -A 10 "execMapper\(\"operation\"" packages/specs/schema/src/
# Let's also check for JsonMethodPath's toJSON implementation
rg -B 5 -A 5 "toJSON\(" packages/specs/schema/src/domain/JsonOperation.ts
Length of output: 10854
Script:
#!/bin/bash
# Let's see what other properties are being set on the operation object
rg -A 5 "operation: \{" packages/specs/schema/src/
Length of output: 595
packages/specs/schema/src/domain/JsonOperationRoute.spec.ts (3)
6-6: Import grouping is consistent.
The addition of JsonMethodPath import aligns well with the co-located JsonOperation import. Good job keeping domain classes grouped.
53-53: Maintain consistency in constructor usage.
Same comment as line 20 regarding referencing OperationVerbs.GET for improved readability.
85-85: Maintain consistency in constructor usage.
Same comment as above regarding referencing OperationVerbs.GET for improved readability.
packages/specs/schema/src/decorators/operations/route.spec.ts (10)
1-1: Centralized test utility import.
Importing inspectOperationsPaths in one place ensures a single entry point for validating operation paths.
17-17: Switching to utility-based inspection.
Using inspectOperationsPaths fosters a more maintainable and consistent test approach.
38-38: Consistent pattern usage.
Calling inspectOperationsPaths is consistent with other test blocks.
58-58: Retaining route-based logic.
Even with the middleware, the route method inspection remains straightforward via inspectOperationsPaths.
95-95: Ensuring correctness for POST.
Verifying with inspectOperationsPaths is more robust than direct property checks.
116-116: Added coverage of operation details.
This assertion also checks the summary, description, and operationId, highlighting the function's flexibility.
140-140: Consistent usage for DELETE.
Again, the approach is uniform, making the test suite easy to maintain.
161-161: Filtering HEAD operation.
No issues found. Maintains uniform pattern for route tests.
182-182: PATCH operation test coverage.
Ensures that partial update routes are accounted for, leveraging the same utility.
203-203: OPTIONS route test coverage is consistent.
Completes the set of RESTful verbs under test.
packages/specs/schema/src/domain/JsonMethodStore.spec.ts (2)
10-10: Use relative imports consistently for fixture utilities.
Everything looks good with this import. Just ensure that all references to fixture utilities follow the same relative import pattern throughout the test suite.
128-128: Promote consistency by using a shared test utility.
Switching to “inspectOperationsPaths” improves readability and consistency across tests. Great job!
packages/specs/schema/.barrelsby.json (1)
3-3: Verify necessity of excluding “fixtures” from barrels.
Excluding test fixtures from the build is typically best practice. Confirm that no essential runtime code resides in “fixtures”.
Execute the following script to ensure only test utilities are located within “fixtures” and that no production code is excluded:
✅ Verification successful
Excluding "fixtures" from barrels is appropriate.
The __fixtures__ directory contains test utilities like inspectOperationsPaths.ts that are used exclusively in test files (*.spec.ts). This is evident from the usage pattern where the fixture is only imported in test files and used for test assertions. Excluding it from barrels is the correct approach as these are not production code components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for potential production code in the __fixtures__ directory
fd "__fixtures__" --type f --exec head -n 5 {}
Length of output: 48
Script:
#!/bin/bash
# Let's try a different approach to find any __fixtures__ directories and their contents
rg -l "__fixtures__" --type-add 'test:*test*' --type-add 'test:*spec*' --type test
# Also search for any imports or references to __fixtures__ to understand usage
rg "__fixtures__" -C 2
Length of output: 3733
Script:
#!/bin/bash
# Let's examine the content of one of the __fixtures__ files to understand its nature
rg -p "inspectOperationsPaths" -C 10
Length of output: 10205
Romakita
force-pushed
the
fix-operation-paths-description
branch
from
9e08d0c to
0b8aac1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/specs/schema/src/domain/JsonOperationRoute.spec.ts (1)
21-21: 🧹 Nitpick (assertive)Standardize HTTP method usage across tests.
For consistency, prefer using
OperationVerbs.GETenum value over string literal"GET". Line 52 already uses the enum, but lines 21 and 82 use string literals.- operationPath: new JsonMethodPath("GET", "/"), + operationPath: new JsonMethodPath(OperationVerbs.GET, "/"),Also applies to: 52-52, 82-82
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/specs/schema/test/integrations/__snapshots__/multi-description-route.integration.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
packages/specs/schema/.barrelsby.json(1 hunks)packages/specs/schema/src/components/open-spec/pathsMapper.ts(1 hunks)packages/specs/schema/src/decorators/operations/operation.ts(3 hunks)packages/specs/schema/src/decorators/operations/publish.spec.ts(2 hunks)packages/specs/schema/src/decorators/operations/route.spec.ts(10 hunks)packages/specs/schema/src/decorators/operations/subscribe.spec.ts(2 hunks)packages/specs/schema/src/domain/JsonMethodStore.spec.ts(3 hunks)packages/specs/schema/src/domain/JsonOperation.ts(3 hunks)packages/specs/schema/src/domain/JsonOperationRoute.spec.ts(4 hunks)packages/specs/schema/src/domain/__fixtures__/inspectOperationsPaths.ts(1 hunks)packages/specs/schema/test/integrations/multi-description-route.integration.spec.ts(1 hunks)
🔇 Additional comments (17)
packages/specs/schema/src/decorators/operations/operation.ts (3)
4-4: Clean import of the new types
Bringing in JsonMethodPath and JsonOperation is consistent with the newly introduced class-based approach for operation paths.
63-65: Conditional assignment of the operation path
Good practice to check if path is truthy before adding the operation path. This avoids creating an empty or invalid operation path.
75-75: Safeguarding calls to summary and description
Using optional chaining on this.operationPath prevents runtime errors if the operation path is undefined. This approach is robust and aligns with TypeScript best practices.
Also applies to: 79-79
packages/specs/schema/src/domain/JsonOperation.ts (3)
11-17: Class-based encapsulation
Switching from an interface to a class for JsonMethodPath clarifies responsibilities and makes it easier to add fluent methods (like summary and description). Good object-oriented choice.
19-23: Fluent pattern for summary and description
Returning 'this' allows chaining and keeps the API intuitive for setting multiple properties.
Also applies to: 25-29
217-221: Chaining operation paths
The addOperationPath method neatly returns a JsonMethodPath instance, enabling further summary/description chaining. This is a clear and flexible API design.
packages/specs/schema/test/integrations/multi-description-route.integration.spec.ts (2)
17-24: Snapshot testing approach
Generating a spec and comparing it to a stored snapshot helps detect regressions in OpenAPI output. This is a solid testing strategy.
7-15: Confirm behavior with multiple decorators
Defining multiple @put decorators on the same method is a valid way to test or declare multiple paths and different descriptions. Ensure downstream code handles multiple method/path combinations as intended.
✅ Verification successful
Let me gather information about how multiple decorators are handled in the codebase.
These scripts will help us understand:
- How route decorators are processed
- If there are other test cases with multiple decorators
- Where the schema generation happens
- How routes are registered and handled
Let me search for the implementation and tests related to handling multiple decorators.
Let me check how the Operation decorator and route handling works to understand how multiple decorators are processed.
Based on the code review, I can now provide a final response as we have sufficient information about how multiple decorators are handled:
Multiple route decorators are properly supported and work as intended
The codebase correctly handles multiple @Put decorators with different paths and descriptions. Each decorator creates a new JsonMethodPath through addOperationPath() method, which is stored in the operation's paths map. The snapshot shows that each path is correctly generated in the OpenAPI spec with its unique description, parameters, and operationId. No issues were found in the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for files that handle route decorators processing
rg -l "Put|Description" packages/specs/schema/src/
Length of output: 1049
Script:
#!/bin/bash
# Look for test files related to route decorators
rg -l "Put|Description" packages/specs/schema/test/
Length of output: 878
Script:
#!/bin/bash
# Search for implementation of decorator handling
ast-grep --pattern 'function $_($$_) {
$$$
route($$$)
$$$
}'
Length of output: 68
Script:
#!/bin/bash
# Check if there are other similar test cases with multiple decorators
rg "class.*Controller.*\{[\s\S]*?@.*@.*\{" packages/specs/schema/test/
Length of output: 72
Script:
#!/bin/bash
# Look for OpenAPI/Swagger schema generation code that might handle multiple routes
rg "OpenAPI|Swagger|Schema" packages/specs/schema/src/
Length of output: 85842
Script:
#!/bin/bash
# Look for the implementation that handles multiple decorators
rg -A 10 "multiple.*decorators" packages/specs/schema/src/
# Look for the test snapshot to see how multiple decorators are handled
cat packages/specs/schema/test/integrations/__snapshots__/multi-description-route.integration.spec.ts.snap
# Look for the implementation of Put decorator
ast-grep --pattern 'function Put($$$) {
$$$
}'
# Look for route handling implementation
rg -A 10 "class.*Operation" packages/specs/schema/src/
Length of output: 7593
Script:
#!/bin/bash
# Look for Operation implementation
cat packages/specs/schema/src/decorators/operations/operation.ts
# Look for how operation paths are added
cat packages/specs/schema/src/domain/JsonOperation.ts
# Look for the test file to understand the test case better
cat packages/specs/schema/test/integrations/multi-description-route.integration.spec.ts
Length of output: 9645
packages/specs/schema/src/domain/JsonMethodStore.spec.ts (3)
10-10: New method import
inspectOperationsPaths centralizes the logic for retrieving operation paths, reducing direct property access. This is a maintainable strategy.
128-128: Readable operation paths inspection
Using inspectOperationsPaths(endpoint) simplifies the test and improves readability compared to manually unwrapping endpoint.operationPaths.
267-271: Type-cast alignment with JsonMethodStore
Casting storeMethod to JsonMethodStore is now aligned with the function’s expected signature. This resolves the previously mentioned “as never” confusion.
packages/specs/schema/.barrelsby.json (1)
3-3: Excluding fixtures
Adding "**/fixtures" to the exclude list keeps fixture files out of barrel exports. This helps maintain a clean public API and avoid unintentional re-exports.
packages/specs/schema/src/decorators/operations/publish.spec.ts (1)
4-4: LGTM! Improved test readability
The changes make the test assertions more readable by abstracting the operation paths inspection logic.
Also applies to: 19-24
packages/specs/schema/src/decorators/operations/subscribe.spec.ts (1)
4-4: LGTM! Well-structured test for multiple decorators
The test effectively validates the behavior of both Subscribe and Publish decorators, with assertions matching the decorator application order. The use of the helper function improves readability.
Also applies to: 21-30
packages/specs/schema/src/components/open-spec/pathsMapper.ts (1)
79-79: LGTM: Correctly spreads operation path properties.
The spread operator placement ensures that path-specific properties (summary, description) correctly override the base operation properties, enabling multiple summaries/descriptions per endpoint as intended.
Let's verify the property override behavior:
✅ Verification successful
From the search results, I can see that the JsonMethodPath class (defined in packages/specs/schema/src/domain/JsonOperation.ts) has a summary method that sets the summary property, and it extends JsonMap which provides the toJSON method. The spread operator usage in pathsMapper.ts correctly spreads these properties into the operation object.
Let me verify the complete implementation:
Based on the code analysis, I can now provide the final response:
LGTM: Property spreading behavior is correct and intentional
The spread operator usage ...operationPath.toJSON(options) is working as designed. The JsonMethodPath class extends JsonMap and provides methods to set summary and description properties. When toJSON() is called, it correctly serializes these properties, allowing them to be spread into the operation object. This enables the intended behavior of having path-specific summaries and descriptions override base operation properties.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if operationPath.toJSON() includes summary/description properties
# and verify there are no unintended property collisions
# Search for summary/description property assignments in JsonMethodPath
ast-grep --pattern 'class JsonMethodPath {
$$$
summary($$$) {
$$$
}
$$$
description($$$) {
$$$
}
$$$
}'
Length of output: 127
Script:
#!/bin/bash
# Let's try a different approach to find the JsonMethodPath class and its toJSON method
rg -A 10 "class JsonMethodPath"
# Also search for any toJSON method implementations
rg -A 5 "toJSON\("
# And look for files containing JsonMethodPath
fd JsonMethodPath
Length of output: 35120
Script:
#!/bin/bash
# Get the full implementation of JsonMethodPath and JsonMap classes
rg -A 20 "class JsonMethodPath" packages/specs/schema/src/domain/JsonOperation.ts
rg -A 20 "class JsonMap" packages/specs/schema/src/domain/JsonMap.ts
Length of output: 936
packages/specs/schema/src/domain/JsonOperationRoute.spec.ts (1)
25-26: LGTM: Test assertions properly validate JsonMethodPath properties.
The new assertions correctly verify both method and path properties of the JsonMethodPath instances, ensuring type-safe operation path handling.
Also applies to: 56-57, 86-87
packages/specs/schema/src/decorators/operations/route.spec.ts (1)
Line range hint 17-22: LGTM: Consistent use of inspectOperationsPaths helper improves test maintainability.
The refactoring to use inspectOperationsPaths across all HTTP method tests improves readability and maintainability by:
- Encapsulating operation path access logic
- Using consistent OperationVerbs enum values
- Following a uniform assertion pattern
Also applies to: 38-43, 58-63, 95-100, 116-121, 140-145, 161-166, 182-187, 203-208
|
🎉 This PR is included in version 8.3.6 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
inspectOperationsPathsfor improved operation path validation.Bug Fixes
inspectOperationsPathsfunction.Documentation
Refactor
JsonMethodPathfrom an interface to a class for better encapsulation and object-oriented design.pathsMapperfunction to enhance the operation object with additional properties.