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.NET interprets and modifies ISO dates when deserializing to JObject #862

Closed
FrancoisBeaune opened this issue Mar 30, 2016 · 172 comments
Closed

Comments

@FrancoisBeaune
Copy link

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?

@FrancoisBeaune
Copy link
Author

Seems like JsonConvert.DefaultSettings would allow to globally override this behavior:
http://james.newtonking.com/archive/2013/05/08/json-net-5-0-release-5-defaultsettings-and-extension-data

I would still be interested in hearing your opinion on this!

@antonovicha
Copy link

I suppose this is for convenience. We in our project do use this behavior because that way dates are human readable. On other hand MS Dates ( /Date(1224043200000)/ ) are not.

@FrancoisBeaune
Copy link
Author

FrancoisBeaune commented Apr 11, 2016

Deserializing a string to a string should not arbitrarily change the string. That really sounds like a bug to me, possibly a design bug. I'm not expecting breaking changes to be introduced to fix this, of course; I'm just interested in hearing the author's opinion on this. We found a workaround and that's good enough for us right now.

@antonovicha
Copy link

Way to represent a Date in Json is quite debatable because there is none in Json standard.
Here is SO question regarding this topic: http://stackoverflow.com/questions/10286204/the-right-json-date-format

Could be that this is why author choose 'use ISO8601 string as Date' behavior.

@FrancoisBeaune
Copy link
Author

@antonovicha That's completely beyond the point. I'm just deserializing a string into a string, I do not expect any changes to the string.

@FrancoisBeaune
Copy link
Author

I'm hitting this problem again, with a different scenario.

Given a string s with the value ['2016-05-10T13:51:20Z', '2016-05-10T13:51:20+00:00'], I need to check that it is indeed a valid JSON array, and I then need to retrieve the unmodified value of the individual items. Unfortunately, setting default settings does not appear to have any effect.

Here is the code:

JsonConvert.DefaultSettings = () => new JsonSerializerSettings
{
    DateParseHandling = DateParseHandling.None
};

var s = "['2016-05-10T13:51:20Z', '2016-05-10T13:51:20+00:00']";

var array = JArray.Parse(s);
foreach (var item in array)
{
    var itemValue = item.Value<string>();
    Console.WriteLine(itemValue);
}

This prints

05/10/2016 13:51:20
05/10/2016 13:51:20

instead of

2016-05-10T13:51:20Z
2016-05-10T13:51:20+00:00

@tangdf
Copy link

tangdf commented May 30, 2016

var s = "['2016-05-10T13:51:20Z', '2016-05-10T13:51:20+00:00']";
using (JsonReader jsonReader = new JsonTextReader(new StringReader(s))) {

        jsonReader.DateParseHandling = DateParseHandling.None;

       var array = JArray.Load(jsonReader);

        foreach (var item in array) {
            var itemValue = item.Value<string>();
               Console.WriteLine(itemValue);
          }
 }

This prints

2016-05-10T13:51:20Z
2016-05-10T13:51:20+00:00

@jetuser
Copy link

jetuser commented May 31, 2016

They've commented on this before and I still disagree with their assessment of "by design" for this scenario:

#865

@JamesNK
Copy link
Owner

JamesNK commented May 31, 2016

Is this the expected behavior?

Yes it is. Use DateParseHandling if you want to change it.

@JamesNK JamesNK closed this as completed May 31, 2016
@FrancoisBeaune
Copy link
Author

@JamesNK The point is that DateParseHandling has no effect on JToken.Parse(), only on JsonConvert.DeserializeObject().

@JamesNK
Copy link
Owner

JamesNK commented Jun 9, 2016

Set it on JsonTextReader that you pass to JToken.Load

@FrancoisBeaune
Copy link
Author

Alright, thanks.

@thomaslevesque
Copy link

Is this the expected behavior?

Yes it is. Use DateParseHandling if you want to change it.

