Make MiqExpression#contains work in Rails 5.2 #20051

kbrock · 2020-04-06T17:37:54Z

make MiqExpression#contains work in rails 5.2

MiqExpression CONTAINS built up the sql for a contains clause.
This would only work with direct simple relations and broke in the upgrade to rails 5.2.

In this PR, rails was asked to build the sql for a query that is close. Then the extra nodes are removed.
There were bind variables that needed to be removed. We typically use just compile the query and turn into a sql literal. Unfortunately, sql IN clauses are picky about the parameter that they accept, so we needed to pass arel.

The merge conditions code broke as well. We are patching it in this PR as well

kbrock · 2020-04-06T19:26:22Z

^ fixed cops

jrafanie · 2020-04-07T16:13:20Z

Will this replace this commit on the rails-5-2 branch? jrafanie@1829461

NickLaMuro · 2020-04-07T16:22:32Z

Followup: Is this Rails 5.2 only, or can we merge this into master now?

NickLaMuro

I think I am 50/50 on grasping this, but left some comments to start with for now. Mostly are docs/clarification since I am still trying to understand this PR.

lib/miq_expression.rb

spec/lib/miq_expression_spec.rb

lib/miq_expression.rb

NickLaMuro · 2020-04-07T22:41:50Z

@kbrock I think the PR description could really use some example cases that are being fixed here. The specs you add only test the private methods, and don't really paint a picture at what the over "SQL" or MiqExpression might be.

kbrock · 2020-04-09T08:02:40Z

heh. added a few more use cases and test
I'm having way too much fun with this
(I already have my next PR which adds multi level tagging.)

@NickLaMuro This started out trying to get this functionality working on rails 5.2. It was tricky to then get it to work on both versions.

This was a "simplification" by using rails to build the query for us, so we didn't have to create the active record objects or handle 2 different active record interfaces.

It ended up becoming an enhancement because rails did so much for us. I started adding tests/use cases for the edge cases (that had no chance of working before)

NickLaMuro

Some comments after talking with @kbrock out of band.

lib/miq_expression.rb

lib/miq_expression/query_helper.rb

lib/miq_expression.rb

spec/lib/miq_expression/query_helper_spec.rb

kbrock · 2020-04-14T03:49:29Z

Added the visitor.
fixed the remaining rubocop.

@NickLaMuro is this closer to what you had in mind?

NickLaMuro

I think I need to look at the visitor a bit more to make sure I fully understand things before I sign off, but I think I have enough critiques with what I have already to provide a first round of feedback.

lib/miq_expression/query_helper.rb

spec/lib/miq_expression/query_helper_spec.rb

NickLaMuro · 2020-04-14T17:03:27Z

spec/lib/miq_expression/query_helper_spec.rb

+
+    # query:  "hosts"."type" = 'abc'
+    # result: "hosts"."type" = 'abc'
+    # This doesn't make sense, but we saw this example


"saw this example" where? In specs? In production? In a magical fairy land where raw SQL is allowed to roam freely?

This is how the parameter was coming into our code for our code example

