bugfix: incorrect apply_patch for the move operation for the same list

Author: Valian

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

test/jsonpatch/operation/move_test.exs

@@ -2,12 +2,96 @@ 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}
 
     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