From 5d53d6fede7ec2d458f06b6208e80138dce447de Mon Sep 17 00:00:00 2001 From: Scott Ames-Messinger Date: Wed, 20 May 2026 10:25:54 -0400 Subject: [PATCH 1/2] Scope the primary-key -> _id rename to the outermost document The MongoDB convention of renaming the schema's primary-key field to `_id` was being applied recursively. Once `Conversions.from_ecto_pk/2` descended into a nested map (for example an `embeds_one` sub-document), the parent schema's `pk` value was still threaded through, so any nested field named `:id` was also rewritten to `:_id`. That meant an embedded value like %{jurisdiction: %{id: "MD", title: "Maryland"}} was stored as {jurisdiction: {_id: "MD", title: "Maryland"}} even when the embedded schema declared its own `@primary_key {:id, :string, autogenerate: false}`. Queries that filter on `jurisdiction.id` (the common shape for apps that put a string identifier on a sub-document) return nothing, and the wire format diverges from what other clients (Ruby driver, Node driver, etc.) write for the same model. The pk -> _id rename is only meaningful at the top of a document. Below that, the field names are user-controlled and should be passed through unchanged. This patch makes the recursive call inside `document/2` and `document/3` pass `nil` for the pk argument, so the rename only fires for the outermost document. `Conversions.to_ecto_pk/2` (the read path) is left as-is for now. Its rename of nested `"_id"` -> `Atom.to_string(pk)` only matters when a sub-document actually contains an `_id` field, which is uncommon in hand-written documents (Mongo only auto-adds `_id` to top-level docs). A symmetric read-side change can follow separately if there's appetite. --- lib/mongo_ecto/conversions.ex | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/mongo_ecto/conversions.ex b/lib/mongo_ecto/conversions.ex index c32a385..4aaeb25 100644 --- a/lib/mongo_ecto/conversions.ex +++ b/lib/mongo_ecto/conversions.ex @@ -74,13 +74,25 @@ defmodule Mongo.Ecto.Conversions do defp document(doc, pk) do map(doc, fn {key, value} -> - pair(key, value, pk, &from_ecto_pk(&1, pk)) + # Recurse with `nil` so the parent schema's primary key isn't + # accidentally rewritten when it appears inside a nested document + # (e.g. an `embeds_one` whose embedded schema has its own `:id` + # field). Without this guard, an embed like + # + # %{jurisdiction: %{id: "MD", title: "Maryland"}} + # + # gets serialized as `jurisdiction._id`, which breaks queries that + # filter on `jurisdiction.id`. The pk → _id remap is only meaningful + # at the top of the document. + pair(key, value, pk, &from_ecto_pk(&1, nil)) end) end defp document(doc, params, pk) do map(doc, fn {key, value} -> - pair(key, value, pk, &inject_params(&1, params, pk)) + # Same rationale as `document/2`: scope the pk → _id rename to the + # outer document. See the comment in `document/2`. + pair(key, value, pk, &inject_params(&1, params, nil)) end) end From 3ef2fb56a4d8b33f6badef7d48166240355d507e Mon Sep 17 00:00:00 2001 From: Scott Ames-Messinger Date: Wed, 20 May 2026 10:32:44 -0400 Subject: [PATCH 2/2] Add unit tests for outer-document-only pk rename Pure unit tests on `Mongo.Ecto.Conversions.from_ecto_pk/2` and `inject_params/3` covering the recursion fix: - top-level `:id` is renamed to `:_id` - a nested map's `:id` is left alone even when the parent pk is `:id` - the same holds inside a list of nested maps - `pk: nil` is a no-op Verified by reverting `conversions.ex` to the pre-patch version: 3 of the 5 tests fail (nested `:id` ends up as `:_id`). With the patch they all pass. --- test/mongo_ecto/conversions_test.exs | 72 ++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 test/mongo_ecto/conversions_test.exs diff --git a/test/mongo_ecto/conversions_test.exs b/test/mongo_ecto/conversions_test.exs new file mode 100644 index 0000000..c933ee8 --- /dev/null +++ b/test/mongo_ecto/conversions_test.exs @@ -0,0 +1,72 @@ +defmodule Mongo.Ecto.ConversionsTest do + use ExUnit.Case, async: true + + alias Mongo.Ecto.Conversions + + describe "from_ecto_pk/2" do + test "renames the primary key to :_id at the top level" do + assert {:ok, encoded} = Conversions.from_ecto_pk(%{id: "abc", title: "t"}, :id) + + assert Map.new(encoded) == %{_id: "abc", title: "t"} + end + + test "does not rename :id inside a nested map when the parent pk is :id" do + input = %{ + id: "abc", + title: "outer", + jurisdiction: %{id: "MD", title: "Maryland"} + } + + assert {:ok, encoded} = Conversions.from_ecto_pk(input, :id) + + encoded_map = Map.new(encoded) + assert encoded_map[:_id] == "abc" + refute Map.has_key?(encoded_map, :id) + + # The nested :id field must be passed through unchanged — it belongs to + # the embedded sub-document, not the parent schema. + assert Map.new(encoded_map[:jurisdiction]) == %{id: "MD", title: "Maryland"} + end + + test "does not rename :id inside a list of nested maps" do + input = %{id: "abc", items: [%{id: "one"}, %{id: "two"}]} + + assert {:ok, encoded} = Conversions.from_ecto_pk(input, :id) + + encoded_map = Map.new(encoded) + assert encoded_map[:_id] == "abc" + + assert Enum.map(encoded_map[:items], &Map.new/1) == [ + %{id: "one"}, + %{id: "two"} + ] + end + + test "leaves nested maps alone when the pk is nil" do + input = %{id: "abc", jurisdiction: %{id: "MD"}} + + assert {:ok, encoded} = Conversions.from_ecto_pk(input, nil) + + encoded_map = Map.new(encoded) + assert encoded_map[:id] == "abc" + assert Map.new(encoded_map[:jurisdiction]) == %{id: "MD"} + end + end + + describe "inject_params/3" do + test "renames the primary key to :_id only at the top level" do + input = %{ + id: "abc", + jurisdiction: %{id: "MD", title: "Maryland"} + } + + assert {:ok, encoded} = Conversions.inject_params(input, {}, :id) + + encoded_map = Map.new(encoded) + assert encoded_map[:_id] == "abc" + refute Map.has_key?(encoded_map, :id) + + assert Map.new(encoded_map[:jurisdiction]) == %{id: "MD", title: "Maryland"} + end + end +end