fix(sync-service): allow DELETE /v1/shape to resolve a shape by its definition (#4754)

DeleteShapePlug narrowed the incoming params to ["table", "handle"]
before handing them to Api.validate_for_delete/2, dropping
where/columns/params/ replica/log. The shape passed to
Shapes.fetch_handle_by_shape/2 was therefore always the bare-table
shape, so any shape defined with a where clause or a column selection
could only ever be deleted by handle.

Widen the take list to every param that Api.Params.define_shape/2 reads
and add plug- and router-level regression tests; the existing
api_test.exs coverage calls validate_for_delete/2 directly and so
bypasses the plug.

<!-- codesmith:footer -->
---
<a
href="https://app.blacksmith.sh/electric-sql/codesmith/electric/pr/4754"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img
alt="View with [code]smith"
src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a>
<a
href="https://backend.blacksmith.sh/track/enable-autofix?expires=1788920734&installation_model_id=8736&pr_number=4754&repository=electric-sql%2Felectric&return_to=https%3A%2F%2Fgithub.com%2Felectric-sql%2Felectric%2Fpull%2F4754&signature=16dba5ae9a134733732fa7467c22ca46a79ca591c0f47db17b7f97464143979f"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img
alt="Autofix with [code]smith"
src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a>
<sup>Need help on this PR? Tag <code>@codesmith-bot</code> with what you
need. Autofix is disabled.</sup>

<!-- codesmith:autofix:disabled -->
<!-- /codesmith:footer -->

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Oleksii Sholik <oleksii@sholik.dev>

Author: 3 people

.changeset/delete-shape-by-definition.md

@@ -0,0 +1,5 @@
+---
+'@core/sync-service': patch
+---
+
+Fix `DELETE /v1/shape` so a shape can be deleted by its definition. The delete plug discarded every shape parameter other than `table` and `handle`, so any shape with a `where`, `columns`, `params`, `replica` or `log` could only ever be deleted by handle.

packages/sync-service/lib/electric/plug/delete_shape_plug.ex

@@ -12,10 +12,11 @@ defmodule Electric.Plug.DeleteShapePlug do
   defp validate_request(%Plug.Conn{assigns: %{config: config}} = conn, _) do
     api = Access.fetch!(config, :api)
 
-    all_params =
-      Map.merge(conn.query_params, conn.path_params)
-      |> Map.take(["table", "handle"])
-      |> Map.put("offset", "-1")
+    # No filtering here: `Api.Params` casting drops unknown parameters and
+    # `validate_for_delete/2` ignores request-only ones such as `offset` and
+    # `live`, so the shape definition is built from the same parameters a GET
+    # request would use.
+    all_params = Map.merge(conn.query_params, conn.path_params)
 
     case Api.validate_for_delete(api, all_params) do
       {:ok, request} ->

packages/sync-service/test/electric/plug/delete_shape_plug_test.exs

@@ -21,6 +21,7 @@ defmodule Electric.Plug.DeleteShapePlugTest do
     flags: %{selects_all_columns: true}
   }
   @test_pg_id "12345"
+  @inspector {__MODULE__, []}
 
   def load_column_info(@users_oid, _),
     do:
@@ -38,6 +39,8 @@ defmodule Electric.Plug.DeleteShapePlugTest do
     Plug.Test.conn(method, "/" <> query_string)
   end
 
+  def query_string(params), do: "?" <> URI.encode_query(params)
+
   def call_delete_shape_plug(conn, ctx, allow \\ true) do
     config =
       Electric.Shapes.Api.plug_opts(
@@ -47,7 +50,7 @@ defmodule Electric.Plug.DeleteShapePlugTest do
         pg_id: @test_pg_id,
         shape_cache: {Electric.ShapeCache, []},
         storage: {Mock.Storage, []},
-        inspector: {__MODULE__, []},
+        inspector: @inspector,
         registry: @registry,
         long_poll_timeout: Access.get(ctx, :long_poll_timeout, 20_000),
         max_age: Access.get(ctx, :max_age, 60),
@@ -135,6 +138,24 @@ defmodule Electric.Plug.DeleteShapePlugTest do
              }
     end
 
