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

[JBPM-9774] Incorrect object type returned in a DMN business-rule task of a BPMN #1947

Merged
merged 1 commit into from
Jun 14, 2021

Conversation

elguardian
Copy link
Member

@elguardian elguardian changed the title [JBPM-9774] Incorrect object type returned in a DMN business-rule task of a BPMN [JBPM-9774] Incorrect object type returned in a DMN business-rule task of a BPMN (WIP) Jun 7, 2021
@jomarko jomarko self-requested a review June 7, 2021 11:00
@elguardian elguardian changed the title [JBPM-9774] Incorrect object type returned in a DMN business-rule task of a BPMN (WIP) [JBPM-9774] Incorrect object type returned in a DMN business-rule task of a BPMN Jun 8, 2021
@elguardian elguardian requested a review from gmunozfe June 8, 2021 10:37
Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

some questions, please?

@elguardian elguardian force-pushed the JBPM-9774 branch 2 times, most recently from cc71911 to c33a65c Compare June 9, 2021 07:51
Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

not resolved.
#1947 (comment)
please leave the editor usage standard.

Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

looks good to me, thank you @elguardian ! 🚀

@sonarcloud
Copy link

sonarcloud bot commented Jun 9, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

@elguardian thank you for the PR. It seems to be working fine. I found just a single issue related to years and moths duration/java.time.Period.

I was not able to successfully map years and months duration from DMN into java.time.Period in BPMN/Java.

See commented parts of my PR elguardian#6

Is that something I should report separately, or I just selected wrong BPMN/Java type for years and months duration?

@tarilabs
Copy link
Member

@jomarko
Copy link

jomarko commented Jun 11, 2021

@tarilabs thank you, that works perfect for object serializing!

See:

However the testBuildDMNPeriodProcess test, where single value is returned, returns org.kie.dmn.feel.lang.types.impl.ComparablePeriod, not a java.time.Period

What is your opinion about it. It is a separate issue, as it not anymore about returned object type?

@tarilabs
Copy link
Member

I don't understand the question @jomarko , sorry?

When a DMN model returns a ComparablePeriod as part of the decision results, this would allow the underlying Jackson to serialize it as a usual Period in the ISO format. I believe once deserialized by Jackson to the Java Pojo type of the process variable, the Pojo would have the Period as a type of the field.

@jomarko
Copy link

jomarko commented Jun 11, 2021

@tarilabs ok, let me try put it differently.

ComparablePeriod variable = (ComparablePeriod) processInstance.getVariable("period");

works, while

Period variable = (Period) processInstance.getVariable("period");

Throws an error:

java.lang.ClassCastException: class org.kie.dmn.feel.lang.types.impl.ComparablePeriod cannot be cast to class java.time.Period (org.kie.dmn.feel.lang.types.impl.ComparablePeriod is in unnamed module of loader 'app'; java.time.Period is in module java.base of loader 'bootstrap')

What is for me hard to decide if:

  • it is an issue
  • it is expected behavior

@tarilabs
Copy link
Member

I would expect a process variable would be of type Period, and not using internal classes of DMN (ComparablePeriod).

jomarko pushed a commit to jomarko/drools that referenced this pull request Jun 14, 2021
@elguardian
Copy link
Member Author

@jomarko the info about deserialization is related to the java class dependant on how you map those clases so @tarilabs is right

@elguardian elguardian merged commit c708f45 into kiegroup:master Jun 14, 2021
@jomarko
Copy link

jomarko commented Jun 14, 2021

@elguardian hello, sorry, this PR was not ready for merge from my point of view. There is no ack from QE side. Also please notice I prepared PR where I fix some cases of mapping DMN types indo BPMN/Java types. #1947 (comment)

Without addressing them or reporting them separately, we can not mark JBPM-9774 as done.

@elguardian
Copy link
Member Author

@jomarko provide a proper test plz. The test you provided is not using the new code. For the code make it work you need to provide class loader and runtime manager.

@jomarko
Copy link

jomarko commented Jun 14, 2021

@elguardian working on tests updates as #1952

However I would like to ask update your added tests. I checked the newly created asset businessRuleCollectionTaskProcess.bpmn2, and it can not be opened in our latest VS Code tooling. See the screenshot below. I think it is importnat we use assets that are compliant with current tooling.

Screenshot from 2021-06-14 14-20-13

@elguardian
Copy link
Member Author

@jomarko This was requested by DMN support for generic collections. the problem is in stunner in this case. please file a jira to stunner team so they can allow this sort of mapping.

@jomarko
Copy link

jomarko commented Jun 15, 2021

@elguardian I got the same for businessRuleTaskProcess.bpmn2. Let me check my understanding. Does a JBPM-9744 brings a support for a DMN <-> BPMN mapping, that can not be modeled using our current tooling? Just manually?

Screenshot from 2021-06-15 05-46-53

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