required string? incorrectly loses nullable and gains MinLength=1

[inghamc]

Fixes #1918.

Root cause

Commit ec1f9c3 correctly wired up RequiredMemberAttribute (C# 11 required) to add properties to the required array in the schema. The mistake was folding it into the existing hasRequiredAttribute boolean, which is also used in both reflection services to suppress nullability and in JsonSchemaGenerator.cs:1216 to add MinLength=1 to strings.

For required string?, propertyTypeDescription.IsNullable is true but hasRequiredAttribute is now also true (due to RequiredMemberAttribute), so isNullable becomes false. nullable:true is dropped and MinLength=1 is added. Both are wrong.

The semantic distinction is:

• [Required] / [JsonRequired]: "this value must be non-null and non-empty" -- suppressing nullability and adding MinLength=1 are correct
• C# 11 required keyword: "this property must be present in object initializers / JSON deserialization, but the value can still be null" -- should only add to the required array; nullability and MinLength come from the type alone

Fix

Introduce hasSemanticRequiredAttribute (excludes RequiredMemberAttribute) for the isNullable and MinLength decisions in both reflection services.

C# property	required array	nullable	minLength
string name	no	no	-
string? name	no	yes	-
[Required] string name	yes	no	1
[Required] string? name	yes	no	1
required string name	yes	no	-
required string? name	yes	yes	-
[JsonRequired] string? name	yes	no	-

The Newtonsoft path has no hasJsonRequiredAttribute concept, so it only needs the requiredAttribute == null guard. The System.Text.Json path keeps [JsonRequired] in the semantic bucket since [JsonRequired] explicitly states the JSON value must not be null, unlike the language-level required keyword.

Tests

Regression tests added to AttributeGenerationTests (Newtonsoft) and SystemTextJsonTests (System.Text.Json) asserting that required string? lands in RequiredProperties, has IsNullable = true, and has MinLength = null.

---

Follow-up (added by @RicoSuter)

Extended the fix to also treat [JsonRequired] as a presence-only marker on the System.Text.Json path, for symmetry with the C# required keyword and to match STJ's actual runtime semantics ([JsonRequired] enforces presence during deserialization, not value non-nullness).

This means the hasSemanticRequiredAttribute concept collapses — [Required] (DataAnnotations) is now the single signal that suppresses nullability and triggers MinLength = 1 on strings, across both STJ and Newtonsoft paths.

Updated truth table:

C# property	required array	nullable	minLength
string name	no	no	—
string? name	no	yes	—
[Required] string name	yes	no	1
[Required] string? name	yes	no	1
required string name	yes	no	—
required string? name	yes	yes	—
[JsonRequired] string name	yes	no	—
[JsonRequired] string? name	yes	yes	—

Behavior changes vs v11.6.0

1. required string? and [JsonRequired] string? are now correctly nullable: true with no MinLength. Also fixes the regression reported in NSwag#5359.
2. required string and [JsonRequired] string (non-nullable types) no longer get MinLength = 1. Neither attribute says anything about string emptiness; users who want that should use [Required] or [MinLength(1)] explicitly.

Pre-v11.6.0 nullability semantics are restored, and the required keyword / [JsonRequired] → required-array feature added in #1908 is preserved.

Added one regression test for [JsonRequired] string? in SystemTextJsonTests.

[RicoSuter]

Wondering whether this is also then correct in the context of OpenAPI?
Tests are failing, can you have a look?

Ref: #1908

[inghamc]

Wondering whether this is also then correct in the context of OpenAPI? Tests are failing, can you have a look?

Ref: #1908

Sorry, I'm not sure why there's a timeout for NJsonSchema.Yaml.Tests (.NETFramework,Version=v4.7.2) on Windows, but not Ubuntu, and on none of the other versions:

[xUnit.net 00:01:36.68] NJsonSchema.Tests: Catastrophic failure: System.InvalidOperationException: Test process did not respond within 60 seconds

The fix also holds in the context of OpenAPI, because in both JSON Schema and OpenAPI required and nullability are orthogonal concepts:

• required array: the property key must be present in the JSON object
• nullable: the value at that key may be null (OpenAPI 3.0: nullable: true; OpenAPI 3.1 / JSON Schema: type: ["string","null"])

The C# required keyword maps cleanly to the first concept only: it's a presence constraint, not a value constraint. So required string? Name should produce a property that is in the required array and marked nullable in both JSON Schema and OpenAPI output.

The one place that's worth scrutinizing is [JsonRequired] being kept in the "semantic" bucket (suppressing nullability). [JsonRequired] means "the property key must be present during STJ deserialization", and doesn't formally say the value can't be null. But that's pre-existing behavior unchanged by this PR; the PR only changes where RequiredMemberAttribute (C# required keyword) sits.

[RicoSuter]

@inghamc @sychare @killergege @JohnGalt1717 @LeeLenaleee
please check the following tables, is this ok? if all is fine ill merge this and release new version of NJS/NSwag ASAP.

Full behavior comparison

Tuple notation: (in required array / IsNullable / MinLength). All rows assume #nullable enable.

System.Text.Json path

C# declaration	Pre-11.6.0	v11.6.0 (broken)	This PR
string Name	no / no / —	no / no / —	no / no / —
string? Name	no / yes / —	no / yes / —	no / yes / —
[Required] string Name	yes / no / 1	yes / no / 1	yes / no / 1
[Required] string? Name	yes / no / 1	yes / no / 1	yes / no / 1
required string Name	no / no / —	yes / no / 1	yes / no / —
required string? Name	no / yes / —	yes / no / 1	yes / yes / —
[JsonRequired] string Name	no / no / —	yes / no / 1	yes / no / —
[JsonRequired] string? Name	no / yes / —	yes / no / 1	yes / yes / —
[DataMember(IsRequired=true)] string? Name	yes / yes / —	yes / yes / —	yes / yes / —

Newtonsoft.Json path

C# declaration	Pre-11.6.0	v11.6.0 (broken)	This PR
string Name	no / no / —	no / no / —	no / no / —
string? Name	no / yes / —	no / yes / —	no / yes / —
[Required] string Name	yes / no / 1	yes / no / 1	yes / no / 1
[Required] string? Name	yes / no / 1	yes / no / 1	yes / no / 1
required string Name	no / no / —	yes / no / 1	yes / no / —
required string? Name	no / yes / —	yes / no / 1	yes / yes / —
[JsonProperty(Required=Always)] string Name	yes / no / —	yes / no / —	yes / no / —
[JsonProperty(Required=Always)] string? Name	yes / no / —	yes / no / —	yes / no / —
[JsonProperty(Required=AllowNull)] string Name	yes / no / —	yes / no / —	yes / no / —
[JsonProperty(Required=AllowNull)] string? Name	yes / yes / —	yes / yes / —	yes / yes / —
[JsonProperty(Required=DisallowNull)] string? Name	no / no / —	no / no / —	no / no / —
[DataMember(IsRequired=true)] string? Name	yes / yes / —	yes / yes / —	yes / yes / —

Newtonsoft has no [JsonRequired] concept, so that row is STJ-only. Newtonsoft's Required.Always/DisallowNull correctly suppress nullability because the Newtonsoft runtime rejects null in those modes — unchanged across all three versions.

NSwag-generated TypeScript client

Template: (IsNullable ? " | null" : "") + (!IsRequired ? " | undefined" : "") (TypeScriptParameterModel.cs:47).

C# source property	NSwag + pre-11.6.0	NSwag + v11.6.0	NSwag + this PR
string Name	name: string	name: string	name: string
string? Name	name: string | null | undefined	name: string | null | undefined	name: string | null | undefined
[Required] string Name	name: string	name: string	name: string
[Required] string? Name	name: string	name: string	name: string
required string Name	name: string | undefined	name: string	name: string
required string? Name	name: string | null | undefined	name: string ❌	name: string | null ✅
[JsonRequired] string Name	name: string | undefined	name: string	name: string
[JsonRequired] string? Name	name: string | null | undefined	name: string ❌	name: string | null ✅

The string | null output (required + nullable) is exactly what NSwag#5359 asked for.

NSwag-generated C# client (typed DTOs)

NSwag's CSharp generator does not emit the required keyword; it uses [Required] and nullability separately.

C# source property	NSwag + pre-11.6.0	NSwag + v11.6.0	NSwag + this PR
string Name	public string Name	public string Name	public string Name
string? Name	public string? Name	public string? Name	public string? Name
[Required] string Name	[Required] public string Name (+ MinLength)	same	same
[Required] string? Name	[Required] public string Name (+ MinLength)	same	same
required string Name	public string Name (not in required array)	[Required] public string Name (+ MinLength)	[Required] public string Name
required string? Name	public string? Name (not in required array)	[Required] public string Name ❌	[Required] public string? Name ✅
[JsonRequired] string? Name	public string? Name	[Required] public string Name ❌	[Required] public string? Name ✅

Exact emitted attributes depend on NSwag settings, but the nullability and required-array membership drive the shape.

Summary of deltas

vs pre-11.6.0 — restores nullability handling, preserves the #1908 feature (required keyword / [JsonRequired] add to required array).

vs v11.6.0 — two intentional behavior changes, both restoring correctness:

1. required T? / [JsonRequired] T? — regain their nullable: true and lose the spurious MinLength=1.
2. required string / [JsonRequired] string — lose the spurious MinLength=1. Use [Required] or [MinLength(1)] if non-empty validation is the intent.

[Required], [JsonProperty(Required=*)], [DataMember(IsRequired=true)] semantics unchanged across all three versions.

Swagger 2.0 addendum

Swagger 2.0 mode shares the same schema-model code path, so the bug applied there too. Swagger 2.0 expresses nullability as the NJsonSchema-specific x-nullable: true extension (JsonSchema.cs:500-502), and has a path-specific quirk in JsonSchemaGenerator.AddProperty:1225 — non-nullable properties are auto-added to the required array (since Swagger 2.0 has no other way to express "must not be null").

Swagger 2.0 serialized output — (in required array / x-nullable / minLength):

C# declaration	Pre-11.6.0	v11.6.0 (broken)	This PR
string Name	yes / — / —	yes / — / —	yes / — / —
string? Name	no / true / —	no / true / —	no / true / —
[Required] string Name	yes / — / 1	yes / — / 1	yes / — / 1
[Required] string? Name	yes / — / 1	yes / — / 1	yes / — / 1
required string Name	yes (auto) / — / —	yes / — / 1	yes / — / —
required string? Name	no / true / —	yes / — / 1 ❌	yes / true / — ✅
[JsonRequired] string Name (STJ)	yes (auto) / — / —	yes / — / 1	yes / — / —
[JsonRequired] string? Name (STJ)	no / true / —	yes / — / 1 ❌	yes / true / — ✅

NSwag reads the required array + x-nullable to decide TypeScript nullability; required string? Name now correctly renders as name: string | null instead of the v11.6.0 name: string.

v11.6.0 broke Swagger 2.0 the same way as JSON Schema / OpenAPI 3.x, and this PR fixes it via the same schema-model correction. Pre-11.6.0 behavior is not fully restored for Swagger 2.0: required / [JsonRequired] now adds to the required array (the intended #1908 feature) — preserved.

[JohnGalt1717]

1. Required doesn't denote a length. Just that it's present with a valid value. So [Required] string Name should be yes/no/0
2. Same here: [Required] string? Name except yes/yes/0
3. Here here: required string Name yes/no/0, and required string? Name yes/yes/0

etc.

required string of any type is not nullable, must be present, and "" is valid as is any other value of string.
required string? of any type is nullable, must be present, and null, "", and any other string value is valid.

The same is true with the [Required] attribute.

Everything else looks ok to me.

[sychare]

I believe that matches what I'd be expecting for the generated TS client, and what specifically broke for us here.

[RicoSuter]

Thanks for the feedback @JohnGalt1717.

The MinLength=1 behavior only applies to [Required] (DataAnnotations) — not to the C# required keyword or [JsonRequired]. Both of those correctly produce no MinLength after this PR:

Scenario	MinLength after this PR
required string / required string?	none
[JsonRequired] string / [JsonRequired] string?	none
[JsonProperty(Required=Always)] string	none
[DataMember(IsRequired=true)] string	none
[Required] string	1
[Required(AllowEmptyStrings = true)] string	none (explicit opt-out)

For [Required], MinLength=1 is the long-standing (and I'd argue, correct) behavior — it faithfully reflects DataAnnotations' own runtime semantics: [Required] rejects "" unless AllowEmptyStrings=true is set (docs). The schema matches what the server will actually validate.

This has been NJsonSchema's behavior since well before v11.6.0 — there's a dedicated test (When_RequiredAttribute_is_set_with_AllowEmptyStrings_false_then_minLength_and_required_are_set) asserting exactly this. Not affected by #1908, not changed by this PR.

Fine to merge and release?

[JohnGalt1717]

One would think that would be StringLength(1) on top of required, but ok.