tmenier / Flurl

Fluent URL builder and testable HTTP client for .NET
https://flurl.dev
MIT License
4.23k stars 387 forks source link

UseNewtonsoft + clientless pattern, default serializer incorrectly used to serialize request body #783

Closed statler closed 10 months ago

statler commented 11 months ago

Using a complex JSON object - something like the following, the object is incorrectly serialized by PostJsonAsync whether using STJ or Json.Net. However, when serialized outside PostJsonAsync using default settings for Json.Net, the result is as expected.

Using latest 4pre6.

  var _linkItems = new
  {
      Int_Key = 0,
      Int_Box = 1111,
      Int_Link = 2222
  };
  dynamic _linkWrapper = new JObject();
  _linkWrapper["0"] = JObject.FromObject(_linkItems);
  var _params = new
  {
      pageToke = "",
      lotIntKey = 11,
      module = 2,
      lotData = new
      {
          Details = (string)null,
          Custom = (string)null,
          LinkItems = _linkWrapper,
          Security = (string)null,
          BIMLinks = (string)null
      }
  };

E.g this doesn't work:

            var path = lot_Segments.Append("MyMethod");
            var _req = _cookieSession.Request(path.ToArray());
            var _postReq = await _req
                .PostJsonAsync(_params);

However, this does:

            var json = JsonConvert.SerializeObject(_params);
            var content = new CapturedJsonContent(json);
            var _postReq = await _req
                .SendAsync(HttpMethod.Post, content);

The latter correctly sends the serialized string as {"pageToke":"","lotIntKey":11,"module":2,"lotData":{"Details":null,"Custom":null,"LinkItems":{"0":{"Int_Key":0,"Int_Box":1111,"Int_Link":2222}},"Security":null,"BIMLinks":null}}

The first one mangles the content of linkitems into some strange set of [[[],[],[]][[]]] etc.

tmenier commented 11 months ago

I see you're using 4.0. Newtonsoft was removed so JObject is no longer supported. Neither are dynamics. Try the just-released Flurl.Http.Newtonsoft companion lib. I'm in the middle of documentation updates now, but here's the quick & dirty of how to use:

If you're already well versed in #770 (most people probably aren't), UseNewtonsoft() is also available on IFlurlClientBuilder when configuring a specific client at startup.

statler commented 11 months ago

Thanks Todd - this isn't just a case of STJ vs NewtonSoft. This behaviour is observed even if you are using Newtonsoft. There is something about the serialization in the PostJsonAsync that differs from doing it outside as per my workaround above. I am already using FlurlHttp.Clients.UseNewtonsoft()

TLDR; UseNewtonSoft does not fix the observed behaviour.

For reference - per the original report using 4pre6.

tmenier commented 11 months ago

I can't repro this.

The first one mangles the content of linkitems into some strange set of [[[],[],[]][[]]] etc.

Yes, I get that by default.

The latter correctly sends the serialized string as {"pageToke":"","lotIntKey":11,"module":2,"lotData":{"Details":null,"Custom":null,"LinkItems":{"0":{"Int_Key":0,"Int_Box":1111,"Int_Link":2222}},"Security":null,"BIMLinks":null}}

When I swap out the default serializer for the one defined in Flurl.Http.Newtonsoft, I get exactly that.

How exactly are you determining what Flurl's internals are doing when you reached your conclusions?

statler commented 11 months ago

In the client, I specified the usenewtonsoft method - travelling so can't give the exact details - but if you can't repro it is probably something not working like i expected so I will do some more research my end and get back - feel free to close it for now and I will hit it again if there is an issue.

tmenier commented 11 months ago

I won't keep it open forever, but I'll keep it open a bit longer if you think you'll look at it again soon-ish. ;)

All the symptoms do seem to suggest STJ is still driving the serialization, even when you think Newtonsoft is in control. I suspect it's something with how you have things wired up.

zhzhwcn commented 11 months ago

Same here:

#r "nuget: Flurl.Http.Newtonsoft, 0.9.0"
#r "nuget: Flurl.Http, 4.0.0"
using System.Net.Http;
using Flurl;
using Flurl.Http;
using Flurl.Http.Newtonsoft;
using Newtonsoft.Json.Linq;

var url = "http://bin.mailgun.net/5b58c2a";
FlurlHttp.Clients.UseNewtonsoft();
var json = new JObject();
json["test"] = "test";
json["number"] = 1;
json["date"] = DateTime.Today;
await url.PostJsonAsync(json);

Receives:

Parameter Value  
test []
number []
date []
jassent commented 10 months ago

Same here:

#r "nuget: Flurl.Http.Newtonsoft, 0.9.0"
#r "nuget: Flurl.Http, 4.0.0"
using System.Net.Http;
using Flurl;
using Flurl.Http;
using Flurl.Http.Newtonsoft;
using Newtonsoft.Json.Linq;

var url = "http://bin.mailgun.net/5b58c2a";
FlurlHttp.Clients.UseNewtonsoft();
var json = new JObject();
json["test"] = "test";
json["number"] = 1;
json["date"] = DateTime.Today;
await url.PostJsonAsync(json);

