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: adjusted DataAssociation model to match bpmn spec #91

Closed
wants to merge 1 commit into from

Conversation

Skaiir
Copy link

@Skaiir Skaiir commented Jan 19, 2022

Related to camunda/camunda-modeler#2333

The spec reasons for this are referenced by our contributor on the above issue.

@Skaiir Skaiir requested review from a team, smbea and barmac and removed request for a team January 19, 2022 11:30
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Jan 19, 2022
@Skaiir
Copy link
Author

Skaiir commented Jan 19, 2022

Looking around I didn't see specific testing of the name property, is this something that needs to be done here, or is it a bit redundant?

@nikku
Copy link
Member

nikku commented Jan 19, 2022

Any fix we shall apply via a transform from the original BPMN 2.0 meta-model.

Please add a test case, i.e. verify that a BPMN 2.0 spec compliant diagram with the name property on a data-association can be read and serialized.

Revert "fix: adjusted DataAssociation model to match spec"

This reverts commit e2f16be.

squash
@Skaiir Skaiir force-pushed the 2333-data-output-association-unknown branch from 4817954 to ffc1d6d Compare January 20, 2022 12:11
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.

Let's also add a test case to the write tests and the roundtrip tests so that we are sure the fix works fine. The roundtrip tests verify that a serialized diagram is a valid BPMN according to the XSD schema.

@Skaiir
Copy link
Author

Skaiir commented Feb 3, 2022

Following discussions with @barmac, we have discovered that this issue is actually a bug in the bpmn specs. The pdf spec allows for the name, but the xml spec does not. Our approach is hence going to change here, as we do not want to support something which is potentially going to break other vendor's tools. We will use the XML as the true spec and instead change the properties panel to not anymore expose the name property.

If my reasoning is appropriate, I am going to close this PR. @nikku @pinussilvestrus any objections?

@nikku
Copy link
Member

nikku commented Feb 3, 2022

@Skaiir Exactly. The XML schema has to be our reference here, no matter what the PDF says.

@Skaiir Skaiir closed this Feb 3, 2022
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Feb 3, 2022
@nikku nikku deleted the 2333-data-output-association-unknown branch June 18, 2022 19:06
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.

3 participants