JamesNK / Newtonsoft.Json

Json.NET is a popular high-performance JSON framework for .NET
https://www.newtonsoft.com/json
MIT License
10.73k stars 3.25k forks source link

Json.NET interprets and modifies ISO dates when deserializing to JObject #862

Closed FrancoisBeaune closed 8 years ago

FrancoisBeaune commented 8 years ago

We're using Json.NET 8.0.2 in a C# 4.5 project, and we love it.

However this behavior bit us pretty hard:

var obj = JsonConvert.DeserializeObject<JObject>("{ 'x': '2016-03-31T07:02:00+07:00' }");

Here, in our time zone, ob["x"].Value<string>() returns "2016-03-31T02:02:00+02:00".

The problem disappears if we specify how to parse dates:

var obj = JsonConvert.DeserializeObject<JObject>("{ 'x': '2016-03-31T07:02:00+07:00' }",
    new JsonSerializerSettings() { DateParseHandling = DateParseHandling.None });

But I find it dangerously surprising that by default Json.NET interprets strings as dates (let alone the performance issues this inevitably brings with it).

Is this the expected behavior? If so, is there a way to disable this behavior globally?

SteveSmith16384 commented 5 years ago

Add me to the list of user's who've just wasted a few hours wondering why my code was failing to parse what I thought was a string, and finally getting to the bottom of it. Now just waiting for the code to break again when a future person decides to send a different date format and Json.Net stops thinking that it's a date.

I'm sure it's not the first time any of us have had their time wasted by code that tries to be too clever and ends up causing more problems.

paulpce commented 5 years ago

Add me as another one, thankfully I'm lucky it only took me half an hour questioning the bahaviour of my app before ending up at this thread. Disappointed.

markzielinski commented 5 years ago

This library is obsolete now anyway, .NET Core 3.0 has built in support for JSON with System.Text.Json which is 1.5 - 2x faster and has more efficient memory use as well.

See this link for details: https://devblogs.microsoft.com/dotnet/try-the-new-system-text-json-apis/

thomaslevesque commented 5 years ago

This library is obsolete now anyway, .NET Core 3.0 has built in support for JSON with System.Text.Json which is 1.5 - 2x faster and has more efficient memory use as well.

I wouldn't say it's obsolete... Sure, System.Text.Json is faster and more efficient, but it's not nearly as feature-rich as JSON.NET. In many situations System.Text.Json will be enough, but in more advanced scenarios, you might need the fine control offered by JSON.NET

PathToLife commented 5 years ago

