Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Json Patch model binding weirdness #146

Closed
Jetski5822 opened this issue Jul 23, 2021 · 12 comments · Fixed by #147
Closed

Json Patch model binding weirdness #146

Jetski5822 opened this issue Jul 23, 2021 · 12 comments · Fixed by #147
Labels
question Further information is requested

Comments

@Jetski5822
Copy link

Environment

  • Nuget Package: JsonPatch.Net
  • Nuget Version: 1.0.5
  • OS: Windows
  • .Net Target: core 3.1

How can I help you today?
I have some patch JSON:

[
   {
      ""value"":true,
      ""OperationType"":""Replace"",
      ""path"":""/IsManualEditIndicator"",
      ""op"":""replace""
   },
   {
      ""value"":""2021-04-02T00:34:11Z"",
      ""OperationType"":""Replace"",
      ""path"":""/ClockingDateTime"",
      ""op"":""replace""
   }
]

When I call and MVC endpoint like so; it works.

        public async Task<IActionResult> Patch(
            int id,
            [FromHeader(Name = "traceId")] string traceId)
        {
            var json = "";

            using (StreamReader reader
                      = new StreamReader(Request.Body, Encoding.UTF8, true, 1024, true))
            {
                json = await reader.ReadToEndAsync();
            }

            var patch = JsonSerializer.Deserialize<Json.Patch.JsonPatch>(json);

Patch is populated, world is good.
If I change it over to this;

        public async Task<IActionResult> Patch(
            int id,
            [FromHeader(Name = "traceId")] string traceId,
            [FromBody] JsonPatch clockingPatch)
        {

I get the following;

{
   "type":"https://tools.ietf.org/html/rfc7231#section-6.5.1",
   "title":"One or more validation errors occurred.",
   "status":400,
   "traceId":"00-d4ceda59a9448c4bbfa0826d24ab93e2-03d71d05696cb54f-00",
   "errors":{
      "Operations[0].From.Segments":[
         "The Segments field is required."
      ],
      "Operations[1].From.Segments":[
         "The Segments field is required."
      ]
   }
}

Any thoughts on how I can fix this? as im a little confused :D

@Jetski5822 Jetski5822 added the question Further information is requested label Jul 23, 2021
@gregsdennis
Copy link
Owner

gregsdennis commented Jul 23, 2021

Interesting scenario you've found. From the docs I'd expect that ASP.Net would just do the same deserialization that you're doing in the manual one.

Would you work up a minimal solution that replicates this, please? I want to make sure that what I debug is the same as what you're seeing.

Thanks for the report.

@gregsdennis
Copy link
Owner

gregsdennis commented Jul 23, 2021

The MS docs for how to do JSON Patch suggest using Newtonsoft and System.Text.Json 🤦

I wonder if this is what's happening: ASP.Net reads that it's a PATCH call and tries to deserialize into whatever Newtonsoft has provided.

@Jetski5822
Copy link
Author

Here you go https://github.com/Jetski5822/JsonPatchBug146

Something weird for you, it works under IISExpress... but NOT Kestrel.

If you clone, build, and then flip it to be Kestrel
image

using Postman do... PATCH to http://localhost:5000/patchme

[
   {
      "value":true,
      "OperationType":"Replace",
      "path":"/IsManualEditIndicator",
      "op":"replace"
   },
   {
      "value":"2021-04-02T00:34:11Z",
      "OperationType":"Replace",
      "path":"/ClockingDateTime",
      "op":"replace"
   }
]

image

So weird.!!!

@Jetski5822
Copy link
Author

Jetski5822 commented Jul 23, 2021

Hmm, In here https://github.com/gregsdennis/json-everything/blob/master/JsonPatch/PatchOperation.cs#L20

Shouldnt Op and Path be the only non nullable objects? with Value and From being nullable objects? based on here http://json.schemastore.org/json-patch

image

@gregsdennis
Copy link
Owner

Having those non-null resolved some processing issues I had while enabling nullable reference types in the lib. I do have it nullable in a DTO in the converter defined in the same file, toward the bottom.

@gregsdennis
Copy link
Owner

gregsdennis commented Jul 24, 2021

Well, it's definitely deserializing the patch properly. I removed the nuget package and just imported the projects. This is just before returning from

return new JsonPatch(operations);

image

Something's happening between the deserialization and entering the controller. Maybe ASP.Net is doing some validation, though I'm not sure what would be instructing it to do so.

@gregsdennis
Copy link
Owner

gregsdennis commented Jul 24, 2021

Okay. Found what might be causing it and a workaround for you.

Update Startup.cs:

// This method gets called by the runtime. Use this method to add services to the container.
public void ConfigureServices(IServiceCollection services)
{
    services.AddRazorPages()
        .AddMvcOptions(options => options.SuppressImplicitRequiredAttributeForNonNullableReferenceTypes = true);
}

What's odd is that the app doesn't have <Nullable>enable</Nullable>, but this is still enforced.

@gregsdennis
Copy link
Owner

Looked at this a bit today. I'm still not sure where that error message is being generated, so I don't know how to fix it other than the above.

@Jetski5822
Copy link
Author

Nice one Greg, I'll give it a go - I'll also pull your code to my sln and see if I can get it to work.

It's weird that it works fine in IIS express though :/

@gregsdennis
Copy link
Owner

gregsdennis commented Jul 25, 2021

The validation engine is saying that JsonPointer.Segments is not nullable and so requires a value. But because your patches don't declare a from value, it gets a default pointer, and .Segments is null for that. If I make that property nullable, the problem goes away. But that doesn't feel right because it should always have a value. (I supply a static Empty pointer to be used instead of default).

Interestingly, I'm using default to fill the from value in the PatchOperationConverter. If I use JsonPointer.Empty instead, the problem also goes away. I think this is the fix.

... I still have no clue where that error message is coming from.

@gregsdennis
Copy link
Owner

Okay. Try v1.0.6.

@Jetski5822
Copy link
Author

It works!!! Nice one - thanks for the fast turn around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants