fix(partition): support map keys in put_newer

CHANGELOG.md

@@ -5,6 +5,16 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
+## Unreleased
+
+### Bug Fixes
+
+- Fixed `put_newer/5` and `put_all_newer/3` raising `ArgumentError` ("not a
+  valid match specification") when the key contains a map. The key is now
+  bound to a match variable and compared in the guard, since ETS rejects raw
+  maps at the key position of a `select_replace/2` match head.
+  [#18](https://github.com/appcues/partitioned_buffer/issues/18).
+
 ## Release 0.4.1
 
 ### Bug Fixes

lib/partitioned_buffer/partition.ex

@@ -518,18 +518,32 @@ defmodule PartitionedBuffer.Partition do
   end
 
   defp replace_match_spec(key, value, version) do
-    # Performance note: The key in the match head is a literal (bound value),
-    # not a pattern variable. This allows ETS to use its hash index for O(1)
-    # lookup rather than scanning the entire table.
+    # Fast path: for keys that do not contain a map, embed the key as a literal
+    # in the match head. On :set/:ordered_set tables a fully bound key lets ETS
+    # do a single keyed lookup instead of a full table scan (see the note about
+    # bound keys in :ets.select_replace/2). This is the common case, so we keep
+    # it O(1).
     #
-    # In match spec bodies, bare tuples are interpreted as operations/function
-    # calls, NOT as literal data. We wrap key and value with ms_literal/1 so
-    # tuples use the {{...}} constructor form and maps use {:const, map} that
-    # ETS understands. This handles tuples, maps, and lists (including nested
-    # combinations).
+    # Slow path: ETS rejects raw maps in the match head at the key position, so
+    # a key that contains a map anywhere would raise "not a valid match
+    # specification". For those keys we fall back to binding the key to "$3" and
+    # enforcing equality in the guard, which works for any term shape at the
+    # cost of a full scan. This only applies to map-containing keys, which
+    # triggered the original crash.
+    if key_has_map?(key) do
+      replace_match_spec_guarded(key, value, version)
+    else
+      replace_match_spec_literal(key, value, version)
+    end
+  end
+
+  defp replace_match_spec_literal(key, value, version) do
+    # In the match *head* the key is a structural pattern, so the raw term is
+    # used directly (tuples/lists match as-is). Only the *body* needs ms_literal/1
+    # to stop bare tuples from being read as match-spec operations.
     [
       {
-        # Match: {entry, key, value, existing_version, updates} where key is literal
+        # Match: {entry, key, value, existing_version, updates}
         entry(key: key, value: :_, version: :"$1", updates: :"$2"),
         # Guard (update only if): new_version > existing_version
         [{:>, version, :"$1"}],
@@ -546,6 +560,43 @@ defmodule PartitionedBuffer.Partition do
     ]
   end
 
+  defp replace_match_spec_guarded(key, value, version) do
+    # In match spec bodies and guards, bare tuples are interpreted as
+    # operations/function calls, NOT as literal data. We wrap key and value
+    # with ms_literal/1 so tuples use the {{...}} constructor form and maps use
+    # {:const, map} that ETS understands. This handles tuples, maps, and lists
+    # (including nested combinations).
+    [
+      {
+        # Match: {entry, key, value, existing_version, updates}
+        entry(key: :"$3", value: :_, version: :"$1", updates: :"$2"),
+        # Guard (update only if): key matches and new_version > existing_version
+        [{:>, version, :"$1"}, {:"=:=", :"$3", ms_literal(key)}],
+        # Result: the new entry with incremented updates counter
+        [
+          {entry(
+             key: :"$3",
+             value: ms_literal(value),
+             version: version,
+             updates: {:+, :"$2", 1}
+           )}
+        ]
+      }
+    ]
+  end
+
+  # Returns true if the term is, or contains anywhere, a map. Such keys cannot
+  # be embedded literally in an ETS match head, so they take the guarded path.
+  defp key_has_map?(value) when is_map(value), do: true
+
+  defp key_has_map?(value) when is_tuple(value) do
+    value |> Tuple.to_list() |> Enum.any?(&key_has_map?/1)
+  end
+
+  defp key_has_map?(value) when is_list(value), do: Enum.any?(value, &key_has_map?/1)
+
+  defp key_has_map?(_value), do: false
+
   # Wraps a term so it is safe to use as a literal in a match spec body.
   # In match spec bodies, bare tuples are interpreted as operations — not
   # data. The {{...}} form tells ETS to construct a tuple from its elements.

test/partitioned_buffer/map_test.exs

@@ -514,6 +514,34 @@ defmodule PartitionedBuffer.MapTest do
         M.put_all_newer(buff, entries)
       end
     end
+
+    test "ok: updates existing entry with a key containing a map", %{buffer: buff} do
+      key = {:user, %{id: 1}}
+
+      assert M.put_newer(buff, key, "v1", 100) == :ok
+      assert M.put_newer(buff, key, "v2", 200) == :ok
+
+      assert M.size(buff) == 1
+      assert M.get(buff, key) == "v2"
+
+      assert_receive {@processing_stop_event, %{duration: _, size: 1},
+                      %{buffer: ^buff, partition: _}},
+                     @default_timeout
+
+      assert_receive {:process_completed, [{^key, "v2", 200, 1}]}, @default_timeout
+    end
+
+    test "ok: updates existing entry with a map nested inside a list key", %{buffer: buff} do
+      key = [:a, [%{id: 1}], :b]
+
+      assert M.put_newer(buff, key, "v1", 100) == :ok
+      assert M.put_newer(buff, key, "v2", 200) == :ok
+
+      assert M.size(buff) == 1
+      assert M.get(buff, key) == "v2"
+
+      assert_receive {:process_completed, [{^key, "v2", 200, 1}]}, @default_timeout
+    end
   end
 
   describe "processing" do