Skip to content

Use Copy Constructor for JsonSerializerSettings#61251

Merged
BrennanConroy merged 2 commits intodotnet:mainfrom
MatthewSteeples:fix-json-serializer-settings
Apr 4, 2026
Merged

Use Copy Constructor for JsonSerializerSettings#61251
BrennanConroy merged 2 commits intodotnet:mainfrom
MatthewSteeples:fix-json-serializer-settings

Conversation

@MatthewSteeples
Copy link
Copy Markdown
Contributor

@MatthewSteeples MatthewSteeples commented Mar 31, 2025

Fixes #29532

Fixes an issue where JsonSerializerSettings.DateFormatString overwrites DateFormatHandling. See #29532 for details


For more details, open the Copilot Workspace session.

@MatthewSteeples MatthewSteeples requested a review from a team as a code owner March 31, 2025 22:23
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 31, 2025
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Mar 31, 2025
@MatthewSteeples
Copy link
Copy Markdown
Contributor Author

@pranavkm the bot hasn't worked out who needs to review this and you were involved in the linked issue. Any chance you could nudge it in the right direction?

Copy link
Copy Markdown
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

@halter73 halter73 enabled auto-merge (squash) April 4, 2025 23:32
auto-merge was automatically disabled April 6, 2025 11:52

Head branch was pushed to by a user without write access

@MatthewSteeples MatthewSteeples force-pushed the fix-json-serializer-settings branch 3 times, most recently from fc4e126 to 7bac082 Compare April 8, 2025 22:13
@MatthewSteeples
Copy link
Copy Markdown
Contributor Author

I can't work out where the latest lot of build failures are coming from, so I'm hoping it's not me...!

@dotnet-policy-service dotnet-policy-service Bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Apr 16, 2025
Fixes dotnet#29532

Fixes an issue where JsonSerializerSettings.DateFormatString overwrites DateFormatHandling. See dotnet#29532 for details
@MatthewSteeples MatthewSteeples force-pushed the fix-json-serializer-settings branch from a91eda9 to 037406f Compare April 27, 2025 20:14
@MatthewSteeples
Copy link
Copy Markdown
Contributor Author

@halter73 I still can't work out why the test is failing but I'm relatively sure it's nothing I'm doing. Do you have any pointers for how to proceed from here?

Copilot AI review requested due to automatic review settings April 4, 2026 00:48
@BrennanConroy BrennanConroy enabled auto-merge (squash) April 4, 2026 00:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates MVC’s Newtonsoft.Json output formatter to clone JsonSerializerSettings via Json.NET’s copy constructor, avoiding the DateFormatString setter side-effect that can cause DateFormatHandling to be ignored (per #29532).

Changes:

  • Replace the formatter’s custom ShallowCopy logic with new JsonSerializerSettings(SerializerSettings).
  • Remove the now-unneeded ShallowCopy helper.
  • Add a regression test covering the new copy-constructor-based behavior (but see comment about strengthening the assertion).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonOutputFormatter.cs Switches settings cloning to Json.NET’s copy constructor and removes ShallowCopy.
src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonOutputFormatterTest.cs Adds a test intended to validate that the copy constructor doesn’t override date formatting behavior.

Comment on lines +560 to +566

// Act
var copiedSettings = new JsonSerializerSettings(originalSettings);

// Assert
Assert.Equal(originalSettings.DateFormatHandling, copiedSettings.DateFormatHandling);
Assert.Equal(originalSettings.DateFormatString, copiedSettings.DateFormatString);
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test only compares DateFormatHandling/DateFormatString property values. The original bug is caused by a hidden side effect of setting DateFormatString (even to null) which flips an internal "DateFormatString has been set" flag; property equality will still pass even when the bug is present. Please assert the observable behavior instead (e.g., serialize a DateTime using the copied settings or via NewtonsoftJsonOutputFormatter and verify the payload uses MicrosoftDateFormat rather than the default ISO date string).

Suggested change
// Act
var copiedSettings = new JsonSerializerSettings(originalSettings);
// Assert
Assert.Equal(originalSettings.DateFormatHandling, copiedSettings.DateFormatHandling);
Assert.Equal(originalSettings.DateFormatString, copiedSettings.DateFormatString);
var value = DateTime.UnixEpoch;
// Act
var copiedSettings = new JsonSerializerSettings(originalSettings);
var originalJson = JsonConvert.SerializeObject(value, originalSettings);
var copiedJson = JsonConvert.SerializeObject(value, copiedSettings);
// Assert
Assert.Equal(originalJson, copiedJson);
Assert.Equal(@"""\/Date(0)\/""", copiedJson);

Copilot uses AI. Check for mistakes.
@BrennanConroy
Copy link
Copy Markdown
Member

/ba-g unrelated hang

@BrennanConroy BrennanConroy merged commit f9b6fcf into dotnet:main Apr 4, 2026
26 of 28 checks passed
@dotnet-policy-service dotnet-policy-service Bot added this to the 11.0-preview4 milestone Apr 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setting JsonSerializerSettings.DateFormatString in ShallowCopy "overrides" JsonSerializerSettings.DateFormatHandling

4 participants