Choice rule payload validation #189

kbrock · 2024-05-22T21:28:32Z

alt to #187

Depend upon:

Overview

Remove un-necessary string allocations. and symbol lookups

Most of the changes have already been pulled out and merged.
So if we don't like this, then that is fine.

Leaving this for legacy reasons

Overview

Choice does not quite behave the same as the AWS States Language reference implementation.

If you specify a Path that points to nothing or the wrong type, aws raises a States.Runtime. In truth, the type checking is not the most consistent, but the missing path is always raised.

Changes:

Validate Next points to a valid state.
Validate Variable and compare key values are the correct data type.
Raise exceptions when Variable or compare key path values not not found or the wrong data type. It used to throw ruby exceptions.
Fix "IsPresent": true to detect values present rather than not null.
All "Is*" comparisons now support true and false values.
Support Choice with no Default provided.

kbrock · 2024-05-22T23:51:08Z

update:

cop: returning false instead of nil for bad lhv
cop: space in spec
rebased

kbrock · 2024-05-23T00:53:12Z

wip: pre-compiling operations

kbrock · 2024-05-23T04:54:47Z

update:

compiled expression into command tree
changed key comparison to not look for String, but rather (String)(GreaterThan)(Path). It has a few false positives, (IntegerMatchesPath) but they will all probably work.

This is a lot more involved but a lot more strict/stringent

update:

cops: hash rocket, indent, annotation
no longer escaping matches up front (can roll back if others want)

kbrock · 2024-05-23T13:40:44Z

rubocop: command line and web have different opinions on whether a regex is freezable. one complains no matter what code I use here.

un-wip: this seems to work

kbrock · 2024-05-23T19:56:58Z

update:

rebase
simplified down choice class lookup code

update:

added freeze back in

lib/floe/workflow/choice_rule/data.rb

Fryguy · 2024-05-23T20:03:13Z

Can you add specs around these new classes? wondering if would catch the invalid namespace thing I found.

kbrock · 2024-05-23T20:08:13Z

update:

change class reference

I commented out every line there, and the specs fail. So every one of those classes are tested

kbrock · 2024-05-23T20:20:43Z

Discussion:
Instead of using a command pattern, we can change this to store the operation so we wouldn't need all these classes.

e.g.:

OPERATIONS = { "StringEquals" => "==" }
lhs.send(OPERATIONS[choice_value], rhs)

Fryguy · 2024-05-23T21:20:16Z

lib/floe/workflow/choice_rule/data.rb

+            values = COMPARE_RULE.match(key)
+            return [key, DATA_RULES[values[2]], !!values[3]] if values


I think this would read better if the match was decomposed or perhaps if named captures were used instead of using values[2], values[3], etc.

Some other quetsions

where is the prefix used (the (String|Numeric|Boolean|Timestamp) part)? It seems to be ignored here.

The caller does compare_key, klass = klass_params(payload), but this method returns 3 items, so is that a bug? Where did the 3rd param go with the path, and why isn't it being used?

the current pattern for the states/command objects to parse the payload themselves.
Here, we need to parse the string to

The old code ignored the first part, so I continued to ignore it. I can see doing type checks or conversions.

returning 2 vs 3 is probably a bug. thanks

yes, we ignore the type prefix part and don't currently do data type conversion. (not sure if that is needed)

changed this code. think all set.

Fryguy · 2024-05-23T21:24:10Z

Discussion: Instead of using a command pattern, we can change this to store the operation so we wouldn't need all these classes.

e.g.:
OPERATIONS = { "StringEquals" => "==" }
lhs.send(OPERATIONS[choice_value], rhs)

Maybe, but this all feels overly complicated to me personally. I don't see why each of these can't be a simple method (or even just the original code the way it was). Just curious what problem you were trying to solve. I could see creating the classes to encapsulate a true? + valid? pair, but all of the new classes just have a single method.

