Modify MiqExpression Contains to work for Rails 5.2

[kbrock]

Overview

Alternative to #20051 Addresses #20032

We need the sql for:

MiqExpression.new(
  "CONTAINS" => {"field" => "Vm.guest_applications-name", "value" => "foo"}
)

Rails typically formats EXISTS and CONTAINS queries in the following way:

contains = Vm.joins(:guest_applications).where(:guest_applications=>{:name=>'foo'})
Vm.where(:id => contains)

There ends up being an extra join in there to vms, but it is using the rails framework instead of fighting it is a big win. We also have bigger performance battles to fight.

The fix

In Rails 5.2, relations and bound variables work differently. So it was not simple to patch the work we did in extract_where_values and WhereExtractionVisitor. It was simpler to change to a different approach that will work with rails 5.2.

The changes do change the SQL produced. There is an extra join as a result, but the logic to get there is much simpler since we are not using a visitor to do it.

Before

"vms"."id" IN (
  SELECT DISTINCT "guest_applications"."vm_or_template_id"
  FROM "guest_applications"
  WHERE "guest_applications"."name" = 'foo'
)

After

The new approach does not use Arel and uses ActiveRecord with a few tweaks:

"vms"."id" IN (
  SELECT "vms"."id"
  FROM "vms"
  INNER JOIN "guest_applications"
  ON "guest_applications"."vm_or_template_id" = "vms"."id"
  WHERE "guest_applications"."name" = 'foo'
)

[NickLaMuro]

I am good with this I think. I have a few clarification questions outside of this review, but it is mostly double checking I understand what problem is being solved, and I think what you have here works.

[NickLaMuro]

You know what, I will concede on this for now since it is such a small area we are dealing with (instead of 50 lines of visitor code that was only for 5.1 compatibility).

That said, I think we should delete this right away as part of #20076 if we are planning to merge this into master right away. I would prefer this conditional not sticking around forever.

[jrafanie]

Yes, great idea. Once this is merged, let's drop this conditional in #20076

[chessbyte]

For this PR, is Travis set up to test BOTH Rails 5.1 and 5.2? Or is the current PR being tested on Rails 5.1 and #20076 will validate on Rails 5.2?

[NickLaMuro]

This PR is still testing against Rails 5.1.

We have a few branches doing cross repo testing that have the branch from #20076 in it:

https://travis-ci.org/github/jrafanie/manageiq-cross_repo-tests/builds/670576839
ManageIQ/manageiq-cross_repo-tests#114

Which are validating it there, but we will confirm this works in 5.2 again when #20076 is rebased again, un-WIP'd, and ready for a final review.

[NickLaMuro]

This is not something new, I take it. We are just adding specs to confirm this existing functionality, correct?

[NickLaMuro]

"vms"."id" IN (
  SELECT DISTINCT "guest_applications"."vm_or_template_id"
  FROM "guest_applications"
  WHERE "guest_applications"."name" = 'foo'
)

fails for rails 5.2

In what way? It looks like valid SQL to me, so I assume it is a generation issue that we are fixing, correct? Or the previous way we generated things used a removed ActiveRecord/Arel API?

(I may have never understood what your were solving for Rails 5.2... only that you were solving "something"...)

[kbrock]

fixed 2 (minor) cops.

[miq-bot]

Checked commits kbrock/manageiq@6f099e9~...edfe0ac with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 👍

[jrafanie]

Merging since this passes against 5.1 on master and we've tested it with cross repo on 5.2 in the past. We will address the points raised here in the 5.2 PR: #20076

[simaishi]

Jansa backport details:

$ git log -1
commit 240b9380bf8945e7504caa4de0873028cb3b5846
Author: Joe Rafaniello <[email protected]>
Date:   Wed Apr 29 17:03:10 2020 -0400

    Merge pull request #20110 from kbrock/miq_expression_contains_3

    Modify MiqExpression Contains to work for Rails 5.2

    (cherry picked from commit cff8306c9fbcd68b4b6285cc537fdf304093a3a8)