Fix Money type deserialization

[oskardudycz]

Description

Currently deserialization of Money type for Json fails for simple scenario as:

• { amount: 200, currency: 'EUR' } - passing amount value as numeric (istead string eg. 200),
• { currency: 'EUR', amount: 200 } - traversing amount and currency in json
• { amount: '200' } - sending only amount
  All of those examples are valid jsons and should be able to serialize.
  Also all of those scenarios failes with meaningless exception index out of range that it makes it hard to debug.

Type of change

Added possibility to pass numeric amount value to MoneyJavaScriptConverter by adding:
var amount = decimal.Parse(Convert.ToString(dictionary["amount"], CultureInfo.InvariantCulture), CultureInfo.InvariantCulture);

instead

decimal amount = decimal.Parse((string)dictionary["amount"], CultureInfo.InvariantCulture);

Changed also the way of selecting values from indexes to the names.
Added also possibility to not pass currency in json.

[x] Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Added new set of tests

• GivenIWantToSerializeMoneyWithJavaScriptConverter
• GivenIWantToDeserializeMoneyWithJavaScriptSerializer
• GivenIWantToDeserializeMoneyWithJavaScriptConverter

Steps to Test or Reproduce

Checkout commit c8e30e71d0018e4b0c85515a1731da6c38c89877 from my branch and run MoneySerializableTests

[RemyDuijkeren]

Thanks for solving this. Nice addition. Great that you have also added tests!

One thing I don't agree with is to allow JSON with only amount and no currency. The definition of Money is that it has a currency. Deserializing the same json, by different people/places could result into different money, which is not acceptable.

I see you used FirstOrDefault() to read the values. It is now allowed to have multiple amount and/or currencies in the JSON, which can cause unpredictable behavior. I like it to fail if multiple amount and/or currencies or found, by for example using SingleOrDefault() or a more explicit exception.

[oskardudycz]

Remy, thanks for good words :)

Regarding the deserialisation without currency, I think that it would be good to keep it consistent. If there is support (constructor) without the currency then imho it would be good to allow to allow similar deserialisation. I understand that sending JSON without currency is a bit meaningless, but I think that it should be consistent with C# api. But I won’t be forcing that ;) what do you think?

Regarding FirstOrDefault - good catch! Will change that.

[RemyDuijkeren]

It is important to be consistent, but (de)serialization serves a very different purpose. Serialization is all about storing and transporting already defined money, but the constructor is used for creating new money.

The constructor allows creating money more easily, by using the current culture information, but it expects from the caller that it knows this behavior. Once it is created the currency is known and fixed.

If we allow serialization without currency, than I can store money on an European server without a currency (but I assume it is EUR). If I now read the money from a server in USA, my money is now magically become USD ;-).

[oskardudycz]

@remyvd fair enough ;) I brought back requirement for json to have both amount and currency.
Updated finding json parameters from FirstOrDefault to SingleOrDefault.
Added also additional tests for negative deserialization scenarios.

Hope that now it's fine for you :)