Fix DataContractJsonSerializer not passing ISerializationSurrogateProvider to internal serializer

[Copilot]

• Initial plan
• Fix DataContractJsonSerializer to pass ISerializationSurrogateProvider to internal serializer contexts
• Complete fix for DataContractJsonSerializer surrogate provider support
• Address PR review feedback:
  • Replace custom test classes with shared NonSerializablePerson/MyPersonSurrogateProvider from SerializationTypes.RuntimeOnly.cs
  • Use consistent single-line pattern for RootContract property matching DataContractSerializer
  • Remove unnecessary cache invalidation logic that sets _rootContract = null
  • Add *WasCalled properties to MyPersonSurrogateProvider for test validation

Summary

Successfully implemented and refined DataContractJsonSerializer surrogate provider support:

1. Core Fix: Added new constructors to XmlObjectSerializerReadContextComplexJson and XmlObjectSerializerWriteContextComplexJson that properly pass the ISerializationSurrogateProvider to base classes
2. Contract Resolution: Fixed RootContract property to call GetSurrogatedType when surrogate provider is present
3. Code Quality: Addressed all review feedback to improve consistency and reuse existing test infrastructure
4. Validation: All tests pass, confirming surrogate provider methods are properly called during JSON serialization/deserialization

The fix ensures DataContractJsonSerializer now properly respects SetSerializationSurrogateProvider() calls, matching the behavior of DataContractSerializer.

Fixes: #100553

[dotnet-policy-service]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[StephenMolloy]

@copilot I left some comments in review. Please update the PR with suggested changes.

[Copilot]

Pull Request Overview

This PR fixes a bug in DataContractJsonSerializer where the ISerializationSurrogateProvider was not being properly passed to the internal XML serializer contexts, preventing surrogate providers from functioning correctly during JSON serialization/deserialization.

Key changes:

• Added new constructors to JSON serializer context classes to accept and pass through the surrogate provider
• Fixed RootContract property to use surrogate type resolution when a provider is available
• Added comprehensive test coverage with tracking properties to verify surrogate provider method calls

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
SerializationTypes.RuntimeOnly.cs	Added tracking properties to MyPersonSurrogateProvider for test validation
DataContractJsonSerializer.cs (tests)	Added comprehensive test to verify surrogate provider integration
XmlObjectSerializerWriteContextComplex.cs	Added constructor overload accepting ISerializationSurrogateProvider
XmlObjectSerializerReadContextComplex.cs	Added constructor overload accepting ISerializationSurrogateProvider
XmlObjectSerializerWriteContextComplexJson.cs	Updated constructor call to pass surrogate provider to base class
XmlObjectSerializerReadContextComplexJson.cs	Updated constructor call to pass surrogate provider to base class
DataContractJsonSerializer.cs	Fixed RootContract property and added GetSurrogatedType helper method

[StephenMolloy]

lgtm

[stephentoub]

@StephenMolloy are you still working on this?

[StephenMolloy]

@StephenMolloy are you still working on this?

Yup. I was cleaning up the serialization area towards the end of 10 and ultimately decided that this didn't make the cut. But I've got a couple of copilot PR's to review when I get back into serialization for 11 - soon.

[github-actions]

🤖 Copilot Code Review — PR #118257

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: The PR fixes a real, verified bug: DataContractJsonSerializer accepted an ISerializationSurrogateProvider via its public SetSerializationSurrogateProvider() API, but never propagated it to the internal read/write contexts or the RootContract resolution — making the surrogate provider entirely non-functional for JSON serialization. The fix is well-justified.

Approach: The fix correctly mirrors how DataContractSerializer handles surrogate providers: (1) pass the surrogate provider from the serializer into the XmlObjectSerializer*ContextComplex constructors, and (2) apply GetSurrogatedType in the RootContract property when a surrogate provider is set. New constructor overloads on the base context classes cleanly extend the existing pattern.

Summary: ✅ LGTM. The fix is correct, minimal, and consistent with the existing DataContractSerializer behavior. Tests mirror the existing DCS surrogate tests. One minor code-quality observation noted below but not blocking.

---

Detailed Findings

✅ Core Fix — Surrogate provider propagation to contexts

The root cause is properly addressed. Before this PR, XmlObjectSerializerReadContextComplexJson and XmlObjectSerializerWriteContextComplexJson called the 4-parameter base constructor that did not set _serializationSurrogateProvider:

// Before (broken):
: base(serializer, serializer.MaxItemsInObjectGraph, ..., serializer.IgnoreExtensionDataObject)

The fix adds a 5th parameter to pass the surrogate provider:

// After (fixed):
: base(serializer, serializer.MaxItemsInObjectGraph, ..., serializer.IgnoreExtensionDataObject, serializer.SerializationSurrogateProvider)

This is identical to how DataContractSerializer sets _serializationSurrogateProvider in its context constructor (via serializer.SerializationSurrogateProvider in XmlObjectSerializerReadContextComplex(DataContractSerializer, ...) on line 21). The new 5-parameter constructors on both XmlObjectSerializerReadContextComplex and XmlObjectSerializerWriteContextComplex are clean and minimal. ✅

✅ RootContract surrogate type resolution

The RootContract property now applies GetSurrogatedType when a surrogate provider is set:

_rootContract = DataContract.GetDataContract(
    (_serializationSurrogateProvider == null) 
        ? _rootType 
        : GetSurrogatedType(_serializationSurrogateProvider, _rootType));

This is an exact match of the pattern used in DataContractSerializer.RootContract (line 230 of DataContractSerializer.cs). ✅

✅ Cache invalidation deliberately omitted — consistent with DCS

The commit history shows cache invalidation (_rootContract = null in the setter) was initially implemented and then deliberately removed per reviewer feedback (commit 16cf4fc0). This is consistent with DataContractSerializer, which also does not invalidate the cache in its SerializationSurrogateProvider setter. The expected usage pattern is set-then-use (as confirmed by all test callsites and the extension method pattern), so _rootContract is lazily initialized after the surrogate provider is already set. ✅

✅ Test coverage — mirrors DCS tests

Both DCJS_MyPersonSurrogate and DCJS_FileStreamSurrogate are near-identical copies of the existing DCS_MyPersonSurrogate and DCS_FileStreamSurrogate tests, swapping DataContractSerializer for DataContractJsonSerializer. This verifies functional parity. The TempFile.cs common helper is correctly included in both .csproj files (standard and ReflectionOnly). The additional Age assertion in the existing DCS test (DataContractSerializer.cs line 4221) is a good catch. ✅

✅ No public API surface changes

The GetSerializationSurrogateProvider() and SetSerializationSurrogateProvider() public methods already existed in the ref assembly (System.Runtime.Serialization.Json.cs lines 56–57). The new GetSurrogatedType method is internal static. No API approval is needed. ✅

💡 Minor code duplication — GetSurrogatedType

The new DataContractJsonSerializer.GetSurrogatedType() (line 660) is an exact copy of DataContractSerializer.GetSurrogatedType() (line 538). Since DCJS already calls other DCS static methods directly (e.g., DataContractSerializer.SurrogateToDataContractType() on line 511, DataContractSerializer.GetDataContract() on line 653), the RootContract getter could call DataContractSerializer.GetSurrogatedType(...) instead. However, the current code works correctly and maintains symmetry, so this is purely a style preference and not blocking.

Generated by Code Review for issue #118257 · ◷

[StephenMolloy]

Added the surrogate test coverage that was identified as missing. However, I chose not to add _rootContract invalidation in SetSerializationSurrogateProvider, for the following reasons:

Consistency with DataContractSerializer. The DataContractSerializerExtensions.SetSerializationSurrogateProvider extension method has never invalidated _rootContract when assigning a new provider. Introducing invalidation in DCJS but not DCS would create an observable behavioral inconsistency between the two serializers.

Historical contract from .NET 4.8. In .NET Framework, both DCS and DCJS accepted surrogates exclusively at construction time (via constructor parameters or settings objects). This made _rootContract — which is computed on first use and incorporates the surrogate — effectively immutable by design: the surrogate was always known before _rootContract could be resolved, so there was never a cache-invalidation concern.

Immutability of _rootContract. The _rootContract field is lazily initialized on first use and never written again. Callers that have already triggered that initialization have implicitly taken a dependency on the resolved contract. Nulling it out after the fact would silently change the behavior of a value that users could reasonably consider stable.

Mitigation. Rather than invalidating the cache, the appropriate response is to document that surrogate providers must be set before the serializer is first used — consistent with how .NET 4.8 enforced this structurally. A debug-build Debug.Assert has also been added to both DCJS's SetSerializationSurrogateProvider and DCS's internal SerializationSurrogateProvider setter to catch violations of this contract during development and testing.

[mangod9]

Overall looks good and is low risk.

[DrewScoggins]

LGTM