bpmn-io / bpmnlint

Validate BPMN diagrams based on configurable lint rules.
MIT License
125 stars 37 forks source link

Add `link-event` rule #128

Closed nikku closed 8 months ago

nikku commented 8 months ago

Enforces good practices for using link events, as laid out in the BPMN specification:

It is part of the spec under Link Event Definition in 10.5.5 Event Definitions

Cf. https://github.com/bpmn-io/bpmnlint/pull/128/commits/0b96cda8a0f167cd1f9ecc17da4fc41f60e2f0b8#diff-53281e111f51ee34e0d60298dd6d4d1c814830877ee9c83edc81b973ec8e1d8bR25 for proposed error messages.

Detected Correct Usage

image

Detected Incorrect Usage

image

philippfromme commented 8 months ago

The existing rule in bpmnlint-plugin-camunda-compat provides additional data that we use to select the corresponding input in the properties panel. If we remove this rule, selecting won't work anymore. That was the main blocker which kept me from just moving the rule to bpmnlint. In bpmnlint we generally don't provide this additional data.

nikku commented 8 months ago

The existing rule in bpmnlint-plugin-camunda-compat provides additional data that we use to select the corresponding input in the properties panel.

The question is: Do we direly need this? What is the loss if we don't focus the name property here? One option is to remove that "additional selection behavior".

The other option could be: Inject these additional properties after the fact, i.e. as part of a linter post-validation hook that bpmnlint-plugin-camunda-compat hooks into.

Either way such a rule belongs to the core. We can on the Camunda side of things decide if we use it or not.

nikku commented 8 months ago

We already attach meta data (property paths) in some places, i.e. here.

philippfromme commented 8 months ago

The question is: Do we direly need this? What is the loss if we don't focus the name property here? One option is to remove that "additional selection behavior".

Well, that would save us a lot of hassle but we've decided to focus the corresponding properties panel input when clicking on an error. However, we need to rethink the current implementation due to https://github.com/camunda/camunda-modeler/issues/3896 anyway. We will probably have to go with a solution that relies on path again. 🤔

philippfromme commented 8 months ago

How is linking across scopes validated? It's not explicitly validated, right? We simply validate that within a scope there's a pair of catch and throw events and if that's not the case it could be due to linking across scopes.

nikku commented 8 months ago

Well, that would save us a lot of hassle

We could simply do it and see when someone notices, especially as the resolution for links is not to "simply fix the name".

Misuse (cf. this PR) can stem from many things, including having the link target in a wrong scope. This is not fixed by correcting the name, but moving the element (or deleting it all together).

nikku commented 8 months ago

How is linking across scopes validated?

We are not here to "explain linking", but to identify errors (and layout the basic rules).

Documentation tries to pickup the user, i.e. clearly tell what wrong usage exists: https://github.com/bpmn-io/bpmnlint/pull/128/files#diff-1c0cd3d147700fbecbfc2f14e57a1438fd5f920c628975153a9b6e9ab29629b0.