Skip to content

Support associations with composite foreign keys #3638

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
4a0264d
Support associations on composite foreign keys
May 18, 2021
fefee0c
Update lib/ecto/association.ex
soundmonster Apr 6, 2022
f29c121
Implement suggestions from PR review
Apr 6, 2022
ce380a8
Add test for many_to_many, clean up
Apr 6, 2022
1d68ad0
Use exact binding positions for joins
Apr 6, 2022
e2dd4a5
Stabilize test by specifying order
Apr 6, 2022
1ed34b4
Move away from dynamic() joins for one and two PK columns
Apr 7, 2022
ce651f3
Remove dynamic() from joins completely
Apr 7, 2022
b21a4e7
Remove dynamic() from almost all cases
Apr 7, 2022
9e18f75
Remove all uses of `dynamic` for association queries
Apr 8, 2022
ca19a2a
Fix post-rebase issues, all unit tests but 1 pass now
Jan 16, 2023
02146bd
Wip fixing tests
Jan 16, 2023
2b5f584
Tweak order of ON clauses
Jan 16, 2023
fb2afcc
All tests are green
Jan 16, 2023
dc652ac
Fix compiler warning, remove 'debugger' module
Jan 17, 2023
b8665ce
Start adding docs
Jan 17, 2023
0333ffc
Auto-derive assoc keys for has_*
Jan 17, 2023
658060d
Update docs
Jan 18, 2023
d02709c
wip
Apr 24, 2023
66d5299
Wip but at least it compiles :sweat_smile:
Apr 25, 2023
13c6f8e
Fix schema tests
Apr 25, 2023
527354d
Undo autoformatter changes
Apr 25, 2023
edb05bd
Fix repo tests
Apr 25, 2023
f3ae0cd
Fix Ecto.assoc
Apr 25, 2023
e82f12d
Fix all tests :tada:
Apr 25, 2023
65a6331
Fix integration tests
Apr 25, 2023
a6588ac
Remove superfluous guard
Apr 25, 2023
458e704
Fix typo
Apr 25, 2023
d887cb3
Simplify chain_through
Apr 25, 2023
304df11
Improve docs, remove checked TODO
Apr 25, 2023
f73f511
Add tests for updating a has_one/has_many
Apr 25, 2023
8551091
Improve test clarity
Apr 25, 2023
1573854
Fix post-compile assoc validation
Apr 25, 2023
d42688c
Fix warnings in preloader tests by adding explicit on-clauses
Apr 26, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Earthfile
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ integration-test-base:
apk del .build-dependencies && rm -f msodbcsql*.sig mssql-tools*.apk
ENV PATH="/opt/mssql-tools/bin:${PATH}"

GIT CLONE https://github.com/elixir-ecto/ecto_sql.git /src/ecto_sql
GIT CLONE --branch composite_foreign_keys https://github.com/soundmonster/ecto_sql.git /src/ecto_sql
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch needs a few changes to the base migration used for the integration tests; I've opened a PR for this so I hope to be able to drop this line soon.

WORKDIR /src/ecto_sql
RUN mix deps.get

Expand Down
78 changes: 78 additions & 0 deletions integration_test/cases/assoc.exs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ defmodule Ecto.Integration.AssocTest do
alias Ecto.Integration.PostUser
alias Ecto.Integration.Comment
alias Ecto.Integration.Permalink
alias Ecto.Integration.CompositePk
alias Ecto.Integration.OneToOneCompositePk

test "has_many assoc" do
p1 = TestRepo.insert!(%Post{title: "1"})
Expand Down Expand Up @@ -42,6 +44,21 @@ defmodule Ecto.Integration.AssocTest do
assert l3.id == lid3
end

test "has_one assoc with composite key" do
c11 = TestRepo.insert!(%CompositePk{a: 1, b: 1, name: "11"})
_c12 = TestRepo.insert!(%CompositePk{a: 1, b: 2, name: "12"})
c22 = TestRepo.insert!(%CompositePk{a: 2, b: 2, name: "22"})

