camunda / camunda-bpm-platform

Flexible framework for workflow and decision automation with BPMN and DMN. Integration with Quarkus, Spring, Spring Boot, CDI.
https://camunda.com/
Apache License 2.0
4.11k stars 1.55k forks source link

Invalid update event trigger set for async message start event #3425

Closed danielkelemen closed 1 year ago

danielkelemen commented 1 year ago

Environment (Required on creation)

7.19 and higher.

Description (Required on creation; please attach any relevant screenshots, stacktraces, log files, etc. to the ticket)

With https://github.com/camunda/camunda-bpm-platform/issues/2663 we automatically set the update event trigger flag for events containing messages. With async message start events this causes an error in the migration plan. The update event trigger flag should not be set for message start events.

Steps to reproduce (Required on creation)

Observed Behavior (Required on creation)

Cockpit migration shows an error.

Screenshot 2023-06-14 at 08 43 53

Expected behavior (Required on creation)

Cockpit migration shows no error.

Root Cause (Required on prioritization)

We change the update event trigger flag for all activities with messageRef.

Solution Ideas

The update event trigger update logic needs to be adjusted so that it doesn't affect the message start events.

Hints

Links

Breakdown

Dev2QA handover

ThorbenLindhauer commented 1 year ago

@danielkelemen please add the additional problem with the plan when migration is executed (see support case for details)

danielkelemen commented 1 year ago

Additionally, on the 7.19.1-ee, even after manually adjusting the mappings for async after message start events, the migration fails at the last step due to the same error about update event trigger. While the /validate request showed no error during the first step. The data sent to migrateAsync still contains the updateEventTrigger set to true.

tasso94 commented 1 year ago

Problem

Solution ideas

  1. Frontend: Exclude process instance message start events from updateEventTrigger: true. Pros: relatively straightforward. Cons: other similar cases might remain uncovered (however, I cannot think of any).
  2. Backend: Use report validation for assigning updateEventTrigger: false. Pros: would solve all situations, also unknown ones. Cons:
    • Report only contains an error message and no static error type or code, and the behavior might break when changing it in the future.
    • Instead of evaluating the error message, we could also add logic that optimistically tries to get rid of the validation error by changing the flag to false; getting this logic right and robust is quite some effort.
  3. Backend: Don't generate instructions for async process instance start events. Pros: relatively simple Cons: this would break existing behavior; i.e., transition instance migration wouldn't be possible anymore.
  4. Backend: Generate report containing updateEventTrigger:true/false Pros:

    • Only one single source of truth
    • Java and REST API users benefit from the feature as well

    Cons:

    • Higher effort (an abstract ruleset of when to define updateEventTrigger:true/false needs to be created)
    • Needs a new config flag to maintain backwards compatibility

Decision

Opting for solution # 1 since it is the simplest, given that async process instance message start event is the only case in which we want to disable the flag. If this assumption falsifies in the future, we can still build a more robust solution like # 4.

danielkelemen commented 1 year ago

Async Message Intermediate Throw Event and Message End Event can also cause this migration problem.

tasso94 commented 1 year ago

@danielkelemen, I fixed the additional scenarios. Please review.

tasso94 commented 1 year ago

Latest change does the following:

This covers the following cases:

Comprehensive test process: event-trigger-process.bpmn.zip

tasso94 commented 1 year ago

Handover DEV ↔️ QA

@gbetances089, I created a comprehensive BPMN Model to test the fixed behavior and to ensure no regressions were introduced for the existing behavior. But please feel free to play around and adjust it.

Comprehensive test process: event-trigger-process.bpmn.zip

tasso94 commented 1 year ago

I forgot some braces and broke the logic:

!bo.$type === 'bpmn:EndEvent'
bo.$type -> true
!bo.$type -> false
false === ‘bpmn:EndEvent’
false === true -> false
gbetances089 commented 1 year ago

Tested on Snapshot camunda-bpm-run-ee-7.20.0-20230614.175338-62 using the model provided and a few more to see it more in details.

Image Image