Skip to content

Add means to throw an error #771

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

Closed
cdavernas opened this issue May 30, 2023 · 24 comments · Fixed by #820
Closed

Add means to throw an error #771

cdavernas opened this issue May 30, 2023 · 24 comments · Fixed by #820
Assignees
Labels
area: spec Changes in the Specification change: feature New feature or request. Impacts in a minor version change
Milestone

Comments

@cdavernas
Copy link
Member

cdavernas commented May 30, 2023

What would you like to be added?

Add an action that allows throwing an error

Why is this needed?

Allows to define and throw workflow-specific errors, which is not possible as of now.

Coupled with the action's condition property, it could provide users a very powerfull - yet extremely simple - tool to perform flow and/or data validation logic.

What is your proposal?

Create a new throw or error action type, that allows users to do the following:

actions:
  - name: is-room-available
    functionRef:
      refName: check-if-room-is-available
      arguments:
        id: 123
    actionDataFilter:
      toStateData: .roomAvailable
  - name: throw-if-not-true
    condition: ${ .roomAvailable == false }
    errorRef:
      refName: room-not-available

If, as suggested in #770, we get rid of the top-level errors property, then it could instead look like:

actions:
  - name: is-room-available
    functionRef:
      refName: check-if-room-is-available
      arguments:
        id: 123
    actionDataFilter:
      toStateData: .roomAvailable
  - name: throw-if-not-true
    condition: ${ .roomAvailable == false }
    throw:
      type: https://fake.hotel.com/errors/room-not-available
      status: 409
      title: Room Not Available
@cdavernas cdavernas self-assigned this May 30, 2023
@cdavernas cdavernas added change: feature New feature or request. Impacts in a minor version change area: spec Changes in the Specification labels May 30, 2023
@ricardozanini ricardozanini added this to the v0.9 milestone May 30, 2023
@ricardozanini ricardozanini moved this from Backlog to Todo in Progress Tracker May 30, 2023
@ricardozanini
Copy link
Member

cc @fjtirado

@fjtirado
Copy link
Collaborator

@fjtirado
Copy link
Collaborator

fjtirado commented May 30, 2023

So basically we nee a $WORKFLOW.lastError or something like that so users can do stuff like the one requested in those zulip discussion

@cdavernas
Copy link
Member Author

@fjtirado So your suggestion is to include in the related PR a mechanism to "store" information about last thrown error?

What if, instead, we provide an array to store all errors that have previously occured, if need be? If you'd want to access the last, you could easily do something like '$WORKFLOW.errors | last'.

WDYT?

@fjtirado
Copy link
Collaborator

@cdavernas yes, they are basically asking how to know (from the state that is executed when the error happen) which was the error that causes the transition. This allow the same state handling different error types or extract info from the error itself

@fjtirado
Copy link
Collaborator

And yes, a stack of errors will do too

@cdavernas
Copy link
Member Author

Do I have the go ahead to open a PR, then @ricardozanini @fjtirado @tsurdilo ?

@fjtirado
Copy link
Collaborator

fjtirado commented May 30, 2023

Im going to play devils advocate, whats the diference between data condition on swith thats goes to an error branch and conditionally throw an error that goes to an error branch? ;)
Im not opposed to this, just a last time check so we wrote the rationale behind for posterity ;)

@cdavernas
Copy link
Member Author

cdavernas commented May 30, 2023

Im going to play devils advocate

Please do, the devil is in the details, as Im sure we all know! 😈

Hmmm, that's a good question. I would be tempted to say there's no difference, and that in some case one way might be more convenient than the other, not sure. Do you think it poses any problem?

@tsurdilo
Copy link
Contributor

tsurdilo commented May 30, 2023

I'm trying to understand this request. is this error thrown by workflow, so does it mean that workflow execution fails on
input validation? Does it mean its a custom error that then user can also handle with exception handling?

I worry that with this we are coupling business logic of specific action to workflow definition.

If the use case for this is input validation (so dont invoke service unless), can this be done on input schema level, a flag that user can set to fail execution on data schema validation? And if this is indeed the use case this users would ask for a "skip" not always "throw" ..i think this could be done on data schema validation as well

@cdavernas
Copy link
Member Author

cdavernas commented May 30, 2023

I'm trying to understand this request. is this error thrown by workflow, so does it mean that workflow execution fails on
input validation?

No, not on input. That's the point. You throw based on a data evaluation, which is not predictable AOT, as it could very well be the output of a remote function

If the use case for this is input validation (so dont invoke service unless),

Nothing to do with validation whatsever. We are here speaking of a feature that allows users to throw an error based on runtime conditions somewhere in their flow. How/when/where they want to throw it is IMHO not our concern. We should only provide the mechanism to do so, like explained in above example

@tsurdilo
Copy link
Contributor

I see, so its basically defining custom in-workflow-definition functions that contain some business logic.
I wonder then if this could also be done on the event filter level?

@cdavernas
Copy link
Member Author

I wonder then if this could also be done on the event filter level?