+    test "returns 400 for a malformed request-only param", ctx do
+      # Request-only params such as `live` are ignored by delete validation,
+      # but they still go through schema casting, so malformed values are
+      # rejected rather than silently dropped.
+      conn =
+        ctx
+        |> conn("DELETE", query_string(table: "public.users", live: "banana"))
+        |> call_delete_shape_plug(ctx)
+
+      assert conn.status == 400
+      assert Plug.Conn.get_resp_header(conn, "cache-control") == ["no-cache"]
+
+      assert Jason.decode!(conn.resp_body) == %{
+               "message" => "Invalid request",
+               "errors" => %{"live" => ["is invalid"]}
+             }
+    end
+
     test "should clean shape based on shape definition", ctx do
       %{stack_id: stack_id} = ctx
 
@@ -151,6 +172,63 @@ defmodule Electric.Plug.DeleteShapePlugTest do
       assert Plug.Conn.get_resp_header(conn, "cache-control") == ["no-cache"]
     end
 
+    test "should clean shape based on shape definition with a where clause", ctx do
+      %{stack_id: stack_id} = ctx
+
+      shape = Shape.new!("public.users", where: "id = 1", inspector: @inspector)
+      {:ok, shape_handle} = Electric.ShapeCache.ShapeStatus.add_shape(stack_id, shape)
+
+      expect_shape_cache(clean_shape: fn ^shape_handle, ^stack_id -> :ok end)
+
+      conn =
+        ctx
+        |> conn(:delete, query_string(table: "public.users", where: "id = 1"))
+        |> call_delete_shape_plug(ctx)
+
+      assert conn.status == 202
+    end
+
+    test "should clean shape based on shape definition with a parameterised where clause", ctx do
+      %{stack_id: stack_id} = ctx
+
+      shape =
+        Shape.new!("public.users",
+          where: "id = $1",
+          params: %{"1" => "1"},
+          inspector: @inspector
+        )
+
+      {:ok, shape_handle} = Electric.ShapeCache.ShapeStatus.add_shape(stack_id, shape)
+
+      expect_shape_cache(clean_shape: fn ^shape_handle, ^stack_id -> :ok end)
+
+      conn =
+        ctx
+        |> conn(
+          :delete,
+          query_string([{"table", "public.users"}, {"where", "id = $1"}, {"params[1]", "1"}])
+        )
+        |> call_delete_shape_plug(ctx)
+
+      assert conn.status == 202
+    end
+
+    test "should clean shape based on shape definition with a column selection", ctx do
+      %{stack_id: stack_id} = ctx
+
+      shape = Shape.new!("public.users", columns: ["id"], inspector: @inspector)
+      {:ok, shape_handle} = Electric.ShapeCache.ShapeStatus.add_shape(stack_id, shape)
+
+      expect_shape_cache(clean_shape: fn ^shape_handle, ^stack_id -> :ok end)
+
+      conn =
+        ctx
+        |> conn(:delete, query_string(table: "public.users", columns: "id"))
+        |> call_delete_shape_plug(ctx)
+
+      assert conn.status == 202
+    end
+
     test "should clean shape based only on shape_handle", ctx do
       %{stack_id: stack_id} = ctx
 

packages/sync-service/test/electric/plug/router_test.exs

@@ -292,6 +292,32 @@ defmodule Electric.Plug.RouterTest do
              ] = response
     end
 
+    @tag with_sql: [
+           "INSERT INTO items VALUES (gen_random_uuid(), 'test value 1')"
+         ]
+    test "DELETE deletes a shape that was defined with a where clause", %{opts: opts} do
+      where = "value = 'test value 1'"
+
+      conn =
+        conn("GET", "/v1/shape?table=items&offset=-1", %{where: where})
+        |> Router.call(opts)
+
+      assert %{status: 200} = conn
+      shape1_handle = get_resp_shape_handle(conn)
+
+      # The shape must be resolvable by its definition, not just by its handle.
+      assert %{status: 202} =
+               conn("DELETE", "/v1/shape?" <> URI.encode_query(table: "items", where: where))
+               |> Router.call(opts)
+
+      conn =
+        conn("GET", "/v1/shape?table=items&offset=-1", %{where: where})
+        |> Router.call(opts)
+
+      assert %{status: 200} = conn
+      assert get_resp_shape_handle(conn) != shape1_handle
+    end
+
     @tag with_sql: ["INSERT INTO items VALUES (gen_random_uuid(), 'test value 1')"]
     test "follows a table and returns last-seen lsn", %{
       opts: opts,