Optional parameters with default values#803
Conversation
christianhelle
added
enhancement
|
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. WalkthroughAdds extraction and type-aware formatting of OpenAPI parameter default values into generated parameter declarations; updates nullable-parameter reordering to accept parameter models; and adds tests validating generated defaults for optional and dynamic query-string parameters. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Swagger as OpenAPI spec
participant Gen as RefitGenerator
participant PE as ParameterExtractor
participant Reorder as ReOrderNullableParameters
participant Default as GetDefaultValueForParameter
participant Format as FormatDefaultValue
Note over Swagger,Gen: Generation start
Gen->>PE: GetParameters(operationModel)
PE->>Reorder: ReOrderNullableParameters(parameters, parameterModels)
Reorder->>Default: lookup default by parameter name
Default-->>Reorder: raw default (or "default")
Reorder->>Format: FormatDefaultValue(type, rawDefault)
Format-->>Reorder: formatted literal (e.g., true, 10, "active")
Reorder-->>PE: parameters with formatted defaults
PE-->>Gen: parameter declarations with defaults
Note right of Gen: Code emitted and tests validate compilation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for optional parameters with default values from OpenAPI specifications. When an optional parameter has a default value defined in the schema, that value will now be used in the generated C# code instead of always using default.
Key changes:
- Enhanced parameter generation to extract and apply default values from OpenAPI schemas
- Added type-specific formatting for boolean, string, and numeric default values
- Updated test suite to verify optional parameters render with their specified default values
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Refitter.Core/ParameterExtractor.cs | Core implementation: added methods to extract and format default values from OpenAPI parameter schemas; updated parameter reordering logic to apply schema-defined defaults instead of always using default keyword |
| src/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs | New test suite validating that optional parameters with boolean, integer, and string default values are correctly generated and that the generated code compiles successfully |
| return type switch | ||
| { | ||
| "bool" => defaultValue.ToString()?.ToLowerInvariant() ?? "default", | ||
| "string" => $"\"{defaultValue}\"", |
There was a problem hiding this comment.
String default values are not properly escaped. If a default value contains special characters like quotes ("), backslashes (\), or newlines (\n), the generated code will be invalid.
Consider using a proper string escaping mechanism:
"string" => $"\"{defaultValue.ToString()?.Replace("\\", "\\\\").Replace("\"", "\\\"")}\"",Or better yet, use a built-in method like System.Text.Json.JsonSerializer.Serialize() or similar to handle all edge cases correctly.
| var variableName = parts[parts.Length - 1].TrimEnd(';', ','); | ||
|
|
||
| var parameterModel = parameterModels.FirstOrDefault(p => p.VariableName == variableName); | ||
| return parameterModel?.Schema?.Default != null |
There was a problem hiding this comment.
The code doesn't handle the case where parameterModel.Type could be null or empty. While this is unlikely given that it comes from NSwag's code generation, it would be safer to add a null check to prevent potential NullReferenceException.
Suggestion:
var parameterModel = parameterModels.FirstOrDefault(p => p.VariableName == variableName);
return parameterModel?.Schema?.Default != null && !string.IsNullOrEmpty(parameterModel.Type)
? FormatDefaultValue(parameterModel.Schema.Default, parameterModel.Type)
: "default";| return parameterModel?.Schema?.Default != null | |
| return parameterModel?.Schema?.Default != null && !string.IsNullOrEmpty(parameterModel?.Type) |
| _ when IsNumericType(type) => defaultValue.ToString() ?? "default", | ||
| _ => "default" | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Floating-point numeric literals need type suffixes to avoid compilation errors. Without the proper suffix:
floatvalues need anforFsuffix (e.g.,1.5f)decimalvalues need anmorMsuffix (e.g.,1.5m)
Without these suffixes, the compiler treats literals as double by default, which can cause type mismatch errors.
Suggestion:
_ when IsNumericType(type) => FormatNumericValue(defaultValue, type),And add a helper method:
private static string FormatNumericValue(object value, string type)
{
var numericString = value is IFormattable formattable
? formattable.ToString(null, CultureInfo.InvariantCulture)
: (value.ToString() ?? "default");
return type switch
{
"float" or "Single" => $"{numericString}f",
"decimal" or "Decimal" => $"{numericString}m",
_ => numericString
};
}| _ when IsNumericType(type) => defaultValue.ToString() ?? "default", | |
| _ => "default" | |
| }; | |
| } | |
| _ when IsNumericType(type) => FormatNumericValue(defaultValue, type), | |
| _ => "default" | |
| }; | |
| } | |
| private static string FormatNumericValue(object defaultValue, string type) | |
| { | |
| var numericString = defaultValue is IFormattable formattable | |
| ? formattable.ToString(null, CultureInfo.InvariantCulture) | |
| : (defaultValue.ToString() ?? "default"); | |
| return type switch | |
| { | |
| "float" or "Single" => $"{numericString}f", | |
| "decimal" or "Decimal" => $"{numericString}m", | |
| _ => numericString | |
| }; | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #803 +/- ##
==========================================
- Coverage 96.25% 95.83% -0.43%
==========================================
Files 23 23
Lines 1841 1871 +30
Branches 298 306 +8
==========================================
+ Hits 1772 1793 +21
- Misses 36 39 +3
- Partials 33 39 +6
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
🧹 Nitpick comments (2)
src/Refitter.Core/ParameterExtractor.cs (1)
169-175: Consider qualified type names.The numeric type check uses string equality, which works for unqualified type names but could miss qualified names like
System.Int32orSystem.Decimal.If qualified type names are possible in
parameterType, consider checking the type name with and without namespace qualification:private static bool IsNumericType(string type) { + // Handle qualified names like System.Int32 + var unqualifiedType = type.Contains('.') ? type.Substring(type.LastIndexOf('.') + 1) : type; + - return type is "int" or "Int32" or "long" or "Int64" or "short" or "Int16" + return unqualifiedType is "int" or "Int32" or "long" or "Int64" or "short" or "Int16" or "byte" or "Byte" or "decimal" or "Decimal" or "float" or "Single" or "double" or "Double" or "sbyte" or "SByte" or "uint" or "UInt32" or "ulong" or "UInt64" or "ushort" or "UInt16"; }src/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs (1)
11-91: Consider adding OpenAPI 2.0 test variant for complete coverage.OpenAPI 2.0 supports default values for parameters, so the current test using only OpenAPI 3.0.1 is incomplete. Per learnings, tests should cover both specifications. However, the similar
OptionalNullableParametersTestsalso uses only OpenAPI 3.0, suggesting this dual-version testing pattern isn't currently enforced in the Examples test suite.Consider adding a second test method using an OpenAPI 2.0 specification to align with the learning requirement and establish a pattern for parameter-related tests. This would strengthen test coverage without breaking the current codebase approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Refitter.Core/ParameterExtractor.cs(6 hunks)src/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Follow existing C# coding conventions defined in .editorconfig
Use PascalCase for public members and camelCase for parameters and local variables
Use meaningful variable and method names
Keep methods focused with single responsibility
Run dotnet format and ensure no formatting changes before committing
Files:
src/Refitter.Core/ParameterExtractor.cssrc/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs
src/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Include XML documentation for public APIs
Files:
src/Refitter.Core/ParameterExtractor.cssrc/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs
src/Refitter.Tests/Examples/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/Refitter.Tests/Examples/**/*.cs: New unit tests must follow the pattern used in Refitter.Tests.Examples (Arrange/Act/Assert style with GenerateCode(), content assertions, and build verification)
Add unit tests for new CLI options in src/Refitter.Tests/Examples/ following the established test structure
Files:
src/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs
🧠 Learnings (6)
📚 Learning: 2025-10-20T20:29:54.111Z
Learnt from: CR
Repo: christianhelle/refitter PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-20T20:29:54.111Z
Learning: Applies to src/Refitter.Core/RefitInterfaceGenerator.cs : Implement or update generation logic in src/Refitter.Core/RefitInterfaceGenerator.cs (or related generator classes) for new features/options
Applied to files:
src/Refitter.Core/ParameterExtractor.cssrc/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs
📚 Learning: 2025-10-20T20:29:54.111Z
Learnt from: CR
Repo: christianhelle/refitter PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-20T20:29:54.111Z
Learning: Applies to src/Refitter.Core/Settings/RefitGeneratorSettings.cs : Add corresponding property to src/Refitter.Core/Settings/RefitGeneratorSettings.cs for new CLI options
Applied to files:
src/Refitter.Core/ParameterExtractor.cssrc/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs
📚 Learning: 2025-10-20T20:29:54.111Z
Learnt from: CR
Repo: christianhelle/refitter PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-20T20:29:54.111Z
Learning: Applies to src/Refitter/GenerateCommand.cs : Map new CLI options in CreateRefitGeneratorSettings() within src/Refitter/GenerateCommand.cs
Applied to files:
src/Refitter.Core/ParameterExtractor.cssrc/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs
📚 Learning: 2025-10-20T20:29:54.111Z
Learnt from: CR
Repo: christianhelle/refitter PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-20T20:29:54.111Z
Learning: Applies to src/Refitter.Tests/Examples/**/*.cs : Add unit tests for new CLI options in src/Refitter.Tests/Examples/ following the established test structure
Applied to files:
src/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs
📚 Learning: 2025-10-20T20:29:54.111Z
Learnt from: CR
Repo: christianhelle/refitter PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-20T20:29:54.111Z
Learning: Applies to src/Refitter.Tests/Resources/{V2,V3}/**/*.json : Always test with both OpenAPI 2.0 and 3.0 specifications using the provided test resources
Applied to files:
src/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs
📚 Learning: 2025-10-20T20:29:54.111Z
Learnt from: CR
Repo: christianhelle/refitter PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-20T20:29:54.111Z
Learning: Applies to src/Refitter.Tests/Examples/**/*.cs : New unit tests must follow the pattern used in Refitter.Tests.Examples (Arrange/Act/Assert style with GenerateCode(), content assertions, and build verification)
Applied to files:
src/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs
🧬 Code graph analysis (2)
src/Refitter.Core/ParameterExtractor.cs (1)
src/Refitter/GenerateCommand.cs (1)
RefitGeneratorSettings(271-318)
src/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs (3)
src/Refitter.Tests/Build/BuildHelper.cs (2)
BuildHelper(6-53)BuildCSharp(8-45)src/Refitter.Tests/TestUtilities/SwaggerFileHelper.cs (1)
SwaggerFileHelper(9-36)src/Refitter.Core/RefitGenerator.cs (1)
RefitGenerator(10-311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: script
- GitHub Check: 👌 Verify build
- GitHub Check: 👌 Verify build
- GitHub Check: 👌 Verify build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (7)
src/Refitter.Tests/Examples/OptionalParametersWithDefaultValuesTests.cs (2)
93-124: LGTM! Comprehensive test coverage.The test suite follows the established AAA pattern and covers all key scenarios:
- Basic code generation
- Optional parameters with correct default values
- Required parameters without defaults
- Build verification using BuildHelper
The assertions are specific and validate the exact generated signatures expected.
Based on learnings
126-138: LGTM! Clean helper implementation.The
GenerateCode()helper follows the established pattern in the test suite, properly configuringRefitGeneratorSettingswithOptionalParameters = trueto enable the new feature.Based on learnings
src/Refitter.Core/ParameterExtractor.cs (5)
89-89: LGTM! Necessary parameter for default value resolution.Passing
operationModel.ParameterstoReOrderNullableParametersenables the new default value extraction logic by providing access to the parameter models and their schema information.
117-137: LGTM! Clear implementation of default value assignment.The extended signature and logic correctly:
- Accepts parameter models for default value lookup
- Orders nullable parameters last
- Assigns appropriate default values via
GetDefaultValueForParameter
200-210: Minor refactor improves readability.Materializing the filtered attributes as a list and using
Count == 0is a small readability improvement over the previous implementation.
364-386: LGTM! Formatting consistency improvements.The changes to use array syntax for split separators and adjust string literal formatting improve code consistency without altering behavior.
139-151: Review comment is based on theoretical concerns that don't materialize in practice.The parameter string parsing logic is actually robust for the codebase's actual usage patterns. Analysis of parameter string construction shows a consistent format:
[attributes] type variableName, where:
- All attribute strings are bracketed and quoted (e.g.,
[Header("X-Custom Header")]), preventing spaces within attributes from breaking parsing- C# types contain no spaces (e.g.,
int?,MyType,Dictionary<string,int>)- Variable names are C# identifiers without spaces
The split-by-space approach correctly isolates the variable name as the last token in all observed construction patterns across route, query, body, header, form, and binary parameters. The defensive
return "default"for missing parameters adds safety.
| private static string FormatDefaultValue(object? defaultValue, string parameterType) | ||
| { | ||
| if (defaultValue == null) | ||
| return "default"; | ||
|
|
||
| var type = parameterType.TrimEnd('?').Trim(); | ||
|
|
||
| return type switch | ||
| { | ||
| "bool" => defaultValue.ToString()?.ToLowerInvariant() ?? "default", | ||
| "string" => $"\"{defaultValue}\"", | ||
| _ when IsNumericType(type) => defaultValue.ToString() ?? "default", | ||
| _ => "default" | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fix string escaping to prevent invalid generated code.
Line 163 wraps string default values in quotes but doesn't escape existing quotes or special characters. If the OpenAPI schema specifies a default like my"test or path\to\file, the generated code will be syntactically invalid:
string? filter = "my"test" // Invalid C#
string? path = "path\to\file" // Invalid escape sequenceApply this diff to add proper string escaping:
- "string" => $"\"{defaultValue}\"",
+ "string" => $"\"{defaultValue.ToString()?.Replace("\\", "\\\\").Replace("\"", "\\\"")}\"",Alternatively, use C#'s verbatim string literals or @$"..." syntax with appropriate escaping, or leverage a library method for C# string literal generation.
🤖 Prompt for AI Agents
In src/Refitter.Core/ParameterExtractor.cs around lines 153 to 167, the string
default-value branch wraps values in quotes without escaping, producing invalid
C# when the default contains quotes or backslashes; fix by emitting a safe C#
string literal: generate a verbatim string literal (prefix with @ and wrap in
quotes) and escape any existing double-quote characters by doubling them
(replace " with "") so backslashes are preserved, or if you prefer non-verbatim
literals, escape backslashes, double-quotes and control characters properly;
update the "string" case to produce that escaped literal.
…query string parameter classes
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Refitter.Core/ParameterExtractor.cs (3)
139-151: Add null/empty check forparameterModel.Type.While the null-conditional operator on line 148 safely guards against a null
parameterModel, theTypeproperty itself could be null or empty, leading to potential issues inFormatDefaultValue.Apply this diff to add the safety check:
- return parameterModel?.Schema?.Default != null + return parameterModel?.Schema?.Default != null && !string.IsNullOrEmpty(parameterModel.Type) ? FormatDefaultValue(parameterModel.Schema.Default, parameterModel.Type) : "default";
163-163: Fix string escaping to prevent invalid generated code.String default values are not properly escaped. If the OpenAPI schema specifies a default containing quotes (
"), backslashes (\), or control characters, the generated C# code will be syntactically invalid (e.g.,string? filter = "my"test").Apply this diff to properly escape string literals:
- "string" => $"\"{defaultValue}\"", + "string" => $"\"{defaultValue.ToString()?.Replace("\\", "\\\\").Replace("\"", "\\\"")}\"",Alternatively, use
SymbolDisplay.FormatLiteralfrom Roslyn or a similar library method to handle all edge cases.
164-167: Add type suffixes for floating-point literals.Floating-point numeric literals require type suffixes to avoid compilation errors. Without suffixes:
floatvalues needforF(e.g.,1.5f)decimalvalues needmorM(e.g.,1.5m)Without these, the compiler treats literals as
doubleby default, causing type mismatch errors.Apply this diff to add proper suffixes:
- _ when IsNumericType(type) => defaultValue.ToString() ?? "default", + _ when IsNumericType(type) => FormatNumericValue(defaultValue, type), _ => "default" }; } + private static string FormatNumericValue(object defaultValue, string type) + { + var numericString = defaultValue is IFormattable formattable + ? formattable.ToString(null, CultureInfo.InvariantCulture) + : (defaultValue.ToString() ?? "default"); + + return type switch + { + "float" or "Single" => $"{numericString}f", + "decimal" or "Decimal" => $"{numericString}m", + _ => numericString + }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Refitter.Core/ParameterExtractor.cs(7 hunks)src/Refitter.Tests/Examples/DynamicQueryStringParametersWithDefaultValuesTests.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Follow existing C# coding conventions defined in .editorconfig
Use PascalCase for public members and camelCase for parameters and local variables
Use meaningful variable and method names
Keep methods focused with single responsibility
Run dotnet format and ensure no formatting changes before committing
Files:
src/Refitter.Core/ParameterExtractor.cssrc/Refitter.Tests/Examples/DynamicQueryStringParametersWithDefaultValuesTests.cs
src/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Include XML documentation for public APIs
Files:
src/Refitter.Core/ParameterExtractor.cssrc/Refitter.Tests/Examples/DynamicQueryStringParametersWithDefaultValuesTests.cs
src/Refitter.Tests/Examples/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/Refitter.Tests/Examples/**/*.cs: New unit tests must follow the pattern used in Refitter.Tests.Examples (Arrange/Act/Assert style with GenerateCode(), content assertions, and build verification)
Add unit tests for new CLI options in src/Refitter.Tests/Examples/ following the established test structure
Files:
src/Refitter.Tests/Examples/DynamicQueryStringParametersWithDefaultValuesTests.cs
🧠 Learnings (4)
📚 Learning: 2025-10-20T20:29:54.111Z
Learnt from: CR
Repo: christianhelle/refitter PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-20T20:29:54.111Z
Learning: Applies to src/Refitter.Core/RefitInterfaceGenerator.cs : Implement or update generation logic in src/Refitter.Core/RefitInterfaceGenerator.cs (or related generator classes) for new features/options
Applied to files:
src/Refitter.Core/ParameterExtractor.cs
📚 Learning: 2025-10-20T20:29:54.111Z
Learnt from: CR
Repo: christianhelle/refitter PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-20T20:29:54.111Z
Learning: Applies to src/Refitter.Tests/Examples/**/*.cs : New unit tests must follow the pattern used in Refitter.Tests.Examples (Arrange/Act/Assert style with GenerateCode(), content assertions, and build verification)
Applied to files:
src/Refitter.Tests/Examples/DynamicQueryStringParametersWithDefaultValuesTests.cs
📚 Learning: 2025-10-20T20:29:54.111Z
Learnt from: CR
Repo: christianhelle/refitter PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-20T20:29:54.111Z
Learning: Applies to src/Refitter.Tests/Examples/**/*.cs : Add unit tests for new CLI options in src/Refitter.Tests/Examples/ following the established test structure
Applied to files:
src/Refitter.Tests/Examples/DynamicQueryStringParametersWithDefaultValuesTests.cs
📚 Learning: 2025-10-20T20:29:54.111Z
Learnt from: CR
Repo: christianhelle/refitter PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-20T20:29:54.111Z
Learning: Applies to src/Refitter.Tests/Resources/{V2,V3}/**/*.json : Always test with both OpenAPI 2.0 and 3.0 specifications using the provided test resources
Applied to files:
src/Refitter.Tests/Examples/DynamicQueryStringParametersWithDefaultValuesTests.cs
🧬 Code graph analysis (2)
src/Refitter.Core/ParameterExtractor.cs (1)
src/Refitter/GenerateCommand.cs (1)
RefitGeneratorSettings(271-318)
src/Refitter.Tests/Examples/DynamicQueryStringParametersWithDefaultValuesTests.cs (3)
src/Refitter.Tests/Build/BuildHelper.cs (2)
BuildHelper(6-53)BuildCSharp(8-45)src/Refitter.Tests/TestUtilities/SwaggerFileHelper.cs (1)
SwaggerFileHelper(9-36)src/Refitter.Core/RefitGenerator.cs (1)
RefitGenerator(10-311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: 👌 Verify build
- GitHub Check: script
- GitHub Check: 👌 Verify build
- GitHub Check: 👌 Verify build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (11)
src/Refitter.Core/ParameterExtractor.cs (6)
89-89: LGTM!Passing
operationModel.Parametersenables default value resolution from the OpenAPI schema. This change correctly supports the PR objective of using spec-defined defaults.
117-137: LGTM!The extended signature and updated logic correctly extract defaults from OpenAPI schema parameter models instead of always using
default. This aligns with the PR objective to respect spec-defined defaults for optional parameters.
169-175: LGTM!The helper correctly identifies all standard .NET numeric types, covering both C# aliases and CLR type names.
200-210: LGTM!The refactor to materialize the filtered attributes as a
Listis functionally equivalent and the performance impact is negligible for typical attribute counts.
315-320: LGTM!The logic correctly integrates default value assignment for dynamic querystring properties. However, ensure the critical issues in
FormatDefaultValue(string escaping and numeric type suffixes) are addressed for this to generate valid code.
367-393: LGTM!Formatting adjustments to XML doc comment generation have no functional impact and improve consistency.
src/Refitter.Tests/Examples/DynamicQueryStringParametersWithDefaultValuesTests.cs (5)
11-91: LGTM!The OpenAPI specification is complete and well-structured, with required parameters (start, end) and optional parameters (includeCancelled, pageSize, filter) that define default values, perfectly supporting the test objectives.
93-98: LGTM!The test correctly follows the Arrange/Act/Assert pattern and verifies that code generation produces non-empty output.
100-107: LGTM!The test correctly verifies that optional parameters with defaults in the OpenAPI spec are generated with the expected default values (
truefor bool,10for int,"active"for string).
119-124: LGTM!The build verification test correctly ensures the generated code compiles, following the established test pattern. Based on learnings.
126-139: LGTM!The helper method follows the established pattern in the test suite: creates a temporary OpenAPI file, configures
RefitGeneratorSettingswithOptionalParametersandUseDynamicQuerystringParameters, and generates code usingRefitGenerator. Based on learnings.
| [Fact] | ||
| public async Task Generated_Code_Should_Have_Required_Parameters_Without_Defaults() | ||
| { | ||
| string generatedCode = await GenerateCode(); | ||
| generatedCode.Should().Contain("System.DateTimeOffset Start { get; set; }"); | ||
| generatedCode.Should().Contain("System.DateTimeOffset End { get; set; }"); | ||
| generatedCode.Should().NotContain("System.DateTimeOffset Start { get; set; } ="); | ||
| generatedCode.Should().NotContain("System.DateTimeOffset End { get; set; } ="); | ||
| } |
There was a problem hiding this comment.
Fix typo in assertion on line 116.
Line 116 contains a double space between DateTimeOffset and End, while line 115 uses a single space. This inconsistency could cause the test to pass incorrectly if the generated code has proper single spacing but the End parameter erroneously includes a default value.
Apply this diff to fix the typo:
- generatedCode.Should().NotContain("System.DateTimeOffset End { get; set; } =");
+ generatedCode.Should().NotContain("System.DateTimeOffset End { get; set; } =");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [Fact] | |
| public async Task Generated_Code_Should_Have_Required_Parameters_Without_Defaults() | |
| { | |
| string generatedCode = await GenerateCode(); | |
| generatedCode.Should().Contain("System.DateTimeOffset Start { get; set; }"); | |
| generatedCode.Should().Contain("System.DateTimeOffset End { get; set; }"); | |
| generatedCode.Should().NotContain("System.DateTimeOffset Start { get; set; } ="); | |
| generatedCode.Should().NotContain("System.DateTimeOffset End { get; set; } ="); | |
| } | |
| [Fact] | |
| public async Task Generated_Code_Should_Have_Required_Parameters_Without_Defaults() | |
| { | |
| string generatedCode = await GenerateCode(); | |
| generatedCode.Should().Contain("System.DateTimeOffset Start { get; set; }"); | |
| generatedCode.Should().Contain("System.DateTimeOffset End { get; set; }"); | |
| generatedCode.Should().NotContain("System.DateTimeOffset Start { get; set; } ="); | |
| generatedCode.Should().NotContain("System.DateTimeOffset End { get; set; } ="); | |
| } |
🤖 Prompt for AI Agents
In
src/Refitter.Tests/Examples/DynamicQueryStringParametersWithDefaultValuesTests.cs
around lines 109 to 117, the assertion on line 116 has a typo with two spaces
between "DateTimeOffset" and "End"; update that NotContain assertion to use a
single space so it matches the other assertions (i.e., "System.DateTimeOffset
End { get; set; } =") to correctly detect default value presence.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@christianhelle I've opened a new pull request, #804, to work on those changes. Once the pull request is ready, I'll request review from you. |
Updated [refitter](https://github.com/christianhelle/refitter) from 1.7.0 to 1.7.1. <details> <summary>Release notes</summary> _Sourced from [refitter's releases](https://github.com/christianhelle/refitter/releases)._ ## 1.7.1 ### Implemented enhancements - Improved handling of optional parameters [\#448](christianhelle/refitter#448) by @christianhelle - Allow omitting certain operation headers \#840 [\#841](christianhelle/refitter#841) by @kmc059000 - Migrate unit tests from xUnit to TUnit [\#830](christianhelle/refitter#830) by @christianhelle - Refit v9.0.2 [\#829](christianhelle/refitter#829) - Add .NET 10 support [\#822](christianhelle/refitter#822) ([christianhelle](https://github.com/christianhelle)) - Fix missing XML doc for CancellationToken [\#819](christianhelle/refitter#819) by @christianhelle - Fix incorrect casing on multi-part form data parameters [\#806](christianhelle/refitter#806) by @christianhelle - Optional parameters with default values [\#803](christianhelle/refitter#803) by @christianhelle - Asana API specs strange naming [\#364](christianhelle/refitter#364) by @christianhelle ### Fixed bugs - Using cancellation tokens with xml doc comments plus TreatWarningsAsErrors and documentation file [\#817](christianhelle/refitter#817) - Multipart form data parameters wrong casing [\#805](christianhelle/refitter#805) - Use of non generic `JsonStringEnumConverter` prohibits usage of Json-SourceGenerationContext [\#778](christianhelle/refitter#778) - SourceGenerator 1.5 and newer causes build error with Visual Studio 2022 [\#627](christianhelle/refitter#627) - Fix string escaping, null safety, and numeric literals in optional parameter default values [\#804](christianhelle/refitter#804) - Fix typo in class name and Spectre.Console markup escaping issue [\#828](christianhelle/refitter#828) ### Merged pull requests - docs: add kmc059000 as a contributor for ideas, and code [\#842](christianhelle/refitter#842) ([allcontributors[bot]](https://github.com/apps/allcontributors)) - Bump actions/upload-artifact from 5 to 6 [\#838](christianhelle/refitter#838) ([dependabot[bot]](https://github.com/apps/dependabot)) - Update dependency TUnit to 1.5.37 [\#836](christianhelle/refitter#836) ([renovate[bot]](https://github.com/apps/renovate)) - Update dotnet monorepo [\#834](christianhelle/refitter#834) ([renovate[bot]](https://github.com/apps/renovate)) - Update dependency TUnit to 1.5.6 [\#833](christianhelle/refitter#833) ([renovate[bot]](https://github.com/apps/renovate)) - Update dependency TUnit to 1.5.0 [\#832](christianhelle/refitter#832) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency spectre.console.cli to 0.53.1 [\#827](christianhelle/refitter#827) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency polly to 8.6.5 [\#826](christianhelle/refitter#826) ([renovate[bot]](https://github.com/apps/renovate)) - Bump actions/checkout from 5 to 6 [\#825](christianhelle/refitter#825) ([dependabot[bot]](https://github.com/apps/dependabot)) - chore\(deps\): update nswag monorepo to 14.6.3 [\#824](christianhelle/refitter#824) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency microsoft.build.utilities.core to v18 [\#821](christianhelle/refitter#821) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency swashbuckle.aspnetcore to 10.0.1 [\#820](christianhelle/refitter#820) ([renovate[bot]](https://github.com/apps/renovate)) - docs: add karoberts as a contributor for bug [\#818](christianhelle/refitter#818) ([allcontributors[bot]](https://github.com/apps/allcontributors)) - chore\(deps\): update dependency swashbuckle.aspnetcore to v10 [\#816](christianhelle/refitter#816) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency microsoft.extensions.http.resilience to v10 [\#815](christianhelle/refitter#815) ([renovate[bot]](https://github.com/apps/renovate)) - Update Spectre.Console.Cli to 0.53.0 [\#814](christianhelle/refitter#814) ([Copilot](https://github.com/apps/copilot-swe-agent)) - chore\(deps\): update dotnet monorepo to v10 \(major\) [\#813](christianhelle/refitter#813) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency microsoft.net.test.sdk to 18.0.1 [\#808](christianhelle/refitter#808) ([renovate[bot]](https://github.com/apps/renovate)) - docs: add mhartmair-cubido as a contributor for bug [\#807](christianhelle/refitter#807) ([allcontributors[bot]](https://github.com/apps/allcontributors)) ### New Contributors * @kmc059000 made their first contribution in christianhelle/refitter#841 Full Changelog: christianhelle/refitter@1.7.0...1.7.1 Commits viewable in [compare view](christianhelle/refitter@1.7.0...1.7.1). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Improve optional parameters support by using the default values defined in the OpenAPI specifications document
Given the following OpenAPI spec:
{ "openapi": "3.0.1", "info": { "title": "Test API", "version": "v1" }, "paths": { "/api/schedule/list": { "get": { "tags": ["Schedule"], "operationId": "List", "parameters": [ { "name": "start", "in": "query", "required": true, "schema": { "type": "string", "format": "date" } }, { "name": "end", "in": "query", "required": true, "schema": { "type": "string", "format": "date" } }, { "name": "includeCancelled", "in": "query", "required": false, "schema": { "type": "boolean", "default": true } }, { "name": "pageSize", "in": "query", "required": false, "schema": { "type": "integer", "format": "int32", "default": 10 } }, { "name": "filter", "in": "query", "required": false, "schema": { "type": "string", "default": "active" } } ], "responses": { "200": { "description": "Success", "content": { "application/json": { "schema": { "type": "array", "items": { "type": "object" } } } } } } } } } }generates the following Refit interface:
And when the option
UseDynamicQuerystringParametersis enabled generates the following Refit interface and query string parameter class:CoPilot Generated Description
This pull request enhances how default values for optional parameters are handled and generated in the code, ensuring that parameters defined with defaults in the OpenAPI specification are reflected accurately in the generated C# code. It also introduces comprehensive tests to verify this behavior for both method parameters and dynamic query string parameters.
Default Value Handling Improvements
ReOrderNullableParametersinParameterExtractor.csto accept parameter models and assign explicit default values for optional parameters based on their OpenAPI schema, instead of always usingdefault. [1] [2] [3]GetDefaultValueForParameter,FormatDefaultValue, andIsNumericTypeto extract and format default values for different parameter types (bool, string, numeric).Code Generation Enhancements
Test Coverage
OptionalParametersWithDefaultValuesTests.csto verify that generated code for optional parameters includes correct default values and required parameters do not have defaults.DynamicQueryStringParametersWithDefaultValuesTests.csto ensure dynamic query string parameters with defaults are generated accurately and that the code builds successfully.Minor Code Quality Improvements
JoinAttributesfor clarity and robustness.