kipcole9 / money_sql

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

Possible bug in ecto? Asking here first for clarification #12

Closed doughsay closed 3 years ago

doughsay commented 3 years ago

Hi there,

I just tried upgrading to ex_money_sql ~> 1.4 and I have compilation errors in my application now. I think what's going on is that the Ecto type function from the Query API doesn't support parameterized types, but I'm not quite sure... If this is an issue with ecto itself, I can open an issue with them as well.

I've created a minimal repo to demonstrate the issue: https://github.com/doughsay/ecto_money The details are in the readme, but I'll duplicate them here for convenience.

# insert some moneys:

%EctoMoney.Money{amount: Money.new("100.00", :USD)} |> EctoMoney.Repo.insert()
%EctoMoney.Money{amount: Money.new("50.00", :USD)} |> EctoMoney.Repo.insert()
%EctoMoney.Money{amount: Money.new("75.00", :USD)} |> EctoMoney.Repo.insert()

# run a custom sum query and cast the response to a Money struct:

import Ecto.Query

query =
  from(
    money in "moneys",
    select: %{
      total: type(sum(money.amount), EctoMoneyType),
    }
  )

EctoMoney.Repo.all(query)

Expected:

This is the response you get when using ex_money_sql == 1.3.1

[%{total: #Money<:USD, 225.00>}]

Actual:

Using ex_money_sql ~> 1.4, with the new parameterized type, we get a compilation error:

** (UndefinedFunctionError) function Money.Ecto.Composite.Type.type/0 is undefined or private. Did you mean one of:

      * type/1

    (ex_money_sql 1.4.2) Money.Ecto.Composite.Type.type()
    (ecto 3.5.7) lib/ecto/type.ex:421: Ecto.Type.match?/2
    (ecto 3.5.7) lib/ecto/query/builder.ex:646: Ecto.Query.Builder.assert_type!/3
    (ecto 3.5.7) lib/ecto/query/builder.ex:415: Ecto.Query.Builder.escape/5
    (ecto 3.5.7) lib/ecto/query/builder.ex:462: Ecto.Query.Builder.escape_with_type/5
    (ecto 3.5.7) lib/ecto/query/builder/select.ex:111: anonymous fn/4 in Ecto.Query.Builder.Select.escape_pairs/4
    (elixir 1.11.3) lib/enum.ex:1533: Enum."-map_reduce/3-lists^mapfoldl/2-0-"/3
    (ecto 3.5.7) lib/ecto/query/builder/select.ex:82: Ecto.Query.Builder.Select.escape/4

Workarounds

You can remove the casting, but then you get back a tuple instead of a Money struct.

query =
  from(
    money in "moneys",
    select: %{
      total: sum(money.amount),
    }
  )

EctoMoney.Repo.all(query)

Results in:

[%{total: {"USD", #Decimal<225.00>}}]
kipcole9 commented 3 years ago

Thats definitely curious and I wonder if it is indeed an Ecto bug. I'll do some digging. In the meantime I can workaround this since I need to push an update today for issue #14.

Thanks much for the issue and sorry for the inconvenience. Moving to a parameterised type has proved more problematic on edge cases than expected.

kipcole9 commented 3 years ago

Turns out I can't work around this. Adding type/0 to the composite type seems to make Ecto think its a non-parameterised type and therefore other bad things happen.

kipcole9 commented 3 years ago

Confirmed it was an Ecto bug which is now fixed on the main branch. I confirmed is works in my tests.

I expect this will be fixed when Ecto 3.6 comes out. Until then, if there is no other way, I recommend you use ecto directly from GitHub using:

# Your query should look like this
# init_options is either `[]` or a keyword list of options
# you would pass to the `:field` definition on an ecto
# schema
    query =
      from(
        organization in "organizations",
        select: %{
          total: type(sum(organization.payroll),
            ^Ecto.ParameterizedType.init(Money.Ecto.Composite.Type, init_options)
          )
        }
      )

# Which basically is the same as:
    query =
      from(
        organization in "organizations",
        select: %{
          total: type(sum(organization.payroll),
            ^{:parameterized, Money.Ecto.Composite.Type, []}
          )
        }
      )

# mix.exs
def deps do
  ...
  {:ecto, github: "elixir-echo/ecto", override: true},
  ...
end

Closing this issue for now, let me know if there is anything else I can help with.