fix: fix invalid select on sorting by some calculations

Author: zachdaniel

lib/data_layer.ex

@@ -759,7 +759,7 @@ defmodule AshPostgres.DataLayer do
            query,
            AshSql.repo_opts(repo, AshPostgres.SqlImplementation, nil, nil, resource)
          )
-         |> remap_mapped_fields(query)}
+         |> AshSql.Query.remap_mapped_fields(query)}
       end)
     end
   rescue
@@ -942,7 +942,7 @@ defmodule AshPostgres.DataLayer do
         path
       ) do
     {calculations_require_rewrite, aggregates_require_rewrite, query} =
-      rewrite_nested_selects(query)
+      AshSql.Query.rewrite_nested_selects(query)
 
     case lateral_join_query(
            query,
@@ -964,7 +964,11 @@ defmodule AshPostgres.DataLayer do
             lateral_join_query,
             AshSql.repo_opts(repo, AshPostgres.SqlImplementation, nil, nil, source_resource)
           )
-          |> remap_mapped_fields(query, calculations_require_rewrite, aggregates_require_rewrite)
+          |> AshSql.Query.remap_mapped_fields(
+            query,
+            calculations_require_rewrite,
+            aggregates_require_rewrite
+          )
 
         {:ok, results}
 
@@ -973,100 +977,6 @@ defmodule AshPostgres.DataLayer do
     end
   end
 
-  defp rewrite_nested_selects(query) do
-    case query.select do
-      %Ecto.Query.SelectExpr{
-        expr:
-          {:merge, [],
-           [
-             {:&, [], [0]},
-             {:%{}, [], merging}
-           ]}
-      } = select ->
-        {merging, aggregate_merges} = remap_sub_select(merging, :aggregates)
-
-        {new_sub_selects, calculation_merges} =
-          remap_sub_select(merging, :calculations)
-
-        new_query =
-          %{
-            query
-            | select: %{select | expr: {:merge, [], [{:&, [], [0]}, {:%{}, [], new_sub_selects}]}}
-          }
-
-        {calculation_merges, aggregate_merges, new_query}
-
-      _ ->
-        {%{}, %{}, query}
-    end
-  end
-
-  # sobelow_skip ["DOS.StringToAtom"]
-  defp remap_sub_select(merging, sub_key) do
-    case Keyword.fetch(merging, sub_key) do
-      {:ok, {:%{}, [], nested}} ->
-        Enum.reduce(nested, {Keyword.delete(merging, sub_key), %{}}, fn {name, expr},
-                                                                        {subselect, remapping} ->
-          new_name = String.to_atom("__#{sub_key}__#{name}")
-          {Keyword.put(subselect, new_name, expr), Map.put(remapping, new_name, name)}
-        end)
-
-      :error ->
-        {merging, %{}}
-    end
-  end
-
-  defp remap_mapped_fields(
-         results,
-         query,
-         calculations_require_rewrite \\ %{},
-         aggregates_require_rewrite \\ %{}
-       ) do
-    calculation_names = query.__ash_bindings__.calculation_names
-    aggregate_names = query.__ash_bindings__.aggregate_names
-
-    if Enum.empty?(calculation_names) and Enum.empty?(aggregate_names) and
-         Enum.empty?(calculations_require_rewrite) and Enum.empty?(aggregates_require_rewrite) do
-      results
-    else
-      Enum.map(results, fn result ->
-        result
-        |> remap_to_nested(:calculations, calculations_require_rewrite)
-        |> remap_to_nested(:aggregates, aggregates_require_rewrite)
-        |> remap(:calculations, calculation_names)
-        |> remap(:aggregates, aggregate_names)
-      end)
-    end
-  end
-
-  defp remap_to_nested(record, _subfield, mapping) when mapping == %{} do
-    record
-  end
-
-  defp remap_to_nested(record, subfield, mapping) do
-    Map.update!(record, subfield, fn subfield_values ->
-      Enum.reduce(mapping, subfield_values, fn {source, dest}, subfield_values ->
-        subfield_values
-        |> Map.put(dest, Map.get(record, source))
-        |> Map.delete(source)
-      end)
-    end)
-  end
-
-  defp remap(record, _subfield, mapping) when mapping == %{} do
-    record
-  end
-
-  defp remap(record, subfield, mapping) do
-    Map.update!(record, subfield, fn subfield_values ->
-      Enum.reduce(mapping, subfield_values, fn {dest, source}, subfield_values ->
-        subfield_values
-        |> Map.put(dest, Map.get(subfield_values, source))
-        |> Map.delete(source)
-      end)
-    end)
-  end
-
   defp lateral_join_query(
          query,
          root_data,
@@ -1425,7 +1335,7 @@ defmodule AshPostgres.DataLayer do
                 end)
 
               if options[:return_records?] do
-                {:ok, remap_mapped_fields(results, query)}
+                {:ok, AshSql.Query.remap_mapped_fields(results, query)}
               else
                 :ok
               end
@@ -1691,7 +1601,7 @@ defmodule AshPostgres.DataLayer do
             end)
 
           if options[:return_records?] do
