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

Fixed error with dialog expression when virtual column involved #17215

Conversation

yrudman
Copy link
Contributor

@yrudman yrudman commented Mar 27, 2018

ISSUE:
When virtual column involved in expression for automate method than error raised during attempt to order service. Error operator 'token' is not supported... raised when MiqExpression trying to convert expression to sql.
The same expression works fine in other places, like report.

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

Automation vs Report:
Automation do special pre-processing of expression (before converting to sql) and substitute value in expression with supplied tokens (if any). This pre-processing includes adding temporary entry like :token=>1 and supposed to be removed after automation specific pre-processing finished.

There is place on UI side were those temporary tokens removed before saving record: https://github.com/ManageIQ/manageiq-ui-
classic/blob/f16e3f542c0463773ca19291ddd8884e8863aa9c/app/controllers/application_controller/filter.rb#L343 , but there is no clean-up on automation side during method invocation

FIX:
Remove temporary tokens from expression during token substitution.
It would allow to skip attempt to transfer expression to_sql here:

sql = to_arel(@pexp, tz).to_sql if @pexp.present?

EXPRESSION:
setting-virtual

BEFORE:
before

AFTER:
after

@miq-bot add-label bug

\cc @gtanzillo @gmcculloug

@miq-bot miq-bot added the bug label Mar 27, 2018
@yrudman
Copy link
Contributor Author

yrudman commented Mar 28, 2018

@miq-bot assign @gtanzillo

@mkanoor
Copy link
Contributor

mkanoor commented Mar 28, 2018

This is the same as #16514

@yrudman
Copy link
Contributor Author

yrudman commented Mar 28, 2018

@miq-bot add-label gaprindashvili/yes

@yrudman yrudman force-pushed the remove-temporary-tokens-from-expression-after-substitution branch from 57a027e to 3d65dc0 Compare March 29, 2018 13:06
@Fryguy
Copy link
Member

Fryguy commented Apr 3, 2018

@kbrock Please review

it "removes :token key from passed expression" do
exp = {"=" => {"field" => "Vm-active", "value" => "true"}, :token => 1}
test_obj.exp_replace_qs_tokens(exp, {})
expect(exp).to eq("=" => {"field" => "Vm-active", "value" => "true"})
Copy link
Member

Choose a reason for hiding this comment

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

This looks good. But it may be good to test with a more complex expression. Something like -

{"and"=>[{"="=>{"field"=>"Vm-active", "value"=>"true"}, :token => 1}, {"="=>{"field"=>"Vm-archived", "value"=>"true"}, :token => 1}], :token => 1}

Just a suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gtanzillo ok, make sense, will do with this expression:
{"and"=>[{"="=>{"field"=>"Vm-active", "value"=>"true"}, :token => 1}, {"="=>{"field"=>"Vm-archived", "value"=>"true"}, :token => 1}]}. (there is no tokens added on the level of and or or)

@yrudman yrudman force-pushed the remove-temporary-tokens-from-expression-after-substitution branch from 1daa294 to a199611 Compare April 4, 2018 13:56
@miq-bot
Copy link
Member

miq-bot commented Apr 4, 2018

Checked commits yrudman/manageiq@40ba300~...a199611 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

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.

deleting the :token looks good.
Not exactly sure where/why it was added.

I'm good with this.
But would it make more sense for the ui that added the :token key to remove it?

@gtanzillo gtanzillo added this to the Sprint 83 Ending Apr 9, 2018 milestone Apr 5, 2018
@gtanzillo gtanzillo merged commit 54589f6 into ManageIQ:master Apr 5, 2018
simaishi pushed a commit that referenced this pull request Apr 9, 2018
…xpression-after-substitution

Fixed error with dialog expression when virtual column involved
(cherry picked from commit 54589f6)

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

simaishi commented Apr 9, 2018

Gaprindashvili backport details:

$ git log -1
commit 4a2af379cc61523e99bc32b0b0499e3199bdc697
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Thu Apr 5 14:26:27 2018 -0400

    Merge pull request #17215 from yrudman/remove-temporary-tokens-from-expression-after-substitution
    
    Fixed error with dialog expression when virtual column involved
    (cherry picked from commit 54589f650f2c4026a43dcf9b4230d77cd7b5d504)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1565139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants