Skip to content
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

Add diff for many associations without primary keys #144

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
66 changes: 38 additions & 28 deletions lib/ecto_diff.ex
Original file line number Diff line number Diff line change
Expand Up @@ -303,37 +303,47 @@ defmodule EctoDiff do
defp diff_association(previous, current, %{cardinality: :many, field: field, related: struct}, acc) do
primary_key_fields = struct.__schema__(:primary_key)

if primary_key_fields == [],
do: raise("cannot determine difference in many association with no primary key for `#{struct}`")

{previous_map, keys} =
Enum.reduce(previous, {%{}, []}, fn x, {map, keys} ->
key = Map.take(x, primary_key_fields)
{Map.put(map, key, x), [key | keys]}
end)

{current_map, keys} =
Enum.reduce(current, {%{}, keys}, fn x, {map, keys} ->
key = Map.take(x, primary_key_fields)
{Map.put(map, key, x), [key | keys]}
end)

keys = keys |> Enum.reverse() |> Enum.uniq()

diffs =
keys
|> Enum.map(fn key ->
prev_child = Map.get(previous_map, key)
current_child = Map.get(current_map, key)
if primary_key_fields == [] do
from = max(length(previous), length(current)) - 1

do_diff(prev_child, current_child)
end)
|> Enum.reject(&no_changes?/1)
from
|> Range.new(0)
|> Enum.map(fn i ->
prev_child = Enum.at(previous, i)
current_child = Enum.at(current, i)

if diffs == [] do
acc
else
[{field, diffs} | acc]
do_diff(prev_child, current_child)
end)
Comment on lines -326 to +317
Copy link
Collaborator

@doughsay doughsay Feb 10, 2022

Choose a reason for hiding this comment

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

The performance of this isn't great, because Enum.at traverses the whole list each time. It would be better to zip the two lists together first and traverse the zipped list instead. The problem of course is that Enum.zip stops at the shortest list. Here's a simple implementation of a zip that zips all the way to the longest list, defaulting missing elements to nil: https://gist.github.com/nathan-cruz77/586092fa2487da2bdd5603934e7db4ad

I would recommend adding this util function somewhere and using it here.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, good idea

Comment on lines +308 to +317
Copy link
Contributor

@vanvoljg vanvoljg Feb 15, 2022

Choose a reason for hiding this comment

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

Along with doughsay's suggestion, once you bring in that zip_longest function, this implementation becomes:

zip_longest(previous, current)
|> Enum.map(&do_diff(&1, &2))

else
{previous_map, keys} =
Enum.reduce(previous, {%{}, []}, fn x, {map, keys} ->
key = Map.take(x, primary_key_fields)
{Map.put(map, key, x), [key | keys]}
end)

{current_map, keys} =
Enum.reduce(current, {%{}, keys}, fn x, {map, keys} ->
key = Map.take(x, primary_key_fields)
{Map.put(map, key, x), [key | keys]}
end)

keys
|> Enum.reverse()
Copy link
Contributor

Choose a reason for hiding this comment

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

However, once you make the above change, you'll need to pull this reverse back out to the end of this whole function body, in the pipeline after Enum.reject/2.

|> Enum.uniq()
|> Enum.map(fn key ->
prev_child = Map.get(previous_map, key)
current_child = Map.get(current_map, key)

do_diff(prev_child, current_child)
end)
end

diffs
|> Enum.reject(&no_changes?/1)
|> case do
[] -> acc
changed -> [{field, changed} | acc]
end
end

Expand Down
9 changes: 9 additions & 0 deletions priv/repo/migrations/20220113193719_create_boxes.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
defmodule InventoryCore.Repo.Migrations.CreateBoxes do
use Ecto.Migration

def change do
create table(:boxes) do
add :shapes, {:array, :map}, default: []
end
end
end
144 changes: 144 additions & 0 deletions test/ecto_diff_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -743,5 +743,149 @@ defmodule EctoDiffTest do
}
} = diff
end

# Embeds Many Association without primary key

test "update with embeds_many while adding, shuffling and removing records without pk" do
{:ok, initial} =
%{shapes: [%{angles: 1}, %{angles: 2}]}
|> Box.new()
|> Repo.insert()

assert {:ok, :unchanged} = EctoDiff.diff(initial, initial)

{:ok, added} =
initial
|> Box.update(%{shapes: [%{angles: 1}, %{angles: 2}, %{angles: 3}]})
|> Repo.update()

assert {
:ok,
%EctoDiff{
struct: Box,
effect: :changed,
changes: %{
shapes: [
%EctoDiff{
struct: Shape,
effect: :added,
changes: %{angles: {nil, 3}}
}
]
}
}
} = EctoDiff.diff(initial, added)

{:ok, shuffled} =
added
|> Box.update(%{shapes: [%{angles: 3}, %{angles: 1}, %{angles: 2}]})
|> Repo.update()

assert {:ok,
%EctoDiff{
struct: Box,
effect: :changed,
changes: %{
shapes: [
%EctoDiff{
struct: Shape,
effect: :changed,
changes: %{angles: {3, 2}}
},
%EctoDiff{
struct: Shape,
effect: :changed,
changes: %{angles: {2, 1}}
},
%EctoDiff{
struct: Shape,
effect: :changed,
changes: %{angles: {1, 3}}
}
]
}
}} = EctoDiff.diff(added, shuffled)

{:ok, removed_first} =
added
|> Box.update(%{shapes: [%{angles: 2}, %{angles: 3}]})
|> Repo.update()

assert {
:ok,
%EctoDiff{
struct: Box,
effect: :changed,
changes: %{
shapes: [
%EctoDiff{
struct: Shape,
effect: :deleted,
changes: %{}
},
%EctoDiff{
struct: Shape,
effect: :changed,
changes: %{angles: {2, 3}}
},
%EctoDiff{
struct: Shape,
effect: :changed,
changes: %{angles: {1, 2}}
}
]
}
}
} = EctoDiff.diff(added, removed_first)

{:ok, removed_last} =
added
|> Box.update(%{shapes: [%{angles: 1}, %{angles: 2}]})
|> Repo.update()

assert {
:ok,
%EctoDiff{
struct: Box,
effect: :changed,
changes: %{
shapes: [
%EctoDiff{
struct: Shape,
effect: :deleted,
changes: %{}
}
]
}
}
} = EctoDiff.diff(added, removed_last)

{:ok, removed_middle} =
added
|> Box.update(%{shapes: [%{angles: 1}, %{angles: 3}]})
|> Repo.update()

assert {
:ok,
%EctoDiff{
struct: Box,
effect: :changed,
changes: %{
shapes: [
%EctoDiff{
struct: Shape,
effect: :deleted,
changes: %{}
},
%EctoDiff{
struct: Shape,
effect: :changed,
changes: %{angles: {2, 3}}
}
]
}
}
} = EctoDiff.diff(added, removed_middle)
end
end
end
2 changes: 1 addition & 1 deletion test/support/data_case.ex
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ defmodule EctoDiff.DataCase do

using do
quote do
alias EctoDiff.{Owner, Pet, Repo}
alias EctoDiff.{Box, Owner, Pet, Repo, Shape}

import EctoDiff.DataCase
end
Expand Down
19 changes: 19 additions & 0 deletions test/support/test_schemas/box.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
defmodule EctoDiff.Box do
@moduledoc false

use Ecto.Schema
import Ecto.Changeset

schema("boxes") do
embeds_many :shapes, EctoDiff.Shape, on_replace: :delete
end

def new(params), do: changeset(%__MODULE__{}, params)
def update(struct, params), do: changeset(struct, params)

defp changeset(struct, params) do
struct
|> cast(params, [])
|> cast_embed(:shapes)
end
end
13 changes: 13 additions & 0 deletions test/support/test_schemas/shape.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
defmodule EctoDiff.Shape do
@moduledoc false

use Ecto.Schema
import Ecto.Changeset

@primary_key false
embedded_schema do
field :angles, :integer
end

def changeset(struct, params), do: cast(struct, params, [:angles])
end