From 767eecd9857111c06cd76a208b14761e9fea3fbc Mon Sep 17 00:00:00 2001
From: Valian <jakub.skalecki@gmail.com>
Date: Thu, 17 Jul 2025 01:07:36 +0200
Subject: [PATCH] bugfix: incorrect apply_patch for the move operation for the
 same list

---
 lib/jsonpatch/operation/move.ex        | 25 ++++++--
 test/jsonpatch/operation/move_test.exs | 86 +++++++++++++++++++++++++-
 2 files changed, 106 insertions(+), 5 deletions(-)

diff --git a/lib/jsonpatch/operation/move.ex b/lib/jsonpatch/operation/move.ex
index 7f16274..be121e6 100644
--- a/lib/jsonpatch/operation/move.ex
+++ b/lib/jsonpatch/operation/move.ex
@@ -10,8 +10,9 @@ defmodule Jsonpatch.Operation.Move do
       {:ok, %{"a" => %{"e" => %{"c" => "Bob"}}, "d" => false}}
   """
 
-  alias Jsonpatch.Operation.{Copy, Move, Remove}
+  alias Jsonpatch.Operation.{Add, Move, Remove}
   alias Jsonpatch.Types
+  alias Jsonpatch.Utils
 
   @enforce_keys [:from, :path]
   defstruct [:from, :path]
@@ -28,12 +29,28 @@ defmodule Jsonpatch.Operation.Move do
   end
 
   defp do_move(from, path, target, opts) do
-    copy_patch = %Copy{from: from, path: path}
     remove_patch = %Remove{path: from}
 
-    with {:ok, res} <- Copy.apply(copy_patch, target, opts),
-         {:ok, res} <- Remove.apply(remove_patch, res, opts) do
+    with {:ok, destination} <- Utils.get_destination(target, from, opts),
+         {:ok, from_fragments} = Utils.split_path(from),
+         {:ok, copy_value} <- extract_copy_value(destination, from_fragments),
+         {:ok, res} <- Remove.apply(remove_patch, target, opts),
+         {:ok, res} <- Add.apply(%Add{value: copy_value, path: path}, res, opts) do
       {:ok, res}
     end
   end
+
+  defp extract_copy_value({%{} = destination, fragment}, from_path) do
+    case destination do
+      %{^fragment => val} -> {:ok, val}
+      _ -> {:error, {:invalid_path, from_path}}
+    end
+  end
+
+  defp extract_copy_value({destination, index}, from_path) when is_list(destination) do
+    case Utils.fetch(destination, index) do
+      {:ok, _} = ok -> ok
+      {:error, :invalid_path} -> {:error, {:invalid_path, from_path}}
+    end
+  end
 end
diff --git a/test/jsonpatch/operation/move_test.exs b/test/jsonpatch/operation/move_test.exs
index 6df5114..b2865dd 100644
--- a/test/jsonpatch/operation/move_test.exs
+++ b/test/jsonpatch/operation/move_test.exs
@@ -2,7 +2,7 @@ defmodule Jsonpatch.Operation.MoveTest do
   use ExUnit.Case
   doctest Jsonpatch.Operation.Move
 
-  # Move is a combination of the copy and remove operation.
+  # Basic move operation with atoms
   test "move a value with atoms" do
     move = %Jsonpatch.Operation.Move{from: "/a/b", path: "/a/e"}
     target = %{a: %{b: %{c: "Bob"}}, d: false}
@@ -10,4 +10,88 @@ defmodule Jsonpatch.Operation.MoveTest do
     assert Jsonpatch.Operation.Move.apply(move, target, keys: :atoms) ==
              {:ok, %{a: %{e: %{c: "Bob"}}, d: false}}
   end
+
+  # Move within the same list
+  test "move element within the same list" do
+    move = %Jsonpatch.Operation.Move{from: "/arr/0", path: "/arr/2"}
+    target = %{"arr" => ["a", "b", "c"]}
+
+    assert Jsonpatch.Operation.Move.apply(move, target, []) ==
+             {:ok, %{"arr" => ["b", "c", "a"]}}
+  end
+
+  # Move to end of list using "-" syntax
+  test "move element to end of list using -" do
+    move = %Jsonpatch.Operation.Move{from: "/arr/0", path: "/arr/-"}
+    target = %{"arr" => ["a", "b", "c"]}
+
+    assert Jsonpatch.Operation.Move.apply(move, target, []) ==
+             {:ok, %{"arr" => ["b", "c", "a"]}}
+  end
+
+  # Move between different lists
+  test "move element between different lists" do
+    move = %Jsonpatch.Operation.Move{from: "/arr1/0", path: "/arr2/1"}
+    target = %{"arr1" => ["a", "b"], "arr2" => ["x", "y", "z"]}
+
+    assert Jsonpatch.Operation.Move.apply(move, target, []) ==
+             {:ok, %{"arr1" => ["b"], "arr2" => ["x", "a", "y", "z"]}}
+  end
+
+  # Move from list to object
+  test "move element from list to object" do
+    move = %Jsonpatch.Operation.Move{from: "/arr/0", path: "/obj/key"}
+    target = %{"arr" => ["value"], "obj" => %{"existing" => "data"}}
+
+    assert Jsonpatch.Operation.Move.apply(move, target, []) ==
+             {:ok, %{"arr" => [], "obj" => %{"existing" => "data", "key" => "value"}}}
+  end
+
+  # Move from object to list
+  test "move element from object to list" do
+    move = %Jsonpatch.Operation.Move{from: "/obj/key", path: "/arr/0"}
+    target = %{"obj" => %{"key" => "value"}, "arr" => ["existing"]}
+
+    assert Jsonpatch.Operation.Move.apply(move, target, []) ==
+             {:ok, %{"obj" => %{}, "arr" => ["value", "existing"]}}
+  end
+
+  # Move with nested paths
+  test "move nested list element" do
+    move = %Jsonpatch.Operation.Move{from: "/nested/arr/0", path: "/nested/arr/2"}
+    target = %{"nested" => %{"arr" => ["a", "b", "c"]}}
+
+    assert Jsonpatch.Operation.Move.apply(move, target, []) ==
+             {:ok, %{"nested" => %{"arr" => ["b", "c", "a"]}}}
+  end
+
+  # Edge case: from and path are the same
+  test "move when from and path are the same" do
+    move = %Jsonpatch.Operation.Move{from: "/arr/0", path: "/arr/0"}
+    target = %{"arr" => ["a", "b", "c"]}
+
+    assert Jsonpatch.Operation.Move.apply(move, target, []) ==
+             {:ok, %{"arr" => ["a", "b", "c"]}}
+  end
+
+  # Error cases
+  test "move with invalid source path" do
+    move = %Jsonpatch.Operation.Move{from: "/arr/10", path: "/arr/0"}
+    target = %{"arr" => ["a", "b", "c"]}
+
+    assert match?(
+             {:error, {:invalid_path, ["arr", "10"]}},
+             Jsonpatch.Operation.Move.apply(move, target, [])
+           )
+  end
+
+  test "move with invalid destination path" do
+    move = %Jsonpatch.Operation.Move{from: "/arr/0", path: "/arr/10"}
+    target = %{"arr" => ["a", "b", "c"]}
+
+    assert match?(
+             {:error, {:invalid_path, ["arr", "10"]}},
+             Jsonpatch.Operation.Move.apply(move, target, [])
+           )
+  end
 end