Skip to content

Commit

Permalink
Remove unnecessary includes in requests query
Browse files Browse the repository at this point in the history
Sorting the requests by the Source, Target or Type columns led to
missing results.

This was caused by a wrong DISTINCT clause, that was taking fields from
two tables (`bs_request_actions` and `requests`) instead of one
(`requests`). Those extra fields in the DISTINCT clause were introduced
by an `includes` finder method.

The problem was fixed by replacing the `includes` in the `.with_action`
scope with a `joins` finder method, so the fields from
`bs_requests_actions` are no longer part of the DISTINCT modifier in the
SQL sentence.

Additionally, the `includes` in the
BsRequest::DataTable::FindForUserOrGroup#requests method was redundant.
We add `preload(:bs_request_actions)` instead.

In order to prevent using LIMIT inside subqueries, we moved the limit
finder method to the last step of the query.

Co-authored-by: Saray Cabrera Padrón <scabrerapadron@suse.de>
  • Loading branch information
eduardoj and saraycp committed Mar 31, 2021
1 parent 4e3ea11 commit 7338281
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 3 deletions.
2 changes: 1 addition & 1 deletion src/api/app/controllers/request_controller.rb
Expand Up @@ -37,9 +37,9 @@ def render_request_collection
params[:ids] = params[:ids].split(',').map(&:to_i) if params[:ids]

rel = BsRequest.find_for(params)
rel = BsRequest.where(id: rel.select(:id)).preload([{ bs_request_actions: :bs_request_action_accept_info, reviews: { history_elements: :user } }])
rel = rel.limit(params[:limit].to_i) if params[:limit].to_i.positive?

rel = BsRequest.where(id: rel.select(:id)).preload([{ bs_request_actions: :bs_request_action_accept_info, reviews: { history_elements: :user } }])
xml = Nokogiri::XML('<collection/>', &:strict).root
matches = 0
rel.each do |r|
Expand Down
2 changes: 1 addition & 1 deletion src/api/app/models/bs_request.rb
Expand Up @@ -25,7 +25,7 @@ class BsRequest < ApplicationRecord

scope :to_accept_by_time, -> { where(state: ['new', 'review']).where('accept_at < ?', Time.now) }
# Scopes for collections
scope :with_actions, -> { includes(:bs_request_actions).references(:bs_request_actions).distinct.order(priority: :asc, id: :desc) }
scope :with_actions, -> { joins(:bs_request_actions).distinct.order(priority: :asc, id: :desc) }
scope :with_involved_projects, ->(project_ids) { where(bs_request_actions: { target_project_id: project_ids }) }
scope :with_involved_packages, ->(package_ids) { where(bs_request_actions: { target_package_id: package_ids }) }

Expand Down
Expand Up @@ -15,7 +15,7 @@ def requests
.offset(@params[:offset])
.limit(@params[:limit])
.reorder(@params[:sort])
.includes(:bs_request_actions)
.preload(:bs_request_actions)
end

def records_total
Expand Down

0 comments on commit 7338281

Please sign in to comment.