kipcole9 / money_sql

Money functions for the serialization of a money data type in Elixir
Other
28 stars 18 forks source link

Treat persisted NaN and Inf values as errors whe loading #34

Closed dhedlund closed 1 year ago

dhedlund commented 1 year ago

Before :ex_money 5.12.2, it was possible to persist money values that are no longer valid. When trying to load these values with a version of :ex_money > 5.12.2, the field gets set to a value like:

%MySchema{
  ...
  my_field: {:error, {Money.InvalidAmountError, "Invalid money amount. Found Decimal.new(\"NaN\")."}},
  ...
}

I would expect loading to either fail, or for a %Money{} struct to still be returned that bypasses the casting performed by Money.new/_. This PR handles the Money.InvalidAmountError the same way as invalid currencies.

I originally started down the path of allowing an :invalid_amount option to be specified as a parameter on the ecto schema field that would allow controlling what to do such as :zero, :cast, and :raise. Eventually, I decided it was probably best for whoever has any bad data to see the exception raised by the load error and just fix the data in the database; making it an option means it has to be supported for a long time, and a very small number of people are likely to have these values.

kipcole9 commented 1 year ago

@dhedlund great catch and thank you very much for the PR. Sorry for the delay, just landed from a long flight. Will merge and publish a new version ASAP.

kipcole9 commented 1 year ago

🥳🥳🥳🥳🥳

kipcole9 commented 1 year ago

I agree with your opinion that it's better to raise an exception when bad data is detected. It's a goal to never represent an invalid currency and amount and your PR reflects that intent. Its not great that older versions permitted invalid data but that's not a good enough reason to perpetuate that bad data.

kipcole9 commented 1 year ago

I've published ex_money_sql version 1.9.3 with the following changelog entry:

Bug Fixes

Thanks again for the PR and support, really appreciate it.