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

Do not raise error in MiqExpression#to_sql if (:token) present as one of operators #19176

Merged
merged 2 commits into from
Aug 22, 2019
Merged

Do not raise error in MiqExpression#to_sql if (:token) present as one of operators #19176

merged 2 commits into from
Aug 22, 2019

Conversation

yrudman
Copy link
Contributor

@yrudman yrudman commented Aug 20, 2019 •
edited
Loading

ISSUE: Sometimes :token key (used in automate) is not removed from MiqExpression's Hash before saving or attempting to execute.
This will trigger below Error if another entry in MiqExpression does not support SQL generation:

[----] F, [2019-08-15T11:46:54.428352 #10513:3fecc1715820] FATAL -- : Error caught: [RuntimeError] operator 'token' is not supported
/Users/yrudman/work/rh/manageiq/lib/miq_expression.rb:1437:in `to_arel'
/Users/yrudman/work/rh/manageiq/lib/miq_expression.rb:289:in `to_sql'
/Users/yrudman/work/rh/manageiq/lib/rbac/filterer.rb:257:in `search'
/Users/yrudman/work/rh/manageiq/lib/rbac/filterer.rb:146:in `search'
/Users/yrudman/work/rh/manageiq/lib/rbac.rb:3:in `search'
/Users/yrudman/work/rh/manageiq/app/models/miq_report/search.rb:117:in `paged_view_search'
/Users/yrudman/work/rh/manageiq-ui-classic/app/controllers/application_controller.rb:1325:in `get_view'
/Users/yrudman/work/rh/manageiq-ui-classic/app/controllers/application_controller.rb:422:in `report_data'

Note: if another entry in MiqExpression does support SQL generation than no Error; it may explain why this bug was not found before.

FIX: remove :token from MiqExpression's Hash before attempting to generate SQL

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1741243

There is another PR #17215 on the same topic, but apparently it did not solve issue for all situations.

@miq-bot add-label changelog/no, bug, core, hammer/yes, ivanchuk/yes

@yrudman
Copy link
Contributor Author

yrudman commented Aug 20, 2019

@kbrock can you review
\cc @gtanzillo

@yrudman
during sql generation unexpected error was raised if SQL generation f…
3ad5e19 
…or expression is not supportyed and extra entry (:token) was present in MiqExpression's Hash.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1741243
@yrudman yrudman force-pushed the remove-not-expected-token-when-initializiing-miq-expression branch from 3286763 to 3ad5e19 Compare August 20, 2019 20:02
@kbrock
Copy link
Member

kbrock commented Aug 21, 2019

looked over the other PR. Why are we adding an invalid tag (i.e.: :token)to the MiqExpression?

@yrudman
Copy link
Contributor Author

yrudman commented Aug 21, 2019

@kbrock #16514 (comment)

@yrudman yrudman mentioned this pull request Aug 21, 2019
Closed
kbrock
kbrock requested changes Aug 21, 2019
View reviewed changes
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

options in order of preference:

  1. Don't put invalid entries (i.e. :token) into the expression
  2. The code responsible for adding the :token should remove it
  3. MiqExpresion supports having :token in the tree
  4. MiqExpression remove :token from the hash

Since @mkanoor seems to not want 1. And it looks like 2 is difficult, can we go with 3?
I'd really prefer to avoid option 4. (and I probably missed other options.)

lib/miq_expression.rb Outdated
@@ -285,7 +285,7 @@ def self._to_ruby(exp, context_type, tz)

def to_sql(tz = nil)
tz ||= "UTC"
@pexp, attrs = preprocess_for_sql(@exp.deep_clone)
@pexp, attrs = preprocess_for_sql(@exp.deep_clone.except!(:token))
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer we don't go this route

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kbrock is you objection because it looks as a "dirty fix" ?
on the bright side it is very save (modification done on clone) and will not do any harm now and in the future

I am not clear what you mean by solution 3.
Everywhere in the code of MiqExpression we assumed that only single entry in the top level of Hash is present, this code operator = exp.keys.first is all over

Copy link
Member

Choose a reason for hiding this comment

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

@kbrock, I'm looking at this with @yrudman. It seems like options 4 is the best, perhaps safe, solution. But I'm not clear why it's your least preffered option. Perhaps the three of us can discus?

@miq-bot
Copy link
Member

miq-bot commented Aug 21, 2019

Checked commits yrudman/manageiq@3ad5e19~...759a67d with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

kbrock
kbrock approved these changes Aug 22, 2019
View reviewed changes
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

like the change

@kbrock kbrock added this to the Sprint 119 Ending Sep 2, 2019 milestone Aug 22, 2019
@kbrock kbrock merged commit 4c39040 into ManageIQ:master Aug 22, 2019
@yrudman yrudman deleted the remove-not-expected-token-when-initializiing-miq-expression branch August 22, 2019 22:58
simaishi pushed a commit that referenced this pull request Oct 21, 2019
@kbrock @simaishi
Merge pull request #19176 from yrudman/remove-not-expected-token-when…
c10710a 
…-initializiing-miq-expression

Do not raise error in MiqExpression#to_sql if (:token) present as one of operators

(cherry picked from commit 4c39040)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1761525
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit c10710a70d5687d15ba39765380993b0bb4e0cce
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Thu Aug 22 15:35:01 2019 -0400

    Merge pull request #19176 from yrudman/remove-not-expected-token-when-initializiing-miq-expression
    
    Do not raise error in MiqExpression#to_sql if (:token) present as one of operators
    
    (cherry picked from commit 4c3904032d184eff1fb6aabe8c19bc73892b556e)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1761525

simaishi pushed a commit that referenced this pull request Dec 16, 2019
@kbrock @simaishi
Merge pull request #19176 from yrudman/remove-not-expected-token-when…
630d4d7 
…-initializiing-miq-expression

Do not raise error in MiqExpression#to_sql if (:token) present as one of operators

(cherry picked from commit 4c39040)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1784097
@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit 630d4d7e622a5f4c51a62b61d562162b2313a539
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Thu Aug 22 15:35:01 2019 -0400

    Merge pull request #19176 from yrudman/remove-not-expected-token-when-initializiing-miq-expression

    Do not raise error in MiqExpression#to_sql if (:token) present as one of operators

    (cherry picked from commit 4c3904032d184eff1fb6aabe8c19bc73892b556e)

    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1784097

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtanzillo gtanzillo gtanzillo left review comments

@kbrock kbrock kbrock approved these changes

Assignees
No one assigned
Labels
bug changelog/no core hammer/backported ivanchuk/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yrudman @kbrock @miq-bot @simaishi @gtanzillo