Skip to content

Choice rule payload validation path#253

Merged
agrare merged 2 commits into
ManageIQ:masterfrom
kbrock:choice_rule_payload_validation-path
Aug 8, 2024
Merged

Choice rule payload validation path#253
agrare merged 2 commits into
ManageIQ:masterfrom
kbrock:choice_rule_payload_validation-path

Conversation

@kbrock
Copy link
Copy Markdown
Member

@kbrock kbrock commented Aug 1, 2024

extracted from #189

Before

If the path referenced a missing value in the input, treat it as a nil.

After

If the path references a missing value in the input, raise an error.

  • introduced presence_check method to check if it is present up front.
  • This also affects Intrinsic methods. You can see the changed spec.
  • This respects IsPresent: false and IsPresent: true

@kbrock kbrock added the enhancement New feature or request label Aug 1, 2024
@kbrock kbrock requested review from Fryguy and agrare as code owners August 1, 2024 17:26
context "referencing the global context" do
it "with a missing value" do
expect(described_class.new("$$.foo").value({})).to be_nil
expect { described_class.new("$$.foo").value({}, {"foo" => "bar"}) }.to raise_error(Floe::PathError, "Path [$$.foo] references an invalid value")
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is the essence of the issue

@kbrock kbrock mentioned this pull request Aug 1, 2024
11 tasks
@kbrock kbrock force-pushed the choice_rule_payload_validation-path branch from 580ca92 to dceeab9 Compare August 2, 2024 19:49
@kbrock
Copy link
Copy Markdown
Member Author

kbrock commented Aug 2, 2024

update:

  • removed spec updates/refactor (I'll put in another PR)

Before:

If the path referenced a missing value in the input, treat it as a nil.

After:

If the path references a missing value in the input, raise an error.
This makes it necessary to catch errors and ensure that the context
is marked appropriately.

Since a path lookup is so stringent, introduce the presence_check.
@kbrock kbrock force-pushed the choice_rule_payload_validation-path branch from dceeab9 to 22c08f9 Compare August 2, 2024 21:05
@miq-bot
Copy link
Copy Markdown
Member

miq-bot commented Aug 2, 2024

Checked commits kbrock/floe@29f923a~...22c08f9 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
10 files checked, 0 offenses detected
Everything looks fine. 👍

@kbrock
Copy link
Copy Markdown
Member Author

kbrock commented Aug 2, 2024

update:

  • removed changes to is_present? -- it is not useful now, will delete after the dust settles.

@kbrock kbrock changed the title Choice rule payload validation path [WIP] Choice rule payload validation path Aug 8, 2024
@kbrock kbrock added the wip label Aug 8, 2024
@kbrock
Copy link
Copy Markdown
Member Author

kbrock commented Aug 8, 2024

WIP: moving stuff around. Want to get other stuff merged before this.
There is no outstanding work for this item.

@kbrock kbrock changed the title [WIP] Choice rule payload validation path Choice rule payload validation path Aug 8, 2024
@kbrock kbrock removed the wip label Aug 8, 2024
@kbrock
Copy link
Copy Markdown
Member Author

kbrock commented Aug 8, 2024

WIP: oops. thought this was something else.
This is good to go

Copy link
Copy Markdown
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@agrare agrare merged commit 6f47120 into ManageIQ:master Aug 8, 2024
@kbrock kbrock deleted the choice_rule_payload_validation-path branch August 8, 2024 15:46
agrare added a commit that referenced this pull request Aug 12, 2024
Added
- Choice rule payload validation path (#253)
- Intrinsics JsonToString and StringToJson (#256)
- Add States.Format intrinsic function (#258)
- Intrinsics States.JsonMerge (#255)
- Enable support for Hashes in States.Hash (#260)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants