Repository: elixir-ecto/ecto
Pull request #4359 (2024-01-28 09:32:18)

Fix attach_prefix
It looks like this broke in one of the recent commits

josevalim: Thank you for cleaning up my mess.
greg-rychlewski: It seems like some migration tests are failing due to using atom prefixes. And the type specs are saying they allow atom:https://github.com/elixir-ecto/ecto_sql/blob/master/lib/ecto/migration.ex#L485

Maybe it is ok for prefix to be an atom? From the stacktrace it seems like it's raising while checking the completed migrations

```elixir
(ecto 3.11.1) lib/ecto/query/planner.ex:1815: Ecto.Query.Planner.attach_prefix/2
   +integration-test *failed* |        (ecto 3.11.1) lib/ecto/repo/queryable.ex:211: Ecto.Repo.Queryable.execute/4
   +integration-test *failed* |        (ecto 3.11.1) lib/ecto/repo/queryable.ex:19: Ecto.Repo.Queryable.all/3
   +integration-test *failed* |        (ecto_sql 3.11.1) lib/ecto/migrator.ex:586: anonymous fn/5 in Ecto.Migrator.lock_for_migrations/4
   +integration-test *failed* |        (ecto_sql 3.11.1) lib/ecto/migrator.ex:585: Ecto.Migrator.lock_for_migrations/4
   +integration-test *failed* |        test/ecto/migrator_test.exs:262: anonymous fn/0 in Ecto.MigratorTest."test migrator prefix"/1
   +integration-test *failed* |        (ex_unit 1.15.6) lib/ex_unit/capture_log.ex:113: ExUnit.CaptureLog.with_log/2
   +integration-test *failed* |        (ex_unit 1.15.6) lib/ex_unit/capture_log.ex:75: ExUnit.CaptureLog.capture_log/2
   +integration-test *failed* |        test/ecto/migrator_test.exs:261: (test)
```

So probably atom prefixes work with regular queries?
josevalim: The query layer requires them to be strings at compile-time and I think normalizing them to strings is a good choice. So I would change migrations typespecs to force them to be strings and add a deprecation warning to attach_prefix if they are atoms. Thoughts?
greg-rychlewski: > The query layer requires them to be strings at compile-time 

Oh I see what you mean now. What you are saying makes sense to me.

> add a deprecation warning to attach_prefix if they are atoms

Do you mean something like this:

```elixir
def attach_prefix(%{prefix: nil} = query, opts) when is_list(opts) do
    case Keyword.fetch(opts, :prefix) do
      {:ok, prefix} when is_binary(prefix) or is_nil(prefix) ->
        %{query | prefix: prefix}
      
       {:ok, prefix} when is_atom(prefix)->
         Logger.warning("atom prefixes are deprecated")
         %{query | prefix: prefix}
      
      :error ->
        query
    end
  end
```


Patch:
diff --git a/Earthfile b/Earthfile
index 3b98e575..d626dc68 100644
--- a/Earthfile
+++ b/Earthfile
@@ -1,4 +1,4 @@
-VERSION 0.5
+VERSION 0.6
 
 all:
     BUILD \
diff --git a/lib/ecto/query.ex b/lib/ecto/query.ex
index f4cddd12..87df08c2 100644
--- a/lib/ecto/query.ex
+++ b/lib/ecto/query.ex
@@ -2838,6 +2838,13 @@ defmodule Ecto.Query do
           "callback function for with_named_binding/3 should return an Ecto.Query struct, got: #{inspect(other)}"
   end
 
+  @doc """
+  The same as `has_named_binding?/2` but allowed in guards.
+  """
+  @doc guard: true
+  defguard is_named_binding(query, name)
+           when is_struct(query, Ecto.Query) and is_map_key(query.aliases, name)
+
   @doc """
   Reverses the ordering of the query.
 
diff --git a/lib/ecto/query/planner.ex b/lib/ecto/query/planner.ex
index 8c999cf1..15b690be 100644
--- a/lib/ecto/query/planner.ex
+++ b/lib/ecto/query/planner.ex
@@ -1811,9 +1811,15 @@ defmodule Ecto.Query.Planner do
   """
   def attach_prefix(%{prefix: nil} = query, opts) when is_list(opts) do
     case Keyword.fetch(opts, :prefix) do
-      {:ok, prefix} when is_binary(prefix) -> %{query | prefix: prefix}
-      {:ok, prefix} -> raise ArgumentError, ":prefix must be a binary, got: #{inspect(prefix)}"
-      :error -> query
+      {:ok, prefix} when is_binary(prefix) or is_nil(prefix) ->
+        %{query | prefix: prefix}
+
+      {:ok, prefix} when is_atom(prefix)->
+        IO.warn("atom prefixes are deprecated. Please use a string instead.")
+        %{query | prefix: prefix}
+
+      :error ->
+        query
     end
   end
 


Test patch:
diff --git a/test/ecto/query_test.exs b/test/ecto/query_test.exs
index 0ff4762e..4b87073f 100644
--- a/test/ecto/query_test.exs
+++ b/test/ecto/query_test.exs
@@ -1076,6 +1076,15 @@ defmodule Ecto.QueryTest do
                    ~r"protocol Ecto.Queryable not implemented for \[\]",
                    fn -> has_named_binding?([], :posts) end
     end
+
+    test "is_named_binding/2 guard" do
+      named_binding_query = from p in "posts", as: :posts
+      no_binding_query = from p in "posts"
+
+      assert is_named_binding(named_binding_query, :posts)
+      refute is_named_binding(no_binding_query, :posts)
+      refute is_named_binding("posts", :posts)
+    end
   end
 
   describe "with_named_binding/3" do