Consider Path Item-level parameters #20
Conversation
WalkthroughThe changes introduce type-only imports and exports for specific TypeScript types, clarify separation between runtime and type exports, and update CLI invocation logic in the entry point. Additionally, parameter handling in OpenAPI tool extraction is enhanced to merge path-level and operation-level parameters, allowing proper overriding and schema generation. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI User
participant index.ts
participant CLI Program
CLI User->>index.ts: Executes script directly
index.ts->>CLI Program: Calls parse() method
CLI Program-->>CLI User: Runs CLI logic
sequenceDiagram
participant extractToolsFromApi
participant generateInputSchemaAndDetails
extractToolsFromApi->>generateInputSchemaAndDetails: Pass operation + path-level parameters
generateInputSchemaAndDetails->>generateInputSchemaAndDetails: Merge path-level, then operation-level parameters
generateInputSchemaAndDetails-->>extractToolsFromApi: Return input schema and details
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (1)
src/parser/extract-tools.ts (1)
101-112: LGTM! Correct implementation of parameter precedence.The parameter merging logic correctly implements the OpenAPI specification behavior:
- Path Item-level parameters are processed first
- Operation-level parameters are processed second, allowing them to override path-level parameters with the same name
- The
Array.isArraychecks provide proper safety for optional parametersThis implementation aligns perfectly with the PR objectives and the provided sample OpenAPI specification example where operation-level parameters override path-level ones.
Consider a minor type safety improvement:
- allParameters.push(...pathParameters.map((p) => p as OpenAPIV3.ParameterObject)); + allParameters.push(...pathParameters.filter(p => !('$ref' in p)) as OpenAPIV3.ParameterObject[]);- allParameters.push(...operation.parameters.map((p) => p as OpenAPIV3.ParameterObject)); + allParameters.push(...operation.parameters.filter(p => !('$ref' in p)) as OpenAPIV3.ParameterObject[]);This would properly handle any unresolved references, though after dereferencing they should already be resolved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/api.ts(1 hunks)src/index.ts(2 hunks)src/parser/extract-tools.ts(3 hunks)
🔇 Additional comments (6)
src/api.ts (1)
9-9: LGTM! Good TypeScript practice improvement.Converting to a type-only import clarifies that
McpToolDefinitionis used solely for typing purposes and won't be included in the emitted JavaScript code. This improves tree-shaking and aligns with the type-only exports introduced insrc/index.ts.src/index.ts (2)
35-36: LGTM! Proper separation of runtime and type exports.Converting
McpToolDefinitionandGetToolsOptionsto type-only exports while keepinggetToolsFromOpenApias a runtime export correctly separates types from runtime functionality. This aligns with the type-only import changes insrc/api.tsand improves code clarity.
88-91: LGTM! Enables direct execution for improved development experience.The conditional block correctly detects when the file is executed directly and automatically invokes the CLI program. This supports the PR objective of enabling direct execution with the
tsxtool for better debugging and development workflow.src/parser/extract-tools.ts (3)
25-25: LGTM! Correctly captures Path Item-level parameters.This properly extracts the path-level parameters from the pathItem, which is essential for implementing the Path Item-level parameter inheritance described in the PR objectives.
53-56: LGTM! Proper parameter passing to schema generation.The function call correctly passes both the operation and the newly captured pathParameters to
generateInputSchemaAndDetails, enabling the parameter merging logic.
90-93: LGTM! Function signature correctly updated.The optional
pathParametersparameter is properly typed and documented, maintaining backward compatibility while enabling the new Path Item-level parameter functionality.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@harsha-iiiv You're absolutely correct! The current implementation may not handle parameter deduplication properly according to the OpenAPI specification. Let me examine the actual implementation to verify this concern. 🏁 Script executed: Length of output: 7907 You're absolutely right! Looking at the current implementation, I can confirm there are several issues with the parameter handling: Issues in Current Implementation
The Correct ApproachAccording to the OpenAPI 3.0 specification, parameters are uniquely identified by the combination of Here's how the deduplication should work: // Proper deduplication logic
const parameterMap = new Map<string, OpenAPIV3.ParameterObject>();
// Add path-level parameters first
if (Array.isArray(pathParameters)) {
pathParameters.forEach(p => {
const param = p as OpenAPIV3.ParameterObject;
const key = `${param.name}:${param.in}`;
parameterMap.set(key, param);
});
}
// Add operation-level parameters (these override path-level ones)
if (Array.isArray(operation.parameters)) {
operation.parameters.forEach(p => {
const param = p as OpenAPIV3.ParameterObject;
const key = `${param.name}:${param.in}`;
parameterMap.set(key, param); // This overwrites any path-level parameter
});
}
const allParameters = Array.from(parameterMap.values());This ensures that:
Would you like me to create a follow-up issue to track this parameter deduplication fix? ✏️ Learnings added
|
|
@kym6464 Hi, are you still interested in completing this pull request? |
2 participants
Fixes a bug where Path Item-level parameters are not considered.
Sample OpenAPI spec
Also makes some additional changes to aid local development:
src/index.tsto support direct execution so that you can run the program using tsx without having to compile (VS Code's JavaScript Debug terminal lets you step through the Typescript code which is handy)Summary by CodeRabbit
New Features
Refactor