specification icon indicating copy to clipboard operation
specification copied to clipboard

Add means to throw an error

Open cdavernas opened this issue 2 years ago • 23 comments
trafficstars

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 avatar May 30 '23 12:05 cdavernas

cc @fjtirado

ricardozanini avatar May 30 '23 13:05 ricardozanini

Including an error object has been requested by community see https://kie.zulipchat.com/#narrow/stream/232676-kogito/topic/Exception.20Handling/near/360818735 and https://kie.zulipchat.com/#narrow/stream/232676-kogito/topic/Serverless.20Workflow.20error.20events

fjtirado avatar May 30 '23 14:05 fjtirado

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

fjtirado avatar May 30 '23 14:05 fjtirado

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

cdavernas avatar May 30 '23 14:05 cdavernas

@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 avatar May 30 '23 14:05 fjtirado

And yes, a stack of errors will do too

fjtirado avatar May 30 '23 14:05 fjtirado

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

cdavernas avatar May 30 '23 15:05 cdavernas

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 ;)

fjtirado avatar May 30 '23 16:05 fjtirado

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?

cdavernas avatar May 30 '23 16:05 cdavernas

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

tsurdilo avatar May 30 '23 16:05 tsurdilo

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

cdavernas avatar May 30 '23 16:05 cdavernas

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?

tsurdilo avatar May 30 '23 16:05 tsurdilo

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.

cdavernas avatar May 30 '23 16:05 cdavernas

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 avatar May 30 '23 16:05 tsurdilo

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 avatar May 30 '23 16:05 tsurdilo

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

tsurdilo avatar May 30 '23 16:05 tsurdilo

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

cdavernas avatar May 30 '23 16:05 cdavernas

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

tsurdilo avatar May 30 '23 16:05 tsurdilo

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

cdavernas avatar May 30 '23 16:05 cdavernas

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: "..."

tsurdilo avatar May 30 '23 16:05 tsurdilo

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

cdavernas avatar May 30 '23 17:05 cdavernas

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.

tsurdilo avatar May 30 '23 17:05 tsurdilo

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 😉

cdavernas avatar May 30 '23 17:05 cdavernas

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

cdavernas avatar May 17 '24 08:05 cdavernas