kipcole9 / money_sql

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

function Money.Ecto.Composite.Type.type/0 is undefined or private when using Ecto.Query.Api.type/2 #16

Closed mjquinlan2000 closed 3 years ago

mjquinlan2000 commented 3 years ago

Versions

Elixir: 1.11.3 (compiled with Erlang/OTP 23) Ecto: 3.5.8 Erlang: 23.2.5 ex_money_sql: 1.4.3 ex_money: 5.5.1

The issue

I'm running into an issue where I am selecting a column and casting it to a composite money type with the type/2 function from Ecto.Query.API. Example:

MyModel
|> select([m], %{money: type(m.money, Money.Ecto.Composite.Type)})

And I get this error:

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

      * type/1

The Ecto docs still specify type/0 as a valid callback so it's unclear to me why this was removed from Money.Ecto.Composite.Type. Any help would be greatly appreciated.

Thanks!

mjquinlan2000 commented 3 years ago

I see you've been talking to Jose about this here https://github.com/elixir-ecto/ecto/issues/3558

Closing.

mjquinlan2000 commented 3 years ago

Also mentioned here #12

kipcole9 commented 3 years ago

@mjquinlan2000, thanks for raising the issue. As you found, this is fixed on Ecto master but not yet released on Hex. Nevertheless, you remind me I need to update documentation to note that using the type/2 macro is a different for a parameterised type versus one that is not.

kipcole9 commented 3 years ago

Just looking at your query, are you sure you need to cast the type there? Usually it only needs to be cast when there are aggregate functions involved. Otherwise its type detection is pretty good, especially when using a schema as you are. ie does the following work?

MyModel
|> select([m], %{money: m.money})
mjquinlan2000 commented 3 years ago

The actual query is in production code which is both very complex and proprietary so I can't really share it. I reduced it to the simplest query that would produce the error. I am just going to wait until type(t, {:parametarized, Money.Ecto.Composite.Type, []}) is out on Ecto

kipcole9 commented 3 years ago

Ah, ok, thanks for the update. Make sense.

I have just added Money.Ecto.Composite.Type.cast_type/{0, 1} make the transition as easy as possible but this is clearly a breaking change in Ecto that I wasn't aware of. I would have made this a new type if I had known and I will send a PR for the ecto docs to make that clearer for others.

Any thoughts on how to handle the situation? I should have made this a major release but thats too late now.

Would creating a new type that is non-parameterised (ie same as the old behaviour) help you in this case?

mjquinlan2000 commented 3 years ago

To be honest, the proposal on elixir-ecto/ecto#3558 (specifying parameterized in the type/2 call) seems like a messy contract.

In my opinion, Ecto.ParametarizedType should enforce a type/0 callback as well or figure out how to pass empty opts when no {:parameterized, Type, opts} tuple is passed to type/2

In addition, I would get rid of the type/2 implementation of using parameterized types and create a new parameterized_type/3 function in Ecto.Query.API that looks something like this parameterized_type(t, Money.Ecto.Composite.Type, opts) so that the contract is explicitly defined by the function name and not the pattern of the arguments.

However, I don't think there's anything to be done in this library so I can just wait for a new version of Ecto