Repository: elixir-ecto/ecto
Pull request #4601 (2025-04-05 13:11:50)

Fix nil map merges
Closes https://github.com/elixir-ecto/ecto/issues/4598

The problem was actually more general than CTEs. Any map merging where the right side of the merge is a `{source, schema, prefix}` with a `nil` return value would do the same thing. For example, you can recreate the problem with this query

```elixir
TestRepo.insert!(%Post{title: "hi"})

from p in Post,
  left_join: s in Comment,
  on: p.id == s.post_id,
  select: %{title: p.title},
  select_merge: map(s, [:text])

Repo.all(query)
# [%{title: "hi"}]
```

greg-rychlewski: It looks like one test is failing here https://github.com/elixir-ecto/ecto/blob/master/integration_test/cases/repo.exs#L1548

This changes the behaviour when the left and right side have common key names. Previously it would take the left side value if the right side is all `nil`. Now it overwrites common keys with `nil`. 

Without knowing the background behind the current logic, I think we want the new behaviour. If you ran the query in a psql client or something like that the right side would return `nil` it wouldn't disappear. But it might be we have the logic this way for a good reason. So don't want to assume anything without asking.
josevalim: I think we could argue both ways, actually. We could also argue that nils in SQL are mostly a no-op (i.e. they disappear) so in this case we should ignore the nils, as if nothing is being merged.

Perhaps if you the behaviour in this PR, then you should do an explicit map instead `select_merge: %{stake: c.stake}`? Then we document that when merging another source, nothing is merged if the source is nil.
greg-rychlewski: I'm ok with either. I'll change the docs instead to explain the existing behaviour.
greg-rychlewski: After playing around with this more, I think it is not so simple =/.

If we are not dealing with joins then this still returns a map instead of just `nil`

```elixir
TestRepo.insert!(%Post{title: nil})

q = from p in Post, select: map(p, [:title])
TestRepo.all(q)
# [%{title: nil}]
```

It seems a bit inconsistent to me if it didn't retain the same behaviour on merging/joining.  But tbh it's not clear to me what kind of query would trigger that `all_nil` behaviour except these joins. So I need to dig more.
greg-rychlewski: For example

```elixir
query =
  from(Barebone)
  |> join(:left, [s], t in Barebone, on: s.id != t.id)
  |> select([s, t], {s, t})

TestRepo.all(query)

# [
#  {%Ecto.Integration.Barebone{
#     __meta__: #Ecto.Schema.Metadata<:loaded, "barebones">,
#     num: nil
#   }, nil}
# ]
```

So it seems intentional this is only for joins. But the rationale is not super clear to me. If you're sure we want to keep it like this I can document it. But just wanted to show you this in case it changes your mind
josevalim: The difference is between returning a row where all values are nils and returning a row that there is no join. In your case, all values are nil, because if there was no entry, there would be no row. In the join case, both "all values are nil" and "returning a row" would be very similar except that, since it is almost certain you are relying on a non-nil condition somewhere (such as foreign keys), we can safely assume that if all values are nil, it is because nothing matched. So I think we have two options:

1. Go with this PR for simplicity
2. Make sure left_outer/right_outer joins include the column it was joined on so we can distinguish between nil columns and missing "row"

2 is probably too complex and come with corner cases, so it probably makes more sense to go with 1 for simplicity. Thoughts?
greg-rychlewski: Thanks that makes it clear the reason for the existing behaviour. I agree about option 2...we are probably asking for trouble. 

By going with this PR, you mean documenting the existing behaviour right? If so, that would be my choice too. I'm not too crazy about my original submission now because it seems this behaviour exists even without `select_merge`. So to keep it all acting the same it's probably clearer if we don't change anything. But let me know if you are thinking something else. 
josevalim: > By going with this PR, you mean documenting the existing behaviour right? 

I believe today we guarantee that joins merge correctly because we always return their primary keys/foreign keys, so we can distinguish a nil columns from no left joins, right? If that's the case, then I'd document the behaviour. However, if today we don't distinguish them, then it is saner to just always consider the values are nil.

Patch:
diff --git a/lib/ecto/repo/queryable.ex b/lib/ecto/repo/queryable.ex
index 8e4ced5c..f9d5afd0 100644
--- a/lib/ecto/repo/queryable.ex
+++ b/lib/ecto/repo/queryable.ex
@@ -310,7 +310,7 @@ defmodule Ecto.Repo.Queryable do
 
   defp process(row, {:merge, left, right}, from, adapter) do
     {left, row} = process(row, left, from, adapter)
-    {right, row} = process(row, right, from, adapter)
+    {right, row} = process_merge(row, right, from, adapter)
 
     data =
       case {left, right} do
@@ -427,6 +427,15 @@ defmodule Ecto.Repo.Queryable do
     end)
   end
 
+  defp process_merge(row, {:source, {source, schema}, prefix, types}, _from, adapter) do
+    struct = Ecto.Schema.Loader.load_struct(schema, prefix, source)
+    struct_load!(types, row, [], false, struct, adapter)
+  end
+
+  defp process_merge(row, process, from, adapter) do
+    process(row, process, from, adapter)
+  end
+
   @compile {:inline, load!: 5}
   defp load!(type, value, field, struct, adapter) do
     case Ecto.Type.adapter_load(adapter, type, value) do
diff --git a/lib/ecto/schema.ex b/lib/ecto/schema.ex
index c8768f14..6fd48378 100644
--- a/lib/ecto/schema.ex
+++ b/lib/ecto/schema.ex
@@ -130,6 +130,9 @@ defmodule Ecto.Schema do
   inspect on the schema unless the schema module is tagged with
   the option `@derive_inspect_for_redacted_fields false`.
 
