fix: validate args in the proper order

lib/ash/changeset/changeset.ex

@@ -524,9 +524,7 @@ defmodule Ash.Changeset do
                 authorize?: opts[:authorize?]
               }
 
-              if opts[:tracer] do
-                opts[:tracer].set_metadata(:changeset, metadata)
-              end
+              Ash.Tracer.set_metadata(opts[:tracer], :changeset, metadata)
 
               changeset
               |> handle_errors(action.error_handler)
@@ -538,6 +536,7 @@ defmodule Ash.Changeset do
               |> Map.put(:action, action)
               |> cast_params(action, params)
               |> set_argument_defaults(action)
+              |> require_arguments(action)
               |> run_action_changes(
                 action,
                 opts[:actor],
@@ -547,7 +546,6 @@ defmodule Ash.Changeset do
               )
               |> add_validations(opts[:tracer], metadata)
               |> mark_validated(action.name)
-              |> require_arguments(action)
             end
           end
         end
@@ -649,9 +647,7 @@ defmodule Ash.Changeset do
               authorize?: opts[:authorize?]
             }
 
-            if opts[:tracer] do
-              opts[:tracer].set_metadata(:changeset, metadata)
-            end
+            Ash.Tracer.set_metadata(opts[:tracer], :changeset, metadata)
 
             changeset =
               changeset
@@ -667,6 +663,7 @@ defmodule Ash.Changeset do
               |> Map.put(:__validated_for_action__, action.name)
               |> cast_params(action, params || %{})
               |> set_argument_defaults(action)
+              |> require_arguments(action)
               |> validate_attributes_accepted(action)
               |> require_values(action.type, false, action.require_attributes)
               |> run_action_changes(
@@ -679,7 +676,6 @@ defmodule Ash.Changeset do
               |> set_defaults(changeset.action_type, false)
               |> add_validations(opts[:tracer], metadata)
               |> mark_validated(action.name)
-              |> require_arguments(action)
               |> eager_validate_identities()
 
             if Keyword.get(opts, :require?, true) do
@@ -798,11 +794,6 @@ defmodule Ash.Changeset do
   end
 
   defp require_arguments(changeset, action) do
-    changeset
-    |> do_require_arguments(action)
-  end
-
-  defp do_require_arguments(changeset, action) do
     action.arguments
     |> Enum.filter(&(&1.allow_nil? == false))
     |> Enum.reduce(changeset, fn argument, changeset ->
@@ -966,9 +957,7 @@ defmodule Ash.Changeset do
                  resource_short_name: Ash.Resource.Info.short_name(changeset.resource),
                  validation: inspect(module)
                } do
-                 if tracer do
-                   tracer.set_metadata(:validation, metadata)
-                 end
+                 Ash.Tracer.set_metadata(opts[:tracer], :validation, metadata)
 
                  module.validate(changeset, opts) == :ok
                end
@@ -981,9 +970,7 @@ defmodule Ash.Changeset do
             } do
               {:ok, opts} = module.init(opts)
 
