From bd4fdff3d4a85cf3743fcbbbd3d303c985502921 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 15 Apr 2024 01:06:59 +0200 Subject: [PATCH] Avoid executing `.includes(...).order("ids.ordering").ids` Apparently, the `ids` method, which originally was implemented as `pluck(:id)`, sometimes returned duplicate ids [1]. Fixing that issue generated another issue [2] when combining `.includes` and `order`. There was an attempt to fix it [3], but it still doesn't fix the case where the ordering column belongs to an association [4]. This means that, when upgrading to Rails 7.1, calling `.ids` on `budget.investments.includes(:heading).sort_by_random.for_render` results in an invalid SQL statement: ``` ActiveRecord::StatementInvalid: PG::GroupingError: ERROR: column "ids.ordering" must appear in the GROUP BY clause or be used in an aggregate function LINE 1: ... = $4 GROUP BY "budget_investments"."id" ORDER BY ids.orderi... ``` To solve it, we could once again use `.pluck(:id)` instead of `ids`, but I'm not sure whether there would be a risk to get duplicate IDs in some cases. We cannot call `.reorder` or `.unscope(:order)` either because we need the IDs to be ordered randomly. So we're removing the `includes(:heading)` part when getting the IDs. Since using `includes` generates a separate query that doesn't affect the query to get the IDs, removing it makes no difference. Another option would be to remove the `for_render` call, since we're including the headings to avoid the N+1 queries problem, but we're doing so without benchmarks showing that it does actually make a difference. [1] Issue 46455 in https://github.com/rails/rails [2] Issue 48080 in https://github.com/rails/rails [3] Pull request 48101 in https://github.com/rails/rails [4] Issue 50263 in https://github.com/rails/rails --- app/controllers/budgets/investments_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/budgets/investments_controller.rb b/app/controllers/budgets/investments_controller.rb index 81bff7ff9..69f51cfd7 100644 --- a/app/controllers/budgets/investments_controller.rb +++ b/app/controllers/budgets/investments_controller.rb @@ -38,7 +38,7 @@ module Budgets def index @investments = investments.page(params[:page]).per(PER_PAGE).for_render - @investment_ids = @investments.ids + @investment_ids = @investments.unscope(:includes).ids @investments_in_map = investments @tag_cloud = tag_cloud