select_merge + map + join(:left) fails by returning nil instead of map

[halostatue]

My colleague @iog-kc originally posted this on Elixir Forum yesterday, but the more I think about it, the more that it feels like there’s something not quite right with Ecto’s handling of this. I’d be happy to tackle this issue if it is an Ecto issue, but I don’t know where to begin.

We’re on Elixir 1.8.2, PostgreSQL 9.6, Postgrex 0.15, Ecto 3.2 (we will be on Ecto 3.3, etc. soon). We’re testing this on MacOS but expect this to happen on any OS. The code example included is a deliberately simplified version of the problem; the fields in question are dynamic as we’re building a translations strategy where different tables may have different translatable fields and we will be adding virtual fields to the parent schema for this to work.

Current behavior

We have a query that does a left outer join on a child table and then uses select_merge to merge dynamically selected fields from the child row into the parent. However, when there is no child row, the call to map fails with cannot merge because the right side is not a map, got: nil. Is there a way to get the call to map to use an empty map for merging when there isn’t a row to merge?

As a simplified example (assume that Product exists and has virtual fields for the translated fields in Product.Translation), we have the function below where the translated_fields are provided contextually (for a product list, you might just want the name, but on a product display page, you want the description as well):

def product_with_translations(sku_code, locale, translated_fields \\ ~w(name)a) do
  Product
  |> from(as: :product)
  |> where(sku_code: ^sku_code)
  |> join(
    :left, [product: p],
    t in Product.Translation,
    on: t.product_id == p.id and t.locale == ^locale,
    as: :translation
  )
  |> select([product: p], p)
  |> select_merge([translation: t], map(t, ^translated_fields))
end

As long as the associated Product.Translation exists, this works beautifully. However, if we were to ask for locale de-DE and don’t have it we receive an ArgumentError:

iex> Repo.one(product_with_translations("123", "fr-CA", ~w(name description)a))
** (ArgumentError) cannot merge because the right side is not a map, got: nil

Expected Behaviour

No exception should be thrown and the name and description fields should be nil. The SELECT works.

Can we dynamically fill a map in select_merge from a left outer join in some way that we’re not seeing, or is this a currently unhandled case in Ecto? I think that the behaviour of map(t, ^translated_fields) should result in the equivalent of Map.take(t || %{}, translated_fields).

[josevalim]

It does look like a bug indeed - or rather a feature we don't quite support yet. Thanks for the report.

[halostatue]

If you have any suggestions where I can look at this, I’d be happy to look at a PR for this.

[josevalim]

Here is the code: https://github.com/elixir-ecto/ecto/blob/master/lib/ecto/repo/queryable.ex#L282-L288 :)

Here is where you can add tests: https://github.com/elixir-ecto/ecto/blob/master/integration_test/cases/repo.exs#L1239

Since they are integration tests, you will need ecto_sql to run them. So follow the steps here: https://github.com/elixir-ecto/ecto#running-tests

[halostatue]

Thanks. We’ll look at this. If I’m reading this correctly, it should be a relatively easy change:

        {%{}, nil} ->
          left

The hard part will be the tests.

[halostatue]

Thinking more about this, I’m not 100% sure that that’s the right place for this, because it might lead to unexpected results, whereas possible changes at queryable.ex:317-325 or queryable.ex:239-243 might be better places.

It’s ultimately a better question whether map(s, ^fields) should return nil, %{}, or Map.new(Enum.map(fields, &{&1, nil})).

Given the following definition:

defmodule Product do
  use Ecto.Schema

  defmodule Product.Translation do
    use Ecto.Schema

    schema "product_translations", primary_key: false do
      field :locale, :string, primary_key: true
      belongs_to :product, Product, primary_key: true

      field :name, :string
      field :description, :string
    end
  end

  schema "products" do
    field :sku_code, :string

    field :name, :string
    field :description, :string, virtual: true

    has_many :translations, Product.Translation
  end

  def product_with_translations(sku_code, locale, translated_fields \\ ~w(name)a) do
    Product
    |> from(as: :product)
    |> where(sku_code: ^sku_code)
    |> join(
      :left, [product: p],
      t in Product.Translation,
      on: t.product_id == p.id and t.locale == ^locale,
      as: :translation
    )
    |> select([product: p], p)
    |> select_merge([translation: t], map(t, ^translated_fields))
  end
