Default case in switch

[matthias-pichler]

What would you like to be added:

Currently the DSL states for the switch case that: when

A runtime expression used to determine whether or not the case matches.
If not set, the case will be matched by default if no other case match.
Note that there can be only one default case, all others MUST set a condition.

This wording implies that the default case does not follow ordering because it is only matched by default if no other case match

therefore these two would be equivalent:

document:
  dsl: 1.0.0-alpha1
  namespace: test
  name: sample-workflow
  version: 0.1.0
do:
  - processOrder:
      switch:
        - case1:
            when: .orderType == "electronic"
            then: processElectronicOrder
        - case2:
            when: .orderType == "physical"
            then: processPhysicalOrder
        - default:
            then: handleUnknownOrderType

and

document:
  dsl: 1.0.0-alpha1
  namespace: test
  name: sample-workflow
  version: 0.1.0
do:
  - processOrder:
      switch:
        - case1:
            when: .orderType == "electronic"
            then: processElectronicOrder
        - default:
            then: handleUnknownOrderType
        - case2:
            when: .orderType == "physical"
            then: processPhysicalOrder

I think this is a bit ambiguous and would change the wording to something like:

If when is not specified it always matches.

This would then mean that the only logical place to put a default case would be at the end. Interested in what you folks think.

Note that there can be only one default case, all others MUST set a condition.

This is currently not validated by the schema. I think it could be It is doable with contains. This schema ensures only one case has no when

type: object
required: [ switch ]
unevaluatedProperties: false
properties:
  switch:
    type: array
    minItems: 1
    contains:
      type: object
      minProperties: 1
      maxProperties: 1
      additionalProperties:
        type: object
        properties:
          when: false
    minContains: 0
    maxContains: 1
    items:
      type: object
      minProperties: 1
      maxProperties: 1
      title: SwitchItem
      additionalProperties:
        type: object
        title: SwitchCase
        required: [ then ]
        properties:
          when:
            type: string
            description: A runtime expression used to determine whether or not the case matches.
          then:
            $ref: '#/$defs/flowDirective'
            description: The flow directive to execute when the case matches.