It could, but that's not the point here. It's not really a filter, and is not necessarily related to events.

Just see it as a way for user to choose to fault their workflow, for some reason. When combined with retries, this can be an absolutly fantastic feature, painless to ipmplement, and which has been requested a couple of times in the past.

@tsurdilo
Copy link
Contributor

tsurdilo commented May 30, 2023

feature that allows users to throw an error based on runtime conditions somewhere in their flow

trying to understand ^^, its not somewhere its part of (from example) action chain
so what im trying to say is i think "why have to add an extra action (or a set of) just to validate output of an action"? I think this is great idea but implementation-wise think could be added to maybe action definition itself (so no additional actions have to be added) or similar, maybe data filtering

@tsurdilo
Copy link
Contributor

to add, what im thinking is more like:

      actions:
        - name: is-room-available
          functionRef:
            refName: check-if-room-is-available
            arguments:
              id: 123
          actionDataFilter:
            toStateData: .roomAvailable
          <TIE TO THE EXCEPTION YOU HAVE HERE RATHER THAN SEPARATE ACTION>

@tsurdilo
Copy link
Contributor

you could then also make these if/else checks with throw reusable components across multiple actions maybe

@cdavernas
Copy link
Member Author

cdavernas commented May 30, 2023

or similar, maybe data filtering

Data filtering would (at best) fail silently, which is the opposite of the intention here.
Actually, in most cases with a data filter, the error would not occur, you would possibly have a switch state that takes some action, and that's perfectly fine.

Again, really, the point is to provide an explicit, declarative way to, basically, fault the workflow. As said before, that would also enable bubbling up the error for handlers to catch and possibly retry.

you could then also make these if/else checks with throw reusable components across multiple actions maybe

Not sure I understand what you mean by that

@tsurdilo
Copy link
Contributor

tsurdilo commented May 30, 2023

Not sure I understand what you mean by that

make these conditions reusable like we do for errors so

 throw-if-not-true
condition: ${ .roomAvailable == false }
errorRef:
  refName: room-not-available

could be a reusable definition that you could reference in an action definitions rather than having to put each individual as action as in example
think this would give same benefits as the other reusable defs we have as in you could define different criteria during testing etc w/o having to change the wf definition

@cdavernas
Copy link
Member Author

could be a reusable definition that you could reference in an action definitions rather than having to put each individual as action as in example

Yeah, sure, reusability is indeed a big stress here, and we could outfactor the concept in a reusable component, but errors produced that way will be mostly flow/branch specific. Anyways, that's IMO another concern that I'm very happy with., but it still does not address the ability to explicitly and declaratively throw an error

@tsurdilo
Copy link
Contributor

tsurdilo commented May 30, 2023

or something like:

     actions:
      - name: is-room-available
        functionRef:
          refName: check-if-room-is-available
          arguments:
            id: 123
        actionDataFilter:
          toStateData: .roomAvailable
        outputConditions:
             - type (maybe???) default to throw?
               condition: "..."
                errorRef: "..."

@cdavernas
Copy link
Member Author

cdavernas commented May 30, 2023

That's just my opinion, but even though that could obviously do the trick, the last sample is a bit more verbose and a bit more confusing that what's proposed... You know me and ubiquity, right 😆 !?

Instead of using yet another keyword that we would have to explain carefully (because not ubiquitous enough), I'd personally prefer that error action approach. Actions are IMO a bit bloated now, like kind of god-concept with a bit of everything: filtering, error handling, retries, ...

Again, just a personal opinion, but I'd rather go with my proposal, though modified to satisfy your perfectly rational concern regarding reusability.

Something like the first of my two examples:

actions:
  - name: is-room-available
    functionRef:
      refName: check-if-room-is-available
      arguments:
        id: 123
    actionDataFilter:
      toStateData: .roomAvailable
  - name: throw-if-not-true
    condition: ${ .roomAvailable == false }
    errorRef:
      refName: room-not-available

@tsurdilo
Copy link
Contributor

Got it, ok will keep reading. One think that comes to mind in your example is what if you don't control the check if room available action (its impl), how do you know if it changes its impl to throw error rather than returning false if room not avail?

What if i need to do checks before and after action invocation? I'm trying to understand if putting these as "actions" might confuse users or not too. Let's discuss in meet too if thats ok.

@cdavernas
Copy link
Member Author

Very good question. I do have dome ideas regarding that, as I'm sure you do, but yeah, with pleasure, let's go over that in meet 😉

@ricardozanini ricardozanini moved this from Todo to In Progress in Progress Tracker Feb 15, 2024
@cdavernas cdavernas mentioned this issue Feb 15, 2024
9 tasks
@cdavernas
Copy link
Member Author

cdavernas commented May 17, 2024

Closed as resolved by 1.0.0-alpha1, and therefore as part of #843

@github-project-automation github-project-automation bot moved this from In Progress to Done in Progress Tracker May 17, 2024
This was referenced May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: spec Changes in the Specification change: feature New feature or request. Impacts in a minor version change
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants