Git Issue - 2749: Add decisionInstanceId to evaluate DMN decision api response

[kmannuru]

GIT Issue: #2749

[CLAassistant]

All committers have signed the CLA.

[yanavasileva]

Hi @kmannuru,

Thank you for raising this.
I will have a look at it and get back to you.
Please note before merge, you need to sign one time only Contributor License Agreement (CLA).

Best,
Yana

[kmannuru]

@yanavasileva Did you get a chance to review my code changes. Thanks

[yanavasileva]

Hi @kmannuru, I will need more time to review the PR as the change is not trivial.
Please stay tuned and thank you for your patience in advance.

[yanavasileva]

Hi, just checking in.
I am not sure about the proposed approach, I will discuss the topic with the team and get back to you.
Some points that I can already raise after checking the engine implementation:

• What will happen if a user has a variable that's called decisionInstanceId? I don't we should mess with the variables that are passed for evaluating a decision.
• The test coverage (in the dmn-engine and engine) doesn't seem sufficient at first glance.
• Would it be possible to avoid changing the scope of commons-lang3?

Best,
Yana

[kmannuru]

@yanavasileva thanks for your feedback. I'll update the PR with additional unit test coverage and change scope of commons-lang3.
Regarding user variable map, I'm planning to keep it more dynamic building variable map key prefix with decisionDefinitionKey.
variables.put(decisionDefinition.getKey()+"decisionInstanceId", decisionInstanceId);
Please let me know if I can proceed with this approach. Thank you.

[yanavasileva]

@kmannuru,
We don't want to pass the decisionInstanceId via the variables, as I mentioned earlier, I confirmed it with the team as well. You can try to explore other options to pass the id to the evaluated result. My initial proposal that I didn't weighted thoroughly is to pass it via the DecisionInvocation. Maybe you can check the options there.
Nevertheless, we won't proceed with the variables approach.

[yanavasileva]

I will wait for adjusting the handling of decisionInstanceId before continuing with the review.

[yanavasileva]

Closing due to inactivity. The PR can be reopened again when you get back to the topic.

[kmannuru]

@yanavasileva I'm currently working on the changes to create decisionInstanceId part of the invocation logic in DecisionInvocation.java and not pass part of variable map. The decisionInstanceId will be passed as method parameter to DefaultDmnDecisionContext.java. Can you please help reopen the PR as i'm planning to push my changes shortly. Thank you.