%OneToOneCompositePk{id: id_o11} = TestRepo.insert!(%OneToOneCompositePk{composite_a: 1, composite_b: 1})
%OneToOneCompositePk{} = TestRepo.insert!(%OneToOneCompositePk{composite_a: 1, composite_b: 2})
%OneToOneCompositePk{id: id_o22} = TestRepo.insert!(%OneToOneCompositePk{composite_a: 2, composite_b: 2})

[o11, o22] = TestRepo.all(Ecto.assoc([c11, c22], :one_to_one_composite_pk))
assert o11.id == id_o11
assert o22.id == id_o22
end


test "belongs_to assoc" do
%Post{id: pid1} = TestRepo.insert!(%Post{title: "1"})
%Post{id: pid2} = TestRepo.insert!(%Post{title: "2"})
Expand All @@ -55,6 +72,22 @@ defmodule Ecto.Integration.AssocTest do
assert p2.id == pid2
end

test "belongs_to assoc with composite key" do
TestRepo.insert!(%CompositePk{a: 2, b: 1, name: "foo"})
TestRepo.insert!(%CompositePk{a: 2, b: 2, name: "bar"})
TestRepo.insert!(%CompositePk{a: 2, b: 3, name: "unused"})

p1 = TestRepo.insert!(%Post{title: "first", composite_a: 2, composite_b: 1})
p2 = TestRepo.insert!(%Post{title: "none"})
p3 = TestRepo.insert!(%Post{title: "second", composite_a: 2, composite_b: 2})

assert [c1, c2] = TestRepo.all Ecto.assoc([p1, p2, p3], :composite)
assert c1.a == 2
assert c1.b == 1
assert c2.a == 2
assert c2.b == 2
end

test "has_many through assoc" do
p1 = TestRepo.insert!(%Post{})
p2 = TestRepo.insert!(%Post{})
Expand Down Expand Up @@ -231,6 +264,22 @@ defmodule Ecto.Integration.AssocTest do
assert u2.id == uid2
end

test "many_to_many composite PK" do
c11 = TestRepo.insert!(%CompositePk{a: 1, b: 1, name: "11"})
c12 = TestRepo.insert!(%CompositePk{a: 1, b: 2, name: "12"})
c22 = TestRepo.insert!(%CompositePk{a: 2, b: 2, name: "22"})

TestRepo.insert_all "composite_pk_composite_pk", [[a_1: 1, b_1: 1, a_2: 1, b_2: 2],
[a_1: 1, b_1: 1, a_2: 2, b_2: 2],
[a_1: 1, b_1: 2, a_2: 2, b_2: 2]]

assert [^c12, ^c22] = TestRepo.all Ecto.assoc([c11], :composites)
assert [^c22] = TestRepo.all Ecto.assoc([c12], :composites)
assert [] = TestRepo.all Ecto.assoc([c22], :composites)

assert [^c12, ^c22, ^c22] = TestRepo.all Ecto.assoc([c11, c12, c22], :composites)
end

## Changesets

test "has_one changeset assoc (on_replace: :delete)" do
Expand Down Expand Up @@ -725,6 +774,27 @@ defmodule Ecto.Integration.AssocTest do
assert perma.post_id == nil
end

test "belongs_to changeset assoc on composite key" do
changeset =
%CompositePk{a: 1, b: 2}
|> Ecto.Changeset.change()
|> Ecto.Changeset.put_assoc(:posts, [%Post{title: "1"}])

composite = TestRepo.insert!(changeset)
assert [post] = composite.posts
assert post.id
assert post.composite_a == composite.a
assert post.composite_b == composite.b
assert post.title == "1"

composite = TestRepo.get_by! from(CompositePk, preload: [:posts]), [a: composite.a, b: composite.b]
assert [%Post{title: "1"}] = composite.posts

