parameters: merge the path and operation parameters #54
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughgenerateInputSchemaAndDetails now accepts optional path-level parameters and merges them with operation-level parameters; operation-level parameters override on matching name+in. extractToolsFromApi now forwards Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Extractor as extractToolsFromApi
participant SchemaGen as generateInputSchemaAndDetails
participant OpParams as Operation.parameters
participant PathParams as PathItem.parameters (optional)
Extractor->>SchemaGen: generateInputSchemaAndDetails(operation, pathItem.parameters?)
SchemaGen->>OpParams: read operation.parameters
SchemaGen->>PathParams: read pathParameters (if present)
note over SchemaGen: Merge parameters\n- Build allParameters from path + operation\n- Deduplicate by name+in\n- Operation parameters override path parameters on conflict
SchemaGen-->>Extractor: return { inputSchema, parameters, requestBodyContentType }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/parser/extract-tools.ts(2 hunks)
🔇 Additional comments (3)
src/parser/extract-tools.ts (3)
84-84: LGTM: Call site correctly updated to pass path-level parameters.The call to
generateInputSchemaAndDetailsnow correctly passespathItem.parameters, enabling support for path-level parameters as intended.
116-126: LGTM: Function signature correctly updated.The signature change appropriately adds support for path-level parameters while maintaining backward compatibility through the optional parameter.
209-209: LGTM: Return statement correctly uses merged parameters.The function now returns
allParameters, which correctly includes both path-level and operation-level parameters after merging.
Waiting for the following PR to be merged: - harsha-iiiv/openapi-mcp-generator#54 - harsha-iiiv/openapi-mcp-generator#49
goneri
force-pushed
the
goneri/parameters-merge-the-path-and-operation-parameters_14458
branch
2 times, most recently
from
f02524d to
cb02fb1
Compare
|
@coderabbitai review this PR |
|
@harsha-iiiv: I'll review the changes in this PR. This PR implements proper handling of path-level and operation-level parameter merging in OpenAPI parsing, where operation parameters can override path parameters based on name and location. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements proper merging of path-level and operation-level parameters according to the OpenAPI specification. Path-level parameters are defined once and apply to all operations under a path, while operation-level parameters can override them.
Key Changes:
- Modified
generateInputSchemaAndDetailsto accept optional path parameters and merge them with operation parameters - Updated parameter merging logic to ensure operation-level parameters override path-level parameters with the same name and location
- Updated function call in
extractToolsFromApito pass path-level parameters
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
goneri
force-pushed
the
goneri/parameters-merge-the-path-and-operation-parameters_14458
branch
from
cb02fb1 to
3e5d690
Compare
`parameters` can be defined at the path and operation levels. Path level: ``` A list of parameters that are applicable for all the operations described under this path.These parameters can be overridden at the operation level, but cannot be removed there. The list MUST NOT include duplicated parameters. A unique parameter is defined by a combination of a name and location. The list can use the Reference Object to link to parameters that are defined in the OpenAPI Object's components.parameters. ```
goneri
force-pushed
the
goneri/parameters-merge-the-path-and-operation-parameters_14458
branch
from
3e5d690 to
abdcace
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/parser/extract-tools.ts (1)
133-156: Concatenation order is now correct! However, unsafe type casting remains unresolved.The merge logic now correctly processes path parameters first, then operation parameters (
pathParametersResolved.concat(operationParameters)), allowing operation parameters to override path parameters as required by the OpenAPI specification.However, the unsafe casting on lines 134 and 138 still needs to be addressed. Parameters can be
ReferenceObjecttypes containing only a$refproperty. Casting without resolving references will cause runtime errors when the code attempts to access properties likename,in, orschemaon unresolved references.Before casting, check for and resolve
ReferenceObjecttypes:const operationParameters: OpenAPIV3.ParameterObject[] = Array.isArray(operation.parameters) - ? operation.parameters.map((p) => p as OpenAPIV3.ParameterObject) + ? operation.parameters.map((p) => { + if ('$ref' in p) { + console.warn(`Unresolved parameter reference: ${p.$ref}`); + // Skip or throw error - references should be resolved before this point + throw new Error(`Parameter reference ${p.$ref} must be resolved before merging`); + } + return p as OpenAPIV3.ParameterObject; + }) : []; const pathParametersResolved: OpenAPIV3.ParameterObject[] = Array.isArray(pathParameters) - ? pathParameters.map((p) => p as OpenAPIV3.ParameterObject) + ? pathParameters.map((p) => { + if ('$ref' in p) { + console.warn(`Unresolved parameter reference: ${p.$ref}`); + throw new Error(`Parameter reference ${p.$ref} must be resolved before merging`); + } + return p as OpenAPIV3.ParameterObject; + }) : [];Alternatively, if references are expected to be resolved elsewhere in the codebase, verify that resolution happens before this function is called.
🧹 Nitpick comments (1)
src/parser/extract-tools.ts (1)
147-147: Consider renamingpathParamtoexistingfor clarity.The variable name
pathParamis misleading sinceallParameterscontains both path and operation parameters at different stages of iteration.const existingIndex = allParameters.findIndex( - (pathParam) => pathParam.name === param.name && pathParam.in === param.in + (existing) => existing.name === param.name && existing.in === param.in );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/parser/extract-tools.ts(2 hunks)
🔇 Additional comments (2)
src/parser/extract-tools.ts (2)
83-86: LGTM! Call site correctly updated to pass path-level parameters.The call to
generateInputSchemaAndDetailsnow correctly includespathItem.parameters, enabling the function to merge path-level and operation-level parameters as intended.
118-124: LGTM! Function signature properly updated with clear documentation.The function signature correctly accepts optional path-level parameters, and the JSDoc clearly documents the new parameter's purpose.
2 participants
parameterscan be defined at the path and operation levels.Path level:
Summary by CodeRabbit
Bug Fixes
Public API