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.

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

fix(engine): Added null-check to ProcessMessageStartEventMessageNameValidator #7764

Merged
2 commits merged into from
Sep 8, 2021

Conversation

remcowesterhoud
Copy link
Contributor

@remcowesterhoud remcowesterhoud commented Sep 6, 2021

Description

Fix for NPE during validation of new deployment.

Related issues

closes #5510

Definition of Done

Not all items need to be done depending on the issue and the pull request.

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/0.25) 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

Copy link
Member

@saig0 saig0 left a comment

Choose a reason for hiding this comment

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

Looks good and works 👍

Please have a look at my suggestion 🍰

Comment on lines 645 to 667
@Test
public void shouldRejectDeploymentWhenNoMessageReferenced() {
// given
final BpmnModelInstance definition =
Bpmn.createExecutableProcess("processId")
.startEvent("startEvent")
.messageEventDefinition()
.id("messageEvent")
.done();

// when
final Record<DeploymentRecordValue> deployment =
ENGINE.deployment().withXmlResource("process.bpmn", definition).expectRejection().deploy();

// then
Assertions.assertThat(deployment)
.hasRejectionType(RejectionType.INVALID_ARGUMENT)
.hasRejectionReason(
"Expected to deploy new resources, but encountered the following errors:\n"
+ "'process.bpmn': - Element: messageEvent\n"
+ " - ERROR: Must reference a message\n");
}

Copy link
Member

Choose a reason for hiding this comment

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

❌ First of all, the test case is nicely written 👍 Since the class contains already many test cases, we should add the new test case somewhere else.

The unit test ProcessMessageStartEventMessageNameValidatorTest would be an option. The downside is that it just test the validator itself.

I suggest creating a new test class for message validation tests. It could look similar to TimerValidationTest (without the parameterized style) and follow the "new" JUnit 5 style.

In order to verify the expected message using ProcessValidationUtil. We need to add the ZeebeDesignTimeValidators to the validation result.

Copy link
Member

@saig0 saig0 left a comment

Choose a reason for hiding this comment

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

I like it 🥇

One more JUnit 5 test 🚀

@remcowesterhoud
Copy link
Contributor Author

bors -r

@ghost
Copy link

ghost commented Sep 8, 2021

Did you mean "r-"?

@remcowesterhoud
Copy link
Contributor Author

bors r+

ghost pushed a commit that referenced this pull request Sep 8, 2021
7764: fix(engine): Added null-check to ProcessMessageStartEventMessageNameValidator r=remcowesterhoud a=remcowesterhoud

## Description

Fix for NPE during validation of new deployment.

## Related issues

closes #5510 



Co-authored-by: Remco Westerhoud <remco@westerhoud.nl>
@ghost
Copy link

ghost commented Sep 8, 2021

Build failed:

@remcowesterhoud
Copy link
Contributor Author

bors retry

@ghost
Copy link

ghost commented Sep 8, 2021

Build succeeded:

@ghost ghost merged commit 6a7b6fb into develop Sep 8, 2021
@ghost ghost deleted the 5510-fix-npe-in-validation branch September 8, 2021 13:59
@saig0
Copy link
Member

saig0 commented Sep 9, 2021

@remcowesterhoud let's backport this fix to stable/1.0 and stable/1.1.

@remcowesterhoud
Copy link
Contributor Author

/backport

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2021

Backport failed for stable/1.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin stable/1.0
git worktree add -d .worktree/backport-7764-to-stable/1.0 origin/stable/1.0
cd .worktree/backport-7764-to-stable/1.0
git checkout -b backport-7764-to-stable/1.0
ancref=$(git merge-base 73de24c042080c6261acab1c7aeb4a0c189a7e84 2add92eae7c4dbe16dbc25ce9129772b9167b2a3)
git cherry-pick -x $ancref..2add92eae7c4dbe16dbc25ce9129772b9167b2a3

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2021

Successfully created backport PR #7791 for stable/1.1.

ghost pushed a commit that referenced this pull request Sep 9, 2021
7791: [Backport stable/1.1] fix(engine): Added null-check to ProcessMessageStartEventMessageNameValidator r=remcowesterhoud a=github-actions[bot]

# Description
Backport of #7764 to `stable/1.1`.

relates to #5510

Co-authored-by: Remco Westerhoud <remco@westerhoud.nl>
ghost pushed a commit that referenced this pull request Sep 9, 2021
7793: Backport 7764 to stable/1.0 r=saig0 a=remcowesterhoud

## Description

Backport of #7764 to stable/1.0.

ProcessValidationUtil class did not exist yet in the stable branch. I had changes so I needed to add this class to resolve the conflict.

## Related issues

relates to #5510

Co-authored-by: Remco Westerhoud <remco@westerhoud.nl>
ghost pushed a commit that referenced this pull request Sep 9, 2021
7791: [Backport stable/1.1] fix(engine): Added null-check to ProcessMessageStartEventMessageNameValidator r=remcowesterhoud a=github-actions[bot]

# Description
Backport of #7764 to `stable/1.1`.

relates to #5510

Co-authored-by: Remco Westerhoud <remco@westerhoud.nl>
@npepinpe npepinpe deleted the 5510-fix-npe-in-validation branch February 3, 2022 10:05
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NPE in ProcessMessageStartEventMessageNameValidator.validateMessageName
3 participants