Skip the default cache_statement when :comments are given

Author: alesasnouski

integration_test/sql/logging.exs

@@ -199,12 +199,18 @@ defmodule Ecto.Integration.LoggingTest do
              end) =~ ~r{/\* before_q \*/ SELECT.* /\* after_q \*/}
     end
 
-    test "renders with query_cache: false (the escape hatch for dynamic comments)" do
+    test "dynamic comments render and skip the query cache by default" do
       assert capture_log(fn ->
-               TestRepo.all(Post, comments: [pre: "dyn_#{System.unique_integer()}"], query_cache: false, log: :error)
+               TestRepo.all(Post, comments: [pre: "dyn_#{System.unique_integer()}"], log: :error)
              end) =~ ~r{/\* dyn_-?\d+ \*/ SELECT}
     end
 
+    test "query_cache: true opts back into caching for static comments" do
+      assert capture_log(fn ->
+               TestRepo.all(Post, comments: [pre: "static_q"], query_cache: true, log: :error)
+             end) =~ "/* static_q */ SELECT"
+    end
+
     test "comments insert/update/delete/insert_all" do
       assert capture_log(fn ->
                TestRepo.insert!(%Post{title: "1"}, comments: [pre: "insert_create_post_q"], log: :error)

lib/ecto/adapters/myxql.ex

@@ -373,15 +373,9 @@ defmodule Ecto.Adapters.MyXQL do
     insert_opts = if opts[:insert_mode], do: [insert_mode: opts[:insert_mode]], else: []
     sql = @conn.insert(prefix, source, fields, [fields], on_conflict, [], [], insert_opts)
 
-    opts =
-      if is_nil(Keyword.get(opts, :cache_statement)) do
-        [{:cache_statement, "ecto_insert_#{source}_#{length(fields)}"} | opts]
-      else
-        opts
-      end
-
     # This adapter overrides insert/6 instead of going through
     # Ecto.Adapters.SQL.struct/10, so wrap the `:comments` here too.
+    opts = Ecto.Adapters.SQL.put_default_cache_statement(opts, "ecto_insert_#{source}_#{length(fields)}")
     sql = Ecto.Adapters.SQL.wrap_comments(sql, opts)
 
     case Ecto.Adapters.SQL.query(adapter_meta, sql, values ++ query_params, opts) do

lib/ecto/adapters/sql.ex

@@ -985,13 +985,7 @@ defmodule Ecto.Adapters.SQL do
 
     sql = conn.insert(prefix, source, header, rows, on_conflict, returning, placeholders, opts)
 
-    opts =
-      if is_nil(Keyword.get(opts, :cache_statement)) do
-        [{:cache_statement, "ecto_insert_all_#{source}"} | opts]
-      else
-        opts
-      end
-
+    opts = put_default_cache_statement(opts, "ecto_insert_all_#{source}")
     sql = wrap_comments(sql, opts)
 
     all_params = placeholders ++ Enum.reverse(params, conflict_params)
@@ -1179,6 +1173,20 @@ defmodule Ecto.Adapters.SQL do
     [pre, sql | post]
   end
 
+  # Comments become part of the statement text, so a varying comment under a
+  # fixed cache name would make the driver close and re-prepare the statement
+  # on every call (drivers compare the cached text). Skip the default statement
+  # cache whenever comments are given; an explicit :cache_statement still wins,
+  # which keeps caching available for callers with static comments.
+  @doc false
+  def put_default_cache_statement(opts, name) do
+    if is_nil(Keyword.get(opts, :cache_statement)) and Keyword.get(opts, :comments, []) == [] do
+      [{:cache_statement, name} | opts]
+    else
+      opts
+    end
+  end
+
   @doc false
   def comments(comments) when is_list(comments) do
     {pre, post} =
@@ -1222,13 +1230,7 @@ defmodule Ecto.Adapters.SQL do
         returning,
         opts
       ) do
-    opts =
-      if is_nil(Keyword.get(opts, :cache_statement)) do
-        [{:cache_statement, "ecto_#{operation}_#{source}_#{length(params)}"} | opts]
-      else
-        opts
-      end
-
+    opts = put_default_cache_statement(opts, "ecto_#{operation}_#{source}_#{length(params)}")
     sql = wrap_comments(sql, opts)
 
     case query(adapter_meta, sql, values, [source: source] ++ opts) do

test/ecto/adapters/sql_test.exs

@@ -54,4 +54,41 @@ defmodule Ecto.Adapters.SQLTest do
       assert wrap("INSERT INTO posts ...", timeout: 5000) == "INSERT INTO posts ..."
     end
   end
+
+  describe "put_default_cache_statement/2" do
+    test "sets the default name" do
+      opts = Ecto.Adapters.SQL.put_default_cache_statement([timeout: 5000], "ecto_insert_posts")
+      assert Keyword.get(opts, :cache_statement) == "ecto_insert_posts"
+    end
+
+    test "honors an explicit :cache_statement" do
+      opts = Ecto.Adapters.SQL.put_default_cache_statement([cache_statement: "mine"], "default")
+      assert Keyword.get(opts, :cache_statement) == "mine"
+    end
+
+    test "skips the default when comments are given" do
+      opts =
+        Ecto.Adapters.SQL.put_default_cache_statement(
+          [comments: [pre: "dyn_123"]],
+          "ecto_insert_posts"
+        )
+
+      assert Keyword.get(opts, :cache_statement) == nil
+    end
+
+    test "an explicit :cache_statement wins even with comments" do
+      opts =
+        Ecto.Adapters.SQL.put_default_cache_statement(
+          [comments: [pre: "static_tag"], cache_statement: "mine"],
+          "default"
+        )
+
+      assert Keyword.get(opts, :cache_statement) == "mine"
+    end
+
+    test "an empty :comments list still gets the default" do
+      opts = Ecto.Adapters.SQL.put_default_cache_statement([comments: []], "ecto_insert_posts")
+      assert Keyword.get(opts, :cache_statement) == "ecto_insert_posts"
+    end
+  end
 end