Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix 4099 by opening event subscriptions after start event's output mappings were applied #9047

Merged
merged 4 commits into from
Apr 6, 2022
Merged

Fix 4099 by opening event subscriptions after start event's output mappings were applied #9047

merged 4 commits into from
Apr 6, 2022

Conversation

pihme
Copy link
Contributor

@pihme pihme commented Apr 4, 2022

Description

This moves the step when event subscriptions are opened from ProcessProcessor and SubprocessProcessor to StartEventProcessor. This way, subscriptions and the values used for them will only be evaluated after output mappings of the start event have been applied.

This fits better to user expectations and fixes #4099

Related issues

closes #4099

Definition of Done

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • New content is added to the release announcement
  • If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

@pihme
test(engine): add test for unwanted behavior
73d45c7
@pihme
fix(engine): Open subscriptions at end of start event
f09c61e 
author: Philipp Ossler

The subscriptions will now be opened after the output mappings of the start event have been applied. This is in line with user expectations and fixes issue #4099
@pihme pihme requested a review from korthout April 4, 2022 09:39
@pihme
Copy link
Contributor Author

pihme commented Apr 4, 2022

Should be merged into stable/8.0 after it has been created

korthout
korthout requested changes Apr 4, 2022
View reviewed changes
Copy link
Member

@korthout korthout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @pihme 👍

❌ The test case shows clearly that the bug has been resolved. I think the only thing that's missing is a test case that shows that the output mappings of a start event are applied before the subscriptions are opened. Since this is the behavior that changes for users, I'd like to have a test for it.

🔧 Next to that, I just have a few nitpick suggestions.

Lastly, please open a bug that output mappings of a none start event don't allow to modify how the variables are merged into the process. This is the bug we discovered when we played around with none start event.

engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/event/StartEventProcessor.java Outdated Show resolved Hide resolved
engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/event/StartEventProcessor.java Outdated Show resolved Hide resolved
@pihme
refactor(engine): incorporate review feedback
e1c3972
@pihme
test(engine): test event subscription that depends on output mapping
c2d2c27 
@korthout If you have an idea for a short commit message, please let me know

 These test covers the following scenario: A process has a message/none start event (A) with output
 mappings. This process also has an event subprocess with a message start event (B) which
 references a variable that is only created after the output mapping of (A) has been applied.
 The expectation is that the output mapping for A will be applied, and only after that the
 message subscription for B will be opened. At this point the message subscription can be
 opened. If it were opened earlier, it would produce a "variable not found" incident.
@pihme pihme requested a review from korthout April 6, 2022 07:32
@pihme
Copy link
Contributor Author

pihme commented Apr 6, 2022

Lastly, please open a bug that output mappings of a none start event don't allow to modify how the variables are merged into the process. This is the bug we discovered when we played around with none start event.

No longer necessary. The fix solved this. I added another test with none start event. Please double check.

korthout
korthout approved these changes Apr 6, 2022
View reviewed changes
Copy link
Member

@korthout korthout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes @pihme LGTM 👍

Note that the additional bug we discovered is not fixed by these changes. If you create a process instance with some variables and the none start event has output mappings, then variables that were passed in are still copied into the process instance. This shouldn't happen as the output mappings should allow to completely redefine how and which variables are merged into the process instance. Please open an issue for it. If you want we can also discuss it F2F.

engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/event/StartEventProcessor.java Show resolved Hide resolved
.../test/java/io/camunda/zeebe/engine/processing/message/MessageStartEventSubscriptionTest.java Show resolved Hide resolved
@pihme
Copy link
Contributor Author

pihme commented Apr 6, 2022

Bug created: #9061

@pihme
Copy link
Contributor Author

pihme commented Apr 6, 2022

bors merge

zeebe-bors-camunda bot added a commit that referenced this pull request Apr 6, 2022
@zeebe-bors-camunda @pihme
merge: #9047
bcb3324 
9047: Fix 4099 by opening event subscriptions after start event's output mappings were applied r=pihme a=pihme

## Description

This moves the step when event subscriptions are opened from `ProcessProcessor` and `SubprocessProcessor` to `StartEventProcessor`. This way, subscriptions and the values used for them will only be evaluated after output mappings of the start event have been applied.

This fits better to user expectations and fixes #4099 

## Related issues

closes #4099



Co-authored-by: pihme <pihme@users.noreply.github.com>
@zeebe-bors-camunda
Copy link
Contributor

Build failed:

@pihme
Copy link
Contributor Author

pihme commented Apr 6, 2022

bors merge

@zeebe-bors-camunda
Copy link
Contributor

Build succeeded:

@zeebe-bors-camunda zeebe-bors-camunda bot merged commit b069188 into main Apr 6, 2022
@zeebe-bors-camunda zeebe-bors-camunda bot deleted the 4099-fix3 branch April 6, 2022 13:56
@github-actions github-actions bot mentioned this pull request Apr 6, 2022
Merged
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

Successfully created backport PR #9064 for stable/1.3.

@github-actions github-actions bot mentioned this pull request Apr 6, 2022
Merged
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

Successfully created backport PR #9065 for stable/8.0.

zeebe-bors-camunda bot added a commit that referenced this pull request Apr 11, 2022
@zeebe-bors-camunda @pihme
merge: #9064
5ff9084 
9064: [Backport stable/1.3] Fix 4099 by opening event subscriptions after start event's output mappings were applied r=korthout a=github-actions[bot]

# Description
Backport of #9047 to `stable/1.3`.

relates to #4099 #4099

Co-authored-by: pihme <pihme@users.noreply.github.com>
zeebe-bors-camunda bot added a commit that referenced this pull request Apr 11, 2022
@zeebe-bors-camunda @pihme
merge: #9065
fc61f30 
9065: [Backport stable/8.0] Fix 4099 by opening event subscriptions after start event's output mappings were applied r=korthout a=github-actions[bot]

# Description
Backport of #9047 to `stable/8.0`.

relates to #4099 #4099

Co-authored-by: pihme <pihme@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@korthout korthout korthout approved these changes

Assignees
No one assigned
Labels
version:1.3.8
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A workflow with start event message will deploy but fail to start if it has an event sub process
3 participants
@pihme @korthout @npepinpe