kbrock · 2024-05-31T02:01:17Z

sorry - this is for previous commit

update:

rebase
dropped comparison classes and put into an operation's hash.
No longer modify parent ChoiceRule class
storing path or value in ref to simplify the initialize code

kbrock · 2024-05-31T02:03:10Z

update

went back to all the is_{type} methods
dropped all separate classes
more pedantic at build time (fewer invalid cases go through

kbrock · 2025-09-05T19:58:43Z

update:

rebased (to make merge-able)

update:

converted "string".to_sym to :"string"

un-WIP: this is working, no more dependencies. want to either merge or just drop

Fryguy · 2025-09-08T14:57:26Z

lib/floe/workflow/choice_rule/data.rb

      class Data < Floe::Workflow::ChoiceRule
        TYPES      = ["String", "Numeric", "Boolean", "Timestamp", "Present", "Null"].freeze
        COMPARES   = ["Equals", "LessThan", "GreaterThan", "LessThanEquals", "GreaterThanEquals", "Matches"].freeze
+        OPERATIONS = TYPES.each_with_object({}) { |dt, a| a[dt] = :"is_#{dt.downcase}?" } \


Line continuation is not needed

Suggested change

OPERATIONS = TYPES.each_with_object({}) { |dt, a| a[dt] = :"is_#{dt.downcase}?" } \

OPERATIONS = TYPES.each_with_object({}) { |dt, a| a[dt] = :"is_#{dt.downcase}?" }

strange, thought I did this a while back. must have rebased it out of here.
It is back

Fryguy · 2025-09-08T14:58:18Z

lib/floe/workflow/choice_rule/data.rb

-        attr_reader :variable, :compare_key, :operation, :type, :compare_predicate, :path
+        attr_reader :variable, :compare_key, :operator, :type, :compare_predicate, :path


This is changing the "public" api - not a big deal if it's internal, but is anything else using this?

we've been pretty lax on public and private api for these models.

Every attribute here is internal to this class only.

In a separate PR, do we want to circle through and mark private/public?
Would it buy us anything, or will it just make debugging more difficult?

Come to think of it, I think only Workflow and Context have a public api.

No it's fine, I just wanted to make sure that was accounted for or not.

Fryguy · 2025-09-08T15:01:23Z

This PR got renamed? Having trouble following the history here.

kbrock · 2025-09-09T20:40:52Z

This was a PR with a ton of stuff. almost all of the stuff had been extracted (as you can see from the punch list at the top)
The only thing left in a minimal (and not mandatory) change to not build the names of the operations at runtime, but rather just use a lookup.

Originally, the type checks (IsString) and operations/compares (StringGreaterThan) stored in TYPES and COMPARES were hashes from the keys to the method names. As we extracted out the PRs, this hash structure was lost.

I am not a fan of the non-grep style of send("#{var1}#{var2}"). I guess OPERATIONS is essentially just that, thought defined up front. Alternately, explicitly listing the operations in the hash form of TYPES and COMPARES may make sense.

update:

rebased
spelling fixes in comments
some descriptions around the checks and parameters
removed unnecessary line continuation

Fryguy · 2025-09-10T18:58:17Z

lib/floe/workflow/choice_rule/data.rb

            elsif (match_value = TYPE_CHECK.match(key))
              @compare_key = key
-              _operator, type = match_value.captures
+              _is, @operator = match_value.captures


If we don't need the Is, you can also just not capture it in the first place. i.e.

- TYPE_CHECK = /^(Is)(#{TYPES.join("|")})$/ + TYPE_CHECK = /^Is(#{TYPES.join("|")})$/

@Fryguy Yes, totally agree that the Is does not need to be captured, and it is probably more resource intensive.

I like the parallelism:

operations has (String)(Equals), (String)(GreaterThan)(Path).

type check has (Is)(String)

Where we have a type (i.e.: String) and a verb (e.g.: Is, Equals, GreaterThan).

Fryguy · 2025-09-10T19:01:44Z

lib/floe/workflow/choice_rule/data.rb

+        OPERATIONS = TYPES.each_with_object({}) { |dt, a| a[dt] = :"is_#{dt.downcase}?" } \
+                          .merge(COMPARES.each_with_object({}) { |op, a| a[op] = :"op_#{op.downcase}?" }).freeze
        # e.g.: (Is)(String), (Is)(Present)
        TYPE_CHECK = /^(Is)(#{TYPES.join("|")})$/


I just noticed this now, so not for this PR, but instead of TYPES.join(|), you should probably use Regex.union(TYPES). Same goes for the OPERATION constant

irb(main):006> /Is(#{['a', 'b'].join("|")})/ => /Is(a|b)/ irb(main):008> /Is(#{Regexp.union(['a', 'b'])})/ => /Is((?-mix:a|b))/

Are these the same thing?

I have a separate PR all set for this

Fryguy · 2025-09-10T19:03:46Z

I'm fine with the PR rename now that the scope has been narrowed - it was just confusing because the opening PR had this list of dependent PRs that seemed unrelated.

kbrock · 2025-09-11T18:57:15Z

@Fryguy I think there is a question that asks, do we want to even do this?

A while ago, we used to have hashes that were manually created:

COMPARES = {"GreaterThan" => "is_gt?", ...}

I liked that simple lookup. Currently, those are arrays not hashes and we manually generate the methods on the fly.

This PR suggests auto generating a hash, but maybe that is just overkill/un-necessary.

Do we think a static hash makes this any more sense/grepable/readable? Or is there another way to make this more accessible? (Just put the operation comment at the beginning of every op_gt? kind of method?).

Performance wise, allocation difference will be negligible. Not sure the best way to benchmark this - but doesn't seem worth the work to gauge this.

kbrock · 2025-09-12T15:28:30Z

update

remove line continuation (could have sworn I did this a while ago)
removed (Is) capture

- No longer access workflow#payload out side of Workflow (ManageIQ#321) - Cache choice rule operations (ManageIQ#189) - Fix `Context#==` to compare contents (ManageIQ#317) - Declare active support dependency (ManageIQ#316)

Added - Declare active support dependency (#316) - Add Context equality comparison (#317) - Choice rule payload validation (#189) Changed - In ChoiceRule use Regexp.union (#323) - Remove special case for choice rule IsPresent (#322) - Rename data operation parameters (#324) - Refactor workflow state check to not use payload (#326) Fixed - Fix missing parameters in item_batcher_spec (#325)

kbrock added the enhancement New feature or request label May 22, 2024

kbrock requested review from Fryguy and agrare as code owners May 22, 2024 21:28

kbrock changed the title ~~Choice rule payload validation~~ [WIP] Choice rule payload validation May 23, 2024

miq-bot added the wip label May 23, 2024

kbrock force-pushed the choice_rule_payload_validation branch from a0d0c26 to 0993b77 Compare May 23, 2024 04:47

kbrock force-pushed the choice_rule_payload_validation branch from 0993b77 to 07a1411 Compare May 23, 2024 04:58

kbrock changed the title ~~[WIP] Choice rule payload validation~~ Choice rule payload validation May 23, 2024

miq-bot removed the wip label May 23, 2024

kbrock force-pushed the choice_rule_payload_validation branch 3 times, most recently from fdab00e to e1bce92 Compare May 23, 2024 19:55

Fryguy reviewed May 23, 2024

View reviewed changes

lib/floe/workflow/choice_rule/data.rb Outdated Show resolved Hide resolved

kbrock force-pushed the choice_rule_payload_validation branch from e1bce92 to 4ff6108 Compare May 23, 2024 20:07

Fryguy reviewed May 23, 2024

View reviewed changes

kbrock changed the title ~~Choice rule payload validation~~ [WIP] Choice rule payload validation May 24, 2024

kbrock added the wip label May 24, 2024

kbrock force-pushed the choice_rule_payload_validation branch 2 times, most recently from 856fce4 to 1ceb8f4 Compare May 31, 2024 02:01

kbrock force-pushed the choice_rule_payload_validation branch from 49a5ec5 to e5455a3 Compare September 5, 2025 19:58

kbrock changed the title ~~[WIP] Choice rule payload validation~~ Remove unnecessary string allocations for operator lookups Sep 5, 2025

kbrock removed the wip label Sep 5, 2025

Fryguy reviewed Sep 8, 2025

View reviewed changes

kbrock force-pushed the choice_rule_payload_validation branch from e5455a3 to afab6ce Compare September 9, 2025 20:22

kbrock mentioned this pull request Sep 10, 2025

[WIP] Add choice_rule payload validation #187

Closed

kbrock force-pushed the choice_rule_payload_validation branch from afab6ce to b783324 Compare September 10, 2025 14:14

kbrock changed the title ~~Remove unnecessary string allocations for operator lookups~~ Choice rule payload validation Sep 10, 2025

Fryguy reviewed Sep 10, 2025

View reviewed changes

Fryguy closed this Sep 10, 2025

Fryguy reopened this Sep 10, 2025

kbrock force-pushed the choice_rule_payload_validation branch 2 times, most recently from 38a20cf to fe8a2db Compare September 12, 2025 15:22

Lookup for operations

a5d6f21

kbrock force-pushed the choice_rule_payload_validation branch from fe8a2db to a5d6f21 Compare September 12, 2025 15:28

Fryguy approved these changes Sep 12, 2025

View reviewed changes

Fryguy merged commit 948fbf1 into ManageIQ:master Sep 12, 2025
5 checks passed

Fryguy assigned Fryguy and unassigned agrare Sep 12, 2025

kbrock deleted the choice_rule_payload_validation branch September 15, 2025 16:05

		values = COMPARE_RULE.match(key)
		return [key, DATA_RULES[values[2]], !!values[3]] if values

	OPERATIONS = TYPES.each_with_object({}) { \|dt, a\| a[dt] = :"is_#{dt.downcase}?" } \
	OPERATIONS = TYPES.each_with_object({}) { \|dt, a\| a[dt] = :"is_#{dt.downcase}?" }

		attr_reader :variable, :compare_key, :operation, :type, :compare_predicate, :path
		attr_reader :variable, :compare_key, :operator, :type, :compare_predicate, :path

Choice rule payload validation #189

Choice rule payload validation #189

Uh oh!

Conversation

kbrock commented May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Overview

Uh oh!

kbrock commented May 22, 2024

Uh oh!

kbrock commented May 23, 2024

Uh oh!

kbrock commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbrock commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbrock commented May 23, 2024

Uh oh!

Uh oh!

Fryguy commented May 23, 2024

Uh oh!

kbrock commented May 23, 2024

Uh oh!

kbrock commented May 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fryguy commented May 23, 2024

Uh oh!

kbrock commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbrock commented May 31, 2024

Uh oh!

kbrock commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbrock Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fryguy commented Sep 8, 2025

Uh oh!

kbrock commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fryguy Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbrock Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fryguy commented Sep 10, 2025

Uh oh!

kbrock commented Sep 11, 2025

Uh oh!

kbrock commented Sep 12, 2025

kbrock commented May 22, 2024 •

edited

Loading

kbrock commented May 23, 2024 •

edited

Loading

kbrock commented May 23, 2024 •

edited

Loading

kbrock commented May 31, 2024 •

edited

Loading

kbrock commented Sep 5, 2025 •

edited

Loading

kbrock Sep 9, 2025 •

edited

Loading

kbrock commented Sep 9, 2025 •

edited

Loading

Fryguy Sep 10, 2025 •

edited

Loading

kbrock Sep 11, 2025 •

edited

Loading