JsonPatch.SystemTextJson: When serializing a `JsonPatchDocument`, `"from":null` is incorrectly emitted for "add", "remove", "replace" and "test" operations.

[dbc2]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

In .NET 10.0.100-rc.1.25451.107, I am trying to manually create a JsonPatchDocument (rather than receive one created by a client). I can do that quite easily, but when I serialize it using System.Text.Json, "from":null properties will be shown for operations that should not have a "from" property:

• "add"
• "remove"
• "replace"
• "test"

For instance, taking an example from JSON Patch RFC 6902, if I try to create the following patch:

[{ "op": "add", "path": "/a/b/c", "value": [ "foo", "bar" ] }]

Using the following code:

JsonPatchDocument patchDocument = new ();
patchDocument.Add("/a/b/c", new [] {  "foo", "bar" });
var json = JsonSerializer.Serialize(patchDocument);

I will get

[{"value":["foo","bar"],"path":"/a/b/c","op":"add","from":null}]

That "from":null definitely should not be present as can be seen from the RFC. What's more, the serialized JSON Patch could be considered to be invalid, since null is a malformed JSON Pointer value.

In addition, when I checked the docs and reference source for OperationBase.from, I see it is not marked as nullable:

[JsonPropertyName(nameof(from))]
public string from { get; set; }

Since from will in fact be null for many operations, this is not strictly correct.

---

Checking a bit further, it seems you copied the code for the new OperationBase almost verbatim from the Json.NET-based Microsoft.AspNetCore.JsonPatch.Operations.OperationBase. That code has a method ShouldSerializefrom() when prevents inappropriate "from" values from being serialized. Because over in Microsoft.AspNetCore.JsonPatch.SystemTextJson.Operations.OperationBase there is also a public function ShouldSerializeFrom(). However, System.Text.Json doesn't support the ShouldSerializeXXX() pattern out of the box, which seems to be causing the issue I am reporting.

I have not checked whether there is a similar problem serializing JsonPatchDocument<TModel>.

Expected Behavior

• "from" values should only be emitted for Move and Copy operations, and so "from":null should never be emitted when a JsonPatchDocument is serialized.
• OperationBase.from probably should be marked as nullable.
• Consider removing the useless OperationBase.ShouldSerializeFrom().

E.g. perhaps the following would be sufficient:

[JsonPropertyName(nameof(from)), JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public string? from { get; set; } 

Alternatively, if you want to preserve the logic that only writes "from" for Move and Copy operations (rather than just skipping it when null) you might need to write a manual converter since System.Text.Json does not have builtin support for the ShouldSerializeXXX() pattern.

Steps To Reproduce

//dotnet new console
//dotnet add package Microsoft.AspNetCore.JsonPatch.SystemTextJson --prerelease
using System;
using System.Collections;
using System.Collections.Generic;
using System.Text;
using System.Diagnostics;

using System.Text.Json;
using System.Text.Json.Serialization;
using System.Text.Json.Nodes;

using Microsoft.AspNetCore.JsonPatch.SystemTextJson;

JsonPatchDocument patchDocument = new ();
patchDocument.Add("/a/b/c", new [] {  "foo", "bar" });

var json = JsonSerializer.Serialize(patchDocument);
Console.WriteLine(json); // Prints [{"value":["foo","bar"],"path":"/a/b/c","op":"add","from":null}]

var expectedJson = """[{ "op": "add", "path": "/a/b/c", "value": [ "foo", "bar" ] }]"""; // Taken from  https://www.rfc-editor.org/rfc/rfc6902#section-4.1
Debug.Assert(JsonNode.DeepEquals(JsonNode.Parse(expectedJson), JsonNode.Parse(json)));  // Fails due to "from":null

Exceptions (if any)

None (incorrect JSON is generated without an exception thrown.)

.NET Version

10.0.100-rc.1.25451.107

Anything else?

dotnet --info:

.NET SDK:
 Version:           10.0.100-rc.1.25451.107
 Commit:            2db1f5ee2b
 Workload version:  10.0.100-manifests.0e2d47c4
 MSBuild version:   17.15.0-preview-25451-107+2db1f5ee2

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19045
 OS Platform: Windows
 RID:         win-x64
 Base Path:   C:\Program Files\dotnet\sdk\10.0.100-rc.1.25451.107\

[akshay-zz]

I would like take this issue. As details added in the issue.

• I think just ignoring the null should work as there no way one can add value for from for these operation.
• Also we can remove unused ShouldSerializefrom in OpearationBase and Operation
• Marking from as nullable

[captainsafia]

@akshay-zz Thanks for offering to take on this issue! PRs welcome!

At this point, this will likely land in .NET 11 although if it proves important we can backport it to the first servicing release of .NET 10. I'll stick it in the backlog for the now and mark as help wanted.

[dotnet-policy-service]

Looks like this issue has been identified as a candidate for community contribution. If you're considering sending a PR for this issue, look for the Summary Comment link in the issue description. That comment has been left by an engineer on our team to help you get started with handling this issue. You can learn more about our Help Wanted process here