bpmn-io / bpmnlint

Validate BPMN diagrams based on configurable lint rules.
MIT License
119 stars 36 forks source link

feat: add `no-overlapping-elements` rule #107

Closed david-d-le closed 1 year ago

david-d-le commented 1 year ago

Closes #72 Closes #71

david-d-le commented 1 year ago

issue #71 will be probably solved with this too.

Sequence flows are not considered here yet (will be a little bit more harder to solve)

david-d-le commented 1 year ago

Hi, I added the rules and README without images. However I am getting errors in build:

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:

I am not really sure what that means because I am not really experienced in node at all. Could you take a look at this so I can fix this?

nikku commented 1 year ago

With https://github.com/bpmn-io/bpmnlint/pull/107/commits/6702cfa6229c8d1efcfe6dc50c69565290e39848 I've added a bunch of additional test cases that I'd like us to cover.

Element outside of parent boundary

image

Collapsed sub-processes

image

david-d-le commented 1 year ago

With 6702cfa I've added a bunch of additional test cases that I'd like us to cover.

Element outside of parent boundary

image

Collapsed sub-processes

image

How these two cases should be solved? Element outside of parent boundary is good point that is not solved at the moment but second case Collapsed sub-processes is solved (lint errors are shown inside collapsed process)

nikku commented 1 year ago

@david-d-le Thanks for following up! I'd suggest to detect "Element outside of parent boundary" and flag it accordingly. It may otherwise lead to false impressions and, to my knowledge, also fits into the no-overlapping-elements. What do you think?

david-d-le commented 1 year ago

Yes I also think that "Element outside of parent boundary" should be here. I will add it into this rule with warning message "Element outside of parent boundary" and add a test case.

However I need help with Data Store Reference elements. They are little bit tricky because they can be outside Participant elements but they can belong to any of these processes.

nikku commented 1 year ago

However I need help with Data Store Reference elements. They are little bit tricky because they can be outside Participant elements but they can belong to any of these processes.

I'd code an exception for that case (data store belongs to participant, but lies outside of it) => it does not violate the rule.

david-d-le commented 1 year ago

However I need help with Data Store Reference elements. They are little bit tricky because they can be outside Participant elements but they can belong to any of these processes.

I'd code an exception for that case (data store belongs to participant, but lies outside of it) => it does not violate the rule.

I think I haven't explained it how I wanted to. I will probably need to explain it more deeply with images below.

both belong to participant 1 image image

one belongs to part. 1 and second to 2 image image

Problem is that when one belongs to other Process/Collaboration and second to the other it is not inside of scope of one process and thus it is ignored in checking.

nikku commented 1 year ago

@david-d-le I think what you showed is an absolutely OK edge case we can ignore for now.

nikku commented 1 year ago

@david-d-le I'll have another look at your contribution next week. Is there anything you wanted to add?

david-d-le commented 1 year ago

@david-d-le I'll have another look at your contribution next week. Is there anything you wanted to add?

Yes i wanted to implement "Element outside of parent boundary". However i will be offline cca for two weeks and i am not sure i will complete it today.

david-d-le commented 1 year ago

@nikku I tried to implement ```Element outside of parent boundary````. Test cases must be updated and overall code must be checked. I will be offline so I will not be available for some time to check/fix/update the code.

nikku commented 1 year ago

@david-d-le Thanks for the heads-up. I'll check if I can complete the remainders.

nikku commented 1 year ago

Released as v9.1.0. :tada: