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
This commit is contained in:
@@ -38,7 +38,7 @@ module Budgets
|
|||||||
|
|
||||||
def index
|
def index
|
||||||
@investments = investments.page(params[:page]).per(PER_PAGE).for_render
|
@investments = investments.page(params[:page]).per(PER_PAGE).for_render
|
||||||
@investment_ids = @investments.ids
|
@investment_ids = @investments.unscope(:includes).ids
|
||||||
|
|
||||||
@investments_in_map = investments
|
@investments_in_map = investments
|
||||||
@tag_cloud = tag_cloud
|
@tag_cloud = tag_cloud
|
||||||
|
|||||||
Reference in New Issue
Block a user