elixir-ecto / ecto

A toolkit for data mapping and language integrated query.
https://hexdocs.pm/ecto
Apache License 2.0
6.14k stars 1.43k forks source link

Ecto.Type.match?/2 calls schema_type.type which does not exist for parameterised types #3558

Closed kipcole9 closed 3 years ago

kipcole9 commented 3 years ago

Environment

Current behavior

As reported by @emaiax on the ex_money_sql repo, using the type/2 macro in a schema-less query with a custom Ecto.ParameterizedType results in a call to Ecto.Type.match?/2 which in turns calls schema_type.type/0 which for a parameterised type does not exist, thereby raising an exception.

Example

  # money_sql/test/db_test.exs
  test "aggregate from a keyword query using a schemaLESS query" do
    m = Money.new(:USD, 100)
    {:ok, _} = Repo.insert(%Organization{payroll: m})
    {:ok, _} = Repo.insert(%Organization{payroll: m})
    {:ok, _} = Repo.insert(%Organization{payroll: m})

    query =
      from(
        organization in "organizations",
        select: %{
          total: type(sum(organization.payroll), Money.Ecto.Composite.Type)
        }
      )

    assert Repo.all(query) == [%{total: Money.new(:USD, 300)}]
  end

Reported exception

== Compilation error in file test/db_test.exs ==
** (UndefinedFunctionError) function Money.Ecto.Composite.Type.type/0 is undefined or private. Did you mean one of:

      * type/1

    (ex_money_sql 1.4.3) 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
    (ecto 3.5.7) lib/ecto/query/builder/select.ex:189: Ecto.Query.Builder.Select.build/5
    (ecto 3.5.7) expanding macro: Ecto.Query.select/3
    test/db_test.exs:83: Money.DB.Test."test aggregate from a keyword query using a schemaLESS query"/1
    (ecto 3.5.7) expanding macro: Ecto.Query.from/2
    test/db_test.exs:83: Money.DB.Test."test aggregate from a keyword query using a schemaLESS query"/1
    (elixir 1.11.3) lib/kernel/parallel_compiler.ex:416: Kernel.ParallelCompiler.require_file/2
    (elixir 1.11.3) lib/kernel/parallel_compiler.ex:316: anonymous fn/4 in Kernel.ParallelCompiler.spawn_workers/7

Money.Ecto.Composite.Type

The Money.Ecto.Composite.Type implements type/1 as required. Adding type/0 to satisfy match?/2 seems to cause Ecto to treat the type as a non-parameterised type so I don't think thats an option in this case.

To reproduce

Expected behavior

That match?/2 should work with parameterised types and schema less queries. Perhaps schema_type.type should also be schema_type.type()?

josevalim commented 3 years ago

Thanks @kipcole9 for the report. This is a bit more complicated because you can't refer to a composite type like that. A composite type is always instantiated, so you need to refer to a proper instance.

Have you tried this approach:


    query =
      from(
        organization in "organizations",
        select: %{
          total: type(sum(organization.payroll), organization.payroll)
        }
      )

You should be able to refer to an existing field to use its type.

kipcole9 commented 3 years ago

@josevalim Unfortunately while that compile ok, it produces a query that attempts to cast to a type of any which is invalid. The reason is that this is a schemaless query meaning that it has no type information that can be referred to from another field. In this case there is no organization.payroll since there is no schema module.

Revised query

    query =
      from(
        organization in "organizations",
        select: %{
          total: type(sum(organization.payroll), organization.payroll)
        }
      )

Error result

  1) test aggregate from a keyword query using a schemaLESS query (Money.DB.Test)
     test/db_test.exs:76
     ** (Postgrex.Error) ERROR 42601 (syntax_error) syntax error at or near "any"

         query: SELECT sum(o0."payroll")::any FROM "organizations" AS o0
     code: assert Repo.all(query) == [%{total: Money.new(:USD, 300)}]
     stacktrace:
       (ecto_sql 3.5.4) lib/ecto/adapters/sql.ex:751: Ecto.Adapters.SQL.raise_sql_call_error/1
       (ecto_sql 3.5.4) lib/ecto/adapters/sql.ex:684: Ecto.Adapters.SQL.execute/5
       (ecto 3.5.7) lib/ecto/repo/queryable.ex:229: Ecto.Repo.Queryable.execute/4
       (ecto 3.5.7) lib/ecto/repo/queryable.ex:17: Ecto.Repo.Queryable.all/3
       test/db_test.exs:90: (test)

Works with a schema

A test using a schema module passes, For example, a test with the following passes.

    query =
      from(
        organization in Organization,
        select: %{
          total: type(sum(organization.payroll), organization.payroll)
        }
      )
josevalim commented 3 years ago

