openapi-apps icon indicating copy to clipboard operation
openapi-apps copied to clipboard

Bug in a workflow with 2 path to the same app

Open tristandostaler opened this issue 3 years ago • 5 comments

I have the following workflow with 2 path possible to the same node: Option 1: if $exec.fields contains computerName, do node A, then do node B Option 2: if $exec.fields does not contains computerName, do node B.

However, when testing with a case where the Option 1 should be followed, the node B is never executed because the condition for the Option 2 is not fulfilled (that's the reason given when looking at the output of the skipped node B).

image

tristandostaler avatar Jan 24 '22 20:01 tristandostaler

I have the following workflow with 2 path possible to the same node: Option 1: if $exec.fields contains computerName, do node A, then do node B Option 2: if $exec.fields does not contains computerName, do node B.

However, when testing with a case where the Option 1 should be followed, the node B is never executed because the condition for the Option 2 is not fulfilled (that's the reason given when looking at the output of the skipped node B).

image

Hola,

I think you're right! It should work like this. Right now, it works with AND rather than OR between them, meaning everything has to match going into the same node. The reason it hasn't been explored enough is probably because we've done what's seen below all along instead. The piece of code that handles this is also in the app itself, and can be seen here: https://github.com/frikky/Shuffle/blob/7a388f3c055baac37cc0a20ba583f4380f8f1561/backend/app_sdk/app_base.py#L1996

Do you think the best way to handle it may be to see if just one matches? It would just be a counter, checking if it's more than 0 at that point. It may also break someone elses workflow, but I totally do agree with this one.

image

frikky avatar Jan 25 '22 09:01 frikky

Hum ok the thinking is different than what I had in mind. But I think it translates to a branch should be an OR. So as soon as any branch goes to a node, the node executes. It would work in both examples we have in this thread. With your examples it works out of the box, and for my example I don't have to add an empty node between the router and the node B

tristandostaler avatar Jan 25 '22 18:01 tristandostaler

The way to handle "the node executes if all branches are true" would be through conditions in the output of the previous nodes.

tristandostaler avatar Jan 25 '22 18:01 tristandostaler

Btw, what I have in mind when I see this type of workflow is a "pipe and filter" architectural pattern (documentation bellow). In a pipe and filter pattern, there is no "if logic" on a pipe (a branch between 2 nodes) so here there is a small nuance. But for me, it's an "if" that stand before (or after) the pipe logically, and the pipe is simply a link between 2 objects. In your context, a pipe would be an object which link 2 nodes toghether and can have an array of conditions, but when executing the workflow, I would follow the pipe and filter pattern and evaluate the if just before or just after sending data to the following node.

https://www.dossier-andreas.net/software_architecture/pipe_and_filter.html https://docs.microsoft.com/en-us/azure/architecture/patterns/pipes-and-filters https://student.cs.uwaterloo.ca/~cs446/1171/Arch_Design_Activity/PipeFilter.pdf

tristandostaler avatar Jan 25 '22 21:01 tristandostaler

Hum ok the thinking is different than what I had in mind. But I think it translates to a branch should be an OR. So as soon as any branch goes to a node, the node executes. It would work in both examples we have in this thread. With your examples it works out of the box, and for my example I don't have to add an empty node between the router and the node B

Small clarification: it will ALWAYS wait for all previous nodes to have done something before executing, even when just one has to be correct. This is to ensure that we're taking all results into account, and it's not running the workflow in the wrong order

This should be working when we update our apps to the latest SDK which has a proper fix for it :)

frikky avatar Jan 26 '22 22:01 frikky