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

kmannuru · 2024-03-21T17:15:24Z

GIT Issue: #2749

CLAassistant · 2024-03-21T17:15:30Z

All committers have signed the CLA.

yanavasileva · 2024-03-22T07:31:49Z

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 · 2024-04-04T19:27:24Z

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

yanavasileva · 2024-04-08T09:40:54Z

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 · 2024-04-17T14:33:57Z

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 · 2024-04-24T15:10:55Z

@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 · 2024-04-25T06:47:47Z

@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 · 2024-05-22T06:35:19Z

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

kmannuru · 2024-05-22T14:37:30Z

@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.

kmannuru · 2024-05-28T14:25:06Z

@yanavasileva I've made changes to the PR as per your recommendation. Please help review my code changes. Thanks.

kmannuru · 2024-06-05T19:09:04Z

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

yanavasileva

Hey @kmannuru,

Thank you for your patience with this. I have started the review but unfortunately there are open questions that I won't have a look with fresh mind.
Nevertheless, the changes look promising and I added initial round of feedback that you can check to clean up the contribution.

Best,
Yana

yanavasileva · 2024-05-29T13:51:35Z

engine-dmn/engine/src/main/java/org/camunda/bpm/dmn/engine/impl/DefaultDmnEngine.java

 }

- public DmnDecisionResult evaluateDecision(DmnDecision decision, VariableContext variableContext) {
+ public DmnDecisionResult evaluateDecision(DmnDecision decision, VariableContext variableContext, String decisionInsanceId) {


Suggested change

public DmnDecisionResult evaluateDecision(DmnDecision decision, VariableContext variableContext, String decisionInsanceId) {

public DmnDecisionResult evaluateDecision(DmnDecision decision, VariableContext variableContext, String decisionInstanceId) {

yanavasileva · 2024-05-29T13:51:41Z

engine-dmn/engine/src/main/java/org/camunda/bpm/dmn/engine/impl/DefaultDmnEngine.java

 ensureNotNull("decision", decision);
 ensureNotNull("variableContext", variableContext);

 if (decision instanceof DmnDecisionImpl) {
 DefaultDmnDecisionContext decisionContext = new DefaultDmnDecisionContext(dmnEngineConfiguration);
- return decisionContext.evaluateDecision(decision, variableContext);
+ return decisionContext.evaluateDecision(decision, variableContext, decisionInsanceId);


Suggested change

return decisionContext.evaluateDecision(decision, variableContext, decisionInsanceId);

return decisionContext.evaluateDecision(decision, variableContext, decisionInstanceId);

yanavasileva · 2024-06-13T09:13:54Z

engine/pom.xml

+<!-- <dependency>-->
+<!-- <groupId>org.camunda.bpm.juel</groupId>-->
+<!-- <artifactId>camunda-juel</artifactId>-->
+<!-- </dependency>-->
+
 <dependency>
 <groupId>org.camunda.bpm.juel</groupId>
 <artifactId>camunda-juel</artifactId>
+ <version>7.20.0</version>


❌ The version of the juel module is the current project version:

Suggested change









<dependency>

<groupId>org.camunda.bpm.juel</groupId>

<artifactId>camunda-juel</artifactId>

<version>7.20.0</version>

<dependency>

<groupId>org.camunda.bpm.juel</groupId>

<artifactId>camunda-juel</artifactId>

engine-dmn/engine/src/main/java/org/camunda/bpm/dmn/engine/DmnDecisionResult.java

yanavasileva · 2024-06-13T13:14:01Z

engine-dmn/engine/src/main/java/org/camunda/bpm/dmn/engine/DmnDecisionResult.java

@@ -111,4 +111,15 @@ public interface DmnDecisionResult extends List<DmnDecisionResultEntries>, Seria
 */
 <T extends TypedValue> T getSingleEntryTyped();

+ /** Returns the value of decisionInstanceId that's part of DMN decision evaluation
+ * @return the decisionInstanceId generated
+ * GIT Issue: https://github.com/camunda/camunda-bpm-platform/issues/2749


❌ Please remove all of the git ticket references from the code. The modern IDE and GitHub offer enough support to track the origins to ticket and pull request.

Suggested change

* GIT Issue: https://github.com/camunda/camunda-bpm-platform/issues/2749

yanavasileva · 2024-06-13T13:52:54Z

...c/main/java/org/camunda/bpm/engine/impl/history/producer/DefaultDmnHistoryEventProducer.java

+ public boolean isNotBlank(String value) {
+ return value != null && !value.isEmpty();
+ }
+


❌ Reuse existing utils.

Suggested change

public boolean isNotBlank(String value) {

return value != null && !value.isEmpty();

}

yanavasileva · 2024-06-13T13:53:42Z

...c/main/java/org/camunda/bpm/engine/impl/history/producer/DefaultDmnHistoryEventProducer.java

+ * Setting the generated decisionInstanceId to the root event
+ * Generating custom decisionInstanceId to persist the history event and return the id in the response
+ */
+ if(isNotBlank(evaluationEvent.getDecisionInstanceId())) {


❌ Reuse existing utils (don't forget to add import):

Suggested change

if(isNotBlank(evaluationEvent.getDecisionInstanceId())) {

if(StringUtil.hasText(evaluationEvent.getDecisionInstanceId())) {

yanavasileva · 2024-06-13T14:04:05Z

...c/main/java/org/camunda/bpm/engine/impl/history/producer/DefaultDmnHistoryEventProducer.java

+ * Generating custom decisionInstanceId to persist the history event and return the id in the response
+ */
+ if(isNotBlank(evaluationEvent.getDecisionInstanceId())) {
+ rootDecisionEvent.setId(evaluationEvent.getDecisionInstanceId());


Note to myself:
❓ Shouldn't the id of the root to be pointing to the root event DI id? What's the reference (ROOT_DEC_INST_ID_, ROOT_PROC_INST_ID_, history cleanup)?

yanavasileva · 2024-06-13T14:14:10Z

...est/src/main/java/org/camunda/bpm/engine/rest/sub/repository/DecisionDefinitionResource.java

+ @Path("/evaluate-with-id")
+ @Consumes(MediaType.APPLICATION_JSON)
+ @Produces(MediaType.APPLICATION_JSON)
+ DecisionEvaluationDto evaluateDecisionWithId(@Context UriInfo context, EvaluateDecisionDto parameters);


Note to myself:
❓ Could we avoid adding the new endpoint?

yanavasileva · 2024-06-13T14:15:26Z

...-openapi/src/main/templates/models/org/camunda/bpm/engine/rest/dto/DecisionEvaluationDto.ftl

📓 There's a compilation error in OpenAPI tests, please have a look. Hints:
cd engine-rest
mvn clean install
Let me know if you need more guidance

kmannuru · 2024-06-17T07:33:46Z

@yanavasileva Thanks for your feedback. I'm currently working on changes to incorporate your review comments. I'll share the updated PR shortly. Thanks.

kmannuru · 2024-06-17T17:39:39Z

engine-dmn/engine/src/main/java/org/camunda/bpm/dmn/engine/impl/DefaultDmnEngine.java

@@ -155,16 +155,16 @@ public DmnDecisionTableResult evaluateDecisionTable(String decisionKey, DmnModel
 public DmnDecisionResult evaluateDecision(DmnDecision decision, Map<String, Object> variables) {
 ensureNotNull("decision", decision);
 ensureNotNull("variables", variables);
- return evaluateDecision(decision, Variables.fromMap(variables).asVariableContext());
+ return evaluateDecision(decision, Variables.fromMap(variables).asVariableContext(), null);


This will be replaced with evaluateDecision(decision, variableContext)

kmannuru · 2024-06-17T17:40:26Z

engine-dmn/engine/src/main/java/org/camunda/bpm/dmn/engine/impl/DefaultDmnEngine.java

@@ -181,7 +181,7 @@ public DmnDecisionResult evaluateDecision(String decisionKey, InputStream inputS
 List<DmnDecision> decisions = parseDecisions(inputStream);
 for (DmnDecision decision : decisions) {
 if (decisionKey.equals(decision.getKey())) {
- return evaluateDecision(decision, variableContext);
+ return evaluateDecision(decision, variableContext, null);


This will be replaced with evaluateDecision(decision, variableContext)

kmannuru · 2024-06-17T17:40:33Z

engine-dmn/engine/src/main/java/org/camunda/bpm/dmn/engine/impl/DefaultDmnEngine.java

@@ -197,7 +197,7 @@ public DmnDecisionResult evaluateDecision(String decisionKey, DmnModelInstance d
 List<DmnDecision> decisions = parseDecisions(dmnModelInstance);
 for (DmnDecision decision : decisions) {
 if (decisionKey.equals(decision.getKey())) {
- return evaluateDecision(decision, variableContext);
+ return evaluateDecision(decision, variableContext, null);


This will be replaced with evaluateDecision(decision, variableContext)

kmannuru · 2024-06-17T17:41:40Z

engine-dmn/engine/src/main/java/org/camunda/bpm/dmn/engine/impl/DefaultDmnEngine.java

@@ -112,7 +112,7 @@ public DmnDecisionTableResult evaluateDecisionTable(DmnDecision decision, Variab
 if (decision instanceof DmnDecisionImpl && decision.isDecisionTable()) {
 DefaultDmnDecisionContext decisionContext = new DefaultDmnDecisionContext(dmnEngineConfiguration);

- DmnDecisionResult decisionResult = decisionContext.evaluateDecision(decision, variableContext);
+ DmnDecisionResult decisionResult = decisionContext.evaluateDecision(decision, variableContext, null);


@yanavasileva This will not change because we need to pass the decisionInsanceId to decisionContext.evaluateDecision(decision, variableContext, null);

When you are ready, please submit your changes, I will have a look then.

github-actions · 2024-06-26T04:05:42Z

Closing due to lack of requested feedback. If you would like us to look at this, please provide the requested information to re-open the PR.

kmannuru · 2024-07-17T13:40:24Z

@yanavasileva Can you please help reopen the PR so I can merge my changes for your review? Thanks

yanavasileva · 2024-07-18T05:15:05Z

PR reopened.

kmannuru · 2024-09-11T18:59:28Z

@yanavasileva Please help review the PR. The latest commit on the PR includes the changes that addresses the review comments. Thanks.

yanavasileva · 2024-09-12T06:16:40Z

@kmannuru, I don't see any new changes pushed to the PR. Have you added everything?

kmannuru · 2024-09-13T16:15:04Z

@kmannuru, I don't see any new changes pushed to the PR. Have you added everything?

@yanavasileva Yes. The PR is updated with the changes to reflect the review comments.
Here's the commit: cddcc56

Please help review when you get a chance. Thanks.

kmannuru mentioned this pull request Mar 21, 2024

Add DecisionInstanceID to API response for evaluating a DRD #2749

Open

kmannuru changed the title ~~ENTDAPCOE-94: Add decisionInstanceId to evaluate DMN decision api response~~ Git Issue - 2749: Add decisionInstanceId to evaluate DMN decision api response Mar 21, 2024

yanavasileva assigned kmannuru Mar 22, 2024

yanavasileva self-requested a review March 22, 2024 07:31

yanavasileva requested changes Apr 26, 2024

View reviewed changes

Add decisionInstanceId to DMN decision evaluation response

03d06ee

yanavasileva closed this May 22, 2024

yanavasileva reopened this May 22, 2024

brianwarner force-pushed the feature/evaluate_dmn_with_dec_instant_id branch 2 times, most recently from 7de6638 to 03d06ee Compare May 24, 2024 19:45

yanavasileva requested changes Jun 13, 2024

View reviewed changes

yanavasileva added the group:requested-info DRI: Yana label Jun 13, 2024

github-actions bot added the group:stale DRI: Yana label Jun 14, 2024

Merge branch 'master' into feature/evaluate_dmn_with_dec_instant_id

609734b

kmannuru commented Jun 17, 2024

View reviewed changes

DMN evaluate decision api review comment changes

cddcc56

github-actions bot added group:stale DRI: Yana and removed group:stale DRI: Yana labels Jun 18, 2024

github-actions bot added the group:closed-as-inactive DRI: Yana label Jun 26, 2024

github-actions bot closed this Jun 26, 2024

yanavasileva reopened this Jul 18, 2024

yanavasileva removed group:requested-info DRI: Yana group:closed-as-inactive DRI: Yana group:stale DRI: Yana labels Jul 18, 2024

yanavasileva self-requested a review September 16, 2024 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kmannuru commented Mar 21, 2024 •

edited

Loading

CLAassistant commented Mar 21, 2024 •

edited

Loading

yanavasileva commented Mar 22, 2024

kmannuru commented Apr 4, 2024

yanavasileva commented Apr 8, 2024

yanavasileva commented Apr 17, 2024

kmannuru commented Apr 24, 2024

yanavasileva commented Apr 25, 2024

yanavasileva left a comment

yanavasileva commented May 22, 2024

kmannuru commented May 22, 2024

kmannuru commented May 28, 2024

kmannuru commented Jun 5, 2024

yanavasileva left a comment

yanavasileva May 29, 2024

yanavasileva May 29, 2024

yanavasileva Jun 13, 2024

yanavasileva Jun 13, 2024

yanavasileva Jun 13, 2024

yanavasileva Jun 13, 2024

yanavasileva Jun 13, 2024 •

edited

Loading

yanavasileva Jun 13, 2024

yanavasileva Jun 13, 2024

kmannuru commented Jun 17, 2024

kmannuru Jun 17, 2024

kmannuru Jun 17, 2024

kmannuru Jun 17, 2024

kmannuru Jun 17, 2024

yanavasileva Jun 26, 2024

github-actions bot commented Jun 26, 2024

kmannuru commented Jul 17, 2024

yanavasileva commented Jul 18, 2024

kmannuru commented Sep 11, 2024

yanavasileva commented Sep 12, 2024 •

edited

Loading

kmannuru commented Sep 13, 2024

	public DmnDecisionResult evaluateDecision(DmnDecision decision, VariableContext variableContext, String decisionInsanceId) {
	public DmnDecisionResult evaluateDecision(DmnDecision decision, VariableContext variableContext, String decisionInstanceId) {

	return decisionContext.evaluateDecision(decision, variableContext, decisionInsanceId);
	return decisionContext.evaluateDecision(decision, variableContext, decisionInstanceId);

	public boolean isNotBlank(String value) {
	return value != null && !value.isEmpty();
	}

	if(isNotBlank(evaluationEvent.getDecisionInstanceId())) {
	if(StringUtil.hasText(evaluationEvent.getDecisionInstanceId())) {

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

Are you sure you want to change the base?

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

Conversation

kmannuru commented Mar 21, 2024 • edited Loading

CLAassistant commented Mar 21, 2024 • edited Loading

yanavasileva commented Mar 22, 2024

kmannuru commented Apr 4, 2024

yanavasileva commented Apr 8, 2024

yanavasileva commented Apr 17, 2024

kmannuru commented Apr 24, 2024

yanavasileva commented Apr 25, 2024

yanavasileva left a comment

Choose a reason for hiding this comment

yanavasileva commented May 22, 2024

kmannuru commented May 22, 2024

kmannuru commented May 28, 2024

kmannuru commented Jun 5, 2024

yanavasileva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavasileva Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmannuru commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 26, 2024

kmannuru commented Jul 17, 2024

yanavasileva commented Jul 18, 2024

kmannuru commented Sep 11, 2024

yanavasileva commented Sep 12, 2024 • edited Loading

kmannuru commented Sep 13, 2024

kmannuru commented Mar 21, 2024 •

edited

Loading

CLAassistant commented Mar 21, 2024 •

edited

Loading

yanavasileva Jun 13, 2024 •

edited

Loading

yanavasileva commented Sep 12, 2024 •

edited

Loading