fix: account for ETS matching maps when comparing shapes (#2824)

Author: icehaunter

.changeset/serious-cats-wink.md

@@ -0,0 +1,5 @@
+---
+"@core/sync-service": patch
+---
+
+fix: ensure correct shape lookup & comparison

packages/sync-service/lib/electric/replication/eval/expr.ex

@@ -70,7 +70,7 @@ defmodule Electric.Replication.Eval.Expr do
 
   defimpl Electric.Shapes.Shape.Comparable do
     def comparable(%Electric.Replication.Eval.Expr{} = expr) do
-      %{expr | eval: Electric.Shapes.Shape.Comparable.comparable(expr.eval)}
+      {:eval_expr, expr.query, expr.returns}
     end
   end
 end

packages/sync-service/lib/electric/shape_cache/shape_status.ex

@@ -179,13 +179,11 @@ defmodule Electric.ShapeCache.ShapeStatus do
   end
 
   def get_existing_shape(meta_table, %Shape{} = shape) do
-    case :ets.select(meta_table, [
-           {{{@shape_hash_lookup, Shape.comparable(shape)}, :"$1"}, [true], [:"$1"]}
-         ]) do
-      [] ->
+    case :ets.lookup_element(meta_table, {@shape_hash_lookup, Shape.comparable(shape)}, 2, nil) do
+      nil ->
         nil
 
-      [shape_handle] ->
+      shape_handle when is_binary(shape_handle) ->
         try do
           {shape_handle, latest_offset!(meta_table, shape_handle)}
         rescue

packages/sync-service/lib/electric/shapes/shape.ex

@@ -11,10 +11,16 @@ defmodule Electric.Shapes.Shape do
   require Logger
 
   defprotocol Comparable do
+    @fallback_to_any true
+
     @spec comparable(t()) :: t()
     def comparable(term)
   end
 
+  defimpl Comparable, for: Any do
+    def comparable(term), do: term
+  end
+
   @default_replica :default
 
   @enforce_keys [:root_table, :root_table_id]
@@ -67,14 +73,21 @@ defmodule Electric.Shapes.Shape do
           storage: %{required(String.t()) => String.t()}
         }
 
+  @doc """
+  Return a comparable representation of the shape.
+
+  This is used to compare shapes for equality as an ETS key - and thus it'll be matched in some cases,
+  not just compared equal. This representation must therefore not contain any maps (as they are matched when one
+  is missing a key for example).
+
+  This representation must contain all the information that identifies user-specified properties of the shape. We're
+  omitting storage configuration and other internal state.
+  """
   def comparable(%__MODULE__{} = shape) do
-    shape
-    |> Map.drop([:table_info, :storage])
-    |> Map.update!(:where, fn
-      nil -> nil
-      %Expr{} = expr -> Electric.Shapes.Shape.Comparable.comparable(expr)
-      statement when is_binary(statement) -> statement
-    end)
+    {:shape, {shape.root_table_id, shape.root_table}, shape.root_pk,
+     Comparable.comparable(shape.where), shape.selected_columns,
+     Enum.flat_map(shape.flags, fn {k, v} -> if(v, do: [k], else: []) end) |> Enum.sort(),
+     shape.replica}
   end
 
   def hash(%__MODULE__{} = shape),

packages/sync-service/test/electric/shape_cache_test.exs

@@ -127,6 +127,23 @@ defmodule Electric.ShapeCacheTest do
 
       assert shape_handle1 != shape_handle2
     end
+
+    test "should not return the same shape_handle for all columns and selected columns", %{
+      shape_cache_opts: opts
+    } do
+      alias Electric.Replication.Eval.Parser
+
+      shape1 = @shape
+
+      shape2 = Shape.new!("items", inspector: @stub_inspector, columns: ["id", "value"])
+
+      {shape_handle2, @zero_offset} = ShapeCache.get_or_create_shape_handle(shape2, opts)
+      {shape_handle1, @zero_offset} = ShapeCache.get_or_create_shape_handle(shape1, opts)
+
+      assert shape_handle1 != shape_handle2
+
+      assert {^shape_handle2, @zero_offset} = ShapeCache.get_or_create_shape_handle(shape2, opts)
+    end
   end
 
   describe "get_or_create_shape_handle/2 shape initialization" do