[Repo Assist] feat: add WrapNullableStrings static parameter to OpenApiClientProvider by github-actions[bot] · Pull Request #357 · fsprojects/SwaggerProvider

github-actions · 2026-04-06T22:05:27Z

🤖 This PR was created by Repo Assist, an automated AI assistant.

Closes #122

Root Cause

DefinitionCompiler wraps non-required value types (e.g., bool option, int option) in Option<T>, but skips reference types entirely because they're already nullable by the CLR. This means nullable: true string fields come through as plain string even though the user might want the F# type system to reflect the optional/absent distinction.

Fix

Add a WrapNullableStrings static parameter (default false) to OpenApiClientProvider. When set to true, optional/nullable string fields are wrapped in Option<string>, consistent with how FSharp.Data.SqlClient maps varchar NULL → string option.

The default is false to preserve full backwards compatibility — existing users who read nullable strings as string are unaffected unless they opt in.

// opt-in: nullable strings become Option<string>
type Api = OpenApiClientProvider<"schema.yaml", WrapNullableStrings=true>

Changes

File	Change
`src/SwaggerProvider.DesignTime/v3/DefinitionCompiler.fs`	Add `?wrapNullableStrings: bool` optional parameter; apply `Option<string>` wrapping when enabled
`src/SwaggerProvider.DesignTime/Provider.OpenApiClient.fs`	Add `WrapNullableStrings` static parameter, thread it through to `DefinitionCompiler`, include in cache key
`tests/SwaggerProvider.Tests/v3/Schema.TestHelpers.fs`	Add `compilePropertyTypeWith` helper that accepts `provideNullable` and `wrapNullableStrings`
`tests/SwaggerProvider.Tests/v3/Schema.TypeMappingTests.fs`	Add 5 tests covering WrapNullableStrings=false (default), =true with optional string, =true with required string, =true with date-time, and =true with integer

Test Status

✅ Build: succeeded (0 errors, warnings are pre-existing)
✅ Unit tests: 268 passed, 0 failed, 2 skipped (same as baseline)
✅ New WrapNullableStrings tests: all 5 pass
✅ Format check: fantomas applied and re-verified clean

Integration/provider tests require a running Swashbuckle test server and were not run; the new parameter does not touch HTTP or serialization logic.

Generated by 🌈 Repo Assist, see workflow run. Learn more.

To install this agentic workflow, run
gh aw add githubnext/agentics/workflows/repo-assist.md@7ee2b60744abf71b985bead4599640f165edcd93

