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

Support catch-all Error And Escalation Events #190

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

sombrek
Copy link
Contributor

@sombrek sombrek commented Aug 30, 2024

Proposed Changes

Closes #184

Error and escalation throw events with reference objects (e.g. errorRef) now match catch events without reference object (catch-all events).

When multiple events in a scope match, they are checked in the following order: event sub process with same reference, event sub process catch-all, boundary event with same reference, boundary catch-all.

Demo

visual-demo

@nikku nikku self-requested a review September 12, 2024 21:03
@barmac barmac self-requested a review September 25, 2024 07:30
@barmac
Copy link
Member

barmac commented Sep 25, 2024

I am looking into this right now. Thanks for the PR and sorry for a delay in review.

Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

Great contribution. Let's merge it as is. Thank you!

@barmac barmac merged commit b346a24 into bpmn-io:main Sep 25, 2024
5 checks passed
@barmac
Copy link
Member

barmac commented Sep 25, 2024

Published in v0.35.0

matchingSubscriptions, subscription => refsMatch(event, subscription.event)
);

if (matchingSubscriptions.every(subscription => subscription.event.boundary)
Copy link
Member

Choose a reason for hiding this comment

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

I have a hard time to understand what this does. Do you remember @sombrek? Why is the boundary event check necessary, which scenarios do we cover here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikku Yes, I remember.

There are now two types of handlers: Catch-all handlers and specific handlers that have an error/escalation reference.

Scenario: When an error/escalation occurs, you have to find the nearest matching event handler. Handlers are checked in this order: specific event sub process, catch-all event sub process, specific boundary event, catch-all boundary event. If there's no match, repeat on parent scope (at least for escalation).

const referenceSubscriptions = filterSet(
matchingSubscriptions, subscription => refsMatch(event, subscription.event)
);
if (matchingSubscriptions.every(subscription => subscription.event.boundary)
&& referenceSubscriptions.some(subscription => subscription.event.boundary)
|| referenceSubscriptions.some(subscription => !subscription.event.boundary)) {
matchingSubscriptions = referenceSubscriptions;
}

The check determines whether there's a specific handler: Starting with the second part. If there's a specific event sub process, use that. Otherwise, if there are only boundary events and at least one of them has a specific reference, use that. If that check fails, there are only catch-all handlers.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the details. I think I'll need to further debug this "in action" to see how it works, and leave a comment for future reference.

@sombrek sombrek deleted the support-catch-all-events branch October 8, 2024 17:33
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.

Implement catch-all For Error And Escalation Events
3 participants