@JamesNK could you explain the reasons behind this design? It seems pretty strange to me that a string value should arbitrarily be interpreted as a date just because it looks like a date...

@JamesNK
Copy link
Owner

JamesNK commented Aug 3, 2016

Because JSON doesn't have a date format.

@thomaslevesque
Copy link

thomaslevesque commented Aug 3, 2016

Because JSON doesn't have a date format.

Maybe I'm missing something, but I don't understand how this answers my question... or maybe my question was unclear. I'll try to clarify.

It doesn't matter which date format is used; the problem is that JSON.NET tries to parse the string as a date when we're just asking for the raw string. If I have something like this:

[
    {
        "id": 123,
        "name": "foo",
        "comment": "blah blah"
    },
    {
        "id": 456,
        "name": "bar",
        "comment": "2016-05-10T13:51:20Z"
    },
]

The comment field is just a string, and isn't supposed to be interpreted as a date, even though it happens to contain something that looks like a date in the second element. When I do this:

var array = JArray.Parse(json); // with the JSON sample above
string value = array[1]["comment"].Value<string>();

I expect value to be exactly "2016-05-10T13:51:20Z", not the result of parsing it as a date and reformatting it in whatever happens to be the current culture. Right now, this code gives me "05/10/2016 13:51:20" (on a machine with the French culture). The string shouldn't be parsed as a date unless we explicitly request a date (e.g. with .Value<DateTime>()).

So what I'm asking is, why is the default value for DateParseHandling DateTime, rather than None?

@thomaslevesque
Copy link

Whether a date is a string or not is determined when loading the JObject, not when converting it to a strongly typed object.

(I just saw this message in the other discussion; GitHub seems to be having trouble delivering notifications)

This might be the root cause of the problem. Since there is no standard date format in JSON, JSON.NET shouldn't even try to determine the type when loading the JObject... it should just assume it's a string, and leave the decision to treat it as a date to higher layers that actually know what it's supposed to be.

Anyway, I realize that's not something that can be changed easily, as it would likely affect many users. But is there a way to get the DateParseHandling.None behavior when using JToken.Parse? i.e. without reverting to JToken.ReadFrom and setting DateParseHandling on the reader?

@JamesNK
Copy link
Owner

JamesNK commented Aug 3, 2016

i.e. without reverting to JToken.ReadFrom and setting DateParseHandling on the reader?

No. If you want to customize that setting then use ReadFrom.

So what I'm asking is, why is the default value for DateParseHandling DateTime, rather than None?

Because I judged most people want a date rather than a string. And for people who don't then there is a setting to change it.

@FrancoisBeaune
Copy link
Author

While I really like Json.NET and thank you for the great work you're doing on that library, I deeply regret this design decision.

As I and other have been pointing out again and again, here and elsewhere, it really is surprising that what is supposed to be an opaque string gets mutated based on the culture.

Having to sprinkle

JsonConvert.DefaultSettings = () => new JsonSerializerSettings
{
    DateParseHandling = DateParseHandling.None
};

at the beginning of every project using Json.NET is annoying and extremely error-prone. Moreover there are cases that even this setting doesn't handle, requiring deep convolutions just to get a string out of a string...

@FrancoisBeaune
Copy link
Author

Moreover, this got to have performance implications, because for every string property of a JSON object, you need to find out if it's parsable as a date, even when one just want to get strings back!

@JamesNK
Copy link
Owner

JamesNK commented Aug 4, 2016

Sometimes you can't please everyone and in this case you're the person that isn't pleased.

Performance isn't an issue. A couple of simple checks on the string length, and certain chars exist at certain indexes eliminates 99% of non-ISO8601 strings instantly.

You can keep discussing this among yourselves but I like what it does, I have no plans to change it, and I would do it again if give the chance.

And if you don't like the default it is literally one line to change: DateParseHandling.None

@FrancoisBeaune
Copy link
Author

Fair enough. Regardless of this particular issue, thanks for the hard work and the high quality library.

@mjamesTX
Copy link

