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

v10: De-serialization of Decimal fails when there is no leading zero #1510

Open
Richard-Payne opened this issue Nov 21, 2017 · 13 comments
Open

Comments

@Richard-Payne
Copy link

Source/destination types

Source: string
Dest: Decimal

Source/destination JSON

source: ".1"
dest: 0.1

Expected behavior

Expect num = 0.1

Actual behavior

Newtonsoft.Json.JsonReaderException: Input string '.1' is not a valid decimal

Steps to reproduce

var num = JsonConvert.DeserializeObject(".1", typeof(Decimal));

Works fine with v9 but fails in v10.

@JonHanna
Copy link
Contributor

.1 is not a valid JSON representation of a number. The period must be preceded by either a single 0 or by a digit in the range 19 followed by zero or more digits.

@Richard-Payne
Copy link
Author

ok, but it worked in the prior versions. I entirely understand making the serializer output correct JSON but limiting the deserializer in this way means that any stored data is now broken.

Also, incidentally, if you change Decimal to Double then it still works fine with ".1"

@JonHanna
Copy link
Contributor

Ahh, if it used to work then it should probably accept such invalid input as long as it doesn't cause it to somehow do the wrong thing on valid input.

@TylerBrinkley
Copy link
Contributor

See here for a discussion of this when the optimized double parsing was in the works for Json.NET 10.0 which was then rolled back due to double precision issues when parsing. The same parsing logic was used in the optimized decimal parsing PR for Json.NET 10.0 which has not been rolled back.

@AlexisColes
Copy link

Just to be clear, we are sacrificing backwards compatibility for speed with v10 and there is no plan to make this work as it used to in v9?

So we need to to change our code to deal with this before we upgrade to 10+

@TylerBrinkley
Copy link
Contributor

TylerBrinkley commented Nov 21, 2017

To quote @JamesNK

I want the new method to reject those as invalid. The only place that this parser will be used is JsonTextReader and the spec requires that doubles follow an exact format. That Json.NET accepted them before was a bug.

The parser could easily be updated to accept leading periods and maintain the performance improvements but it was considered a bug by James that it supported parsing those values before. Yes, you'll need to change your code to deal with this before you upgrade to 10+ perhaps by just reading in the json and immediately writing it back out which should fix the formatting.

@Richard-Payne
Copy link
Author

It's not just code though. Any serialized, persisted data will now by broken too.

Yes, you'll need to change your code to deal with this before you upgrade to 10+ perhaps by just reading in the json and immediately writing it back out which should fix the formatting.

So every application out there has to be slowed down with unnecessary writes just so the library can remain "pure". And that's not to mention the huge amount of work involved in implementing those write backs and testing to ensure that we've caught them all.

I agree with James, it is a bug, but breaking behaviour which has been around for a long time is bad. If there were a significant performance hit in supporting the "wrong" way then I could maybe see it but if, as you say, support is inexpensive performance wise then why break anything?

@TylerBrinkley
Copy link
Contributor

I agree with you but that wasn't my call.

@JamesNK
Copy link
Owner

JamesNK commented Nov 21, 2017

Hmmm, I'm torn on one vs the other. You really shouldn't have number values that start with a decimal point - they won't work with any other JSON framework - but on the other hand it isn't difficult to parse those values.

@Richard-Payne
Copy link
Author

Are there any other frameworks these days? Your's is pretty much the de facto standard. ;-)

@AlexisColes
Copy link

Maybe we could have strict setting that we could set to false.

@wallymathieu
Copy link

wallymathieu commented Sep 25, 2018

@Richard-Payne there are a couple of JSON frameworks out there.
I've used the following (besides Newtonsoft.Json):

And there are additional new Json implementations:

@Richard-Payne
Copy link
Author

@JamesNK did this ever get looked at?

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

6 participants