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] Catch and display path errors #246

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

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jul 11, 2024

Depends upon

2.5 things:

  • If a user specifies a bad path in a workflow, display a meaningful message.
  • If a user specifies a path that fails a lookup in runtime, display a meaningful message.
  • (This is not working with templates - not worse, but not informative/better either)

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 kbrock added the bug Something isn't working label Jul 11, 2024
@kbrock kbrock mentioned this pull request Jul 11, 2024
11 tasks
@kbrock
Copy link
Member Author

kbrock commented Jul 11, 2024

update:

  • rubocop changes

update:

  • removed some changes to process_output (code climate changes)

@kbrock kbrock changed the title Path errors Catch and display path errors Jul 12, 2024
@Fryguy
Copy link
Member

Fryguy commented Jul 12, 2024

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
Copy link
Member Author

kbrock commented Jul 12, 2024

@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
Copy link
Member

Fryguy commented Jul 12, 2024

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 kbrock changed the title Catch and display path errors [WIP] Catch and display path errors Jul 14, 2024
@kbrock kbrock added the wip label Jul 14, 2024
@kbrock
Copy link
Member Author

kbrock commented Jul 14, 2024

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

lib/floe.rb Outdated
class ExecutionError < Error
attr_reader :floe_error

def initialize(message, floe_error = "States.RuntimeError")
Copy link
Member

Choose a reason for hiding this comment

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

As Adam asked elsewhere, is States.RuntimeError an actual error?

Copy link
Member

Choose a reason for hiding this comment

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

#189 (review) it isn't listed in the states-language spec but it is in the AWS StepFunctions docs and he fixed it to be States.Runtime to match the docs there

@kbrock
Copy link
Member Author

kbrock commented Jul 17, 2024

update:

  • rebased onto latest choice payload validation

@miq-bot
Copy link
Member

miq-bot commented Jul 17, 2024

Checked commits kbrock/floe@825efcb~...8c0ee6c 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. 🍰

@miq-bot
Copy link
Member

miq-bot commented Aug 8, 2024

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

@kbrock
Copy link
Member Author

kbrock commented Aug 9, 2024

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

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

miq-bot commented Nov 11, 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
Labels
bug Something isn't working stale unmergeable wip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants