search

camunda / camunda-bpm-platform

Flexible framework for workflow and decision automation with BPMN and DMN. Integration with Quarkus, Spring, Spring Boot, CDI.
https://camunda.com/
Apache License 2.0
4.13k stars 1.56k forks source link

DMN model API: exception when adding second inputData element #2529

Open ThorbenLindhauer opened 3 years ago

ThorbenLindhauer commented 3 years ago

This issue was imported from JIRA:

Field Value
JIRA Link CAM-12831
Reporter p5yV3BJ
What is this name? This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.
Has restricted visibility comments true

I'm trying to use the DMN model API to create a conversion from modelling platform logic to DMN. I use the DMN and XML model plugins of version 7.14. I found out that an exception occurs when adding a second inputData element to a modelInstance.

See https://github.com/oneton/camunda-inputDataReference for my basic reproduction.

See the (partial) stack trace below. The error message is confusing, since InputData is a subclass of DrgElement. When digging further, I noticed that the existing inputData element actually gets converted to the InputDataReference class in ModelUtil.getModelElementCollection. This call ends up in ModelUtil.getModelElement which skips creation of the element instance when the reference is set on the DOM element (this provides a workaround - see second unit test). The actual problem  occurs in ModelImpl.getTypeForName, because InputData and InputDataReference share the same XML element name. The Map used therefore contains the last defined element which is the InputDataReference. Looking at the DmnModelConstants, the same problem seems to exist for itemDefinition. 

org.camunda.bpm.model.xml.ModelException: New child is not a valid child element type: inputData; valid types are: <description, extensionElements, import, itemDefinition, drgElement, artifact, elementCollection, businessContextElement>
    at org.camunda.bpm.model.xml.impl.util.ModelUtil.getIndexOfElementType(ModelUtil.java:203)
    at org.camunda.bpm.model.xml.impl.instance.ModelElementInstanceImpl.findElementToInsertAfter(ModelElementInstanceImpl.java:330)
    at org.camunda.bpm.model.xml.impl.instance.ModelElementInstanceImpl.addChildElement(ModelElementInstanceImpl.java:288)
    at org.camunda.bpm.unittest.DmnCreateMultipleInputDataTestCase.shouldCreateMultipleInputData(DmnCreateMultipleInputDataTestCase.java:73)

I am willing to contribute a pull request, but I'm not sure how to address this since it seems to be quite a fundamental issue. If the problem (of duplicate XML names) is limited to reference/non-reference elements, using a secondary map for reference elements (based on the presence of the href attribute) could be a direction to investigate. 

Links:

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user @yanavasileva

Hi p5yV3BJ,

Thank you for reaching out to us with this. We will try to provide you feedback by the end of the week.

Best regards, Yana

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user p5yV3BJ

What is this name? This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Yana, thank you for looking into this. Please note that this message mostly intended to show that I am still following this issue. (Basically I want to make sure that the lack of feedback is not due to the fact that I didn't reply) Since there is a workaround, there is no high priority from my side. My offer to provide a pull request to resolve this still stands though.

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user @yanavasileva

p5yV3BJ thank you for your interest in contribution. Unfortunately, I was not able to investigate further the issue. I think I have all of the required information so far, thank you for providing the example repo. I hope it won't be a problem to update you with my findings at the beginning of next month.

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user @yanavasileva

Hi p5yV3BJ,

Please accept my apologies for the delay and the created inconvenience. I took me some time to remember that I have seen the issue before until I started debugging it. Old topic for reference: https://forum.camunda.org/t/query-dmn-model/5414

I can confirm that the root cause is the duplicated name of the elements. (DmnModelConstants) (dmn.xsd) And as you can see in the previous topic the issue can impact different parts of the DMN model (previous report was about querying "inputData"). That being said, I think the topic is quite big, affects different parts of the API and I might need to discuss solution ideas internally with the team. However, you can already have a look at the previous thread and explore the discussed. The final goal of the fix would be that model API should be able to handle the duplicated names of the elements ("inputData" and "itemDefinition"). We need to ensure that different test cases are covered - the reported in this ticket, the one from the old issue and come up with some further in case it's necessary. (Side note: It might seem a bit fundamental but this is only the second request that I have seen on the problem. So it might be uncommon issue to run into?)

Please let me know if you are still interested in working on this and I will update with further findings, including help/support during the implementation.

Best regards, Yana

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user p5yV3BJ

What is this name? This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Yana,

Now I'll apologise for replying late. You didn't cause any inconvenience since I'm still using the workaround I found. Thank you for looking into this.

Your analysis matches my thoughts about the issue. I do wonder whether this problem is also present in the BPMN or CMMN model API. Due to (unfortunately) limited time I've only had a quick look at the previous issue.

Regarding your side note. I can imagine this problem only affects people that interact with the model API for building or querying a DMN model. I reckon that most users won't use code to build or analyse a DMN model and therefore the issue isn't observed often.

I still like to offer my assistance in implementing the solution if you could provide a general direction for the solution you'd like to see. Unfortunately I won't be able to commit to a timeframe.

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user @yanavasileva

Brief update: I checked and the other model APIs do not have similar problems, the names of the elements there are unique (duplication has only from different namespaces - Camunda & OMG for example). I located the exact problematic code in DMN, I will confirm if there are further inconsistency and share more details in the upcoming week or two.

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user p5yV3BJ

What is this name? This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Thank you for the update. I guess just worrying about DMN might make things easier, although it doesn't make the problem any less fundamental. 

I wonder whether a solution might also come in handy when using the model with custom extension elements, where the allowed extension elements depend on the type of parent node (for instance: one set for inputData, another set for decisions). In such cases, it could be useful to subclass the ExtensionElements class with one that allows access to the custom children. However, that would also lead to duplicate names. (But I admit this definitely goes further than fixing the immediate problem)

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user @yanavasileva

Hello p5yV3BJ,

Finally, here is the idea for the fix:

Please let me know what do you think or in case of any questions. I will try to provide support along the way of the contribution.

Best regards, Yana

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user p5yV3BJ

What is this name? This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Dear Yana,

Thank you for providing your detailed input. Unfortunately I've been quite busy the past month so I haven't had an opportunity to look at it yet. Just want to let you know that I'm still willing to make this contribution, provided you're not in a hurry to get it in. ;)

 

Anton

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user @yanavasileva

Hi Anton,

Happy to hear that you didn't get discouraged over the time! All go so far from our side in terms of time, if anything change we will update the ticket. Add the ticket number to the pull request when you start it.

Best regards, Yana