Skip to content

Bug: Serialization fails on Deserialize<Money> (JavaScriptSerializer) #23

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

Closed
itssimple opened this issue Mar 16, 2015 · 6 comments
Closed
Assignees
Labels
Milestone

Comments

@itssimple
Copy link

When serializing/deserializing the Money-class, it throws an exception when it tries to set CurrencySymbol.

at System.Globalization.NumberFormatInfo.set_CurrencySymbol(String value)
at NodaMoney.Money.ConvertToString(String format, IFormatProvider formatProvider)
at NodaMoney.Money.ToString()

There's also a problem with formatting, the currency symbol for USD is appended last, when it's supposed to be first.

var c = new Money(100m, "USD");
Console.WriteLine(c.ToString()); // Generates 100,00 $

Serialized output from JavaScriptSerializer

{ 
  "Amount" : 100,
  "Currency" : { 
      "Code" : "USD",
      "DecimalDigits" : 2,
      "EnglishName" : "United States dollar",
      "MajorUnit" : 1,
      "MinorUnit" : 0.01,
      "Number" : "840",
      "Sign" : "$"
    }
}
@RemyDuijkeren
Copy link
Owner

We have a lot of unit-tests, but for serialization there weren't any yet (was on the todo list ;-)). Thanks for the report, will fix it asap and create unit-tests for them.

Concerning the ToString() method. It will format Money in the current culture. If you want to format it in the culture of United States, then you should do the explicit (see https://github.com/remyvd/NodaMoney, the formatting area for examples.).

If you still think its wrong, what is the culture you are using and what do you expect in that culture? We are using the formatting that CultureInfo in .NET gives us.

@RemyDuijkeren RemyDuijkeren self-assigned this Mar 17, 2015
@RemyDuijkeren RemyDuijkeren added this to the v0.3.0 milestone Mar 17, 2015
@itssimple
Copy link
Author

Yeah, about the CultureInfo, it's true that I don't have en-US as default. :)
I was just staring blindly at your example where is said it used en-US implicitly from the current culture, only I missed "current".

RemyDuijkeren pushed a commit that referenced this issue Mar 30, 2015
@RemyDuijkeren
Copy link
Owner

Added unit-test and support for the following serializers: XmlSerializer, DataContractSerializer, DataContractJsonSerializer, Newtonsoft Json.NET & JavaScriptSerializer

NodaMoney has now a dependency on Json.Net, to support it. It's used a lot, so I took the risk to have a dependency on it. If needed this could be a separated library.

JavaScriptSerializer can only be supported by a new library NodaMoney.Serializer.AspNet. You can download the code and compile this. I will create a nuget package in the upcoming release.

You can use it by registering it, like the example below:

// create a new serializer and tell it about the NodaMoney converter.
serializer = new JavaScriptSerializer();
serializer.RegisterConverters(new JavaScriptConverter[] { new MoneyJavaScriptConverter() });

@mattjohnsonpint
Copy link

FYI - in Noda Time, we split out the JSON serializer into a separate assembly with its own nuget package. XML serialization was left in the main library.

Really though, I wouldn't bend over backwards for JavaScriptSerializer or DataContractJsonSerializer. Almost everything uses JSON.Net now. (IMHO)

@RemyDuijkeren
Copy link
Owner

Your right about for JavaScriptSerializer or DataContractJsonSerializer. Creating an own nuget package is maybe to much effort for this. it's only one class
MoneyJavaScriptConverter.cs at the moment, that people could copy or create themselves and include in their own projects, if they really need it.

I was doubting to have the Json.NET serialzier in a seperate assembly and it's own nuget package, but as you already mentioned, it's used everywhere so I took the risk by depending on it. I think people using RavenDB and MongoDB would benefit from this (need to check this).

It's not yet version 1.0, so I can pull it apart if it's not the right decision. Do you know of any other downsides, apart from a dependency, that made Noda Time to put it in a separate assambly?

@mattjohnsonpint
Copy link

The dependency was the biggest thing. There are plenty of NodaTime consumers that only use it in their business logic, and thus don't need serialization. We thought it was safe enough to support XML though, since we only have to take a dependency on System.Xml from the framework.

WRT RavenDB, it was important to write a custom implementation so that the Noda Time types could be used in entities, indexes, and queries. I haven't done a MongoDB one yet, but it's on my list.

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

No branches or pull requests

3 participants