Fixes #37109 - allow to use eager loading on latest_version_object

[sbernhard]

Strips away 480 sql queries because of needs_publish? and especially the implementation of latest_version_object

Display 20 Content Views on UI (index page)

Total amount of queries:
[root@or v2]# grep SELECT /tmp/cv_without_opt1.log | wc -l
1234
[root@or v2]# grep SELECT /tmp/cv_opt1.log | wc -l
755

[root@or v2]# grep SELECT /tmp/cv_without_opt1.log | grep content_view_version | wc -l
641
[root@or v2]# grep SELECT /tmp/cv_opt1.log | grep content_view_version | wc -l
181

Details: https://pawelurbanek.com/rails-eager-ordered (and thank you so much for this page)

[ekohl]

Test failures look related.

[jeremylenz]

Great find @sbernhard, thanks!

I tested this on my setup. With my ~20 CVs with 2 versions each, this improved my loading time from >3 sec every time to about 2.5 sec.

just a couple minor comments/questions

[sbernhard]

Katello tests are now green.

[jeremylenz]

@sbernhard looking good, please squash or reword your commit to make 'Redmine issues' happy :)

[ekohl]

Can create_new_version somehow invalidate the latest_version_object cache on the object?

[sbernhard]

• Rails load the @composite content view.
• Somewhere, latest_version_object is used and stored (and would be re-used later on)
• create_new_version creates a new version but the reference in the already loaded @composite is not updated.

[ekohl]

Yes, I got that. That's why I was asking if you can somehow flush that cached latest_version_object from Rails. Because I'm worried you might end up serving some outdated data somewhere. Looks like this is a Rails 7.1 feature: https://www.shakacode.com/blog/rails-7-1-allows-resetting-singular-associations/

So I think you can some something like:

# Rails 7.1+
reset_latest_version_object if respond_to?(:reset_latest_version_object)

[jeremylenz]

I seem to be on Rails 6.1.7.6. (I didn't think we had upgraded to 7.1 already..)

Anyway, reload seems fine to me for the time being.

[ekohl]

I seem to be on Rails 6.1.7.6. (I didn't think we had upgraded to 7.1 already..)

Yes, we're not there yet. I wish we were, but not yet. We're working on it.

Anyway, reload seems fine to me for the time being.

My point was that this can also break things at runtime. Previously you could count on latest_version_object being up to date, but after this it may be stale. For example, if a controller creates the new object and then uses latest_version_object it will show the old data.

I don't know if that happens and if we have any coverage for it, but it's an API change that can be breaking.

[sbernhard]

Is it really necessary to reload it? AFAIK, if API / UI is used, you run a action, like "Create a new Version" and then you run another action - which would be a completely new request and therefore all the structure will be reloaded automatically. I think, we are trying to solve a "unit-test-issue" which doesn't occur in production.

[ekohl]

No, I'm not saying you need to call reload. All I'm saying is that there is a cache invalidation that doesn't happen and that's a risk.

For example, if you have a REST API that is something like POST /content_views/:id/versions (so effectively an API controller) and the implementation is roughly:

@content_view.create_new_version
{
  'id' => @content_view.id,
  'latest' => @content_view.latest_version_object,
}

Before this patch it would work. After this patch it would show incorrect data.

It would be very easy to do this, because the actual use is in RABL. Also note that there are other indirections, such as latest_version and latest_version_id. So these do show up here:

katello/app/views/katello/api/v2/content_views/base.json.rabl

Line 8 in 3dcae86

attributes :latest_version, :latest_version_id

So all I'm saying is that we must verify such code doesn't exist, or if it does, deal with it. If it does exist, there is a very good chance there are no tests for such a thing, so it doesn't show up as a failure. It's very easy to introduce a regression here.

[sbernhard]

So, should we:

1. do a reload in create_new_version or
2. run a reload after create_new_version in the tests?

[ekohl]

I think that from reading the source of the actions it's probably safe to assume it's not a problem, but I'd still leave at least a TODO comment for Rails 7.1 that it can safely invalidate the cached entry.

[sbernhard]

ok

[jeremylenz]

I think all we're waiting on here is a TODO comment, right?

[sbernhard]

Broken React Test not related to this change.

[jeremylenz]

LGTM pending green tests. Failures are unrelated but let's wait just to be safe.

[jeremylenz]

[test katello]

[jeremylenz]

@sbernhard I think this may need another rebase due to the recent CI changes.

[jeremylenz]

actually no, it ain't a required check! Merging