Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThe PR migrates the OpenAPI reader pipeline to the OasReader/OpenApiMultiFileReader API, replaces concrete Microsoft.OpenApi model types with their interface counterparts (IOpenApi*), updates parameter-name and schema-type sampling logic, removes manual 3.1→3.0 downgrade/read helpers, and adds/adjusts many unit tests covering generation and validation behaviors. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #349 +/- ##
==========================================
+ Coverage 81.92% 85.02% +3.10%
==========================================
Files 12 12
Lines 603 521 -82
Branches 92 103 +11
==========================================
- Hits 494 443 -51
+ Misses 83 56 -27
+ Partials 26 22 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/HttpGenerator.Core/HttpFileGenerator.cs (1)
445-456:⚠️ Potential issue | 🟠 MajorUse a stable synthetic key for unnamed parameters.
The new null-name path still doesn't actually tolerate unnamed parameters safely:
parameter.ToString()can leak a CLR/debug string into query keys and@variables, and the non-OneRequestPerFilebranch still produces names likeOperation_whenparameter.Nameis blank. That can generate malformed.httpfiles instead of a best-effort placeholder.💡 One way to stabilize the fallback
+ private static string GetParameterKey(IOpenApiParameter parameter, ref int unnamedIndex) + => string.IsNullOrWhiteSpace(parameter.Name) + ? $"param{++unnamedIndex}" + : parameter.Name!; + private static Dictionary<string, string> AppendParameters( OpenApiDocument document, KeyValuePair<string, IOpenApiPathItem> operationPath, GeneratorSettings settings, OpenApiOperation operation, @@ - var parameterNameMap = new Dictionary<string, string>(); + var parameterNameMap = new Dictionary<string, string>(); + var unnamedIndex = 0; foreach (var parameter in parameters) { + var parameterKey = GetParameterKey(parameter, ref unnamedIndex); var parameterName = GetParameterName( settings, document, operation, operationNameGenerator, verb, operationPath.Key, - parameter); - parameterNameMap[parameter.Name ?? parameter.ToString()] = parameterName; + parameter, + parameterKey); + parameterNameMap[parameterKey] = parameterName; @@ private static string GetParameterName( GeneratorSettings settings, OpenApiDocument document, OpenApiOperation operation, IOperationNameGenerator operationNameGenerator, string verb, string operationPathKey, - IOpenApiParameter parameter) + IOpenApiParameter parameter, + string parameterKey) { if (settings.OutputType == OutputType.OneRequestPerFile) - return parameter.Name ?? parameter.ToString(); + return parameterKey; @@ - return $"{name}_{parameter.Name}"; + return $"{name}_{parameterKey}"; }Also applies to: 488-506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HttpGenerator.Core/HttpFileGenerator.cs` around lines 445 - 456, The issue is that unnamed parameters use unstable fallbacks (parameter.ToString()) and produce empty or duplicate parameter names (e.g., "Operation_") leading to malformed .http files; instead, use a stable synthetic key and placeholder name: when building parameterNameMap in HttpFileGenerator (the parameters loop that calls GetParameterName) replace parameter.Name ?? parameter.ToString() with a deterministic key like $"_unnamed_{parameterIndex}" (use the loop index or a running counter) and ensure GetParameterName returns a non-empty placeholder (e.g., "param_{index}" or "unnamed_{index}") for blank names so both the parameterNameMap and the non-OneRequestPerFile branch produce stable, valid identifiers; apply the same change to the corresponding code block around the other occurrence (the 488-506 region).
🧹 Nitpick comments (1)
src/HttpGenerator.Tests/GenerateCommandTests.cs (1)
86-99: Rename this test now that it asserts success.
Should_Fail_Validating_V31_Specnow expects exit code0, so the test name documents the opposite behavior. Renaming it (and keeping a separate invalid-spec case if you still want a negative assertion here) will make the new 3.1 validation behavior much easier to read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HttpGenerator.Tests/GenerateCommandTests.cs` around lines 86 - 99, The test method Should_Fail_Validating_V31_Spec currently asserts a success exit code (0); rename the test to reflect the new expectation (e.g., Should_Pass_Validating_V31_Spec or Should_Succeed_Validating_V31_Spec) and update any references/usages accordingly; keep the test body (settings.SkipValidation, TestFile.CreateSwaggerFile, and the call to sut.ExecuteAsync(context, settings, ...).Should().Be(0)) unchanged, and if you still need a negative coverage case add a separate test that asserts a non-zero exit for an invalid 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 `@src/HttpGenerator.Core/HttpFileGenerator.cs`:
- Around line 509-516: The switch on parameter.Schema.Type in
GetParameterDefaultValue is using ToString() which fails for flagged
JsonSchemaType values (e.g., String|Null); update the logic to handle flag enums
by extracting identifiers via OpenApiTypeMapper.ToIdentifiers(schema.Type) or by
testing flags with schema.Type.HasFlag(JsonSchemaType.Null) and similar checks,
then base the switch/selection on those identifiers (lowercased) or flag checks
to return correct defaults for nullable/combined types; adjust the related
switch blocks in GetParameterDefaultValue and any other schema-type switches
mentioned so they use the identifiers or HasFlag approach rather than
ToString().
---
Outside diff comments:
In `@src/HttpGenerator.Core/HttpFileGenerator.cs`:
- Around line 445-456: The issue is that unnamed parameters use unstable
fallbacks (parameter.ToString()) and produce empty or duplicate parameter names
(e.g., "Operation_") leading to malformed .http files; instead, use a stable
synthetic key and placeholder name: when building parameterNameMap in
HttpFileGenerator (the parameters loop that calls GetParameterName) replace
parameter.Name ?? parameter.ToString() with a deterministic key like
$"_unnamed_{parameterIndex}" (use the loop index or a running counter) and
ensure GetParameterName returns a non-empty placeholder (e.g., "param_{index}"
or "unnamed_{index}") for blank names so both the parameterNameMap and the
non-OneRequestPerFile branch produce stable, valid identifiers; apply the same
change to the corresponding code block around the other occurrence (the 488-506
region).
---
Nitpick comments:
In `@src/HttpGenerator.Tests/GenerateCommandTests.cs`:
- Around line 86-99: The test method Should_Fail_Validating_V31_Spec currently
asserts a success exit code (0); rename the test to reflect the new expectation
(e.g., Should_Pass_Validating_V31_Spec or Should_Succeed_Validating_V31_Spec)
and update any references/usages accordingly; keep the test body
(settings.SkipValidation, TestFile.CreateSwaggerFile, and the call to
sut.ExecuteAsync(context, settings, ...).Should().Be(0)) unchanged, and if you
still need a negative coverage case add a separate test that asserts a non-zero
exit for an invalid spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 874708f1-ac2d-45c5-a68f-d4efdc8cd8d4
📒 Files selected for processing (10)
src/HttpGenerator.Core/HttpFileGenerator.cssrc/HttpGenerator.Core/HttpGenerator.Core.csprojsrc/HttpGenerator.Core/OpenApiDocumentFactory.cssrc/HttpGenerator.Core/OperationNameGenerator.cssrc/HttpGenerator.Tests/GenerateCommandTests.cssrc/HttpGenerator/GenerateCommand.cssrc/HttpGenerator/HttpGenerator.csprojsrc/HttpGenerator/Validation/OpenApiStats.cssrc/HttpGenerator/Validation/OpenApiValidationResult.cssrc/HttpGenerator/Validation/OpenApiValidator.cs
💤 Files with no reviewable changes (1)
- src/HttpGenerator/HttpGenerator.csproj
- Test BaseUrl environment variable templates - Test SkipHeaders flag - Test authorization header loading from environment - Test custom authorization variable name - Test unique filename generation for duplicate operations - Test custom content type - Test empty specs and operations
…Extensions, OpenApiValidator - PrivacyHelper: test empty input, non-authorization text passthrough, multiple headers - SupportKeyInitializer: test non-ISupportProperties telemetry path - StringExtensions: test empty strings, null inputs, empty prefix - OpenApiValidator: test IsValid property true/false branches
- Add GenerateWithSpecificArgs function for targeted tests - Test authorization header with Bearer token - Test loading authorization from environment variable - Test skip-headers flag - Test custom content-type (XML) - Test environment variable base URL template - Apply additional tests to petstore fixtures for v2.0 and v3.0
- Test sample JSON generation for request bodies - Test query parameter variable generation - Test parameter default values (integer, number, boolean, string) - Test custom headers in generated content - Test IntelliJ test generation
|
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.
fixes #331
fixes #332
fixes #333
fixes #334
This pull request refactors the codebase to replace the usage of Microsoft's
OpenApilibraries with theOasReaderpackage for OpenAPI document handling. This change streamlines OpenAPI file reading and parsing, simplifies code for handling different OpenAPI versions, and updates method signatures to use new interfaces fromOasReader. It also updates the test suite to reflect the new validation logic.Dependency and library updates:
Microsoft.OpenApi,Microsoft.OpenApi.Readers, and related packages, and added theOasReaderpackage to bothHttpGenerator.CoreandHttpGeneratorprojects. [1] [2]Core logic refactoring for OpenAPI reading and parsing:
Replaced all usages of
OpenApiDocument,OpenApiParameter,OpenApiSchema, etc., with theirOasReaderinterface equivalents (such asIOpenApiParameter,IOpenApiSchema, andIOpenApiPathItem) throughout the codebase, including method signatures and internal logic inHttpFileGenerator.cs,OpenApiStats.cs, and related files. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Refactored OpenAPI document loading to use
OpenApiMultiFileReader.Read, removing custom logic for HTTP/file handling, OpenAPI version downgrading, and decompression. This simplifies the code and improves support for OpenAPI 3.1 and multi-file specs. [1] [2] [3]Validation and test updates:
General code cleanup:
These changes modernize the codebase, reduce maintenance overhead, and improve compatibility with newer OpenAPI specifications.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Documentation