mjamesTX commented Aug 8, 2016

But if JSON.NET chooses to parse ISO8601 strings as DateTimes (which is fair given JSON has no date time format), shouldn't it also ToString them into the same format? That could eliminate the issue, making the fact that it was technically parsed into a DateTime in between irrelevant. I have a hard time accepting that this test should not pass:

JObject obj = JObject.Parse("{'time':'2016-08-01T03:04:05Z'}");
Assert.AreEqual("2016-08-01T03:04:05Z", obj.Value<string>("time")); // This will fail, regardless of the value of JsonConvert.DefaultSettings

@sehrgut
Copy link

sehrgut commented Sep 1, 2016

Because JSON doesn't have a date format.

That's the point. JSON doesn't have a date format. You claim this is a JSON parser, but when you force-enable incompatible extensions to the standard, you no longer have a JSON parser. You have a NewtonsoftObjectNotation parser.

@EliseAv
Copy link
Contributor

EliseAv commented Sep 1, 2016

but when you force-enable incompatible extensions to the standard, you no longer have a JSON parser. You have a NewtonsoftObjectNotation parser.

While I do agree with your point, it made me very happy to find out that it permits comments even though the standard format explicitly disallows them. An obvious-to-use strict mode would go a long way for people who need strict JSON conformity.

@Cobaltikus
Copy link

I have been experiencing the same problem: a string being changed just because it happens to look like a date. This could easily be resolved by adding a new method in Json.NET to get the raw string value of a given property. As far as performance goes, it may not have a noticeable impact on deserializing a single object, but put it in a loop and now every action matters. The larger your dataset, the larger the performance impact. I hope to see a GetRawStringValue method in the future. In the meantime thank you for pointing out the workaround.

@sehrgut
Copy link

sehrgut commented Nov 4, 2016

@ekevoo An obvious-to-use non-strict mode would be the way to go, since many times this library is called from within compiled assemblies. I can't modify every third-party library I use just because I've upgraded the version of Newtonsoft.Json attached to a project, to make them all opt-in to strict mode.

The users who want non-standard behaviour are the ones who have the responsibility to opt-in.

@EliseAv
Copy link
Contributor

EliseAv commented Nov 4, 2016 via email

@sehrgut
Copy link

sehrgut commented Nov 5, 2016

The date parsing is a new feature. This operated correctly for most of the
time it was BECOMING such a widely-used library, and was introduced as a
breaking change very recently. Search the closed issues.

On Friday, November 4, 2016, Ekevoo notifications@github.com wrote:

@sehrgut That little horse has left the barn. This is a decision to make at
library inception time, not to a years-old library that is literally THE
most popular library of the runtime (if NuGet is to be believed).

Besides, it's not like strict mode is ever default. Look at the defunct
Perl (use strict;) or lively Javascript ('use strict'; in es5).


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#862 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABIFLIZPgTIibEQDWQwSRt0BP_CtGA1Oks5q672rgaJpZM4H7thy
.

@sungam3r
Copy link

👍 will see, will see

@stmax82
Copy link

stmax82 commented Jan 24, 2022

Following workaround seems to work fine for simple cases and I actually prefer it over setting DateParseHandling to DateParseHandling.None because I can't forget doing it and it won't haunt me in a year:

open System.Text.Json

let j = JsonDocument.Parse("""{ "Timestamp": "2022-01-01T00:00:00+00:00" }""")

j.RootElement.GetProperty("Timestamp").GetString()

// val it : string = "2022-01-01T00:00:00+00:00"
// yay

@codermrrob
Copy link

codermrrob commented Jan 25, 2022

This date issue continues to needlessly occupy time - despite regular warnings to project developers until they actually experience it they never remember and consequently again this issue has negatively impacted a current project. As much as this library has been well used and appreciated by us over the years it is time to move away and we will be mandating usage of System.Text.Json in all new projects. The last hurdle for this decision was functionality supported by Newtonsoft that was not supported in System.Text.Json. We will be using the json-everything library to provide that functionality as we need or other alternatives if need be.

