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

[WIP] Miq expression interp #22989

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Apr 12, 2024

parent:

Made some progress

I wanted to see what our code looks like for preprocessing the whole tree up front. The code was parsing the same fields over and over for to_ruby. The thought is if to_ruby has all the data it needs, then an interpreter will have all the data it needs.

  • don't lookup fields multiple times in to_ruby but rather once in the preprocessor
  • don't lookup fields and validate in valid? but rather one time in the preprocessor
  • embed field objets in the expr tree itself.
  • drop col_details and just use the cached field value from the preprocessor

There are still a few places to remove more Field.parse calls, but seems good enough.


From here we want to transition focus on Condition.subst.
This is where we spend the most of our time and create the most objects.

If we can verify that we don't use mode="tag_expr", then we can avoid the _to_ruby.gsub() { _subst}.gsub() { _subst } code and have the to_ruby produce the correct code in the first place.

@Fryguy do you know anything about mode="tag_expr" and if we would have customers still using that?

@Fryguy
Copy link
Member

Fryguy commented Apr 12, 2024

@Fryguy do you know anything about mode="tag_expr" and if we would have customers still using that?

I did a little spelunking, and in 2008 @gtanzillo (Hi Greg!) introduced mode=tag_expr_v2 in 37762294131e0cc24e05adf13fa5bbc85fa19200 (pre-open-source), and replaced its usage in a bunch of seeded policies to use that. Below is an example diff of one of those changes where you can see the old format and the new format. Note that tag_expr_v2 is the MiqExpression format, as you can see in https://github.com/ManageIQ/manageiq/blob/master/app/models/condition.rb#L68 where it's used directly in MiqExpression.

Maybe @gtanzillo can remember anything here on this old format?

Even so, I'd be super-surprised if tag_expr still existed in any form for existing users/customers as 2008 was the big switch. git grep and org wide searches tell me this isn't used anymore and we can drop it.

@ vmdb/product/policies/env_prod_vm_host.yml:7 @ modifier: "allow"
 who: "system"
 what: "start"
 rule:
-    mode: "tag_expr"
-    expr: |-
-        <exist ref=host>/managed/environment/prod</exist> &&
-        <exist>/managed/environment/prod</exist>
+    mode: "tag_expr_v2"
+    expr:
+        and:
+            - exist:
+                field: Host.managed-environment
+                value: prod
+            - exist:
+                 field: Vm.managed-environment
+                 value: prod

@Fryguy Fryguy changed the title Miq expression interp [WIP] Miq expression interp Apr 12, 2024
@miq-bot miq-bot added the wip label Apr 12, 2024
@kbrock kbrock requested a review from agrare as a code owner April 15, 2024 16:48
@kbrock
Copy link
Member Author

kbrock commented May 24, 2024

update:

  • rebase

…s, value)

basically just moving up the type checking to the method caller
This changes the method interface so we can pass in a field object instead of calling col_details
use that value when converting datatype
converge value_in_sql and field_in_sql
still work even if :token is the first key in the exp
still process find sub targets

this gets us more values with field-field
Think this was the only reference to MiqExpressoin#valid?
@miq-bot
Copy link
Member

miq-bot commented Jul 16, 2024

Checked commits kbrock/manageiq@4150ce6~...ddf4470 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
3 files checked, 2 offenses detected

lib/miq_expression.rb

  • ❗ - Line 335, Col 11 - Style/HashTransformValues - Prefer transform_values over each_with_object.
  • ❗ - Line 356, Col 14 - Performance/CollectionLiteralInLoop - Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant.

@miq-bot miq-bot added the stale label Nov 1, 2024
@miq-bot
Copy link
Member

miq-bot commented Nov 1, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

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.

3 participants