Receives:

Parameter Value   test []
number []
date []

What happens if you do this:

try {
    var result = await url.PostJsonAsync(json).ReceiveString();
    Console.WriteLine(result);
}
catch (Exception ex) {
  Console.WriteLine($"{ex}");
}

I have run into empty results due to threading and serializer exceptions. The above would help diagnose.

zhzhwcn commented 10 months ago

Same here:

#r "nuget: Flurl.Http.Newtonsoft, 0.9.0"
#r "nuget: Flurl.Http, 4.0.0"
using System.Net.Http;
using Flurl;
using Flurl.Http;
using Flurl.Http.Newtonsoft;
using Newtonsoft.Json.Linq;

var url = "http://bin.mailgun.net/5b58c2a";
FlurlHttp.Clients.UseNewtonsoft();
var json = new JObject();
json["test"] = "test";
json["number"] = 1;
json["date"] = DateTime.Today;
await url.PostJsonAsync(json);

Receives: Parameter Value   test [] number [] date []

What happens if you do this:

try {
    var result = await url.PostJsonAsync(json).ReceiveString();
    Console.WriteLine(result);
}
catch (Exception ex) {
  Console.WriteLine($"{ex}");
}

I have run into empty results due to threading and serializer exceptions. The above would help diagnose.

{"message":"Post received. Thanks!"} test with RoslynPad 19.1 and .net 8.0.100, no exception throws.

jassent commented 10 months ago

This is working for me:

var url = "https://eXXXXXXXXXXXXx.m.pipedream.net";
var a = new { test = "test", number = 1, date = DateTime.Today };
await url.PostJsonAsync(a);

The response received has the correct payload:

example1

This also works with the same payload as above but I don't really know that UseNewtonsoft() is actually doing anything:

var url = "https://eXXXXXXXXXXXXx.m.pipedream.net";
FlurlHttp.Clients.UseNewtonsoft();
var a = new { test = "test", number = 1, date = DateTime.Today };
await url.PostJsonAsync(a);

This does not work:

var url = "https://eXXXXXXXXXXXXx.m.pipedream.net";
FlurlHttp.Clients.UseNewtonsoft();
var json = new JObject();
json["test"] = "test";
json["number"] = 1;
json["date"] = DateTime.Today;
await url.PostJsonAsync(json);

The payload received is empty as others have described.

tmenier commented 10 months ago

Sorry for the delay, this is a priority and I'll look into it as soon as I have some time. In the mean time, you could try using the serializer directly to help determine if it's a problem with that vs the registration shortcuts. It's pretty straightforward:

https://github.com/tmenier/Flurl/blob/dev/src/Flurl.Http.Newtonsoft/NewtonsoftJsonSerializer.cs

tmenier commented 10 months ago

Confirmed. Sorry all, better tests would have prevented this little embarrassment. In short, "global" settings as a stand-alone thing were eliminated in 4.0 but moved to a global cache of clients used just for the clientless pattern. But with that pattern, request serialization happens before a client is selected, so changing the default serializer isn't properly applying to request bodies. (It is applying to JSON responses.) I have it proven in a test and will try to get fix released in short order.

astrohart commented 10 months ago

Who is this "ghost" who keeps deleting my comments? Am I being a bad person or something?

tmenier commented 10 months ago

@astrohart This issue is bug report, not a discussion of the merits of Newtonsoft vs STJ. I've deleted several of your comments recently because they're off-topic noise. Please stop.

astrohart commented 10 months ago

First of all let me apologize if I've been disrespectful. That was never my intent although I admit I was blowing off some steam.

But there was some frustration there given I'm a contributor on a repo that uses Flurl and I had to do hours of work to use a workaround. But it was my intention to say "me too" on this bug, until I found the workaround.

No hard feelings?

tmenier commented 10 months ago

Fixed & released. The core issue was in Flurl.Http; it would have been virtually impossible to fix in Flurl.Http.Newtonsoft alone. Both packages are updated (4.0.1 and 0.9.1 respectively), please update either of them and report back here if it does not resolve the issue. Sorry again for such a blatant bug!

amzhang commented 4 months ago

Thank you @tmenier! You're wonderful!

amzhang commented 4 months ago

This might be related. I try to set global default setting in Program.cs to ignore null fields during JSON serialization:

FlurlHttp.Clients.WithDefaults(
    builder =>
        builder.WithSettings(
            settings =>
                settings.JsonSerializer = new DefaultJsonSerializer(
                    new JsonSerializerOptions
                    {
                        DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull
                    }
                )
        )
);

but it seem to have no effect. Null fields are still serialized when calling:

var response = await request.PostMultipartAsync(mp => {
    mp.AddJson("config", config);
});

Versions

<PackageReference Include="Flurl" Version="4.0.0" />
<PackageReference Include="Flurl.Http" Version="4.0.2" />
tmenier commented 4 months ago

@amzhang Could you open a new issue for this? Comments in closed issues don't stay on my radar, and I suspect this isn't directly related anyway.