in miq test suite (thought it could have been another problem. who knows

NickLaMuro · 2020-04-14T17:05:24Z

spec/lib/miq_expression/query_helper_spec.rb

+      expect(result).to be_nil
+    end
+
+    # query:  <invalid>


Edit: Wow... that didn't come out right... let me try that again...

I feel like <invalid> is not serving much of a purpose, helping to describe what <invalid> is supposed to be, or where it might happen. Again, similar comment to the "but we saw this example" one.

spec/lib/miq_expression/query_helper_spec.rb

spec/lib/miq_expression_spec.rb

NickLaMuro

FYI about Arel::Visitors::Visitor

NickLaMuro · 2020-04-15T20:42:06Z

lib/miq_expression/remove_equality_visitor.rb

+    end
+  end
+  # rubocop:disable Naming/MethodName
+  class RemoveEqualityVisitor < Arel::Visitors::Reduce


I don't think this exists in Rails 6... possibly in Rails 5.2 depending on what version of ActiveRecord we bumped to in the upgrade:

rails/arel#512

That said, it looks like it was effectively Arel::Visitors::Visitor anyway.

we can cross that bridge when we upgrade to 6.0

This is a "I did a bundle open activerecord and couldn't find this class", not a "this might be an issue in the future" thing. I see this being a problem right now, not later.

https://github.com/rails/rails/blob/5-2-stable/activerecord/activerecord.gemspec#L34

5-1 uses v8 ==> https://github.com/rails/arel/blob/v8.0.0/lib/arel/visitors/reduce.rb
5-2 uses v9 ==> https://github.com/rails/arel/blob/v9.0.0/lib/arel/visitors/reduce.rb

I think it is good for rails 5.1 and 5.2.
rails 6.0 just merged it into a basic Visitor

kbrock · 2020-04-22T03:39:10Z

updated the typo and rebased to recent version of miq. hoping that fixes the rubygem fetch issue

this is needed to support rails 5.2

kbrock · 2020-04-25T01:37:19Z

sorry nick. this contains a few rails 5.2 vs 5.1 switches.
once we get this all set, I can remove them. I just wanted this off of master and the other one off of 5.2.

also, thinking I'll drop the traverser commit. Not really doing any good besides making it difficult to rebase stuff.

NickLaMuro · 2020-04-27T13:43:37Z

also, thinking I'll drop the traverser commit. Not really doing any good besides making it difficult to rebase stuff.

That is fine. What I tend to do, though (and you don't have to), is copy the branch prior to dropping a commit like that, and push it to my fork for historical reference, and link it in the PR (assuming you don't prune it later). That way there is a way for someone to take a look at what was considered previously when looking back on this PR.

NickLaMuro · 2020-04-27T13:45:13Z

sorry nick. this contains a few rails 5.2 vs 5.1 switches.

Okay, if that is the case, I won't be reviewing further then. I have made my case against this multiple times, and still disagree with you on this. I don't think it will be productive having me review further.

kbrock · 2020-04-28T12:56:46Z

reverted this PR back to previous form with a visitor.

I prefer new way #20110

miq-bot · 2020-04-29T15:59:02Z

Checked commits kbrock/manageiq@6f099e9~...c97bafe with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
4 files checked, 1 offense detected

lib/miq_expression/remove_equality_visitor.rb

❗ - Line 32, Col 9 - Style/NegatedIf - Favor unless over if for negative conditions.

miq-bot · 2020-04-29T21:05:24Z

This pull request is not mergeable. Please rebase and repush.

NickLaMuro · 2020-04-29T21:10:31Z

Closing this since this is the old form of the PR.

kbrock added the rails5.2 label Apr 6, 2020

kbrock requested review from Fryguy and gtanzillo as code owners April 6, 2020 17:37

kbrock force-pushed the miq_expression_contains_2 branch from 171bc0a to 2ab6ce3 Compare April 6, 2020 19:26

kbrock mentioned this pull request Apr 6, 2020

MiqExpression "CONTAINS" now uses active record #20026

Closed

chessbyte mentioned this pull request Apr 6, 2020

Support Rails 5.2 #20032

Closed

38 tasks

chessbyte changed the title ~~Miq expression contains 5.2 compatible~~ Make MiqExpression#contains work in Rails 5.2 Apr 7, 2020

NickLaMuro suggested changes Apr 7, 2020

View reviewed changes

kbrock force-pushed the miq_expression_contains_2 branch 3 times, most recently from 2a29306 to e8600f7 Compare April 9, 2020 06:17

kbrock force-pushed the miq_expression_contains_2 branch 2 times, most recently from 277ce65 to 806177f Compare April 9, 2020 16:38

NickLaMuro suggested changes Apr 9, 2020

View reviewed changes

kbrock force-pushed the miq_expression_contains_2 branch 5 times, most recently from 3b8b650 to b932ef2 Compare April 14, 2020 02:35

NickLaMuro suggested changes Apr 14, 2020

View reviewed changes

kbrock force-pushed the miq_expression_contains_2 branch 4 times, most recently from 22ef577 to 07334ad Compare April 15, 2020 16:30

kbrock force-pushed the miq_expression_contains_2 branch 2 times, most recently from 90e0640 to ac6aae8 Compare April 15, 2020 19:53

NickLaMuro suggested changes Apr 15, 2020

View reviewed changes

kbrock force-pushed the miq_expression_contains_2 branch from ac6aae8 to 0ee8cdb Compare April 15, 2020 20:58

Fryguy assigned jrafanie Apr 16, 2020

Fryguy added technical debt enhancement and removed technical debt labels Apr 16, 2020

Update miq_query specs to prevent failures

6f099e9

kbrock force-pushed the miq_expression_contains_2 branch from 0ee8cdb to d81cb97 Compare April 22, 2020 03:38

kbrock mentioned this pull request Apr 24, 2020

latest 5-2 tests ManageIQ/manageiq-cross_repo-tests#114

Closed

Fryguy removed their request for review April 24, 2020 22:28

modify contains to use arel instead of sql

5c6a14c

this is needed to support rails 5.2

kbrock force-pushed the miq_expression_contains_2 branch from d81cb97 to c54488b Compare April 25, 2020 01:02

kbrock force-pushed the miq_expression_contains_2 branch from c54488b to a376ca6 Compare April 28, 2020 01:38

kbrock mentioned this pull request Apr 28, 2020

Modify MiqExpression Contains to work for Rails 5.2 #20110

Merged

kbrock force-pushed the miq_expression_contains_2 branch from a376ca6 to 6005c51 Compare April 28, 2020 12:55

convert from a traverser arel to a visitor

c97bafe

kbrock force-pushed the miq_expression_contains_2 branch from 6005c51 to c97bafe Compare April 28, 2020 13:18

miq-bot added the unmergeable label Apr 29, 2020

NickLaMuro closed this Apr 29, 2020

kbrock deleted the miq_expression_contains_2 branch April 30, 2020 00:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make MiqExpression#contains work in Rails 5.2 #20051

Make MiqExpression#contains work in Rails 5.2 #20051

kbrock commented Apr 6, 2020

kbrock commented Apr 6, 2020

jrafanie commented Apr 7, 2020

NickLaMuro commented Apr 7, 2020

NickLaMuro left a comment

NickLaMuro commented Apr 7, 2020

kbrock commented Apr 9, 2020 •

edited

Loading

NickLaMuro left a comment •

edited

Loading

kbrock commented Apr 14, 2020

NickLaMuro left a comment

NickLaMuro Apr 14, 2020

kbrock Apr 14, 2020

kbrock Apr 14, 2020

NickLaMuro Apr 14, 2020 •

edited

Loading

NickLaMuro left a comment

NickLaMuro Apr 15, 2020

NickLaMuro Apr 15, 2020

kbrock Apr 20, 2020

NickLaMuro Apr 21, 2020

kbrock Apr 22, 2020 •

edited

Loading

kbrock commented Apr 22, 2020

kbrock commented Apr 25, 2020

NickLaMuro commented Apr 27, 2020

NickLaMuro commented Apr 27, 2020

kbrock commented Apr 28, 2020

miq-bot commented Apr 29, 2020

miq-bot commented Apr 29, 2020

NickLaMuro commented Apr 29, 2020

Make MiqExpression#contains work in Rails 5.2 #20051

Make MiqExpression#contains work in Rails 5.2 #20051

Conversation

kbrock commented Apr 6, 2020

kbrock commented Apr 6, 2020

jrafanie commented Apr 7, 2020

NickLaMuro commented Apr 7, 2020

NickLaMuro left a comment

Choose a reason for hiding this comment

NickLaMuro commented Apr 7, 2020

kbrock commented Apr 9, 2020 • edited Loading

NickLaMuro left a comment • edited Loading

Choose a reason for hiding this comment

kbrock commented Apr 14, 2020

NickLaMuro left a comment

Choose a reason for hiding this comment

NickLaMuro Apr 14, 2020

Choose a reason for hiding this comment

kbrock Apr 14, 2020

Choose a reason for hiding this comment

kbrock Apr 14, 2020

Choose a reason for hiding this comment

NickLaMuro Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

NickLaMuro left a comment

Choose a reason for hiding this comment

NickLaMuro Apr 15, 2020

Choose a reason for hiding this comment

NickLaMuro Apr 15, 2020

Choose a reason for hiding this comment

kbrock Apr 20, 2020

Choose a reason for hiding this comment

NickLaMuro Apr 21, 2020

Choose a reason for hiding this comment

kbrock Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

kbrock commented Apr 22, 2020

kbrock commented Apr 25, 2020

NickLaMuro commented Apr 27, 2020

NickLaMuro commented Apr 27, 2020

kbrock commented Apr 28, 2020

miq-bot commented Apr 29, 2020

miq-bot commented Apr 29, 2020

NickLaMuro commented Apr 29, 2020

kbrock commented Apr 9, 2020 •

edited

Loading

NickLaMuro left a comment •

edited

Loading

NickLaMuro Apr 14, 2020 •

edited

Loading

kbrock Apr 22, 2020 •

edited

Loading