post = TestRepo.get! from(Post, preload: [:composite]), post.id
assert post.composite.a == 1
assert post.composite.b == 2
end

test "inserting struct with associations" do
tree = %Permalink{
url: "root",
Expand All @@ -750,6 +820,14 @@ defmodule Ecto.Integration.AssocTest do
assert Enum.all?(tree.post.comments, & &1.id)
end

test "inserting struct with associations on composite keys" do
# creates nested belongs_to
%Post{composite: composite} =
TestRepo.insert! %Post{title: "1", composite: %CompositePk{a: 1, b: 2, name: "name"}}

assert %CompositePk{a: 1, b: 2, name: "name"} = composite
end

test "inserting struct with empty associations" do
permalink = TestRepo.insert!(%Permalink{url: "root", post: nil})
assert permalink.post == nil
Expand Down
3 changes: 2 additions & 1 deletion integration_test/cases/joins.exs
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,8 @@ defmodule Ecto.Integration.JoinsTest do
query = from(p in Post, join: pl in assoc(p, :permalink),
join: c in assoc(p, :comments),
preload: [permalink: pl],
select: {p, c})
select: {p, c},
order_by: p.id)
[{p1, ^c1}, {p1, ^c2}, {p2, ^c3}] = TestRepo.all(query)
assert p1.permalink == pl1
assert p2.permalink == pl3
Expand Down
80 changes: 79 additions & 1 deletion integration_test/cases/preload.exs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ defmodule Ecto.Integration.PreloadTest do

alias Ecto.Integration.Post
alias Ecto.Integration.Comment
alias Ecto.Integration.CompositePk
alias Ecto.Integration.Item
alias Ecto.Integration.Permalink
alias Ecto.Integration.User
Expand Down Expand Up @@ -347,6 +348,25 @@ defmodule Ecto.Integration.PreloadTest do
assert [] = pe3.comments
end

test "preload composite foreign key with function" do
c11 = TestRepo.insert!(%CompositePk{a: 1, b: 1, name: "11"})
c12 = TestRepo.insert!(%CompositePk{a: 1, b: 2, name: "12"})
c22 = TestRepo.insert!(%CompositePk{a: 2, b: 2, name: "22"})
c33 = TestRepo.insert!(%CompositePk{a: 3, b: 3, name: "33"})

TestRepo.insert!(%Post{title: "1", composite_a: 1, composite_b: 1})
TestRepo.insert!(%Post{title: "2", composite_a: 1, composite_b: 1})
TestRepo.insert!(%Post{title: "3", composite_a: 1, composite_b: 2})
TestRepo.insert!(%Post{title: "4", composite_a: 2, composite_b: 2})

assert [ce12, ce11, ce33, ce22] = TestRepo.preload([c12, c11, c33, c22],
posts: fn _ -> TestRepo.all(Post) end)
assert [%Post{title: "1"}, %Post{title: "2"}] = ce11.posts
assert [%Post{title: "3"}] = ce12.posts
assert [%Post{title: "4"}] = ce22.posts
assert [] = ce33.posts
end

test "preload many_to_many with function" do
p1 = TestRepo.insert!(%Post{title: "1"})
p2 = TestRepo.insert!(%Post{title: "2"})
Expand Down Expand Up @@ -397,6 +417,52 @@ defmodule Ecto.Integration.PreloadTest do
assert p3.users == [%{id: uid1}, %{id: uid4}]
end

test "preload many_to_many on composite foreign keys with function" do
c11 = TestRepo.insert!(%CompositePk{a: 1, b: 1, name: "11"})
c12 = TestRepo.insert!(%CompositePk{a: 1, b: 2, name: "12"})
c22 = TestRepo.insert!(%CompositePk{a: 2, b: 2, name: "22"})

TestRepo.insert_all "composite_pk_composite_pk", [[a_1: 1, b_1: 1, a_2: 1, b_2: 2],
[a_1: 1, b_1: 1, a_2: 2, b_2: 2],
[a_1: 1, b_1: 2, a_2: 1, b_2: 1],
[a_1: 2, b_1: 2, a_2: 2, b_2: 2]]

