Add NameValueCollection serialization support

[AArnott]

NameValueCollection currently produces a recursive generated shape whose converter never completes, preventing containing objects from being serialized.

Add a built-in converter that represents collection entries as nil, a single string, or an array of strings. The converter captures the configured string converter through ConverterContext, preserving string interning and reference behavior. The generated primitive converter lookup now supports context-dependent converters without caching them globally.

Regression coverage verifies roundtripping, the exact MessagePack token layout, and string interning.

Fixes #1053

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.42%. Comparing base (62ac7ec) to head (563a636).

Additional details and impacted files

@@            Coverage Diff             @@
##             v1.3    #1054      +/-   ##
==========================================
+ Coverage   76.30%   76.42%   +0.12%     
==========================================
  Files         175      175              
  Lines       13382    13457      +75     
  Branches     2709     2722      +13     
==========================================
+ Hits        10211    10285      +74     
  Misses       2293     2293              
- Partials      878      879       +1     

Flag	Coverage Δ
Linux	75.42% <100.00%> (+0.14%)	⬆️
Windows	76.08% <100.00%> (+0.12%)	⬆️
macOS	75.37% <100.00%> (+0.15%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

🟡 Changes recommended

The new NameValueCollectionConverter currently performs DepthStep() for nested arrays but does not pass the stepped context into the nested string reads/writes, so depth checking is unintentionally bypassed in that path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/Nerdbank.MessagePack/Converters/BclConverters.cs:175

• valuesContext.DepthStep() has no effect because the nested string writes still use the unmodified context, so depth checking is skipped for the array-of-strings case. Use valuesContext when writing the array items so the depth step applies to nested serialization.

			writer.WriteArrayHeader(values.Length);
			foreach (string? item in values)
			{
				this.stringConverter.Write(ref writer, item, context);
			}

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new NameValueCollectionConverter computes a scoped valuesContext but doesn’t propagate it to nested element reads/writes, and the template’s dynamic-code gating likely prevents the fix from applying under NativeAOT.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/Nerdbank.MessagePack/Converters/BclConverters.cs:137

• valuesContext is created and DepthStep() is applied for the nested array scope, but the element reads still use context. This defeats the purpose of the scoped/decremented context and is inconsistent with how other converters propagate depth-limited contexts into nested reads.

This issue also appears on line 169 of the same file.

			SerializationContext valuesContext = context;
			valuesContext.DepthStep();
			int valueCount = reader.ReadArrayHeader();
			for (int j = 0; j < valueCount; j++)
			{
				result.Add(key, this.stringConverter.Read(ref reader, valuesContext));
			}

src/Nerdbank.MessagePack/Converters/BclConverters.cs:175

• valuesContext is created and DepthStep() is applied for the nested array scope, but the element writes still use context. This makes the scoped context ineffective and diverges from the usual pattern where the decremented context is passed to nested writes.

			SerializationContext valuesContext = context;
			valuesContext.DepthStep();
			writer.WriteArrayHeader(values.Length);
			foreach (string? item in values)
			{
				this.stringConverter.Write(ref writer, item, valuesContext);
			}

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There are confirmed functional issues in the new converter (empty-array decode dropping keys and schema mismatch) and a dynamic-code gate that can prevent the built-in converter from being selected in some runtimes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

src/Nerdbank.MessagePack/Converters/BclConverters.cs:192

• The converter can write nil elements inside the string array (e.g., when GetValues(i) returns multiple entries including null), but the emitted JSON schema declares array items as only type "string". This makes schema validation inaccurate for some valid serialized values.

					new JsonObject
					{
						["type"] = "array",
						["items"] = new JsonObject { ["type"] = "string" },
					}),

src/Nerdbank.MessagePack/Converters/PrimitiveConverterLookup.cs:647

• The NameValueCollection built-in converter is gated behind RuntimeFeature.IsDynamicCodeSupported, which will cause the lookup to fail when dynamic code is disabled (e.g., NativeAOT). That can reintroduce the original failure mode (falling back to generated shapes) in those environments.

		if (RuntimeFeature.IsDynamicCodeSupported && primitiveTypeName == "NameValueCollection" && (primitiveTypeNamespace ??= typeof(T).Namespace) == "System.Collections.Specialized")

src/Nerdbank.MessagePack/Converters/PrimitiveConverterLookup.tt:46

• Setting RequiresDynamicCode: true for NameValueCollection causes the generated lookup to include a RuntimeFeature.IsDynamicCodeSupported gate, which prevents the converter from being selected when dynamic code is disabled. If the goal is to support NameValueCollection serialization broadly (including NativeAOT), drop this flag so the converter can still be resolved.

	// NativeAOT shares generic code for reference types, so a static reference here would root this type's globalization-heavy implementation even when unused.
	new ConverterInfo("System.Collections.Specialized.NameValueCollection", "NameValueCollectionConverter", IsRefType: true, RequiresContext: true, AutoDiscoverOnlyWithDynamicCode: true) { LazyLoad = true },

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new NameValueCollection converter’s JSON schema does not accurately allow null elements inside multi-value arrays that the converter can emit.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Nerdbank.MessagePack/Converters/BclConverters.cs:192

