Choice rule payload validation

[kbrock]

alt to #187

Depend upon:

• Pass full path name to State.new for better errors #229
• [WIP] Catch and display path errors #246
• Choice rule payload validation path #253
• Split out ChoiceRule spec to be similar to other specs #254
• In ChoiceRule::Data, parse compare key and variable  #257
• Use Numeric over Integer || Float #264
• Implement "IsNumeric": false #266
• Support choices that do not have a Default defined #267
• Choice: rename value to compare_predicate #273
• More granular compare_key and determine path at initialization time #274
• Choice State enforces type checking #276
• Use send for choice operations #295

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]

update:

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

[kbrock]

wip: pre-compiling operations

[kbrock]

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]

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]

update:

• rebase
• simplified down choice class lookup code

update:

• added freeze back in

[Fryguy]

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

[kbrock]

update:

• change class reference

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

[kbrock]

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]

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?

[kbrock]

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

[kbrock]

1. yes, we ignore the type prefix part and don't currently do data type conversion. (not sure if that is needed)
2. changed this code. think all set.

[Fryguy]

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]

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]

update

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

[kbrock]

update:

• Rebased to master.
• Rename RuntimeError with the correct Runtime.

[kbrock]

update:

• rebased

[kbrock]

update:

• rebase
• split 1 commit into 2 (moving @variable from choice_rule to data in 2 steps now vs 1)

[kbrock]

update:

• fix complexity of State#run_nonblock by introducing mark_exception

(that hopefully will be the last code climate issue with this code)
update: that was odd, removed half the method but got the same complexity.

update:

• removed comment referencing @variable (I was pinging someone)

[kbrock]

update:

• rebase
• renamed UNARY_RULE to TYPE_CHECK
• renamed BINARY_RULE to OPERATION
• added some comments around @type and @path variables

just superficial changes. Code hasn't changed

[kbrock]

Before:

• fetches a value, and verifies what ever came out is not null

After:

• fetches a value and verifies it existed in the input hash

[kbrock]

Before: "IsString" => "*" verified it is a string.
After: "IsString" => true works that way, but "IsString" => false does the opposite
(across all checks)

[kbrock]

(moved over from ChoiceRule since this is data specific)
Before:

• convert @variable to a Path at runtime and fetch value.
  After:
• @variable is a Path`, so bad Path detected at initialization time
• Path not present in input is a runtime error
• Data type of input is checked.

[kbrock]

Yea, this got more complicated.

After:

• Can detect bad compare values at initialization time.
• Can detect bad paths at initialization time.
• Store type checking information (only for operations. bad type in the type check are informational (true false) rather than errors.

[kbrock]

This one change has big consequences.

Before: A path pointing to nothing would just treat the value as nil
After: A path pointing to nothing is a runtime error

[kbrock]

This was detected because tests running against a state would work, but running against a workflow would not.

Before:

• Errors needed to be caught by each State
• Errors resulted in a state that was not complete.
• Errors did not always get their way into output.

After:

• runtime errors always set the context accordingly.

[kbrock]

The code for finish hasn't changed, it just now resides in mark_finished.

Reasoning: If State throws an error, it almost always threw it in finish. When an error is thrown, super.finish is not called. This means no logging and FinishedTime is never set -- so the state looks like it is not finished.

Now, we catch errors thrown, and ensure super.finish is called. Easiest way to do this is to move that code into a separate method -- mark_finish.

[kbrock]

WIP: piece by piece pulling out PRs from this

[kbrock]

update:

• fix guard clause - ensured bad operators found in initializer not runtime

update:

• fix } spacing cop

[miq-bot]

Checked commit kbrock@f5db130 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.62.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🍪

[kbrock]

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]

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}?" }

[kbrock]

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

[Fryguy]

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

[kbrock]

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.

[Fryguy]

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

[Fryguy]

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

[kbrock]

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]

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("|")})$/

[kbrock]

@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]

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

[kbrock]

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?

[kbrock]

I have a separate PR all set for this

[Fryguy]

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]

@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]

update

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