-              if tracer do
-                tracer.set_metadata(:change, metadata)
-              end
+              Ash.Tracer.set_metadata(opts[:tracer], :change, metadata)
 
               module.change(changeset, opts, %{
                 actor: actor,
@@ -1178,9 +1165,7 @@ defmodule Ash.Changeset do
           resource_short_name: Ash.Resource.Info.short_name(changeset.resource),
           validation: inspect(validation.module)
         } do
-          if tracer do
-            tracer.set_metadata(:validation, metadata)
-          end
+          Ash.Tracer.set_metadata(tracer, :validation, metadata)
 
           case validation.module.validate(changeset, validation.opts) do
             :ok ->
@@ -1423,7 +1408,30 @@ defmodule Ash.Changeset do
         changeset.before_action,
         {changeset, %{notifications: []}},
         fn before_action, {changeset, instructions} ->
-          case before_action.(changeset) do
+          metadata = %{
+            api: changeset.api,
+            resource: changeset.resource,
+            resource_short_name: Ash.Resource.Info.short_name(changeset.resource),
+            actor: changeset.context[:private][:actor],
+            tenant: changeset.context[:private][:actor],
+            action: changeset.action && changeset.action.name,
+            authorize?: changeset.context[:private][:authorize?]
+          }
+
+          tracer = changeset.context[:private][:tracer]
+
+          result =
+            Ash.Tracer.span :before_action,
+                            "before_action",
+                            tracer do
+              Ash.Tracer.set_metadata(tracer, :before_action, metadata)
+
+              Ash.Tracer.telemetry_span [:ash, :before_action], metadata do
+                before_action.(changeset)
+              end
+            end
+
+          case result do
             {changeset, %{notifications: notifications}} ->
               cont =
                 if changeset.valid? do
@@ -1482,7 +1490,30 @@ defmodule Ash.Changeset do
       changeset.after_action,
       {:ok, result, changeset, %{notifications: before_action_notifications}},
       fn after_action, {:ok, result, changeset, %{notifications: notifications} = acc} ->
-        case after_action.(changeset, result) do
+        tracer = changeset.context[:private][:tracer]
+
+        metadata = %{
+          api: changeset.api,
+          resource: changeset.resource,
+          resource_short_name: Ash.Resource.Info.short_name(changeset.resource),
+          actor: changeset.context[:private][:actor],
+          tenant: changeset.context[:private][:actor],
+          action: changeset.action && changeset.action.name,
+          authorize?: changeset.context[:private][:authorize?]
+        }
+
+        result =
+          Ash.Tracer.span :after_action,
+                          "after_action",
+                          tracer do
+            Ash.Tracer.set_metadata(tracer, :after_action, metadata)
+
+            Ash.Tracer.telemetry_span [:ash, :after_action], metadata do
+              after_action.(changeset, result)
+            end
+          end
+
+        case result do
           {:ok, new_result, new_notifications} ->
             all_notifications =
               Enum.map(notifications ++ new_notifications, fn notification ->

lib/ash/query/query.ex

@@ -290,9 +290,7 @@ defmodule Ash.Query do
           authorize?: opts[:authorize?]
         }
 
-        if opts[:tracer] do
-          opts[:tracer].set_metadata(:query, metadata)
-        end
+        Ash.Tracer.set_metadata(opts[:tracer], :query, metadata)
 
         query = Map.put(query, :action, action.name)
 
@@ -305,9 +303,10 @@ defmodule Ash.Query do
         |> Map.put(:action, action)
         |> Map.put(:__validated_for_action__, action_name)
         |> cast_params(action, args)
+        |> set_argument_defaults(action)
+        |> require_arguments(action)
         |> run_preparations(action, opts[:actor], opts[:authorize?], opts[:tracer], metadata)
         |> add_action_filters(action, opts[:actor])
-        |> require_arguments(action)
       end
     else
       add_error(query, :action, "No such action #{action_name}")
@@ -349,12 +348,6 @@ defmodule Ash.Query do
   end
 
   defp require_arguments(query, action) do
-    query
-    |> set_argument_defaults(action)
-    |> do_require_arguments(action)
-  end
-
-  defp do_require_arguments(query, action) do
     action.arguments
     |> Enum.filter(&(&1.allow_nil? == false))
     |> Enum.reduce(query, fn argument, query ->
@@ -423,9 +416,7 @@ defmodule Ash.Query do
           resource_short_name: Ash.Resource.Info.short_name(query.resource),
           preparation: inspect(module)
         } do
-          if tracer do
-            tracer.set_metadata(:preparation, metadata)
-          end
+          Ash.Tracer.set_metadata(opts[:tracer], :preparation, metadata)
 
           case module.init(opts) do
             {:ok, opts} ->
@@ -777,18 +768,30 @@ defmodule Ash.Query do
   """
   def select(query, fields, opts \\ []) do
     query = to_query(query)
+    fields = List.wrap(fields)
+
+    {fields, non_existent} =
+      Enum.split_with(fields, &Ash.Resource.Info.attribute(query.resource, &1))
+
+    query =
+      Enum.reduce(non_existent, query, fn field, query ->
+        Ash.Query.add_error(
+          query,
+          Ash.Error.Query.NoSuchAttribute.exception(resource: query.resource, name: field)
+        )
+      end)
 
     if opts[:replace?] do
       %{
         query
-        | select: Enum.uniq(List.wrap(fields) ++ Ash.Resource.Info.primary_key(query.resource))
+        | select: Enum.uniq(fields ++ Ash.Resource.Info.primary_key(query.resource))
       }
     else
       %{
         query
         | select:
             Enum.uniq(
-              List.wrap(fields) ++
+              fields ++
                 (query.select || []) ++ Ash.Resource.Info.primary_key(query.resource)
             )
       }