The index reads every plan's tags once, not once per plan (#200)

The last thing #196 left on the table. Its location preload had to count
only the location tables, because counting everything would have failed
on `tag_names` — which was doing a query a row — and the number would
then have been bumped to whatever made it pass. That note called the
pluck the cause and left it alone. The pluck isn't the cause.

`pluck` on a relation that's already loaded reads the loaded records
instead of querying; it has since Rails 6. So `tag_names` is free on a
plan whose tags are loaded and one query on a plan whose tags aren't,
and the whole story is that the API index never eager-loaded `:tags`.
Five plans, five `Tag Pluck` queries; add `:tags` to the includes and
there are none. Every other list endpoint that reads tags — the
libraries organization API, the workspace index, home, search — already
preloads them, which is why this only ever showed up here.

So: `:tags` joins the index's eager loads, and the comment on tag_names
now says which of its two costs a caller is paying, so the next reader
doesn't re-derive this the way I just did.

The spec is the location one's twin — tag-table query count for thirty
plans equals the count for three, plus the assertion that each plan
still gets its own tags rather than a neighbour's, since a preload can
be cheap and wrong. Both now share the counter as a helper. It fails
without the includes: thirty queries against three.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: HamptonMakes

engine/app/controllers/coplan/api/v1/plans_controller.rb

@@ -12,7 +12,7 @@ def index
             # folder's ancestors, and `url` walks those *and* the library for
             # its handle. Left to the associations that's several queries a
             # plan on a list endpoint agents page through.
-            .includes(:plan_type, :created_by_user,
+            .includes(:plan_type, :created_by_user, :tags,
               placement: [ :library, { folder: { parent: :parent } } ])
             .visible_to(current_user)
             .order(updated_at: :desc)

engine/app/models/coplan/plan.rb

@@ -289,6 +289,9 @@ def stripped_content
       end
     end
 
+    # Free when `tags` is already loaded — `pluck` reads the loaded records
+    # instead of querying — and one query when it isn't. So a list endpoint
+    # pays a query a plan unless it eager-loads `:tags`; they all do.
     def tag_names
       tags.pluck(:name)
     end

spec/requests/api/v1/plans_spec.rb

@@ -13,36 +13,36 @@
     alice_token # ensure token exists
   end
 
+  # Counts the index's queries against one set of tables rather than all of
+  # them: each preload spec below is about a single field, and a total would
+  # fail for an unrelated query and then get bumped to whatever number made
+  # it pass. The assertion is a shape — the cost doesn't follow the list —
+  # not a magic number.
+  def index_queries_touching(table_pattern)
+    count = 0
+    sub = ActiveSupport::Notifications.subscribe("sql.active_record") do |*, payload|
+      next if payload[:name].to_s =~ /SCHEMA|TRANSACTION/
+      count += 1 if payload[:sql].to_s.match?(table_pattern)
+    end
+    get api_v1_plans_path, headers: headers
+    ActiveSupport::Notifications.unsubscribe(sub)
+    expect(response).to have_http_status(:success)
+    count
+  end
+
   # Every row carries `url` and `folder_path`, and both want the plan's
   # whole location — the folder's ancestors, and the library for its handle.
   # Reached through the associations that's several queries per plan on a
   # list agents page through, so the location is preloaded.
-  #
-  # Counts only the location tables, not every query: `tag_names` plucks per
-  # plan for reasons of its own, and a total would fail for that instead and
-  # then get bumped to whatever number made it pass. Asserted as a shape —
-  # the cost doesn't follow the list — rather than a magic number.
   it "reads each plan's location once for the whole index, not once per plan" do
     folder = create(:folder, name: "LiveOrder", created_by_user: alice)
     nested = create(:folder, name: "Q3", parent: folder, created_by_user: alice)
 
-    location_queries = lambda do
-      count = 0
-      sub = ActiveSupport::Notifications.subscribe("sql.active_record") do |*, payload|
-        next if payload[:name].to_s =~ /SCHEMA|TRANSACTION/
-        count += 1 if payload[:sql].to_s.match?(/coplan_(plan_placements|folders|libraries)/)
-      end
-      get api_v1_plans_path, headers: headers
-      ActiveSupport::Notifications.unsubscribe(sub)
-      expect(response).to have_http_status(:success)
-      count
-    end
-
     3.times { |i| CoPlan::Plans::Place.call(plan: create(:plan, :considering, created_by_user: alice, title: "Roadmap #{i}"), folder: nested, actor: alice) }
-    few = location_queries.call
+    few = index_queries_touching(/coplan_(plan_placements|folders|libraries)/)
 
     27.times { |i| CoPlan::Plans::Place.call(plan: create(:plan, :considering, created_by_user: alice, title: "Later #{i}"), folder: folder, actor: alice) }
-    many = location_queries.call
+    many = index_queries_touching(/coplan_(plan_placements|folders|libraries)/)
 
     body = JSON.parse(response.body)
     expect(body.size).to eq(30)
@@ -53,6 +53,32 @@
     expect(many).to eq(few)
   end
 
+  # Same story for `tags`: `tag_names` reads them straight off the plan when
+  # they're loaded, and queries for them when they aren't, so leaving them
+  # out of the index's eager loads costs a query a row.
+  it "reads every plan's tags once for the whole index, not once per plan" do
+    tag_plans = lambda do |count, prefix|
+      count.times do |i|
+        plan = create(:plan, :considering, created_by_user: alice, title: "#{prefix} #{i}")
+        plan.tag_names = [ "roadmap", "#{prefix.downcase}-#{i}" ]
+      end
+    end
+
+    tag_plans.call(3, "Roadmap")
+    few = index_queries_touching(/coplan_(tags|plan_tags)/)
+
+    tag_plans.call(27, "Later")
+    many = index_queries_touching(/coplan_(tags|plan_tags)/)
+
+    body = JSON.parse(response.body)
+    expect(body.size).to eq(30)
+    # Cheap and correct: each plan still gets its own two tags, not another's.
+    expected = (0...3).map { |i| [ "roadmap", "roadmap-#{i}" ] } +
+      (0...27).map { |i| [ "later-#{i}", "roadmap" ] }
+    expect(body.map { |p| p["tags"].sort }.sort).to eq(expected.map(&:sort).sort)
+    expect(many).to eq(few)
+  end
+
   it "index returns plans" do
     plan # trigger creation
     get api_v1_plans_path, headers: headers