wrong_preloader = fn composite_ids ->
composite_ids_a = Enum.map(composite_ids, &Enum.at(&1, 0))
composite_ids_b = Enum.map(composite_ids, &Enum.at(&1, 1))
TestRepo.all(
from c in CompositePk,
join: cc in "composite_pk_composite_pk",
on: cc.a_2 == c.a and cc.b_2 == c.b,
where: cc.a_1 in ^composite_ids_a and cc.b_1 in ^composite_ids_b,
order_by: [c.a, c.b],
select: map(c, [:name])
)
end

assert_raise RuntimeError, ~r/invalid custom preload for `composites` on `Ecto.Integration.CompositePk`/, fn ->
TestRepo.preload([c11, c12, c22], composites: wrong_preloader)
end

right_preloader = fn composite_ids ->
composite_ids_a = Enum.map(composite_ids, &Enum.at(&1, 0))
composite_ids_b = Enum.map(composite_ids, &Enum.at(&1, 1))
TestRepo.all(
from c in CompositePk,
join: cc in "composite_pk_composite_pk",
on: cc.a_2 == c.a and cc.b_2 == c.b,
where: cc.a_1 in ^composite_ids_a and cc.b_1 in ^composite_ids_b,
order_by: [c.a, c.b],
select: {[cc.a_1, cc.b_1], map(c, [:name])}
)
end

[c11, c12, c22] = TestRepo.preload([c11, c12, c22], composites: right_preloader)
assert c11.composites == [%{name: "12"}, %{name: "22"}]
assert c12.composites == [%{name: "11"}]
assert c22.composites == [%{name: "22"}]
end

test "preload with query" do
p1 = TestRepo.insert!(%Post{title: "1"})
p2 = TestRepo.insert!(%Post{title: "2"})
Expand Down Expand Up @@ -607,11 +673,23 @@ defmodule Ecto.Integration.PreloadTest do

assert ExUnit.CaptureLog.capture_log(fn ->
assert TestRepo.preload(updated, [:author]).author == u1
end) =~ ~r/its association key `author_id` is nil/
end) =~ ~r/its association keys `\(author_id\)` are nil/

assert TestRepo.preload(updated, [:author], force: true).author == nil
end

test "preload raises with association over composite foreign key is set but without id" do
p1 = TestRepo.insert!(%Post{title: "1"})
c11 = TestRepo.insert!(%CompositePk{a: 1, b: 1, name: "11"})
updated = %{p1 | composite: c11, composite_a: nil, composite_b: nil}

assert ExUnit.CaptureLog.capture_log(fn ->
assert TestRepo.preload(updated, [:composite]).composite == c11
end) =~ ~r/its association keys `\(composite_a, composite_b\)` are nil/

assert TestRepo.preload(updated, [:composite], force: true).composite == nil
end

test "preload skips already loaded for cardinality one" do
%Post{id: pid} = TestRepo.insert!(%Post{title: "1"})

Expand Down
17 changes: 17 additions & 0 deletions integration_test/cases/repo.exs
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,23 @@ defmodule Ecto.Integration.RepoTest do
assert TestRepo.all(PostUserCompositePk) == []
end

@tag :composite_pk
test "insert, update and delete with assoc over composite foreign key" do
composite = TestRepo.insert!(%CompositePk{a: 1, b: 2, name: "name"})
post = TestRepo.insert!(%Post{title: "post title", composite: composite})

assert post.composite_a == 1
assert post.composite_b == 2
assert TestRepo.get_by!(CompositePk, [a: 1, b: 2]) == composite

post = post |> Ecto.Changeset.change(composite: nil) |> TestRepo.update!
assert is_nil(post.composite_a)
assert is_nil(post.composite_b)

TestRepo.delete!(post)
assert TestRepo.all(CompositePk) == [composite]
end