@sam-s4s
Copy link

sam-s4s commented Aug 3, 2022

Just ran into this issue. It's not actually a bug, it's an intentional feature built into the code. Given the author knows about the problem and has refused to fix it, it can and should be considered malicious.

Interpreting any string as a DateTime if it matches - even though JSON does not even have a DateTime data type, and the target class has requested a string - is insanity.

If deserializing into a target class that has a field as a DateTime, then yes this is a nice feature. Literally every other scenario, it is not only dangerous, but almost always will result in incorrect behaviour, often breaking peoples' code.

a) If deserializing into a string, then no matter what the settings are, it should always leave the property alone.
b) If deserializing into a DateTime, then by default it could convert to a DateTime, this would make sense, as a handy feature.
c) If deserializing into an unknown (ie. JObject), then by default it should leave as a string, as it doesn't know what the user wants. But then a setting to change to "try to parse when possible" is fine. But for safety this should be done on a per-property basis.

I can understad the argument that (c) defaults to trying to parse DateTimes where possible, at least this makes some sense, whilst being a bit dangerous.

The most important thing is that (a) preserves a string to string deserialization, and this currently is broken, which is highly dangerous. There is absolutely no argument for a string to string conversion to convert to a DateTime as an intermediate type. It is never, ever a thing that anyone would want, in any scenario, and will always cause a problem.

@josegomez
Copy link

Just ran into this issue in a commercial ERP software using the library caused is a lot of heartache. This should not be default behavior.

@bagginsfrodo
Copy link

bagginsfrodo commented Jul 21, 2023

Just ran into this issue in a commercial ERP software using the library caused is a lot of heartache. This should not be default behavior.

I don't know @josegomez , we had a lot of fun tracking it down 🤣

@hasokeric
Copy link

hasokeric commented Jul 24, 2023

Atleast it can be apparently turned off?

@bvds
Copy link

bvds commented Nov 17, 2023

Please please please fix this bug!

@sungam3r
Copy link

As @JamesNK said it's not a bug, it's a feature by design.

@sehrgut
Copy link

sehrgut commented Nov 24, 2023

As @JamesNK said it's not a bug, it's a feature by design.

That doesn't make it "not a bug". It's a bug by design.

@sungam3r
Copy link

I know.

@Eesof
Copy link

Eesof commented Nov 29, 2023

Feature can be erhm skipped by modifying source data before parsing it.. very very dirty solution but works well for correct format 2023-11-28T11:00:00Z keeping it as it is.

Modify json STRING before parsing it. Changing - to ...

string modifiedJsonData = Regex.Replace(jsonData, @"(\d{4})-(\d{2})-(\d{2})T", "$1...$2...$3T");
var measurement = JObject.Parse(modifiedJsonData);

Parsed json date doesn't detect it's a dateformat and stays as text string

string endresult = customDataArray.ToString();
endresult = Regex.Replace(endresult, @"(\d{4})\.\.\.(\d{2})\.\.\.(\d{2})T", "$1-$2-$3T");

Endresult will be with correct original dateformat.

@stamminator
Copy link

@Eesof This looks dangerous. Unless your regex rules exactly match what Newtonsoft.Json is using to detect datetime strings, there are going to be edge cases that break. Much safer to just use the DateParseHandling.None solution shown in the original question.

@Eesof
Copy link

Eesof commented Nov 29, 2023

@Eesof This looks dangerous. Unless your regex rules exactly match what Newtonsoft.Json is using to detect datetime strings, there are going to be edge cases that break. Much safer to just use the DateParseHandling.None solution shown in the original question.

I agree completely but in this case dirty solution is acceptable in this particular solution where data never changes. This was mostly just to point out framework can't break date format if it doesn't detect date format.

@bartelink
Copy link

Randall has posted a decent summary https://xkcd.com/2881/

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

No branches or pull requests