ManageIQ / floe

Floe is a runner for Amazon States Language workflows
Apache License 2.0
0 stars 5 forks source link

[WIP] Catch and display path errors #246

Open kbrock opened 1 month ago

kbrock commented 1 month ago

Depends upon

2.5 things:

Invalid path (validation)

    "Test1": {
      "Comment": "Parameter not a path",
      "Type":  "Pass",
      "InputPath": "nonparam",
      "End": true
    }

aws:

Field "InputPath" defined at "State Machine.Test1" is not a JSONPath

Before

Path [nonparam] must start with "$"

After

States.Test1 field "InputPath" value "nonparam" Path [nonparam] must start with "$"

Path not found (runtime)

    "Test2": {
      "Type":  "Pass",
      "InputPath": "$.missing"
      "End": true
    }

aws:

Invalid path '$.missing' : No results for path: $['missing']

Before

{}

After

{
  "Error":"States.RuntimeError",
  "Cause":"States.Test2 field \"InputPath\" value \"$.missing\" references an invalid value"
}
kbrock commented 1 month ago

update:

update:

Fryguy commented 1 month ago

This feels like adding a lot of "code" to just present error messages. Is it possible to just not have the wrap_runtime_error as that feels like the method that is adding a lot of this overhead?

I am curious why the PathError itself can't contain the details within, since it is being raised from within the Path itself. Additionally, is every instance of PathError just also an ExecutionError? If so, could it just raise an ExecutionError instead, or, if not, can PathError just be a subclass of ExecutionError?

kbrock commented 1 month ago

@Fryguy The problem is a generically thrown exception will not properly have the context for the error. So either the thing throwing the error needs more information, or we need to add information on the way up.

I'll add the state/path to the Path, so it can throw a better exception.

Fryguy commented 1 month ago

I'm not sure I follow... It looks like the same thing is passed into path that's wrapped outside of it as well

kbrock commented 1 month ago

WIP: I care more about Choice fixes than error display. incorporating these errors on top of #189

kbrock commented 1 month ago

update:

miq-bot commented 1 month ago

Checked commits https://github.com/kbrock/floe/compare/825efcbad46453c1ced676b883d48b1b2085cf6b~...8c0ee6ce6ee0624c99c94615d57d326a0537e136 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 20 files checked, 0 offenses detected Everything looks fine. :cake:

miq-bot commented 4 weeks ago

This pull request is not mergeable. Please rebase and repush.

kbrock commented 4 weeks ago

WIP: keeping up to date locally. waiting for choice to go through before putting too much effort into this. (they all conflict)

Also, my push more information into Path so it throws the fully qualified error instead of partial and having to manipulate the exception on the way up