fix: manual relationship implies no_attributes? true (#2562)

Adds a transformer on relationships.{has_one, has_many} DSLs to automatically set `no_attributes?` to `true` if `manual` is set.

Reworks logic in places that use `no_attributes?` and `manual` together to take this new implication into consideration. (read.ex, relationships.ex, and schema.ex)

Adds some tests to ensure that this implication holds. Also adds regression tests to ensure that `select` callback for manual relationships is respected.

Author: zackattackz

documentation/dsls/DSL-Ash.Resource.md

@@ -442,7 +442,7 @@ end
 
 | Name | Type | Default | Docs |
 |------|------|---------|------|
-| [`manual`](#relationships-has_one-manual){: #relationships-has_one-manual } | `(any, any -> any) \| module` |  | A module that implements `Ash.Resource.ManualRelationship`. Also accepts a 2 argument function that takes the source records and the context. |
+| [`manual`](#relationships-has_one-manual){: #relationships-has_one-manual } | `(any, any -> any) \| module` |  | A module that implements `Ash.Resource.ManualRelationship`. Also accepts a 2 argument function that takes the source records and the context. Setting this will automatically set `no_attributes?` to `true`. |
 | [`no_attributes?`](#relationships-has_one-no_attributes?){: #relationships-has_one-no_attributes? } | `boolean` |  | All existing entities are considered related, i.e this relationship is not based on any fields, and `source_attribute` and `destination_attribute` are ignored. See the See the [relationships guide](/documentation/topics/resources/relationships.md) for more. |
 | [`allow_nil?`](#relationships-has_one-allow_nil?){: #relationships-has_one-allow_nil? } | `boolean` | `true` | Marks the relationship as required. Has no effect on validations, but can inform extensions that there will always be a related entity. |
 | [`from_many?`](#relationships-has_one-from_many?){: #relationships-has_one-from_many? } | `boolean` | `false` | Signal that this relationship is actually a `has_many` where the first record is given via the `sort`. This will allow data layers to properly deduplicate when necessary. |
@@ -544,7 +544,7 @@ end
 
 | Name | Type | Default | Docs |
 |------|------|---------|------|
-| [`manual`](#relationships-has_many-manual){: #relationships-has_many-manual } | `(any, any -> any) \| module` |  | A module that implements `Ash.Resource.ManualRelationship`. Also accepts a 2 argument function that takes the source records and the context. |
+| [`manual`](#relationships-has_many-manual){: #relationships-has_many-manual } | `(any, any -> any) \| module` |  | A module that implements `Ash.Resource.ManualRelationship`. Also accepts a 2 argument function that takes the source records and the context. Setting this will automatically set `no_attributes?` to `true`. |
 | [`no_attributes?`](#relationships-has_many-no_attributes?){: #relationships-has_many-no_attributes? } | `boolean` |  | All existing entities are considered related, i.e this relationship is not based on any fields, and `source_attribute` and `destination_attribute` are ignored. See the See the [relationships guide](/documentation/topics/resources/relationships.md) for more. |
 | [`limit`](#relationships-has_many-limit){: #relationships-has_many-limit } | `integer` |  | An integer to limit entries from loaded relationship. |
 | [`description`](#relationships-has_many-description){: #relationships-has_many-description } | `String.t` |  | An optional description for the relationship |

lib/ash/actions/read/read.ex

@@ -1499,15 +1499,12 @@ defmodule Ash.Actions.Read do
         []
       else
         case Ash.Resource.Info.relationship(query.resource, name) do
+          %{manual: {module, opts}} ->
+            module.select(opts)
+
           %{no_attributes?: true} ->
             []
 
-          %{manual: {module, opts}, source_attribute: source_attribute} ->
-            fields =
-              module.select(opts)
-
-            [source_attribute | fields]
-
           %{source_attribute: source_attribute} ->
             [source_attribute]
         end

lib/ash/actions/read/relationships.ex

@@ -999,11 +999,10 @@ defmodule Ash.Actions.Read.Relationships do
 
   defp select_destination_attribute(related_query, relationship) do
     if Map.get(relationship, :no_attributes?) ||
-         (Map.get(relationship, :manual) &&
-            !Ash.Resource.Info.attribute(
-              relationship.destination,
-              relationship.destination_attribute
-            )) do
+         !Ash.Resource.Info.attribute(
+           relationship.destination,
+           relationship.destination_attribute
+         ) do
       related_query
     else
       Ash.Query.ensure_selected(related_query, [relationship.destination_attribute])

lib/ash/resource/relationships/has_many.ex

@@ -93,6 +93,9 @@ defmodule Ash.Resource.Relationships.HasMany do
   def manual(module) when is_atom(module), do: {:ok, {module, []}}
 
   def transform(relationship) do
-    {:ok, relationship |> Ash.Resource.Actions.Read.concat_filters()}
+    {:ok,
+     relationship
+     |> Ash.Resource.Actions.Read.concat_filters()
+     |> manual_implies_no_attributes()}
   end
 end

lib/ash/resource/relationships/has_one.ex

@@ -99,6 +99,7 @@ defmodule Ash.Resource.Relationships.HasOne do
     {:ok,
      relationship
      |> Ash.Resource.Actions.Read.concat_filters()
-     |> Map.put(:from_many?, relationship.from_many? || not is_nil(relationship.sort))}
+     |> Map.put(:from_many?, relationship.from_many? || not is_nil(relationship.sort))
+     |> manual_implies_no_attributes()}
   end
 end

lib/ash/resource/relationships/shared_options.ex

@@ -145,7 +145,12 @@ defmodule Ash.Resource.Relationships.SharedOptions do
        {:spark_function_behaviour, Ash.Resource.ManualRelationship,
         {Ash.Resource.ManualRelationship.Function, 2}},
      doc: """
-     A module that implements `Ash.Resource.ManualRelationship`. Also accepts a 2 argument function that takes the source records and the context.
+     A module that implements `Ash.Resource.ManualRelationship`. Also accepts a 2 argument function that takes the source records and the context. Setting this will automatically set `no_attributes?` to `true`.
      """}
   end
+
+  def manual_implies_no_attributes(relationship) do
+    relationship
+    |> Map.put(:no_attributes?, relationship.no_attributes? || not is_nil(relationship.manual))
+  end
 end

lib/ash/resource/schema.ex

@@ -223,12 +223,6 @@ defmodule Ash.Schema do
                 %{no_attributes?: true} ->
                   :ok
 
-                %{manual?: true} ->
-                  :ok
-
-                %{manual: manual} when not is_nil(manual) ->
-                  :ok
-
                 %{type: :belongs_to} ->
                   belongs_to relationship.name, relationship.destination,
                     define_field: false,

test/actions/has_many_test.exs

@@ -48,9 +48,38 @@ defmodule Ash.Test.Actions.HasManyTest do
 
     require Ash.Query
 
-    def load(records, _opts, %{query: query, actor: actor, authorize?: authorize?}) do
+    def select(opts) do
+      if opts[:select_likes?] do
+        [:likes]
+      else
+        []
+      end
+    end
+
+    def load(records, opts, %{
+          query: query,
+          actor: actor,
+          authorize?: authorize?,
+          relationship: relationship
+        }) do
       post_ids = Enum.map(records, & &1.id)
 
+      case relationship.name do
+        :meow_comments ->
+          assert opts[:select_likes?] == true
+
+        :meow_comments_no_likes ->
+          assert !opts[:select_likes?]
+      end
+
+      Enum.each(records, fn record ->
+        if opts[:select_likes?] do
+          assert is_nil(record.likes) or is_integer(record.likes)
+        else
+          assert not is_nil(record.likes) and not is_integer(record.likes)
+        end
+      end)
+
       {:ok,
        query
        |> Ash.Query.filter(post_id in ^post_ids)
@@ -116,6 +145,8 @@ defmodule Ash.Test.Actions.HasManyTest do
       attribute :title, :string, public?: true
       attribute :tenant_id, :string, public?: true
 
+      attribute :likes, :integer, public?: true
+
       create_timestamp :inserted_at, public?: true
     end
 
@@ -127,7 +158,11 @@ defmodule Ash.Test.Actions.HasManyTest do
       end
 
       has_many :meow_comments, Comment do
-        manual MeowCommentRelationship
+        manual({MeowCommentRelationship, [select_likes?: true]})
+      end
+
+      has_many :meow_comments_no_likes, Comment do
+        manual({MeowCommentRelationship, [select_likes?: false]})
       end
 
       has_many :meow_list_comments, Comment do
@@ -265,7 +300,8 @@ defmodule Ash.Test.Actions.HasManyTest do
     post =
       Post
       |> Ash.Changeset.for_create(:create, %{
-        title: "buz"
+        title: "buz",
+        likes: 1337
       })
       |> Ash.create!()
 
@@ -274,8 +310,9 @@ defmodule Ash.Test.Actions.HasManyTest do
       |> Ash.Changeset.for_update(:add_comment, %{
         comment: %{content: "meow"}
       })
+      |> Ash.Changeset.deselect(:likes)
       |> Ash.update!()
-      |> Ash.load!(:meow_comments)
+      |> Ash.load!([:meow_comments])
 
     assert length(post.meow_comments) == 1
 
@@ -305,6 +342,14 @@ defmodule Ash.Test.Actions.HasManyTest do
       |> Ash.load!(:meow_comments)
 
     assert length(post.meow_comments) == 1
+
+    post
+    |> Ash.Changeset.for_update(:add_comment, %{
+      comment: %{content: "meow"}
+    })
+    |> Ash.Changeset.deselect(:likes)
+    |> Ash.update!()
+    |> Ash.load!([:meow_comments_no_likes])
   end
 
   test "raise on an invalid manual relationship query" do

test/actions/load_test.exs

@@ -582,6 +582,62 @@ defmodule Ash.Test.Actions.LoadTest do
     end
   end
 
+  defmodule PostSecretRelationship do
+    @moduledoc false
+    use Ash.Resource.ManualRelationship
+
+    require Ash.Query
+
+    @impl true
+    def load(records, _opts, _context) do
+      posts_with_secrets =
+        Post
+        |> Ash.Query.for_read(:all_access)
+        |> Ash.Query.filter(
+          exists(Ash.Test.Actions.LoadTest.PostSecret, contains(parent(secret), secret))
+        )
+        |> Ash.read!()
+
+      Enum.map(records, fn record ->
+        Enum.filter(posts_with_secrets, fn post ->
+          String.contains?(
+            post.secret,
+            record.secret
+          )
+        end)
+      end)
+    end
+  end
+
+  defmodule PostSecret do
+    @moduledoc false
+    use Ash.Resource,
+      domain: Domain,
+      data_layer: Ash.DataLayer.Ets
+
+    ets do
+      private?(true)
+    end
+
+    actions do
+      defaults([:read, create: :*])
+    end
+
+    attributes do
+      attribute :secret, :string do
+        public? true
+        primary_key? true
+        allow_nil? false
+      end
+    end
+
+    relationships do
+      has_many :posts_with_secret, Post do
+        manual PostSecretRelationship
+      end
+    end
+  end
+
   describe "loads" do
     setup do
       start_supervised(
@@ -1378,6 +1434,31 @@ defmodule Ash.Test.Actions.LoadTest do
       assert Ash.load!(event_log, :author).author.id == author.id
       assert Ash.load!(event_log2, :author).author.id == author2.id
     end
+
+    test "it allows loading manual relationships with non :id pkey, regardless of source_attribute" do
+      Post
+      |> Ash.Changeset.for_create(:create, %{title: "post1", secret: "50"})
+      |> Ash.create!(authorize?: false)
+
+      post2 =
+        Post
+        |> Ash.Changeset.for_create(:create, %{title: "post2", secret: "42"})
+        |> Ash.create!(authorize?: false)
+
+      post3 =
+        Post
+        |> Ash.Changeset.for_create(:create, %{title: "post3", secret: "4"})
+        |> Ash.create!(authorize?: false)
+
+      post_secret1 =
+        PostSecret
+        |> Ash.Changeset.for_create(:create, %{secret: "4"})
+        |> Ash.create!()
+        |> Ash.load!([:posts_with_secret])
+
+      assert Enum.sort(Enum.map(post_secret1.posts_with_secret, & &1.secret)) ==
+               Enum.sort([post2.secret, post3.secret])
+    end
   end
 
   describe "forbidden lazy loads" do