Option to disallow duplicate JSON properties

[PranavSenthilnathan]

Adds a new AllowsDuplicateProperties option which has the following behavior when set to false:

• POCOs throw on duplicate properties always (honor case-insensitive, naming policy, etc.)
• Dictionaries throw when TryAdd returns false. For Dictionary<string, TKey> we always use the default comparer (ordinal case-sensitive)
• JsonObject - Throws during deserialization when duplicates are found. We also throw without the option, but that happens lazily, only when the property is accessed.
• JsonDocument - dedups case-sensitively
• Extension properties - behavior depends on backing type but it always matches one of the bullet points above
• Deserialization - AllowDuplicateProperties does not apply

/cc @eiriktsarpalis @jozkee

Closes #108521

Outdated (2025/05/21):

Adds option to disallow duplicate JSON properties. There are some open questions:

• Are duplicate properties an error in the JSON payload itself, or only errors during deserialization? In other words, if AllowsDuplicateProperties=false, should we validate this in the Utf8JsonReader before giving it to converters or should the converters dedup themselves? This PR makes the change in the converters, but we might want to consider moving it down to the reader. Pros/cons for each:

  • Utf8Reader dedup - all converters get deduplication this for free. All JSON that has duplicate properties will be rejected.
  • Converter dedup - types like JsonObject will have dictionaries internally so they can do the dedup more efficiently. There are probably workarounds to improve perf though (e.g. opt out of the reader dedup behavior when TokenType.StartArray is seen)

• For JsonDocument deserialization, do we want to deserialize everything first and then dedup, or dedup while parsing? The former makes the duplicate tracking a little easier since the number of properties for an object would be known. The latter allows failing fast.

• The dictionary converter is not yet implemented. It will also need to decide when to dedup as mentioned in the point above (note the dictionary keys do not have to be string, so we need an additional data structure to do the dedup).

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

[eiriktsarpalis]

should we validate this in the Utf8JsonReader before giving it to converters or should the converters dedup themselves?

Honestly, I don't believe this is possible with the current design of Utf8JsonReader. The reader would need to track arbitrarily many properties in all objects it is currently nested within. This would almost certainly necessitate allocating from the heap for sufficiently complex data. In turn though, this would break the checkpointing semantics that Utf8JsonReader currently enjoys and many converters depend on:

Utf8JsonReader checkpoint = reader;
var value = JsonSerializer.Deserialize<T>(ref reader);
reader = checkpoint; // resets the reader state

For this reason, I think we need to make it strictly a deserialization-level concern.

For JsonDocument deserialization, do we want to deserialize everything first and then dedup, or dedup while parsing?

I think we want to make this a fail-fast operation. Specifically for JsonDocument not doing so makes it easy to bypass validation altogether since you can always forward the underlying JSON using JsonElement.GetRawText().

It will also need to decide when to dedup as mentioned in the point above (note the dictionary keys do not have to be string, so we need an additional data structure to do the dedup).

I suspect you might need to implement this on case-by-case basis. For known mutable dictionary types you can rely on the result itself to track duplicates but immutable or frozen dictionaries (which IIRC have constructor methods with overwrite semantics) you probably need to enlist an auxiliary dictionary type to do so (it might help if you could make this a pooled object that can be used across serializations).

[krwq]

perhaps I missed but I think there might be couple of test cases which would be nice to see:

• recursive structure with duplicate properties in one of the inner, both positive and negative test cases
• two C# properties with same name but different casing
• are duplicates allowed inside of dictionary? regardless of the answer there should be a test case
• arrays - both duplicating entry with array and also item in the array with duplicated entries (might be also worth to add testcase that we don't false trigger)
• if there is per type setting on JsonTypeInfo I'd imagine this should be exercised (I think no but have only skimmed through PR)
• for arrays another interesting case is to create pre-populated list and add properties and see if it appends when it encounters for the first time - is that expected with this setting on? (I'd imagine if someone doesn't like the dups they might want to erase the content first - it's a bit user shooting themselves in the foot so not sure about doing anything special but might be worth defining expectations)

[PranavSenthilnathan]

Arrays are not affected by this change, only objects. Added the remaining tests. Duplicates in dictionaries raise errors but equality is determined by TryAdd (using an ordinal comparer where applicable).

[PranavSenthilnathan]

Current behavior when AllowsDuplicateProperties = false:

• POCOs throw on duplicate properties always (honor case-insensitive, naming policy, etc.)
• Dictionaries throw when TryAdd returns false. For Dictionary<string, TKey> we always use the default comparer (ordinal case-sensitive)
• JsonObject - already throws after deserialization when duplicates are found during evaluation of the lazy ordered dictionary
• JsonDocument - dedups case-sensitively
• Extension properties - behavior depends on type but it always matches one of the bullet points above
• deserialization - AllowDuplicateProperties does not apply at all

Note that JsonObject deserialization is still lazy. I believe there's value in keeping the lazy behavior when AllowDuplicateProperties is true because it avoids unnecessary allocations. This means that deserialization will not throw but accessing the property will. If we keep the lazy behavior for that case, I think it's best to be consistent and also keep the lazy behavior for AllowDuplicateProperties = false. There are likely cases where only a specific part of a JsonObject is evaluated and laziness improves perf for these cases.

Follow up items (in future PRs):

• Add ValueBitArray
• Use HashSet in PropertyNameSet

[eiriktsarpalis]

Note that JsonObject deserialization is still lazy. I believe there's value in keeping the lazy behavior when AllowDuplicateProperties is true because it avoids unnecessary allocations. This means that deserialization will not throw but accessing the property will. If we keep the lazy behavior for that case, I think it's best to be consistent and also keep the lazy behavior for AllowDuplicateProperties = false. There are likely cases where only a specific part of a JsonObject is evaluated and laziness improves perf for these cases.

Keep in mind that there are certain operations that bypass lazy initialization altogether. Crucially, it happens when you try to re-serialize a freshly created JsonObject instance:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.cs

Lines 135 to 139 in b04d40e

	if (dictionary is null && jsonElement.HasValue)
	{
	// Write the element without converting to nodes.
	jsonElement.Value.WriteTo(writer);
	}

Meaning that an object with duplicate properties could end up being forwarded while avoiding detection. I think we need to remove lazy initialization, for the new mode in the very least.

[PranavSenthilnathan]

I think we need to remove lazy initialization, for the new mode in the very least.

I changed it to do the validation on the JsonElement but keep the lazy evaluation which still allows us to avoid allocations. However, for case-insensitive I had to change it to eagerly evaluate all children nodes since we don't thread that option into JsonElement.Parse.

[eiriktsarpalis]

We should make sure this doesn't regress JsonDocument or JsonNode deserialization performance with the option turned off. You can use @EgorBot to set this up relatively quickly.

[PranavSenthilnathan]

We should make sure this doesn't regress JsonDocument or JsonNode deserialization performance with the option turned off. You can use @EgorBot to set this up relatively quickly.

Ran tests locally and all Node/Document diffs were < 5%. The ReadJson benchmarks had more variance but I confirmed those also don't have regressions after spot-checking outliers.

[eiriktsarpalis]

Thanks Pranav!