-            {:ok, remap_mapped_fields(results, query)}
+            {:ok, AshSql.Query.remap_mapped_fields(results, query)}
           else
             :ok
           end

mix.exs

@@ -163,7 +163,7 @@ defmodule AshPostgres.MixProject do
   defp deps do
     [
       {:ash, ash_version("~> 3.0 and >= 3.0.7")},
-      {:ash_sql, ash_sql_version("~> 0.2 and >= 0.2.3")},
+      {:ash_sql, ash_sql_version("~> 0.2 and >= 0.2.4")},
       {:ecto_sql, "~> 3.9"},
       {:ecto, "~> 3.9"},
       {:jason, "~> 1.0"},

mix.lock

@@ -34,7 +34,7 @@
   "spark": {:hex, :spark, "2.1.24", "fb596da83ee85b83f27f3a98df226869963ec5e423b4137884a6176f332e84ff", [:mix], [{:jason, "~> 1.4", [hex: :jason, repo: "hexpm", optional: false]}, {:sourceror, "~> 1.0", [hex: :sourceror, repo: "hexpm", optional: false]}], "hexpm", "e57f62fe1de1b327c2bfe04473496497edb6817371ba677099389deda38da90d"},
   "splode": {:hex, :splode, "0.2.4", "71046334c39605095ca4bed5d008372e56454060997da14f9868534c17b84b53", [:mix], [], "hexpm", "ca3b95f0d8d4b482b5357954fec857abd0fa3ea509d623334c1328e7382044c2"},
   "statistex": {:hex, :statistex, "1.0.0", "f3dc93f3c0c6c92e5f291704cf62b99b553253d7969e9a5fa713e5481cd858a5", [:mix], [], "hexpm", "ff9d8bee7035028ab4742ff52fc80a2aa35cece833cf5319009b52f1b5a86c27"},
-  "stream_data": {:hex, :stream_data, "1.1.0", "ef3a7cac0f200c43caf3e6caf9be63115851b4f1cde3f21afaab220adc40e3d7", [:mix], [], "hexpm", "cccc411d5facf1bab86e7c671382d164f05f8992574c95349d3c8b317e14d953"},
+  "stream_data": {:hex, :stream_data, "1.1.1", "fd515ca95619cca83ba08b20f5e814aaf1e5ebff114659dc9731f966c9226246", [:mix], [], "hexpm", "45d0cd46bd06738463fd53f22b70042dbb58c384bb99ef4e7576e7bb7d3b8c8c"},
   "telemetry": {:hex, :telemetry, "1.2.1", "68fdfe8d8f05a8428483a97d7aab2f268aaff24b49e0f599faa091f1d4e7f61c", [:rebar3], [], "hexpm", "dad9ce9d8effc621708f99eac538ef1cbe05d6a874dd741de2e689c47feafed5"},
   "typable": {:hex, :typable, "0.3.0", "0431e121d124cd26f312123e313d2689b9a5322b15add65d424c07779eaa3ca1", [:mix], [], "hexpm", "880a0797752da1a4c508ac48f94711e04c86156f498065a83d160eef945858f8"},
   "yamerl": {:hex, :yamerl, "0.10.0", "4ff81fee2f1f6a46f1700c0d880b24d193ddb74bd14ef42cb0bcf46e81ef2f8e", [:rebar3], [], "hexpm", "346adb2963f1051dc837a2364e4acf6eb7d80097c0f53cbdc3046ec8ec4b4e6e"},

test/sort_test.exs

@@ -231,20 +231,18 @@ defmodule AshPostgres.SortTest do
       |> Ash.Changeset.for_create(:create, %{title: "aaa", score: 0})
       |> Ash.create!()
 
-    view1 =
-      PostView
-      |> Ash.Changeset.for_action(:create, %{browser: :firefox, post_id: post1.id})
-      |> Ash.create!()
+    PostView
+    |> Ash.Changeset.for_action(:create, %{browser: :firefox, post_id: post1.id})
+    |> Ash.create!()
 
     post2 =
       Post
       |> Ash.Changeset.for_create(:create, %{title: "bbb", score: 0})
       |> Ash.create!()
 
-    view2 =
-      PostView
-      |> Ash.Changeset.for_action(:create, %{browser: :chrome, post_id: post2.id})
-      |> Ash.create!()
+    PostView
+    |> Ash.Changeset.for_action(:create, %{browser: :chrome, post_id: post2.id})
+    |> Ash.create!()
 
     assert [
              %{title: "aaa", views: [%{browser: :firefox}]},
@@ -263,6 +261,8 @@ defmodule AshPostgres.SortTest do
              Ash.read!(
                Post
                |> Ash.Query.load(:views)
+               # this doesn't really make sense to do, you'd want to do something like `max(views, field: :time)` or something.
+               # but it illustrates a bug fix, and nothing currently prevents you from doing it, so we keep the test for now.
                |> Ash.Query.sort({Ash.Sort.expr_sort(views.time, :datetime), :desc}, title: :asc)
              )
   end