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.

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

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

Merged

Conversation

jeremylenz
Copy link
Member

@jeremylenz jeremylenz commented Dec 2, 2022

What are the changes introduced in this pull request?

This PR addresses 2 issues with the RepositorySetsController#index method:

  1. When filtering by status, the results were paginated first, then filtered. This resulted in the pagination data being incorrect. Rather, the correct way is to filter first, then paginate.
  2. When sorting by enabled_by_default, the custom sort method was applied only to the single page of results, not to the entire result list before pagination.

Considerations taken when implementing this change?

I realized that passing status to ProductContentFinder.wrap_with_overrides is what caused the results to be prematurely filtered. So I avoid doing that in index; rather, I do it in index_relation so that the filtering can happen before scoped_search paginates the results. (I also had to transform it back into an ActiveRecord::Relation since index_relation should always return a relation.)

I also realized we had the same problem with our custom sort method - it was only sorting a single page of results. So I moved its use to scoped_search, which can take a custom_sort option.

However, scoped_search's custom_sort function must return a relation, so I had to find a way to use our custom sorting logic in Ruby and then transform that same sort order back into an ActiveRecord::Relation. I ended up using a SQL function array_position, which tells you the position of an item in an array. I pass the sorted IDs to this function; it seems to work well.

I was able to do all of this in the repository sets controller, without altering Katello's scoped_search method in api_controller.rb.

What are the testing steps for this pull request?

New host details page > Content > Repository sets

Sort by "Enabled" column
Filter by Enabled / Disabled / Overridden

Verify that sorting is correct:

  1. Enabled (overridden)
  2. Enabled
  3. Disabled
  4. Disabled (overridden)

Also verify that sorting is correct across pages (e.g. it doesn't just sort pages individually)

Verify that filtering pagination is correct (see description in redmine)

Compare all of this with behavior on master.

@theforeman-bot
Copy link

Issues: #35795

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

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.

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

It's working as intended! One comment to clear up some confusion:

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

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.

@jeremylenz jeremylenz removed the request for review from akofink December 6, 2022 15:11
@jeremylenz jeremylenz merged commit 5b7ede3 into Katello:master Dec 6, 2022
chris1984 pushed a commit to chris1984/katello that referenced this pull request Dec 8, 2022
chris1984 pushed a commit that referenced this pull request Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants