Include cache headers in 304 responses (#2061)

Fixes #2059

.changeset/nine-gorillas-rush.md

@@ -0,0 +1,5 @@
+---
+"@core/sync-service": patch
+---
+
+Include caching headers on 304 responses to prevent client from rechecking the previously cached ones over and over again.

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

@@ -166,9 +166,9 @@ defmodule Electric.Plug.ServeShapePlug do
   plug :listen_for_new_changes
   plug :determine_log_chunk_offset
   plug :determine_up_to_date
+  plug :put_resp_cache_headers
   plug :generate_etag
   plug :validate_and_put_etag
-  plug :put_resp_cache_headers
   plug :serve_log_or_snapshot
 
   # end_telemetry_span needs to always be the last plug here.
@@ -351,7 +351,7 @@ defmodule Electric.Plug.ServeShapePlug do
       get_req_header(conn, "if-none-match")
       |> Enum.flat_map(&String.split(&1, ","))
       |> Enum.map(&String.trim/1)
-      |> Enum.map(&String.trim(&1, ~S|"|))
+      |> Enum.map(&String.trim(&1, <<?">>))
 
     cond do
       conn.assigns.etag in if_none_match ->
@@ -378,7 +378,7 @@ defmodule Electric.Plug.ServeShapePlug do
         "public, max-age=604800, s-maxage=3600, stale-while-revalidate=2629746"
       )
 
-  # For live requests we want shorrt cache lifetimes and to update the live cursor
+  # For live requests we want short cache lifetimes and to update the live cursor
   defp put_resp_cache_headers(%Conn{assigns: %{live: true}} = conn, _),
     do:
       conn

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

@@ -68,9 +68,9 @@ defmodule Electric.Plug.ServeShapePlugTest do
       storage: {Mock.Storage, []},
       inspector: {__MODULE__, []},
       registry: @registry,
-      long_poll_timeout: Access.get(ctx, :long_poll_timeout, 20_000),
-      max_age: Access.get(ctx, :max_age, 60),
-      stale_age: Access.get(ctx, :stale_age, 300)
+      long_poll_timeout: long_poll_timeout(ctx),
+      max_age: max_age(ctx),
+      stale_age: stale_age(ctx)
     ]
 
     ServeShapePlug.call(conn, config)
@@ -364,23 +364,44 @@ defmodule Electric.Plug.ServeShapePlugTest do
         @test_offset
       end)
 
-      conn =
-        ctx
-        |> conn(
-          :get,
-          %{"table" => "public.users"},
-          "?offset=#{@start_offset_50}&handle=#{@test_shape_handle}"
-        )
-        |> put_req_header(
-          "if-none-match",
-          ~s("#{@test_shape_handle}:#{@start_offset_50}:#{@test_offset}")
-        )
-        |> call_serve_shape_plug(ctx)
-
+      conn = get_shape_with_etag(ctx, @start_offset_50)
       assert conn.status == 304
       assert conn.resp_body == ""
     end
 
+    test "the 304 response includes caching headers that are appropriate for the offset", ctx do
+      Mock.ShapeCache
+      |> stub(:has_shape?, fn @test_shape_handle, _opts -> true end)
+      |> expect(:get_shape, 3, fn @test_shape, _opts -> {@test_shape_handle, @test_offset} end)
+
+      Mock.Storage
+      |> stub(:for_shape, fn @test_shape_handle, _opts -> @test_opts end)
+      |> expect(:get_chunk_end_log_offset, fn @before_all_offset, _ -> @test_offset end)
+      |> expect(:get_chunk_end_log_offset, 2, fn @start_offset_50, _ -> @test_offset end)
+
+      conn = get_shape_with_etag(ctx, @before_all_offset)
+      assert conn.status == 304
+      cache_control = "public, max-age=604800, s-maxage=3600, stale-while-revalidate=2629746"
+      assert {"cache-control", cache_control} in conn.resp_headers
+
+      conn = get_shape_with_etag(ctx, @start_offset_50)
+      assert conn.status == 304
+      cache_control = "public, max-age=#{max_age(ctx)}, stale-while-revalidate=#{stale_age(ctx)}"
+      assert {"cache-control", cache_control} in conn.resp_headers
+
+      conn = get_shape_with_etag(ctx, @start_offset_50, live: true)
+      assert conn.status == 304
+
+      cache_control = "public, max-age=5, stale-while-revalidate=5"
+      assert {"cache-control", cache_control} in conn.resp_headers
+
+      expected_cursor =
+        Electric.Plug.Utils.get_next_interval_timestamp(long_poll_timeout(ctx), nil)
+        |> to_string()
+
+      assert {"electric-cursor", expected_cursor} in conn.resp_headers
+    end
+
     test "handles live updates", ctx do
       Mock.ShapeCache
       |> expect(:get_shape, fn @test_shape, _opts ->
@@ -731,4 +752,21 @@ defmodule Electric.Plug.ServeShapePlugTest do
       assert conn.status == 400
     end
   end
+
+  defp get_shape_with_etag(ctx, offset, extra_query_params \\ []) do
+    query_str =
+      URI.encode_query([offset: offset, handle: @test_shape_handle] ++ extra_query_params)
+
+    ctx
+    |> conn(:get, %{"table" => "public.users"}, "?" <> query_str)
+    |> put_req_header(
+      "if-none-match",
+      ~s("#{@test_shape_handle}:#{offset}:#{@test_offset}")
+    )
+    |> call_serve_shape_plug(ctx)
+  end
+
+  defp max_age(ctx), do: Access.get(ctx, :max_age, 60)
+  defp stale_age(ctx), do: Access.get(ctx, :stale_age, 300)
+  defp long_poll_timeout(ctx), do: Access.get(ctx, :long_poll_timeout, 20_000)
 end