fix: Don't throw exceptions raised based on missing context while validating expr refs (#2826)

Author: Rutgerdj

lib/ash/actions/read/read.ex

@@ -1944,49 +1944,22 @@ defmodule Ash.Actions.Read do
       end)
       |> Enum.reduce_while({:ok, []}, fn
         {%Ash.Resource.Calculation{} = resource_calculation, direction}, {:ok, sort} ->
-          case Ash.Query.Calculation.from_resource_calculation(
-                 query.resource,
-                 resource_calculation,
-                 source_context: query.context
-               ) do
-            {:ok, calc} ->
-              case hydrate_calculations(query, [calc]) do
-                {:ok, [{calc, expression}]} ->
-                  {:cont,
-                   {:ok,
-                    [
-                      {%{
-                         calc
-                         | module: Ash.Resource.Calculation.Expression,
-                           opts: [expr: expression]
-                       }, direction}
-                      | sort
-                    ]}}
-
-                {:error, error} ->
-                  {:halt, {:error, error}}
-              end
-
-            {:error, error} ->
-              {:halt, {:error, error}}
+          with {:ok, calc} <-
+                 Ash.Query.Calculation.from_resource_calculation(
+                   query.resource,
+                   resource_calculation,
+                   source_context: query.context
+                 ),
+               {:ok, entry} <- hydrate_sort_calculation(query, calc, direction) do
+            {:cont, {:ok, [entry | sort]}}
+          else
+            {:error, error} -> {:halt, {:error, error}}
           end
 
         {%Ash.Query.Calculation{} = calc, direction}, {:ok, sort} ->
-          case hydrate_calculations(query, [calc]) do
-            {:ok, [{calc, expression}]} ->
-              {:cont,
-               {:ok,
-                [
-                  {%{
-                     calc
-                     | module: Ash.Resource.Calculation.Expression,
-                       opts: [expr: expression]
-                   }, direction}
-                  | sort
-                ]}}
-
-            {:error, error} ->
-              {:halt, {:error, error}}
+          case hydrate_sort_calculation(query, calc, direction) do
+            {:ok, entry} -> {:cont, {:ok, [entry | sort]}}
+            {:error, error} -> {:halt, {:error, error}}
           end
 
         {%Ash.Resource.Aggregate{} = agg, direction}, {:ok, sort} ->
@@ -2035,6 +2008,15 @@ defmodule Ash.Actions.Read do
     end)
   end
 
+  defp hydrate_sort_calculation(query, calc, direction) do
+    with {:ok, [{calc, expression}]} <- hydrate_calculations(query, [calc]),
+         :ok <- Ash.Sort.validate_expression_refs(query.resource, expression) do
+      {:ok,
+       {%{calc | module: Ash.Resource.Calculation.Expression, opts: [expr: expression]},
+        direction}}
+    end
+  end
+
   defp hydrate_aggregates(query) do
     Enum.reduce_while(query.aggregates, {:ok, %{}}, fn {key, aggregate}, {:ok, aggregates} ->
       aggregate = %{

lib/ash/sort/sort.ex

@@ -385,44 +385,25 @@ defmodule Ash.Sort do
   end
 
   defp validate_sortable(resource, field) do
-    with :ok <- validate_expression_refs(resource, field) do
-      if type_sortable?(resource, field) do
-        :ok
-      else
-        {:error, UnsortableField.exception(field: field_name(field), resource: resource)}
-      end
+    if type_sortable?(resource, field) do
+      :ok
+    else
+      {:error, UnsortableField.exception(field: field_name(field), resource: resource)}
     end
   end
 
-  defp validate_expression_refs(
-         resource,
-         %Ash.Query.Calculation{module: module, opts: opts, context: context}
-       ) do
-    if Ash.Resource.Calculation.has_expression?(module) do
-      module
-      |> Ash.Resource.Calculation.expression(opts, context)
-      |> Ash.Filter.hydrate_refs(%{resource: resource, public?: false})
-      |> case do
-        {:ok, expression} ->
-          expression
-          |> Ash.Filter.list_refs(false, false, true)
-          |> Enum.reduce_while(:ok, fn ref, :ok ->
-            case validate_expression_ref(resource, ref) do
-              :ok -> {:cont, :ok}
-              {:error, error} -> {:halt, {:error, error}}
-            end
-          end)
-
-        {:error, error} ->
-          {:error, error}
+  @doc false
+  def validate_expression_refs(resource, expression) do
+    expression
+    |> Ash.Filter.list_refs(false, false, true)
+    |> Enum.reduce_while(:ok, fn ref, :ok ->
+      case validate_expression_ref(resource, ref) do
+        :ok -> {:cont, :ok}
+        {:error, error} -> {:halt, {:error, error}}
       end
-    else
-      :ok
-    end
+    end)
   end
 
-  defp validate_expression_refs(_resource, _field), do: :ok
-
   defp validate_expression_ref(
          resource,
          %Ash.Query.Ref{attribute: field, relationship_path: relationship_path}

test/sort/sort_test.exs

@@ -70,6 +70,10 @@ defmodule Ash.Test.Sort.SortTest do
       calculate :title_calculation, :string, expr(title),
         public?: true,
         sortable?: false
+
+      calculate :context_dependent, :string, Ash.Test.Sort.SortTest.ContextDependent do
+        public? true
+      end
     end
 
     relationships do
@@ -124,6 +128,19 @@ defmodule Ash.Test.Sort.SortTest do
     end
   end
 
+  defmodule ContextDependent do
+    @moduledoc false
+    use Ash.Resource.Calculation
+
+    @impl true
+    def expression(_opts, context) do
+      case context.source_context[:sort_field] do
+        nil -> raise "requires :sort_field in the source context"
+        field -> expr(^ref(field))
+      end
+    end
+  end
+
   defmodule NoSortDataLayer do
     use Spark.Dsl.Extension, sections: []
 
@@ -189,6 +206,33 @@ defmodule Ash.Test.Sort.SortTest do
     end
   end
 
+  describe "sorting on calculations whose expression/2 reads context.source_context" do
+    test "does not raise when the query has no context set" do
+      assert %Ash.Query{valid?: true} = Ash.Query.sort(Post, context_dependent: :asc)
+    end
+
+    test "does not raise when the query context was set before sorting" do
+      assert %Ash.Query{valid?: true} =
+               Post
+               |> Ash.Query.set_context(%{sort_field: :title})
+               |> Ash.Query.sort(context_dependent: :asc)
+    end
+
+    test "reads successfully when the context is set" do
+      b = Post |> Ash.Changeset.for_create(:create, %{title: "b"}) |> Ash.create!()
+      a = Post |> Ash.Changeset.for_create(:create, %{title: "a"}) |> Ash.create!()
+
+      ids =
+        Post
+        |> Ash.Query.set_context(%{sort_field: :title})
+        |> Ash.Query.sort(context_dependent: :asc)
+        |> Ash.read!()
+        |> Enum.map(& &1.id)
+
+      assert ids == [a.id, b.id]
+    end
+  end
+
   describe "parse_input/2" do
     test "simple string sort parses properly" do
       assert {:ok, [title: :asc, contents: :desc]} =
@@ -333,38 +377,47 @@ defmodule Ash.Test.Sort.SortTest do
       end
     end
 
-    test "expression sorts reject references to unsortable fields and relationships" do
+    test "expression sorts reject references to unsortable fields and relationships at read time" do
       require Ash.Sort
 
-      assert %Ash.Query{
-               valid?: false,
-               errors: [
-                 %Ash.Error.Query.UnsortableField{
-                   resource: Ash.Test.Sort.SortTest.Post,
-                   field: :unsortable_title
-                 }
-               ]
-             } = Ash.Query.sort(Post, Ash.Sort.expr_sort(unsortable_title, :string))
-
-      assert %Ash.Query{
-               valid?: false,
-               errors: [
-                 %Ash.Error.Query.UnsortableField{
-                   resource: Ash.Test.Sort.SortTest.Author,
-                   field: :unsortable_name
-                 }
-               ]
-             } = Ash.Query.sort(Post, Ash.Sort.expr_sort(author.unsortable_name, :string))
-
-      assert %Ash.Query{
-               valid?: false,
-               errors: [
-                 %Ash.Error.Query.UnsortableField{
-                   resource: Ash.Test.Sort.SortTest.Post,
-                   field: :unsortable_author
-                 }
-               ]
-             } = Ash.Query.sort(Post, Ash.Sort.expr_sort(unsortable_author.name, :string))
+      assert {:error,
+              %Ash.Error.Invalid{
+                errors: [
+                  %Ash.Error.Query.UnsortableField{
+                    resource: Ash.Test.Sort.SortTest.Post,
+                    field: :unsortable_title
+                  }
+                ]
+              }} =
+               Post
+               |> Ash.Query.sort(Ash.Sort.expr_sort(unsortable_title, :string))
+               |> Ash.read()
+
+      assert {:error,
+              %Ash.Error.Invalid{
+                errors: [
+                  %Ash.Error.Query.UnsortableField{
+                    resource: Ash.Test.Sort.SortTest.Author,
+                    field: :unsortable_name
+                  }
+                ]
+              }} =
+               Post
+               |> Ash.Query.sort(Ash.Sort.expr_sort(author.unsortable_name, :string))
+               |> Ash.read()
+
+      assert {:error,
+              %Ash.Error.Invalid{
+                errors: [
+                  %Ash.Error.Query.UnsortableField{
+                    resource: Ash.Test.Sort.SortTest.Post,
+                    field: :unsortable_author
+                  }
+                ]
+              }} =
+               Post
+               |> Ash.Query.sort(Ash.Sort.expr_sort(unsortable_author.name, :string))
+               |> Ash.read()
     end
 
     test "nested sorts enforce field and relationship flags" do