end

what should the result of Repo.one(product_with_translations("123", "fr-CA", ~w(name description)a)) be?

The choices are:

1. ** (ArgumentError) cannot merge because the right side is not a map, got: nil
2. %Product{id: 1, sku_code: "123", name: nil, description: nil}
3. %Product{id: 1, sku_code: "123", name: "Default Name", description: nil}

Translating the latter two into SQL terms, they’d look like this:

-- #2 %Product{id: 1, sku_code: "123", name: nil, description: nil}
SELECT p.id, p.sku_code, t.name, t.description
  FROM products AS p
  JOIN product_translations AS t ON t.product_id = p.id AND t.locale = 'fr-CA'
 WHERE p.sku_code = '123';

-- #3% Product{id: 1, sku_code: "123", name: "Default Name", description: nil}
SELECT p.id, p.sku_code, COALESCE(t.name, p.name) AS name, t.description
  FROM products AS p
  JOIN product_translations AS t ON t.product_id = p.id AND t.locale = 'fr-CA'
 WHERE p.sku_code = '123';  

From a strict perspective, I think that option 2 is more correct. Option 3 might be more useful in the case where a joined and merged field overwrites an already existing field. If the name field on Product were virtual (like description), then the practical result is option 2 regardless.

[josevalim]

@halostatue oh, good catch. I think we need to fix the place you pointed and the ones I mentioned. I also think that map(nil, [...]) should return nil. This is what makes most sense from the DB perspective. The DB is generally quite happy to do operations on NULL and simply return NULL as outcome.

The above means merging p with the translated fields will return p's original fields in case the translated fields are nil.

I really appreciate you looking into this, the in depth analysis was quite helpful. Well done!

[halostatue]

So is the practical upshot 2 or 3? I’ve figured out a narrow implementation that results in 2—it feels more like a “natural” result to me, but what we’re doing here isn’t quite the same as a COALESCE call here, because Ecto is overlaying two columns in this case (both columns are selected by default).

If we want 3 (only override p.name if t isn’t NULL), then the initial fix suggested should be fine ({%{}, nil}} -> left) for the behaviour of a merge. Otherwise, we need something slightly smarter where the original definition of right isn’t overwritten because we need the types from the tuple. Something like this:

diff --git i/lib/ecto/repo/queryable.ex w/lib/ecto/repo/queryable.ex
index 2719b258..083442c5 100644
--- i/lib/ecto/repo/queryable.ex
+++ w/lib/ecto/repo/queryable.ex
@@ -256,6 +257,7 @@ defmodule Ecto.Repo.Queryable do
   end
 
   defp process(row, {:merge, left, right}, from, adapter) do
+    right_source = right
     {left, row} = process(row, left, from, adapter)
     {right, row} = process(row, right, from, adapter)
 
@@ -279,6 +281,16 @@ defmodule Ecto.Repo.Queryable do
         {%{}, %{}} ->
           Map.merge(left, right)
 
+        {%{}, nil} ->
+          case right_source do
+            {:source, {_source, nil}, _prefix, types} ->
+              Map.merge(left, Map.new(types, fn {k, _v} -> {k, nil} end))
+
+            _ ->
+              raise ArgumentError,
+                    "cannot merge because the right side is not a map, got: #{inspect(right)}"
+          end
+
         {_, %{}} ->
           raise ArgumentError,
                 "cannot merge because the left side is not a map, got: #{inspect(left)}"

[josevalim]

Yes, I am proposing for us to go with 3. :)

[halostatue]

Ok. I’ll work up a PR for this.