+  A schema module tagged with `@schema_redact :all_except_primary_keys` will
+  redact all fields except primary keys.
+
   ## Schema attributes
 
   Supported attributes for configuring the defined schema. They must
@@ -155,6 +158,11 @@ defmodule Ecto.Schema do
       which generates structs and queries without context. Context are not used
       by the built-in SQL adapters.
 
+    * `@schema_redact` - If set to `:all_except_primary_keys`, Ecto will
+      treat all non-primary key fields as if they were individually marked
+      as redacted. Defaults to `false`, as no fields are redacted by default.
+      The value set here can be changed per field through the `:redact` option.
+
     * `@foreign_key_type` - configures the default foreign key type
       used by `belongs_to` associations. It must be set in the same
       module that defines the `belongs_to`. Defaults to `:id`;
@@ -1992,7 +2000,14 @@ defmodule Ecto.Schema do
     writable = opts[:writable] || :always
     put_struct_field(mod, name, Keyword.get(opts, :default))
 
-    if Keyword.get(opts, :redact, false) do
+    redact_field? = Keyword.get_lazy(opts, :redact, fn ->
+      case Module.get_attribute(mod, :schema_redact, false) do
+        :all_except_primary_keys -> not pk?
+        false -> false
+      end
+    end)
+
+    if redact_field? do
       Module.put_attribute(mod, :ecto_redact_fields, name)
     end
 


Test patch:
diff --git a/integration_test/cases/repo.exs b/integration_test/cases/repo.exs
index 9a1bdc0d..e0abe007 100644
--- a/integration_test/cases/repo.exs
+++ b/integration_test/cases/repo.exs
@@ -1538,14 +1538,14 @@ defmodule Ecto.Integration.RepoTest do
                |> select_merge([_l, p], map(p, ^~w(title posted)a))
                |> TestRepo.all()
 
-      # left join record is not present
-      assert [%{url: "Q", title: "Z", posted: nil}] =
+      # left join record is not present, we consider it the same as being present with nils
+      assert [%{url: "Q", title: nil, posted: nil}] =
                Permalink
                |> join(:left, [l], p in Post, on: l.post_id == p.id and p.public == true)
                |> select([l, p], merge(l, map(p, ^~w(title posted)a)))
                |> TestRepo.all()
 
-      assert [%{url: "Q", title: "Z", posted: nil}] =
+      assert [%{url: "Q", title: nil, posted: nil}] =
                Permalink
                |> join(:left, [l], p in Post, on: l.post_id == p.id and p.public == true)
                |> select_merge([_l, p], map(p, ^~w(title posted)a))
diff --git a/test/ecto/changeset_test.exs b/test/ecto/changeset_test.exs
index ede16492..b70216f0 100644
--- a/test/ecto/changeset_test.exs
+++ b/test/ecto/changeset_test.exs
@@ -3607,6 +3607,17 @@ defmodule Ecto.ChangesetTest do
     end
   end
 
+  defmodule RedactAllExceptPrimaryKeysSchema do
+    use Ecto.Schema
+
+    @schema_redact :all_except_primary_keys
+    schema "redacted_schema" do
+      field :password, :string
+      field :username, :string
+      field :virtual_pass, :string, virtual: true
+    end
+  end
+
   defmodule RedactedEmbeddedSchema do
     use Ecto.Schema
 
@@ -3683,5 +3694,12 @@ defmodule Ecto.ChangesetTest do
       assert inspect(changeset) =~ "hunter2"
       refute inspect(changeset) =~ "**redacted**"
     end
+
+    test "redacts all non-primary-key fields when schema sets @schema_redact :all_except_primary_keys" do
+      changeset = Ecto.Changeset.cast(%RedactAllExceptPrimaryKeysSchema{}, %{username: "Hunter", password: "hunter2"}, [:username, :password])
+      assert inspect(changeset) =~ "id"
+      refute inspect(changeset) =~ "hunter2"
+      assert inspect(changeset) =~ "**redacted**"
+    end
   end
 end
diff --git a/test/ecto/schema_test.exs b/test/ecto/schema_test.exs
index 15e9c27b..d84d78a7 100644
--- a/test/ecto/schema_test.exs
+++ b/test/ecto/schema_test.exs
@@ -192,6 +192,37 @@ defmodule Ecto.SchemaTest do
     refute inspect(%Schema{temp: "hunter2"}) =~ "hunter2"
   end
 
+  defmodule SchemaWithRedactAllExceptPrimaryKeys do
+    use Ecto.Schema
+
+    @schema_redact :all_except_primary_keys
+    schema "my_schema" do
+      field :password, :string
+      field :temp, :any, default: "temp", virtual: true
+    end
+  end
+
+  test "schema with @schema_redact: :all_except_primary_keys derives inspect for all non-primary-key fields" do
+    inspected_schema = inspect(%SchemaWithRedactAllExceptPrimaryKeys{password: "hunter2"})
+
+    assert inspected_schema =~ "id"
+    refute inspected_schema =~ "hunter2"
+    refute inspected_schema =~ "temp"
+  end
+
+  defmodule SchemaWithRedactFalse do
+    use Ecto.Schema
+
+    @schema_redact false
+    schema "my_schema" do
+      field :name, :string
+    end
+  end
+
+  test "schema with @schema_redact: false doesn't derive inspect" do
+    assert inspect(%SchemaWithRedactFalse{name: "Hunter"}) =~ "Hunter"
+  end
+
   defmodule SchemaWithoutDeriveInspect do
     use Ecto.Schema