Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #35795,35213 - fix Repo Sets pagination, sorting, filtering #10373

Merged
merged 1 commit into from Dec 6, 2022
Merged

Fixes #35795,35213 - fix Repo Sets pagination, sorting, filtering #10373

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 25 additions & 14 deletions app/controllers/katello/api/v2/repository_sets_controller.rb
View file
Open in desktop
Expand Up @@ -38,12 +38,11 @@ class Api::V2::RepositorySetsController < Api::V2::ApiController
param_group :search, Api::V2::ApiController
add_scoped_search_description_for(Katello::ProductContent)
def index
collection = scoped_search(index_relation, :name, :asc, :resource_class => Katello::ProductContent)
collection = scoped_search(index_relation, :name, :asc, :resource_class => Katello::ProductContent, :custom_sort => ->(relation) { custom_sort_results(relation) })
pcf = ProductContentFinder.wrap_with_overrides(
product_contents: collection[:results],
overrides: @consumable&.content_overrides,
status: params[:status])
collection[:results] = custom_sort_results(pcf)
overrides: @consumable&.content_overrides)
collection[:results] = pcf
respond(:collection => collection)
end

Expand Down Expand Up @@ -154,7 +153,13 @@ def index_relation_with_consumable_overrides(relation)
:match_subscription => !content_access_mode_all,
:match_environment => content_access_mode_env,
:consumable => @consumable)
relation.merge(content_finder.product_content)
unfiltered = relation.merge(content_finder.product_content)
return unfiltered unless params[:status]
filtered_ids = ProductContentFinder.wrap_with_overrides(
product_contents: unfiltered,
overrides: @consumable&.content_overrides,
status: params[:status]).map(&:id).uniq
unfiltered.where(id: filtered_ids)
end

def find_product_content
Expand Down Expand Up @@ -236,18 +241,24 @@ def sort_score(pc) # sort order for enabled
else
0
end
Rails.logger.debug [pc.product_name, pc.enabled_content_override, "Id: #{pc.id}", "Score: #{score}"]
score
end

def custom_sort_results(product_content_finder)
if params[:sort_by] == 'enabled_by_default' && params[:sort_order] == 'desc'
product_content_finder.sort { |pca, pcb| sort_score(pca) <=> sort_score(pcb) }.reverse!
elsif params[:sort_by] == 'enabled_by_default'
product_content_finder.sort { |pca, pcb| sort_score(pca) <=> sort_score(pcb) }
else
product_content_finder
end
def custom_sort_results(unsorted_relation)
return unsorted_relation unless params[:sort_by] == 'enabled_by_default'
product_content_finder = ProductContentFinder.wrap_with_overrides(
product_contents: unsorted_relation,
overrides: @consumable&.content_overrides,
status: params[:status])
sorted_pcps = if params[:sort_by] == 'enabled_by_default' && params[:sort_order] == 'desc'
product_content_finder.sort { |pca, pcb| sort_score(pca) <=> sort_score(pcb) }.reverse!
elsif params[:sort_by] == 'enabled_by_default'
product_content_finder.sort { |pca, pcb| sort_score(pca) <=> sort_score(pcb) }
else
product_content_finder
end
sort_order = sorted_pcps.map(&:id)
unsorted_relation.reorder(Arel.sql("array_position('{#{sort_order.join(',')}}'::int[], #{Katello::ProductContent.table_name}.id)"))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only worked once I realized that I needed to call reorder instead of order, since that eliminates a default ORDER BY that was causing the custom sort to be ignored.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about how this is working. array_position is supposed to return a single position of an element found in an array. But here, instead of an element, a table's ID field is being passed in as the second argument.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{#{sort_order.join(',')}}'::int[] <-- So this creates a sorted array of product_content IDs, sorted in the way we want them sorted. The second argument is saying, "find where this ID is in that array, and then sort by that."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a little weird to me that the controller is turning a relation into an array (i.e. executing a postgres transaction) and then turning it back into a relation.

Instead, would it be possible to ensure that relation is actually a relation when it's sent to scoped_search? Then, the ordering can be applied as usual. It's a little odd that relation isn't actually a relation by the time it is passed into scoped_search anyway (right?).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for example, it'll make an array of IDs like

[8142, 8132, 726]

and then the SQL will be like

SELECT blah FROM table ORDER BY array_position('{#{sort_order.join(',')}}'::int[], #{Katello::ProductContent.table_name}.id)

which evaluates to something like

SELECT blah FROM table ORDER BY array_position([8142, 8132, 726], katello_product_contents.id)

Copy link
Member Author

@jeremylenz jeremylenz Dec 6, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, would it be possible to ensure that relation is actually a relation when it's sent to scoped_search? Then, the ordering can be applied as usual. It's a little odd that relation isn't actually a relation by the time it is passed into scoped_search anyway (right?).

relation is always a relation. I made sure of this, since that's what scoped_search needs. Here's the journey it goes through:

  1. index_relation is a relation at the end of the method. It may or may not continue onto index_relation_with_consumable_overrides. If it does,
  2. During index_relation_with_consumable_overrides, it makes ProductContentFinders to adorn the items with the necessary override data, and do the necessary filtering. It then converts it back to a relation before exiting the method.
  3. It's then passed into scoped_search as a relation, as required.

The reason all this is necessary is because

  1. Content overrides are associated with hosts or activation keys, but this controller is for product contents, which is unreachable from those in terms of database tables because
  2. Content overrides are a regular Ruby class, not a model. This is because they're just a snippet of Candlepin data.
  3. Because of the above, there isn't a viable join we can do to "sort normally." But we already have the ProductContentFinder and ProductContentPresenter which addresses these two issues, so it works out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm convinced! Thanks for the details.

end
end
end