Well it seems like the author, who graciously made public this code, <thanks!>. (Therefore I'm saying this respectfully and thankfully.)

Has made their decision, and seems to not want to change it. (because a twitter poll he did about about changing this resulted in 50/50). I Say Fair enough~! But if he refuses a pull req.. changing it. You could fork this and note him being the original creator.

I feel like a guilty freeloader who's despised by the creator for bumping this issue. But it has definitely cost many new developers time over the years.

With System.Text.Json coming along, hopefully no more new developers will be burnt by this feature.

TLDR; Don't sound too disrespectful, this is a generous opensource project. If you have the skills to fix it. Fork, make pull req & promote here. Use System.text.Json when your project can support it in a few months/years

jhandby commented 4 years ago

I'm a long time Json.NET user, and I am very grateful for its existence, and I love the work you do. Except this one design decision, which is baffling. I've spent hours trying to figure out why my string containing a date was being mysteriously modified on serialization / deserialization, only to discover that Json.NET was unilaterally deciding that it should inspect the contents of my strings and deciding that it should be changing them to other strings.

This is absolutely sucky behaviour.

jhandby commented 4 years ago

Well it seems like the author, who graciously made public this code, <thanks!>. (Therefore I'm saying this respectfully and thankfully.)

and I felt very much like that, but then I read his replies above, and I've got to say I'm quite disappointed by the tone in which he chose to respond to people reporting a bug caused by a breaking change he decided to make.

As much as I value his work thus far, it feels like this is one of those evolutionary moments when you realise that half the modern internet is completely dependent on one guy's library, and whatever design decisions that guy makes, and what mood he wakes up in from day to day. However much one appreciates that guy's contribution, that's not really sustainable.

nicky1038 commented 4 years ago

First of all, many thanks to the author for such a great library.

As for the issue, what has not been said yet: a design decision is good if it is intuitive how to use a feature. If so many people are sure something works one way but in real it works another way, something is definitely wrong.

I can also suggest another measure of this decision's correctness, other than "a lot of people use date strings parsing" (and how many do not?!). "Total time people spent to sort out how to set up serialization/deserialization and what goes wrong is huge and could be greatly decreased by changing the default setting".

aurelien-boubennec commented 4 years ago

It took me 2 hours to figure out newtonsoft was altering string data without consent...

aurelien-boubennec commented 4 years ago

"Feature request: ditch Newtonsoft.Json in favor of System.Text.Json IdentityModel/IdentityModel#286"

LOL

dsaul commented 4 years ago

Yep, this one has messed me up on a number of occasions.

glyngholm commented 4 years ago

Just got bit by this.....

dotnetprofessional commented 4 years ago

The below code saved my project. This issue was leading to very unexpected behavior (aka bugs!) and its not at all intuitive that this issue exists, at a minimum it could honor UTC format and make the datas Utc not Local. Anyway this code fixed my issue, the global setting isn't good for two reasons, 1) you dont want to apply to all aspects and 2) it doesn't work with an existing JObject ie .ToObject.

from: @FrancoisBeaune

The problem disappears if we specify how to parse dates:

var obj = JsonConvert.DeserializeObject<JObject>("{ 'x': '2016-03-31T07:02:00+07:00' }",
    new JsonSerializerSettings() { DateParseHandling = DateParseHandling.None });

I used this on just the section that contained dates as string which was a Dictionary<string, string>.

Thanks!

davidvedvick commented 4 years ago

Another fix workaround that worked well for using JSON.Net in what should be a generic caching library was to convert DateTime types to their binary representation using the ToBinary() method on DateTime within a customer converter. This encodes all of the DateTime information in a long, far away from the prying eyes of JSON.Net's DateTime handling logic. Since the serialized value was hidden from consumers of the library, this was sufficient for my needs, but this behavior was definitely unexpected.

dvick commented 4 years ago

Making this default to deserializing as a DateTime by default is absolutely a terrible design choice. Broke a JsonConverter for me when used with JsonPatchDocument, which doesn't take JsonSerializerSettings (which it definitely should). Had to set the DateParseHandling property to None on the JsonReader in the JsonConverter for the class containing the property with the broken JsonConverter.

osexpert commented 3 years ago

The worst thing here is not that the default is DateParseHandling.DateTime but that when using converters and the string is already converted to DateTime, there is no way to overrule the parsing because no way to get original string. Setting DateParseHandling.None globally may not always be easy\possible\wanted, and it may be difficult to access individual settings as they will be burred inside a library etc. I was actually thinking about forking the library because of this, the problem is still 3rd party libraries...no easy way to get them to use the forked version. I guess we just need to wait until all has migrated to System.Text.Json, but that will take many years.

TritonNet commented 3 years ago

@gdalsnes Just noted your idea to get the right behaviour with forking. I've done the same and you could get the third party libraries to use the forked version with AssemblyRedirection in the config file.

B4rT45 commented 3 years ago

we started using System.Text.Json for all new .NET core projects despite having have quite mature "framework" (libraries/packages) that are are written for/with use of newtonsoft - so far we were able to achieve everything that was done in the old projects/libraries/packages with no issues and given that there will be a lot of new features/improvements in STJ starting with .net 5 I think we can safely say .... bye-bye newton and your "by-design feature that changes users' data" (where serialise(x) = y and deserialise(y) = z) :D

sungam3r commented 3 years ago

with .net 5 I think we can safely say .... bye-bye newton

100% agree

codermrrob commented 3 years ago

You can switch if you use only 10% of the library...

Get Outlook for Androidhttps://aka.ms/ghei36


From: Ivan Maximov notifications@github.com Sent: Sunday, November 1, 2020 4:49:26 PM To: JamesNK/Newtonsoft.Json Newtonsoft.Json@noreply.github.com Cc: codermrrob mr.rob.smith@outlook.com; Comment comment@noreply.github.com Subject: Re: [JamesNK/Newtonsoft.Json] Json.NET interprets and modifies ISO dates when deserializing to JObject (#862)

with .net 5 I think we can safely say .... bye-bye newton

100% agree

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/JamesNK/Newtonsoft.Json/issues/862#issuecomment-720099983, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AE42QAN54A3TILPAMKKEBMLSNVYPNANCNFSM4B7O3BZA.

B4rT45 commented 3 years ago

You can switch if you use only 10% of the library...

I don't where the 10% is coming from (I guess the % would refer to the current state of STJ, not the future versions?) but I know the switch worked for us like a treat :)

tmc101 commented 3 years ago

Also just bitten by this.

When my browser send this JSON string property to my API:

"user-entered-data": "2021-01-01T00:00:00.000Z"

It is serialized/de-serialized using JObject, and ends up in my database as a different string:

"user-entered-data": "2021-01-01T00:00:00Z"

I now need to highlight changed values in the browser - which means that I need to introduce a load of unnecessary date conversion code into my front end application to cope with the completely unexpected changes to the strings.

codermrrob commented 3 years ago

You can switch if you use only 10% of the library...

I don't where the 10% is coming from (I guess the % would refer to the current state of STJ, not the future versions?) but I know the switch worked for us like a treat :)

It was a bit glib I admit :) Just mean to say that the deserialization/serialization is a small part of the json.net libaray usage for me. It provides a lot of valuable functionality that makes json processing much easier that STJ doesn't yet provide. At some point I'm sure it will.

RoyTinker commented 3 years ago

FWIW, I've had to remove my recommendation of Json.NET in my Stack Overflow answers that have helped a lot of people deal with Microsoft's old "/Date(...)/" format. I believe this design decision is harmful and I hope you'll reconsider.

https://stackoverflow.com/questions/206384/how-do-i-format-a-microsoft-json-date/2316066#2316066

https://stackoverflow.com/questions/726334/asp-net-mvc-jsonresult-date-format/1767558#1767558

@JamesNK Let me know if you do reconsider and I'll be glad to add my recommendation of Json.NET back to these answers.

codermrrob commented 3 years ago

As much as I find this library incredibly useful this behavior continues to be problematic. We have to make a corporate development standard to ensure that JSON data is de-serialized correctly when using JSON.Net (leaving dates untouched). It adds code and effort to every deployment and reduces the usefulness of the library. A library level switch on this behavior, with the default being the existing behavior, would remove this issue, improve software reliability and reduce development testing and debugging costs.

Contrary to James's opinion that most people want date behavior like this the fact is that JSON is a data interchange standard and as such dates can be in many formats, not just conforming to the date formats of the server reading them. By manipulating strings containing dates instead of leaving them as strings the standard is broken in an unexpected way.

wdolek commented 3 years ago

Just got bitten by this as well. For completeness, in ASP.NET Core 5.0 you turn this behavior off in startup globally (for MVC) following way:

// call on `IMvcBuilder`
services
    .AddMvc()
    .AddNewtonsoftJson(o => o.SerializerSettings.DateParseHandling = DateParseHandling.None);
JessHolle commented 3 years ago

I just ran into this -- and this is among the most bizarre and user-vicious behaviors I have ever seen, particularly in a JSON library.

Round tripping a string value shouldn't be loused up just because it happens to match a standard date format!

codermrrob commented 3 years ago

Just got bitten by this as well. For completeness, in ASP.NET Core 5.0 you turn this behavior off in startup globally (for MVC) following way:

// call on `IMvcBuilder`
services
    .AddMvc()
    .AddNewtonsoftJson(o => o.SerializerSettings.DateParseHandling = DateParseHandling.None);

Is fine for MVC but when code is backend, using JSON for integration and data interchange between a variety of systems in different countries and timezones presentation frameworks are less useful.

Fundamentally JSON is a data interchange format (json.org) and changing the content of data in anyway should be an opt-in experience at the very least and even that is pretty tenuous.

loop-evgeny commented 3 years ago

Just got bitten by this as well. For completeness, in ASP.NET Core 5.0 you turn this behavior off in startup globally (for MVC) following way:

// call on `IMvcBuilder`
services
    .AddMvc()
    .AddNewtonsoftJson(o => o.SerializerSettings.DateParseHandling = DateParseHandling.None);

I tried doing this on ASP.NET Core 3.1 and ran into an issue with UTC times becoming local times when JSON from the client was deserialized into a DateTime. Probably https://github.com/dotnet/aspnetcore/issues/11584 Something to watch out for if you're not on .NET 5+ yet!

codermrrob commented 3 years ago

I tried doing this on ASP.NET Core 3.1 and ran into an issue with UTC times becoming local times when JSON from the client was deserialized into a DateTime. Probably dotnet/aspnetcore#11584 Something to watch out for if you're not on .NET 5+ yet!

I believe this is a behavior of .Net rather than Newtonsoft lib. When you deserialize into a DateTime .Net will use your local timezone to construct the DateTime. This is a pain if you don't know what timezone your software will be installed into. Using NodaTime is helpful if you have to jump timezones, etc.

augustoproiete commented 3 years ago

I believe this is a behavior of .Net rather than Newtonsoft lib. When you deserialize into a DateTime .Net will use your local timezone to construct the DateTime. This is a pain if you don't know what timezone your software will be installed into. Using NodaTime is helpful if you have to jump timezones, etc.

@codermrrob It's a bug in ASP .NET in .NET Core 3.1 where sending a UTC time string such as 2021-05-18T02:30:04.0576719Z to an ASP .NET Core 3.1 API converts it to a DateTime with DateTimeKind.Local instead of DateTimeKind.Utc causing the API to think a conversion to UTC is necessary.

This has been fixed in .NET 5, but unfortunately the team decided not to back-port the fix to .NET Core 3.1.x despite it being an LTS release :man_shrugging:, so the workaround is to copy/paste a bunch of code to your app as described in https://github.com/dotnet/aspnetcore/issues/11584 if you're not able to move to .NET 5

sehrgut commented 3 years ago

I believe this is a behavior of .Net rather than Newtonsoft lib. When you deserialize into a DateTime

The issue is that JSON does not have a DateTime, so that conversation from String to DateTime should not happen. It DEFINITELY should not happen behind the scenes going from String to String.

While the .NET bug makes this a BIGGER issue, it is still an incorrect and noncompliant JSON implemention on the part of Newtonsoft.

codermrrob commented 3 years ago

it is still an incorrect and noncompliant JSON implemention on the part of Newtonsoft.

To be fair I think it's a resonable option for library like this to have. I just think it's unfortunate that its opt-out functionality rather than opt-in. I think most people in this thread agree that it should be an opt-in. That would actually make the issue go away.

JessHolle commented 3 years ago

It is a stealth bug -- the fact that every piece of code reading JSON has to take special efforts not to have date-formatted strings loused up in ways one would not expect with JSON is "user vicious" behavior.

sehrgut commented 3 years ago

To be fair I think it's a resonable option for library like this to have. I just think it's unfortunate that its opt-out functionality rather than opt-in.

That's what makes it a bug, though. JSON is a defined format with a standard, and this breaks compliance with that standard. It shouldn't even be "opt-in" to have really correct architecture. You wouldn't want an XML parsing library to put cleanup or reformatting features on the parser itself: that should be a separate system entirely that consumes the parser output and post-processes it.

If Newtonsoft wants to support detection of arbitrary non-JSON datatypes to construct non-JSON-compliant output structures, fine. That should be an extension of the parser itself, because it's NOT part of parsing JSON.

wasabii commented 3 years ago

There's nothing about this which breaks the standard. The JSON is still JSON. The standard doesn't define what a library may or may not do while reading JSON, only the structure of the JSON itself.

It's obnoxious counter intuitive and harmful behavior, but not a standard violation.

sehrgut commented 3 years ago

You're confusing a parser with a consumer. Obviously, code that consumes JSON can do anything it wants with the data. But a parser/generator that foists nomstandard datastructures on consuming code is not compliant with the standard.

admin-simeon commented 3 years ago

The bug here (and if you don't want to consider it a bug, it is definitely a misdesigned behavior) - is not that the default behavior converts it to date at parse time. As everyone's pointed out, you can set the DateParseHandling.

The issue is the fact that the default behavior can't be changed except in a highly unexpected, astonishing way.

Consider the following example. It blatantly violates the principle of least astonishment, in that it just doesn't work in one case, and does work in the other case.

            var time = "2016-05-10T13:51:20Z";
            var json = $@"{{
""prop"":""{time}""
}}";

            JsonConvert.DefaultSettings = () => new JsonSerializerSettings()
            {
                DateParseHandling = DateParseHandling.None
            };
            var jo = JObject.Parse(json);

            var val = jo.Value<string>("prop");
            var result = val == time; // FALSE!!! 

            using var sr = new StringReader(json);
            using var jr = new JsonTextReader(sr);
            jr.DateParseHandling = DateParseHandling.None;
            var jo2 = JObject.ReadFrom(jr);

            var val2 = jo2.Value<string>("prop");
            var result2 = val2 == time; // TRUE!!! 

to make this work, you have to do something astonishing - the default settings simply don't have any effect on parsing the JSON

I realize your mind is made up @JamesNK, but the risk of this behavior introducing bugs is profound. This behavior means that there is simply no way to control this behavior at the solution level and a single slip-up that isn't caught in code review where a developer calls JObject.Parse instead of to MyJsonExtensions.Parse will result in a bug making it to production

osexpert commented 3 years ago

Consider the following example. It blatantly violates the principle of least astonishment, in that it just doesn't work in one case, and does work in the other case.

JsonConvert.DefaultSettings only control things that has to do with JsonConvert. A similar DefaultSettings for JsonReader does not exist. So here lies the problem: this setting should have been on something that is common to JsonConvert\JsonTextReader etc. (eg. JsonReader) and it has an imbecile default.

admin-simeon commented 3 years ago

@gdalsnes - that's fair...and having defaults for JsonReader shouldn't be something that is so controversial to fix.

vlaugier commented 3 years ago

Completely abnormal behavior.

A string is a string ; if I wanted a date, I would project to a date. And if I wanted a date as a string, I would do it properly.

stamminator commented 3 years ago

I hate to say it, but I think this is a case of ego getting in the way of the right decision being made. The argument clearly falls on the side of this default behavior being a bad idea, as this whole thread bears testament to. Rip off the band aid and introduce the DateParseHandling.None default behavior as a breaking change in the next major release verison.

stmax82 commented 2 years ago

Today is the 3rd time I came across places where this introduced a bug.

No one expects that reading "2021-09-16T21:06:02.0923812Z" as string results in the string "09/16/2021 21:06:02". That's just silly default behavior that should be changed in the next major release so I don't have to google it again next year.

lol768 commented 2 years ago

Just been bitten by this, I would have never expected a string to be meddled with in this manner. This is the sort of "magic" that I would absolutely shoot down in a code review, and I think the responses to this issue from everyone else who's encountered this illustrate how stupid it was to enable it by default.

Guess I need to speed up my migration to System.Text.Json.

domholmes commented 2 years ago

Add me to the list of bemused users who spent an age chasing down this odd behaviour. I honestly didn't even consider Json.NET could be the culprit at first. I'm struggling to see why anyone would want this behaviour by default. Please re-consider.

bdcoder2 commented 2 years ago

At the VERY LEAST - Update the documentation!! The very first page, https://www.newtonsoft.com/json/help/html/Introduction.htm, should include a big WARNING about this behavior and a link back to this "feature" / issue on github.com.

That is not a big ask -- just do it !

domholmes commented 2 years ago

Am I correct in saying there is no way to globally avoid this behaviour when parsing into JObject/JToken?

jeff-simeon commented 2 years ago

@domholmes

the only option I know of is to avoid the Parse methods altogether and use

JsonConvert.DeserializeObject<JObject>

with custom DefaultSettings

osexpert commented 2 years ago

Am I correct in saying there is no way to globally avoid this behaviour when parsing into JObject/JToken?

yes

lostmsu commented 2 years ago

@aurelien-boubennec

"Feature request: ditch Newtonsoft.Json in favor of System.Text.Json IdentityModel/IdentityModel#286"

Well, that project followed through: https://github.com/IdentityModel/IdentityModel/blob/c54179ee32e35c19a1181a583defb3a6710b7fe7/src/IdentityModel.csproj#L47.

Would do the same if my project needed JSON parsing, and I would be using JSON.NET