…er (closes #122) When WrapNullableStrings=true, optional/nullable string fields are wrapped as Option<string> instead of plain string, matching the treatment of other optional value types (bool, int, float, etc.). Defaults to false to preserve backwards compatibility. - Add WrapNullableStrings static parameter to DefinitionCompiler (optional arg) - Pass parameter through Provider.OpenApiClient.fs to DefinitionCompiler - Include WrapNullableStrings in the type-generation cache key - Add compilePropertyTypeWith helper in test helpers - Add 5 tests covering WrapNullableStrings=false/true/required scenarios Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

sergey-tihon · 2026-04-13T04:52:31Z

@copilot merge develop into this branch

…ue-122-wrap-nullable-strings-362a357b9d702fbf # Conflicts: # tests/SwaggerProvider.Tests/v3/Schema.TestHelpers.fs # tests/SwaggerProvider.Tests/v3/Schema.TypeMappingTests.fs Co-authored-by: sergey-tihon <1197905+sergey-tihon@users.noreply.github.com>

Copilot · 2026-04-13T04:57:10Z

@copilot merge develop into this branch

There's no develop branch in this repo — master is the main branch. I've merged master into this branch in commit 309417a, resolving conflicts in Schema.TestHelpers.fs and Schema.TypeMappingTests.fs. All 333 unit tests pass.

sergey-tihon · 2026-04-13T04:59:35Z

/repo-assist i don't want add WrapNullableStrings static parameters. i want to have the beharior of WrapNullableStrings=true by defaiult

github-actions · 2026-04-13T05:00:18Z

✗ Repo Assist encountered failed, see workflow run.

sergey-tihon · 2026-04-13T07:29:37Z

@copilot i don't want add WrapNullableStrings static parameters. i want to have the beharior of WrapNullableStrings=true by defaiult

…avior (remove WrapNullableStrings param) Agent-Logs-Url: https://github.com/fsprojects/SwaggerProvider/sessions/345881f5-ff34-40a2-aeba-703b53cb2dbc Co-authored-by: sergey-tihon <1197905+sergey-tihon@users.noreply.github.com>

Copilot · 2026-04-13T07:37:00Z

@copilot i don't want add WrapNullableStrings static parameters. i want to have the beharior of WrapNullableStrings=true by defaiult

Done in commit ae9ecdd. The WrapNullableStrings static parameter has been removed entirely. Optional string fields are now always wrapped in Option<string> by default — no opt-in needed. All 328 unit tests pass.

Copilot

Pull request overview

This PR aims to address #122 by allowing nullable/optional OpenAPI string properties to be represented as Option<string> in generated client types, aligning string behavior with existing optional value-type wrapping.

Changes:

Updates v3 DefinitionCompiler to wrap non-required string types in Option<string>.
Updates v3 mapping tests to expect Option<string> for non-required strings (including in PreferNullable mode).
Updates the v2-compat compilation test (via v3 pipeline) to expect Option<string> for an optional string property.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
`src/SwaggerProvider.DesignTime/v3/DefinitionCompiler.fs`	Adds unconditional wrapping of non-required `string` into `Option<string>`.
`src/SwaggerProvider.DesignTime/Provider.OpenApiClient.fs`	No functional change shown in diff; still constructs `DefinitionCompiler` without a `WrapNullableStrings` toggle.
`tests/SwaggerProvider.Tests/v3/Schema.TypeMappingTests.fs`	Changes expectations so optional strings map to `Option<string>`, including under `PreferNullable`.
`tests/SwaggerProvider.Tests/v3/Schema.V2SchemaCompilationTests.fs`	Changes v2-compat expectation so an optional string property becomes `string option`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-13T07:53:56Z

src/SwaggerProvider.DesignTime/v3/DefinitionCompiler.fs

+        else if tyType = typeof<string> then
+            ProvidedTypeBuilder.MakeGenericType(typedefof<Option<obj>>, [ tyType ])


The new else if tyType = typeof<string> branch wraps all non-required plain strings in Option<string> unconditionally. This is a breaking change vs the prior behavior, and it also doesn't match the PR description/title which says this should be controlled by an opt-in WrapNullableStrings static parameter (default false). Gate this wrapping behind a wrapNullableStrings flag (threaded from the type provider) so the default remains string unless explicitly enabled.

Suggested change

else if tyType = typeof<string> then

ProvidedTypeBuilder.MakeGenericType(typedefof<Option<obj>>, [ tyType ])

Copilot · 2026-04-13T07:53:56Z

src/SwaggerProvider.DesignTime/Provider.OpenApiClient.fs

                        let useDateOnly = cfg.SystemRuntimeAssemblyVersion.Major >= 6
+
                        let defCompiler = DefinitionCompiler(schema, preferNullable, useDateOnly)



DefinitionCompiler is still constructed with only (schema, preferNullable, useDateOnly), and there is no threading of the new WrapNullableStrings setting described in the PR. If the intent is an opt-in behavior, add WrapNullableStrings as a static parameter, include it in the cache key, and pass it through here to DefinitionCompiler so default behavior stays backward-compatible.

Copilot · 2026-04-13T07:53:57Z

tests/SwaggerProvider.Tests/v3/Schema.TypeMappingTests.fs

+// ── Optional string is wrapped in Option<string> ─────────────────────────────

 [<Fact>]
-let ``optional string is not wrapped in Option``() =
+let ``optional string maps to Option<string>``() =
    let ty = compilePropertyType "          type: string\n" false

-    // string is a reference type — not wrapped in Option<T> even when non-required
-    ty |> shouldEqual typeof<string>
+    // plain string fields are wrapped in Option<string> when non-required
+    ty
+    |> shouldEqual(typedefof<Option<_>>.MakeGenericType(typeof<string>))


This test now asserts that a non-required plain string maps to Option<string> by default. Per the PR description, WrapNullableStrings is supposed to be opt-in with default false; if so, the default expectation should remain string, and there should be a separate test that enables WrapNullableStrings=true (or uses a helper that passes the flag into DefinitionCompiler) and then expects Option<string>.

Copilot · 2026-04-13T07:53:57Z

tests/SwaggerProvider.Tests/v3/Schema.TypeMappingTests.fs

 [<Fact>]
-let ``PreferNullable: optional string is not wrapped (reference type)``() =
-    // Reference types like string are not wrapped in Nullable<T> since they are
-    // already nullable by nature — same behaviour as Option mode.
+let ``PreferNullable: optional string maps to Option<string>``() =
+    // Strings are always wrapped in Option<string> when non-required, even with PreferNullable.
    let ty = compilePropertyTypeWith true "          type: string\n" false
-    ty |> shouldEqual typeof<string>
+    ty |> shouldEqual typeof<string option>


PreferNullable tests are now asserting Option<string> for optional strings. If WrapNullableStrings is intended to be opt-in, this should be conditional on that flag as well (and the test should set the flag explicitly). Otherwise this changes the semantics of PreferNullable mode for reference types without any configuration toggle.

Copilot · 2026-04-13T07:53:57Z

tests/SwaggerProvider.Tests/v3/Schema.V2SchemaCompilationTests.fs

    nameProp.PropertyType |> shouldEqual typeof<string>
-    // optional string — string is a reference type, not wrapped in Option
-    tagProp.PropertyType |> shouldEqual typeof<string>
+    // optional string — now wrapped in Option<string>
+    tagProp.PropertyType |> shouldEqual typeof<string option>


This v2-compat test now expects an optional string property (tag) to be string option, which makes the behavior change unconditional for Swagger 2.0 documents compiled through the v3 pipeline. If the goal is to keep backward compatibility and require an explicit WrapNullableStrings=true opt-in, update this test to keep the default as string and add a separate opt-in test case.

sergey-tihon · 2026-04-13T07:55:50Z

@copilot why don't you use Option.ofString if runtime to avoid Some(null) ?

github-actions bot added automation repo-assist labels Apr 6, 2026

ci: trigger checks

c34cd13

github-actions bot mentioned this pull request Apr 8, 2026

[Repo Assist] Monthly Activity 2026-04 #352

Open

9 tasks

Copilot started work on behalf of sergey-tihon April 13, 2026 04:52 View session

Copilot finished work on behalf of sergey-tihon April 13, 2026 04:57

Copilot AI requested a review from sergey-tihon April 13, 2026 04:57

Copilot started work on behalf of sergey-tihon April 13, 2026 07:29 View session

Copilot finished work on behalf of sergey-tihon April 13, 2026 07:38

sergey-tihon marked this pull request as ready for review April 13, 2026 07:49

Copilot AI review requested due to automatic review settings April 13, 2026 07:49

Copilot started reviewing on behalf of sergey-tihon April 13, 2026 07:50 View session

Copilot AI reviewed Apr 13, 2026

View reviewed changes

Copilot started work on behalf of sergey-tihon April 13, 2026 07:56 View session

		else if tyType = typeof<string> then
		ProvidedTypeBuilder.MakeGenericType(typedefof<Option<obj>>, [ tyType ])

		let useDateOnly = cfg.SystemRuntimeAssemblyVersion.Major >= 6

		let defCompiler = DefinitionCompiler(schema, preferNullable, useDateOnly)

Conversation

github-actions bot commented Apr 6, 2026

Root Cause

Fix

Changes

Test Status

Uh oh!

sergey-tihon commented Apr 13, 2026

Uh oh!

Copilot AI commented Apr 13, 2026

Uh oh!

sergey-tihon commented Apr 13, 2026

Uh oh!

github-actions bot commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergey-tihon commented Apr 13, 2026

Uh oh!

Copilot AI commented Apr 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

sergey-tihon commented Apr 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Apr 13, 2026 •

edited

Loading