I see. Yes, you will need to give it either a schema field or an instantiated parameterized schema type which you can do by calling this: https://github.com/elixir-ecto/ecto/commit/9dea16ac027c28bc275504f386d1efc7f91d2cdc

kipcole9 commented 3 years ago

Unfortunately that recommendation does not work since that commit is not in a release published on hex, and using the return tuple explicitly is also raising an exception.

  1. The commit 9dea16a is not present in Ecto 3.5.[5, 6, 7, 8] although I can see it on master branch. Therefore Ecto.ParameterizedType.init/2 does not exist.
  2. Handcrafting the return expected from Ecto.ParameterizedType.init/2 raises. It appears that the type/2 macro does not support the 3-tuple.
  3. Same issue on master branch.

Revised query using the recommendation

    query =
      from(
        organization in "organizations",
        select: %{
          total: type(sum(organization.payroll),
            {:parameterized, Money.Ecto.Composite.Type, []}
          )
        }
      )

type/2 does not support the 3-tuple from Ecto.ParameterizedType.init/2

** (Ecto.Query.CompileError) type/2 expects an alias, atom or source.field as second argument, got: `{:parameterized, Money.Ecto.Composite.Type, []}`
    (ecto 3.5.7) expanding macro: Ecto.Query.select/3
    test/db_test.exs:83: Money.DB.Test."test aggregate from a keyword query using a schemaLESS query"/1
    (ecto 3.5.7) expanding macro: Ecto.Query.from/2
    test/db_test.exs:83: Money.DB.Test."test aggregate from a keyword query using a schemaLESS query"/1
    (elixir 1.11.3) lib/kernel/parallel_compiler.ex:416: Kernel.ParallelCompiler.require_file/2
    (elixir 1.11.3) lib/kernel/parallel_compiler.ex:316: anonymous fn/4 in Kernel.ParallelCompiler.spawn_workers/7

Current Environment

iex> Ecto.ParameterizedType.module_info
[
  module: Ecto.ParameterizedType,
  exports: [
    __info__: 1,
    "MACRO-__using__": 2,
    behaviour_info: 1,
    module_info: 0,
    module_info: 1
  ],

Config: {:ecto, "~> 3.5"}
Locked version: 3.5.8
Releases: 3.5.8, 3.5.7, 3.5.6, 3.5.5, 3.5.4, 3.5.3, 3.5.2, 3.5.1, ...

Licenses: Apache-2.0
Links:
  GitHub: https://github.com/elixir-ecto/ecto
kipcole9 commented 3 years ago

My conclusions:

  1. Parameterised types are not supported in schemaless queries in the type/2 macro. If true, updated docs are required
  2. Parameterised types are supported in schemaless queries but not working today. If true, the type/2 macro needs updating to support the 3-tuple returned from Ecto.ParameterizedType.init/2 when it is released
  3. The match?/2 macro could emit a better message if passed a parameterised type module. If true then a PR would be useful.

I can tackle (1) and (3), not sure I understand the internals enough to tackle (2).

josevalim commented 3 years ago

@kipcole9 does this work?

total: type(sum(organization.payroll), ^{:parameterized, Money.Ecto.Composite.Type, []})
kipcole9 commented 3 years ago

Unfortunately not:

     ** (ArgumentError) unsupported type `{:{}, [line: 87], [:parameterized, {:__aliases__, [counter: {Money.DB.Test, 72}, line: 87], [:Money, :Ecto, :Composite, :Type]}, []]}`. The type can either be an atom, a string or a tuple of the form `{:map, t}` or `{:array, t}` where `t` itself follows the same conditions.
     code: assert Repo.all(query) == [%{total: Money.new(:USD, 300)}]
     stacktrace:
       (ecto_sql 3.5.4) lib/ecto/adapters/postgres/connection.ex:1286: Ecto.Adapters.Postgres.Connection.ecto_to_db/1
       (ecto_sql 3.5.4) lib/ecto/adapters/postgres/connection.ex:698: Ecto.Adapters.Postgres.Connection.expr/3
       (ecto_sql 3.5.4) lib/ecto/adapters/postgres/connection.ex:1238: Ecto.Adapters.Postgres.Connection.intersperse_map/4
       (ecto_sql 3.5.4) lib/ecto/adapters/postgres/connection.ex:319: Ecto.Adapters.Postgres.Connection.select/3
       (ecto_sql 3.5.4) lib/ecto/adapters/postgres/connection.ex:113: Ecto.Adapters.Postgres.Connection.all/2
josevalim commented 3 years ago

Fixed on master. Best option is to use master for now. :)

kipcole9 commented 3 years ago

Thank you José, appreciate the support. Confirmed all tests on my side passing too. I'll send a PR to update the documentation for the type/2 soon.