Fix #580: Nullable strings marked correctly#921
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughModifies CSharpClientGeneratorFactory to auto-enable GenerateOptionalPropertiesAsNullable when GenerateNullableReferenceTypes is active, ensuring optional properties are emitted as nullable types in generated code. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
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 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 addresses Refitter’s C# contract generation nullability behavior so that OpenAPI nullable: true strings are emitted as string? when nullable reference types are enabled, aligning output with NSwag’s required configuration combination.
Changes:
- Auto-enables
GenerateOptionalPropertiesAsNullablewhenGenerateNullableReferenceTypesis enabled in the NSwag C# generator settings. - Improves nullable string contract generation when nullable reference types are turned on.
| // Auto-enable optional properties as nullable when nullable reference types enabled | ||
| if (generator.Settings.CSharpGeneratorSettings.GenerateNullableReferenceTypes) | ||
| { | ||
| generator.Settings.CSharpGeneratorSettings.GenerateOptionalPropertiesAsNullable = true; | ||
| } |
There was a problem hiding this comment.
This change alters nullability generation behavior but there’s no regression test asserting that an OpenAPI schema with type: string + nullable: true (and/or an optional string property) generates string? when nullable reference types are enabled. Please add/extend a unit test that checks the generated DTO/contract property type (not just build success) to prevent this from regressing again.
| // Auto-enable optional properties as nullable when nullable reference types enabled | ||
| if (generator.Settings.CSharpGeneratorSettings.GenerateNullableReferenceTypes) | ||
| { | ||
| generator.Settings.CSharpGeneratorSettings.GenerateOptionalPropertiesAsNullable = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
This forces GenerateOptionalPropertiesAsNullable = true whenever GenerateNullableReferenceTypes is enabled, which effectively ignores any explicit configuration that tries to keep generateOptionalPropertiesAsNullable disabled (it’s documented as an independent setting). Consider only auto-enabling when the user has not explicitly set GenerateOptionalPropertiesAsNullable, or introduce a dedicated setting to control this coupling (and document the behavior).
| // Auto-enable optional properties as nullable when nullable reference types enabled | |
| if (generator.Settings.CSharpGeneratorSettings.GenerateNullableReferenceTypes) | |
| { | |
| generator.Settings.CSharpGeneratorSettings.GenerateOptionalPropertiesAsNullable = true; | |
| } |
Updated [refitter](https://github.com/christianhelle/refitter) from 1.7.1 to 2.0.0. <details> <summary>Release notes</summary> _Sourced from [refitter's releases](https://github.com/christianhelle/refitter/releases)._ ## 2.0.0 ## What's Changed * Fix numeric format with pattern quirk - infer type from format for all numeric types by @Copilot in christianhelle/refitter#869 * Read group documentation from document tags. by @DJ4ddi in christianhelle/refitter#887 * Fix null reference and XML escaping in XmlDocumentationGenerator by @Copilot in christianhelle/refitter#890 * Fix build workflow: add dotnet restore before dotnet msbuild in Prepare step by @Copilot in christianhelle/refitter#892 christianhelle/refitter#895 * Fix MSBuild workflow by @christianhelle in christianhelle/refitter#898 * Add support for generating a single client from multiple OpenAPI specifications by @Copilot in christianhelle/refitter#904 * Migrate from Microsoft.OpenApi.Readers 1.x to Microsoft.OpenApi 3.x by @vgmello in christianhelle/refitter#907 * Improve Smoke Tests execution time by @christianhelle in christianhelle/refitter#915 * Fix #580: Nullable strings marked correctly by @christianhelle in christianhelle/refitter#921 * Fix #672: MultipleInterfaces ByTag method naming scoped per-interface by @christianhelle in christianhelle/refitter#922 * Fix #635: Refactor source generator to use context.AddSource() by @christianhelle in christianhelle/refitter#923 * Add custom format mappings configuration by @christianhelle in christianhelle/refitter#927 * Fix multipart form-data parameter extraction by @christianhelle in christianhelle/refitter#928 christianhelle/refitter#911 christianhelle/refitter#902 * Fix smoke tests: --interface-only variant missing using directive for contract types by @Copilot in christianhelle/refitter#933 * Fix SonarCloud Code Quality Issues by @Copilot in christianhelle/refitter#932 * Add debug logging for source generator when searching for .refitter files by @codymullins in christianhelle/refitter#743 * Fix: Base type not generated for types using oneOf with discriminator by @Copilot in christianhelle/refitter#906 * Add option for Method Level Authorization header attribute by @Roflincopter in christianhelle/refitter#897 * Fix PR #897 review feedback and add comprehensive bearer auth tests by @christianhelle in christianhelle/refitter#936 * Fix numeric suffix added to interface method names in ByTag mode by @Copilot in christianhelle/refitter#914 * Improve OpenAPI parse + codegen throughput by removing avoidable allocations and repeated regex work by @Copilot in christianhelle/refitter#937 * Move [JsonConverter] from enum properties to enum types by @christianhelle in christianhelle/refitter#938 * Fix broken CLI tool help text by @christianhelle in christianhelle/refitter#940 * Improve code coverage to >90% by @christianhelle in christianhelle/refitter#941 * Microsoft.OpenApi v3.4 by @christianhelle in christianhelle/refitter#945 * Add Unicode support for XML doc comment generation by @christianhelle in christianhelle/refitter#948 * Add PropertyNamingPolicy support for JSON property naming by @christianhelle in christianhelle/refitter#969 christianhelle/refitter#966 * Fix recursive schema stack overflows by @christianhelle in christianhelle/refitter#971 * Made Header Parameters for Security Schemes safe to use as C# variable name by @smoerijf in christianhelle/refitter#977 * Enhance schema alias handling and property name generation by @christianhelle in christianhelle/refitter#996 * Verify alias handling and sanitize PascalCase properties by @christianhelle in christianhelle/refitter#997 * Harden .refitter settings deserialization and output path handling by @christianhelle in christianhelle/refitter#1000 * Special-case the default .refitter filename before deriving OutputFilename by @coderabbitai[bot] in christianhelle/refitter#1002 * [v2.0 audit] Fix pre-release regressions from #1057 by @christianhelle in christianhelle/refitter#1064 * Resolve high-severity audit findings from #1057 by @christianhelle in christianhelle/refitter#1067 * [v2.0 audit] Close remaining verified #1057 regressions by @christianhelle in christianhelle/refitter#1070 * Harden generation flows by @christianhelle in christianhelle/refitter#1071 * Breaking changes and Migration Guide for v2.0.0 by @christianhelle in christianhelle/refitter#1009 * Handle equivalent duplicate schemas in multi-spec merge by @christianhelle in christianhelle/refitter#1076 * Tighten warning handling across Refitter builds by @christianhelle in christianhelle/refitter#1079 * Fix default solution path and update .NET version in VS Code config by @christianhelle in christianhelle/refitter#1082 * Fix docker smoke tests by @christianhelle in christianhelle/refitter#1081 * Sanitize malformed schema type names without renaming clean schemas by @christianhelle in christianhelle/refitter#1085 ... (truncated) ## 1.7.3 ## What's Changed * Add support for systems running only .NET 10.0 (without .NET 8.0 or 9.0) in Refitter.MSBuild by @christianhelle in christianhelle/refitter#882 * Update to return HttpResponseMessage for file downloads by @frogcrush in christianhelle/refitter#877 ## New Contributors * @frogcrush made their first contribution in christianhelle/refitter#877 **Full Changelog**: christianhelle/refitter@1.7.2...1.7.3 ## 1.7.2 **Implemented enhancements:** - Improve Immutable Records ergonomics [\#844](christianhelle/refitter#844) - Omit certain operation headers and include all others [\#840](christianhelle/refitter#840) - Create .refitter settings file as part of output [\#859](christianhelle/refitter#859) by @[christianhelle - Fix integerType enum deserialization issue [\#855](christianhelle/refitter#855) ([christianhelle](https://github.com/christianhelle)) - support custom nswag template directory \#844 [\#854](christianhelle/refitter#854) by @kmc059000 - Fix missing method parameter XML code-documentation [\#850](christianhelle/refitter#850) ([christianhelle](https://github.com/christianhelle)) **Fixed bugs:** - integerType parsing in settings file fails [\#851](christianhelle/refitter#851) - CS1573 : Method parameter has no matching XML comment [\#846](christianhelle/refitter#846) **Merged pull requests:** - docs: add frogcrush as a contributor for code [\#878](christianhelle/refitter#878) ([allcontributors[bot]](https://github.com/apps/allcontributors)) - Migrate solution files from .sln to .slnx format [\#876](christianhelle/refitter#876) ([Copilot](https://github.com/apps/copilot-swe-agent)) - Fix issue with randomly failing tests to due parallel execution [\#872](christianhelle/refitter#872) ([christianhelle](https://github.com/christianhelle)) - chore\(deps\): update dependency tunit to 1.9.2 [\#863](christianhelle/refitter#863) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency tunit to 1.7.7 [\#862](christianhelle/refitter#862) ([renovate[bot]](https://github.com/apps/renovate)) - Add unit tests for WriteRefitterSettingsFile functionality [\#860](christianhelle/refitter#860) ([Copilot](https://github.com/apps/copilot-swe-agent)) - chore\(deps\): update dependency ruby to v4 [\#858](christianhelle/refitter#858) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency tunit to 1.6.28 [\#857](christianhelle/refitter#857) ([renovate[bot]](https://github.com/apps/renovate)) - docs: add 0x2badc0de as a contributor for bug [\#856](christianhelle/refitter#856) ([allcontributors[bot]](https://github.com/apps/allcontributors)) - chore\(deps\): update dependency swashbuckle.aspnetcore to 10.1.0 [\#853](christianhelle/refitter#853) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency tunit to 1.6.0 [\#852](christianhelle/refitter#852) ([renovate[bot]](https://github.com/apps/renovate)) - docs: add lilinus as a contributor for code [\#849](christianhelle/refitter#849) ([allcontributors[bot]](https://github.com/apps/allcontributors)) - chore\(deps\): update dependency ruby to v3.4.8 [\#845](christianhelle/refitter#845) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency tunit to 1.5.70 [\#837](christianhelle/refitter#837) ([renovate[bot]](https://github.com/apps/renovate)) **Full Changelog**: christianhelle/refitter@1.7.1...1.7.2 Commits viewable in [compare view](christianhelle/refitter@1.7.1...2.0.0). </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 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>
Updated [Refitter.MSBuild](https://github.com/christianhelle/refitter) from 1.7.3 to 2.0.0. <details> <summary>Release notes</summary> _Sourced from [Refitter.MSBuild's releases](https://github.com/christianhelle/refitter/releases)._ ## 2.0.0 ## What's Changed * Fix numeric format with pattern quirk - infer type from format for all numeric types by @Copilot in christianhelle/refitter#869 * Read group documentation from document tags. by @DJ4ddi in christianhelle/refitter#887 * Fix null reference and XML escaping in XmlDocumentationGenerator by @Copilot in christianhelle/refitter#890 * Fix build workflow: add dotnet restore before dotnet msbuild in Prepare step by @Copilot in christianhelle/refitter#892 christianhelle/refitter#895 * Fix MSBuild workflow by @christianhelle in christianhelle/refitter#898 * Add support for generating a single client from multiple OpenAPI specifications by @Copilot in christianhelle/refitter#904 * Migrate from Microsoft.OpenApi.Readers 1.x to Microsoft.OpenApi 3.x by @vgmello in christianhelle/refitter#907 * Improve Smoke Tests execution time by @christianhelle in christianhelle/refitter#915 * Fix #580: Nullable strings marked correctly by @christianhelle in christianhelle/refitter#921 * Fix #672: MultipleInterfaces ByTag method naming scoped per-interface by @christianhelle in christianhelle/refitter#922 * Fix #635: Refactor source generator to use context.AddSource() by @christianhelle in christianhelle/refitter#923 * Add custom format mappings configuration by @christianhelle in christianhelle/refitter#927 * Fix multipart form-data parameter extraction by @christianhelle in christianhelle/refitter#928 christianhelle/refitter#911 christianhelle/refitter#902 * Fix smoke tests: --interface-only variant missing using directive for contract types by @Copilot in christianhelle/refitter#933 * Fix SonarCloud Code Quality Issues by @Copilot in christianhelle/refitter#932 * Add debug logging for source generator when searching for .refitter files by @codymullins in christianhelle/refitter#743 * Fix: Base type not generated for types using oneOf with discriminator by @Copilot in christianhelle/refitter#906 * Add option for Method Level Authorization header attribute by @Roflincopter in christianhelle/refitter#897 * Fix PR #897 review feedback and add comprehensive bearer auth tests by @christianhelle in christianhelle/refitter#936 * Fix numeric suffix added to interface method names in ByTag mode by @Copilot in christianhelle/refitter#914 * Improve OpenAPI parse + codegen throughput by removing avoidable allocations and repeated regex work by @Copilot in christianhelle/refitter#937 * Move [JsonConverter] from enum properties to enum types by @christianhelle in christianhelle/refitter#938 * Fix broken CLI tool help text by @christianhelle in christianhelle/refitter#940 * Improve code coverage to >90% by @christianhelle in christianhelle/refitter#941 * Microsoft.OpenApi v3.4 by @christianhelle in christianhelle/refitter#945 * Add Unicode support for XML doc comment generation by @christianhelle in christianhelle/refitter#948 * Add PropertyNamingPolicy support for JSON property naming by @christianhelle in christianhelle/refitter#969 christianhelle/refitter#966 * Fix recursive schema stack overflows by @christianhelle in christianhelle/refitter#971 * Made Header Parameters for Security Schemes safe to use as C# variable name by @smoerijf in christianhelle/refitter#977 * Enhance schema alias handling and property name generation by @christianhelle in christianhelle/refitter#996 * Verify alias handling and sanitize PascalCase properties by @christianhelle in christianhelle/refitter#997 * Harden .refitter settings deserialization and output path handling by @christianhelle in christianhelle/refitter#1000 * Special-case the default .refitter filename before deriving OutputFilename by @coderabbitai[bot] in christianhelle/refitter#1002 * [v2.0 audit] Fix pre-release regressions from #1057 by @christianhelle in christianhelle/refitter#1064 * Resolve high-severity audit findings from #1057 by @christianhelle in christianhelle/refitter#1067 * [v2.0 audit] Close remaining verified #1057 regressions by @christianhelle in christianhelle/refitter#1070 * Harden generation flows by @christianhelle in christianhelle/refitter#1071 * Breaking changes and Migration Guide for v2.0.0 by @christianhelle in christianhelle/refitter#1009 * Handle equivalent duplicate schemas in multi-spec merge by @christianhelle in christianhelle/refitter#1076 * Tighten warning handling across Refitter builds by @christianhelle in christianhelle/refitter#1079 * Fix default solution path and update .NET version in VS Code config by @christianhelle in christianhelle/refitter#1082 * Fix docker smoke tests by @christianhelle in christianhelle/refitter#1081 * Sanitize malformed schema type names without renaming clean schemas by @christianhelle in christianhelle/refitter#1085 ... (truncated) Commits viewable in [compare view](christianhelle/refitter@1.7.3...2.0.0). </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 show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore <dependency name> major version` will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself) - `@dependabot ignore <dependency name> minor version` will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself) - `@dependabot ignore <dependency name>` will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself) - `@dependabot unignore <dependency name>` will remove all of the ignore conditions of the specified dependency - `@dependabot unignore <dependency name> <ignore condition>` will remove the ignore condition of the specified dependency and ignore conditions </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>



Fixes #580: Nullable strings not being marked correctly
Problem:
Strings marked
nullable: truein OpenAPI specifications were generating asstringinstead of
string?even when using the--nullable-reference-typesflag.Root Cause:
NSwag requires BOTH settings enabled:
The code was enabling the first but not automatically coupling it with the second.
Solution:
Auto-enable
GenerateOptionalPropertiesAsNullablewhenGenerateNullableReferenceTypesis enabled in CSharpClientGeneratorFactory.Create().
Changes:
Validation:
✅ Nullable strings now generate as
string?✅ Nullable doubles still work:
double?✅ Generated code compiles
✅ Build succeeds
Summary by CodeRabbit