• The converter can serialize a multi-valued entry that includes null (e.g., values like ["a", null]) as an array containing a nil element, but the JSON schema currently restricts array items to only { "type": "string" }. This makes the schema inaccurate for valid serialized payloads produced by this converter.

			["type"] = "object",
			["additionalProperties"] = new JsonObject
			{
				["anyOf"] = new JsonArray(
					new JsonObject { ["type"] = "null" },

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new NameValueCollection converter’s JSON schema does not currently reflect that array values can legally contain nil elements when the collection has multiple values including nulls.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Nerdbank.MessagePack/Converters/BclConverters.cs:198

• NameValueCollectionConverter can emit arrays that include nil items (e.g., when a key has multiple values and one of them is null), because it delegates each element to StringConverter.Write, which writes nil for null. The generated JSON schema currently restricts array items to { "type": "string" }, which would incorrectly reject valid payloads produced by this converter. Update the schema to allow null items within the array.

					new JsonObject
					{
						["type"] = "array",
						["items"] = new JsonObject { ["type"] = "string" },
					}),

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The current implementation/documentation still appears inconsistent with the stated goal of automatic built-in discovery (without explicit opt-in), which risks leaving the original failure mode unresolved by default.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The changes appear correct and well-covered by regression tests, with only minor nullable-annotations polish suggested.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

test/Nerdbank.MessagePack.Tests/BuiltInConverterTests.cs:657

• HasNameValueCollection.Values is declared non-nullable, but the tests intentionally roundtrip a null value (and use null! to silence the compiler). Making the property nullable better matches the exercised behavior and avoids needing null-forgiving operators in the tests.

		public NameValueCollection Values { get; set; } = new();

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The NameValueCollection converter’s JSON schema does not accurately describe the values the converter can emit (array items can be nil).

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Nerdbank.MessagePack/Converters/BclConverters.cs:198

• The JSON schema for NameValueCollection says array entries are always strings, but the converter can emit nil array elements (Write iterates foreach (string? item in values) and StringConverter writes nil for null). This makes the generated schema inaccurate for valid NameValueCollection instances that contain null among multiple values for a key.

					new JsonObject
					{
						["type"] = "array",
						["items"] = new JsonObject { ["type"] = "string" },
					}),

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The emitted/accepted wire format can include nil elements inside value arrays, but GetJsonSchema currently models array items as non-null strings only.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Nerdbank.MessagePack/Converters/BclConverters.cs:198

• GetJsonSchema models the array case as items: { type: "string" }, but the converter can emit (and accept) nil elements when NameValueCollection has multiple values and some are null (since values is string?[] and each element is written via stringConverter.Write). This makes the reported schema stricter than the actual wire format. Update the schema to allow null items (or alternatively normalize away null elements during serialization).

					new JsonObject
					{
						["type"] = "array",
						["items"] = new JsonObject { ["type"] = "string" },
					}),

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new NameValueCollection JSON schema and optional-converter registration have a couple of correctness/robustness gaps that should be addressed before merge.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

src/Nerdbank.MessagePack/Converters/BclConverters.cs:198

• The JSON schema says array values have string-only items, but the converter writes each array item via the configured string converter, which can emit msgpack nil for null entries (e.g., when NameValueCollection contains multiple values and some are null). This makes the schema inaccurate for valid outputs; update the schema to allow null items in the value arrays.

This issue also appears on line 199 of the same file.

					new JsonObject
					{
						["type"] = "array",
						["items"] = new JsonObject { ["type"] = "string" },
					}),

src/Nerdbank.MessagePack/OptionalConverters.cs:112

• Calling WithNameValueCollectionConverter multiple times will append duplicate NameValueCollectionConverterFactory instances because ConverterFactories is a plain ImmutableArray with no uniqueness enforcement. This adds redundant factory scans during converter creation and can unnecessarily root extra instances in NativeAOT; consider throwing an ArgumentException if the factory is already present (consistent with other With* methods that reject double-registration).

	public static MessagePackSerializer WithNameValueCollectionConverter(this MessagePackSerializer serializer)
	{
		Requires.NotNull(serializer, nameof(serializer));
		return serializer with
		{

src/Nerdbank.MessagePack/Converters/BclConverters.cs:201

• The schema description says values are "strings or string arrays", but the converter and schema also allow nil (null) values. Update the description to match the actual representation so consumers aren't misled.

			},
			["description"] = "A name/value collection represented as a map of strings or string arrays.",
		};

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The NameValueCollectionConverter JSON schema does not currently reflect that null elements can be emitted inside arrays, which can mislead schema consumers.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Nerdbank.MessagePack/Converters/BclConverters.cs:198

• The JSON schema allows array values only with items.type = "string", but Write can emit nil elements because it iterates foreach (string? item in values) and delegates to StringConverter.Write, which writes nil for null. This makes the reported schema inaccurate for cases where a NameValueCollection key has multiple values including null (e.g., Add("k", "v1"); Add("k", null)). Update the schema to allow null items in the array.

					new JsonObject
					{
						["type"] = "array",
						["items"] = new JsonObject { ["type"] = "string" },
					}),

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The implementation is consistent with the stated opt-in design and is backed by targeted regression tests (including NativeAOT and wire-format validation).

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite