kipcole9 / money_sql

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

Fix Ecto parameterized types to work with embeds #32

Closed redrabbit closed 1 year ago

redrabbit commented 1 year ago

This fixes #31.

Also, I think it is safer to return :dump for both Money.Ecto.Map.Type and Money.Ecto.Composite.Type. While composite types will not work with embeds, nothing prevents from using it:

If a programmer uses a composite type as an embedded schema field, it will not raise and behave like it is a :map. The programmer might think it works but he actually introduce a very hard to find bug because now loading the field is dependent on the used CLDR locale (bypassing parsing & checks made in load/3).

With embed_as/3 returning :dump for both, the programmer will get an error upfront and know that something is wrong.

redrabbit commented 1 year ago

To better understand what was going wrong and how embed_as/3 behaves, you can try:

1) Switching embed_as/3 return value back to :self again (test will fail because it bypasses load/3). 2) Switching the embedded :revenue field to Money.Ecto.Composite.Type and see that load/3 returns :error. 3) Both above to see that nothing prevents somebody using a composite in an embedded schema.

kipcole9 commented 1 year ago

Truly appreciated, thank you. I remember when implementing this that it wasn't clear to me the difference between :self and :dump. I guess now I know. I may revisit the Ecto docs on this and see if there is room for improvement.

🥳🥳🥳🥳🥳

kipcole9 commented 1 year ago

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

Bug Fixes