The DSL does not specify what happens when no case matches. Does it fall through to the next task? (I don't think this should be the intended behavior) Or which error is raised?

And my last question: would it make sense to enforce that the default case is named default?

Why is this needed:

[cdavernas]

Does it fall through to the next task?

That is my understanding, yes: it should fall back to the then defined by the switch task, which actually defaults to next.

IMHO no error should be implicitly thrown if no case matches, as this can be explicitly done by author if - and only if - need be, just like in all languages I know of.

And my last question: would it make sense to enforce that the default case is named default?

IMHO, no: it was like that in previous versions, but it was more restrictive than anything afaik. Think of a switch with two cases: one that defines a condition, and is called "yes", for example, and a second that is the default's. Forcing the latter to be called "default" is counter intuitive, as you would rather call it "no". Don't you agree? This is the option that provides imo the biggest flexibility and freedom to authors.

Related to #627

[matthias-pichler]

That is my understanding, yes: it should fall back to the then defined by the switch task, which actually defaults to next.

IMHO no error should be implicitly thrown if no case matches, as this can be explicitly done by author if - and only if - need be, just like in all languages I know of.

Yeah the implicit flow control of the spec is quite different from a lot of other workflow engines centered around state machines, where no outgoing connection usually means an error. Your answer makes sense here 👍

Do we fear that this might be a cause for common bugs? As any programmer knows: switch fallthrough is neat but somehow always comes back to bite you 😅 (many linters flag switch fallthrough or missing default cases)

IMHO, no: it was like that in previous versions, but it was more restrictive than anything afaik. Think of a switch with two cases: one that defines a condition, and is called "yes", for example, and a second that is the default's. Forcing the latter to be called "default" is counter intuitive, as you would rather call it "no". Don't you agree? This is the option that provides imo the biggest flexibility and freedom to authors.

That makes sense, my thinking behind it was that maybe it would help to standardise workflow definitions a bit and thus help authors & readers to quickly identify the default case. But I have no strong opinions here and think your point is very convincing

[JBBianchi]

I have a slightly different take on the matter.

In my opinion, we should enforce when to be required and leave the default case as being the default behavior of any task, its then flow directive.

... implies that the default case does not follow ordering ...

It doesn't indeed because it's not really a case. It's been introduced to mimic what we have while programming (and it doesn't really make sense. I'll come to it later*)

I think this is a bit ambiguous and would change the wording to something like:

If when is not specified it always matches.

This would then mean that the only logical place to put a default case would be at the end. Interested in what you folks think.

I understand the logic, and it's quite valid, but IMO, it introduces confusion. The term always implies that even if another case is matched, then the default will be too (it won't, I know, I'll come to it later*).

The DSL does not specify what happens when no case matches. Does it fall through to the next task? (I don't think this should be the intended behavior) Or which error is raised?

That is my understanding, yes: it should fall back to the then defined by the switch task, which actually defaults to next.

IMHO no error should be implicitly thrown if no case matches, as this can be explicitly done by author if - and only if - need be, just like in all languages I know of.

I agree. The switch task is just another task, it still complies with the basic definition of a task.

Do we fear that this might be a cause for common bugs? As any programmer knows: switch fallthrough is neat but somehow always comes back to bite you 😅 (many linters flag switch fallthrough or missing default cases)

* I think our switch is slightly different than a regular switch in programming. I'm not sure it's clear enough in the specification but only one match is possible (edit: to be more precises, only the first matching case will be executed, even if more than one match). Therefore, technically, it's impossible to "fall-through".

Given the task:

switchTask:
  switch:
    - myFirstCase:
        when: . == "foo"
        then: something
    - mySecondCase:
        when: . == "bar"
        then: somethingElse
#    - myDefaultCase:
#        then: defaultBehavior

The pseudo-code representation of switch task would be:

function switchTask(input) {
  switch (input) {
    case "foo": // myFirstCase
      return goTo('something');
    case "bar": // mySecondCase
      return goTo('somethingElse');
//    default: // myDefaultCase
//      return goTo('defaultBehavior');
  }
  return gotoNext(); // <-- It's the de facto default, as per specification. If "myDefaultCase" was enabled, it would never be reached but still be "defined". 
}

Each case returns, there is not possible fall-through.

I assume by fall-through you mean the use-case where none of the case matched (e.g. input = 42). Then, I think it's another argument for removing the optional when. The default is the default, not a "possible" case. By introducing the "default case with the empty when", we hide away the default behavior of the task.

IMHO, no: it was like that in previous versions, but it was more restrictive than anything afaik. Think of a switch with two cases: one that defines a condition, and is called "yes", for example, and a second that is the default's. Forcing the latter to be called "default" is counter intuitive, as you would rather call it "no". Don't you agree? This is the option that provides imo the biggest flexibility and freedom to authors.

That's the only argument to keep a default case, and I don't think it's a very strong one. It is like tasks we opinionatedly decided should be named. The "feature" resides in when, not in the name of the case. If we push it a bit too far, it would be possible to define switch cases such as:

switchTask:
  switch:
    - . == "foo":
        label: myFirstCase
        then: something
    - . == "bar":
        then: somethingElse

It's very ugly, I think we'll all agree. My point is it still works, because the mandatory pieces of information are the condition and the transition. The name is just some metadata to improve the aesthetic and readability of the workflow.

Keeping the default case "just" so we can name it feels like a weak argument. I would suggest to rather introduce a new property at the switch task level, like labeled, default or defaultName, e.g.:

askConsent:
  switch:
    - yes:
        when: . == true
        then: continue
    labeled: no 
    then: cancel

or the other way around:

askConsent:
  switch:
    - no:
        when: . == true
        then: cancel
    labeled: yes

So sum up:
Pros

• Keeps switch task consistent with the other tasks
• Doesn't add complexity to the JSON Schema
• Prevents confusions between cases, default, and task transition

Cons

• Loses the possibility of naming the default case (can be mitigated)

[cdavernas]

The "feature" resides in when, not in the name of the case

I disagree. Naming cases is critical for (non author) user feedback and for cosmetic purpose: you do not want to force a user to understand the expression of the when to understand what the case does.

Each case returns, there is not possible fall-through.

In you sample, not commenting the default case would result in the exact same behavior than the one you want to avoid, which is perfectly valid in all languages I know of. At worst, the linter would raise a warning for the last return saying that it won't ever be reached.

It's very ugly, I think we'll all agree.

It's not just ugly, it's invalid: you cannot use such characters in a propert name.

Keeps switch task consistent with the other tasks

It is consistent imo. I dont see why having the possibility to add a possible override for aestetic reason would make it otherwise.

Doesn't add complexity to the JSON Schema

The current solution does not add any complexity to the schema imho. It's just one single property, which could btw be removed and be instead considered a DSL validation responsibility.

Prevents confusions between cases, default, and task transition

I personally dont see the potential confusion here. Either you want the ability to name the default behavior and you use the whenless case, either you dont and use the task's then.

Loses the possibility of naming the default case (can be mitigated)

I disagree. It cannot be mitigated in any non-trick looking fashion, as you demonstrated, whereas the current behavior is clean and intuitive imho.

---

Bottom line is that, as said in previous comment, doing what you suggest would be a regression, and would be confusing/less readable to users with a non technical background.

On a side note, I have a strong - yet personal, of course - aversion for the mitigation solutions you proposed, which I think add complexity and make the overall switch heavier and uglier. I dont see how a prop such as "labeled" or whatever would be intuitively associated to the task's then

[cdavernas]

@ricardozanini @fjtirado @matthias-pichler-warrify What do you guys think? I think we need your input to determine whether or not we should proceed with potential changes.

A good tiebreaker, if need be, would be to present alternatives to non-technical people and gather their feedback.

[fjtirado]

I like @JBBianchi proposal (remove "default")
If there is not match in the switch clauses, go with "regular" then. If there is not "then", then (no pump intended ;)) go to the next task (as per sequence)
As per the label controversy, I do not think is needed. The "regular" then already has a name and is used when there is not match in the condition, so no label is really needed.

[fjtirado]

Let me describe the case I have in mind

switch: 
  - increaseSalary:
     when: employee.isMyFriend
     then: doubleSalary
  - fireEmployee:
     when: employee.writtings contains "retarded boss"
     then: fireHim
 then: sendChristmasGreetingToAllEmployees

There is not default there, I only want to increase salary to my friends and fire the ones that has insulted me. And congratulate christmas to everyone

[cdavernas]

@fjtirado I'm not sure I understand your sample: a matching case will transition, therefore won't ever fire the then, which will only be matched if nor increaseSalary or fireEmployee match.

In other words, you'll only congratulate everyone if you don't promote or fire someone.

There is no way to actually transition to a case's then, then after the whole chain is resolved, execute the tasks's then.

As for the naming, the then contains the name of the task to then perform, and is in no way related to the name of the case. Imagine a switch with yes/no cases, yes could transition to credit, no could transition to debit.

[fjtirado]

hmmm. It would be nice if there is way to tell fireHim and doubleSalary to "return" to the "branching" task so sendChristmasGreentToAllEmployees is executed always.

[fjtirado]

@cdavernas yes, I just realized.
So, in this example

document:
  dsl: '1.0.0-alpha1'
  namespace: test
  name: raise-example
  version: '0.1.0'
do:
  - processTicket:
      switch:
        - highPriority:
            when: .ticket.priority == "high"
            then: escalateToManager
        - mediumPriority:
            when: .ticket.priority == "medium"
            then: assignToSpecialist
        - lowPriority:
            when: .ticket.priority == "low"
            then: resolveTicket
        - default:
            then: raiseUndefinedPriorityError
  - raiseUndefinedPriorityError:
      raise:
        error:
          type: https://fake.com/errors/tickets/undefined-priority
          status: 400
          instance: /raiseUndefinedPriorityError
          title: Undefined Priority
  - escalateToManager:
      call: http
      with:
        method: post
        endpoint: https://fake-ticketing-system.com/tickets/escalate
        body:
          ticketId: ${ .ticket.id }
  - assignToSpecialist:
      call: http
      with:
        method: post
        endpoint: https://fake-ticketing-system.com/tickets/assign
        body:
          ticketId: ${ .ticket.id }
  - resolveTicket:
      call: http
      with:
        method: post
        endpoint: https://fake-ticketing-system.com/tickets/resolve
        body:
          ticketId: ${ .ticket.id }

What happens after issue is escalated?

[cdavernas]

It would be nice if there is way to tell fireHim and doubleSalary to "return" to the "branching" task so sendChristmasGreentToAllEmployees is executed always.

@fjtirado you can do that by configuring the tasks referenced by your cases to then transition to greet.

Doing it in the switch is imho dangerous and confusing: whereas it is a simple branching, your proposal would transform it on a kind of subflow.

[cdavernas]

What happens after issue is escalated?

Because you did not set the then of the escalateToManager task, it defaults to continue, which means it will execute assignToSpecialist, which will in turn execute resolveTicket for the same reason.

[fjtirado]

@cdavernas Ok, that was I thought, is that intended? It wont be safer to define the task that raise the error the latter?

[cdavernas]

@fjtirado this is up to you, and has imho nothing to do with the switch task, but is rather related to how you want to define/control your flow.

Setting the then of the escalateToManager task to end is enough to solve your sample.

In your example, because you did not specify flow control, all cases aside from resolveTicket will behave weirdly.

[fjtirado]

@cdavernas
I guess we have two schools of thoughts
The fully commited branchers, meaning that when there is a switch, you have to branch, so default is indeed required, but then the top level "then" is kind of useless because is never going to be executed.
The ones that see the switch as an if-else if kind of thing (the example I had on my mind)
In the second case, then might be, besides a goto, another task.

so, rewriting previos one

do: 
   - actOnEmployees
       switch: 
          - increaseSalary:
               when: employee.isMyFriend
               do: // increaseSalaryTask
          - fireEmployee:
               when: employee.writtings contains "retarded boss"
               do: // fireEmployeeTask
   - greetAllEmployes 

[fjtirado]

After digging in the DSL, the previous example can be written this way (with not switch)

do: 
     - increaseSalary:
             if: employee.isMyFriend
         // increaseSalaryTask
      - fireEmployee:
             if: employee.writtings contains "retarded boss"
           // fireEmployeeTask
     -  greetAllEmployes 

However note that, being picky, fireemployee should not be checked if salary has been increased. Maybe we need to work on that.

[cdavernas]

However note that, being picky, fireemployee should not be checked if salary has been increased. Maybe we need to work on that

Wouldn't setting the increaseSalary task's then to greetAllEmployees be enough to do so?

Afaik it would mean that if (and only if) said task is executed, meaning that the if is matched, then it transitions to greeting everyone.

[fjtirado]

Sure, it will do, but if later on we decide that we want greetEmployeesOnSummer, besides adding the new task before greetAllEmployees, we will have to remember to change the then on increaseSalary.

[fjtirado]

Anyway, I am deviating (as always) from the original issue, with the new knowledge I just acquired, I would say the discussion is between having a default or relying on the final then.
As far as both are optional, meaning that we have the option of not branching if no condition is met, I would say that relying on then is probably more consistent with the current schema (because, I might be wrong, if default is mandatory, then wont be used) and we save the order problem @matthias-pichler-warrify opened this issue for.