@tag :invalid_prefix
test "insert, update and delete with invalid prefix" do
post = TestRepo.insert!(%Post{})
Expand Down
27 changes: 26 additions & 1 deletion integration_test/support/schemas.exs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ defmodule Ecto.Integration.Post do
has_one :update_permalink, Ecto.Integration.Permalink, foreign_key: :post_id, on_delete: :delete_all, on_replace: :update
has_many :comments_authors, through: [:comments, :author]
belongs_to :author, Ecto.Integration.User
belongs_to :composite, Ecto.Integration.CompositePk,
foreign_key: [:composite_a, :composite_b], references: [:a, :b], type: [:integer, :integer], on_replace: :nilify
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the key names here can probably be inferred from the models themselves; I'm not sure if this is something I should target in this PR, too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think being explicit is probably better for now.

many_to_many :users, Ecto.Integration.User,
join_through: "posts_users", on_delete: :delete_all, on_replace: :delete
many_to_many :ordered_users, Ecto.Integration.User, join_through: "posts_users", preload_order: [desc: :name]
Expand All @@ -63,7 +65,7 @@ defmodule Ecto.Integration.Post do
join_through: Ecto.Integration.PostUserCompositePk
has_many :users_comments, through: [:users, :comments]
has_many :comments_authors_permalinks, through: [:comments_authors, :permalink]
has_one :post_user_composite_pk, Ecto.Integration.PostUserCompositePk
has_many :post_user_composite_pk, Ecto.Integration.PostUserCompositePk
timestamps()
end

Expand Down Expand Up @@ -294,6 +296,12 @@ defmodule Ecto.Integration.CompositePk do
field :a, :integer, primary_key: true
field :b, :integer, primary_key: true
field :name, :string
has_many :posts, Ecto.Integration.Post, foreign_key: [:composite_a, :composite_b], references: [:a, :b]
many_to_many :composites, Ecto.Integration.CompositePk,
join_through: "composite_pk_composite_pk", join_keys: [[a_1: :a, b_1: :b], [a_2: :a, b_2: :b]],
on_delete: :delete_all, on_replace: :delete
has_one :one_to_one_composite_pk, Ecto.Integration.OneToOneCompositePk,
foreign_key: [:composite_a, :composite_b], references: [:a, :b]
end
def changeset(schema, params) do
cast(schema, params, ~w(a b name)a)
Expand Down Expand Up @@ -332,6 +340,23 @@ defmodule Ecto.Integration.PostUserCompositePk do
end
end

defmodule Ecto.Integration.OneToOneCompositePk do
@moduledoc """
This module is used to test:

* Composite primary keys for 2 has_one fields

"""
use Ecto.Integration.Schema

schema "one_to_one_composite_pk" do
belongs_to :composite, Ecto.Integration.CompositePk,
foreign_key: [:composite_a, :composite_b], references: [:a, :b], type: [:integer, :integer], on_replace: :nilify
timestamps()
end
end


defmodule Ecto.Integration.Usec do
@moduledoc """
This module is used to test:
Expand Down
14 changes: 10 additions & 4 deletions lib/ecto.ex
Original file line number Diff line number Diff line change
Expand Up @@ -523,10 +523,16 @@ defmodule Ecto do
refl = %{owner_key: owner_key} = Ecto.Association.association_from_schema!(schema, assoc)

values =
Enum.uniq for(struct <- structs,
assert_struct!(schema, struct),
key = Map.fetch!(struct, owner_key),
do: key)
structs
|> Enum.filter(&assert_struct!(schema, &1))
|> Enum.map(fn struct ->
case owner_key do
single_key when is_atom(single_key) -> Map.fetch!(struct, single_key)
[single_key] -> Map.fetch!(struct, single_key)
[_ | _] -> owner_key |> Enum.map(&Map.fetch!(struct, &1)) # |> List.to_tuple()
end
end)
|> Enum.uniq

case assocs do
[] ->
Expand Down
Loading