AAE-22465 Added logic to validate the cardinality #4654

AkhileshPamidimarthi · 2024-05-10T11:27:24Z

What kind of change does this PR introduce?

Bugfix

Feature

Code style update (formatting, local variables)

Refactoring (no functional changes, no api changes)

Build related changes

Documentation

Other... Please describe:

Description

Added logic to validate the cardinality

Issue Link: https://hyland.atlassian.net/browse/AAE-22465

Does this PR introduce a breaking change? (check one with "x")

Yes

No

eromano

I would prefer review our approach here with @gicappa @igdianov @erdemedeiros, let's have a call next week

jesty · 2024-05-13T08:18:31Z

...re/activiti-process-validation/src/main/java/org/activiti/validation/validator/Problems.java

@@ -100,6 +100,7 @@ public interface Problems {
  String THROW_EVENT_INVALID_EVENTDEFINITION = "activiti-throw-event-invalid-eventdefinition";

  String MULTI_INSTANCE_MISSING_COLLECTION = "activiti-multi-instance-missing-collection";
+    String MULTI_INSTANCE_HIGHER_CARDINALITY = "activiti-multi-instance-cardinality-greater-than-200";


can u fix formatting? Thanks!

jesty · 2024-05-27T08:07:10Z

...ss-validation/src/main/java/org/activiti/validation/validator/impl/FlowElementValidator.java

+                try {
+                    int loopCardinality=  Integer.parseInt(multiInstanceLoopCharacteristics.getLoopCardinality());
+                    if(loopCardinality >200){
+                        addError(errors, Problems.MULTI_INSTANCE_HIGHER_CARDINALITY, process, activity,


please replace addError with addWarning

jesty · 2024-05-28T15:29:58Z

...ss-validation/src/main/java/org/activiti/validation/validator/impl/FlowElementValidator.java

+            if(!StringUtils.isEmpty(multiInstanceLoopCharacteristics.getLoopCardinality())){
+                try {
+                    int loopCardinality=  Integer.parseInt(multiInstanceLoopCharacteristics.getLoopCardinality());
+                    if(loopCardinality >200){


can we extract the value in an external property? thanks

jesty · 2024-05-29T10:12:03Z

...ss-validation/src/main/java/org/activiti/validation/validator/impl/FlowElementValidator.java

-	  	}
-	  }
+    protected static final int ID_MAX_LENGTH = 255;
+    @Value("${loop.cardinality.threshold}")


add a blank new line above, prefix it with acitivi and specify the default value here.

Suggested change

@Value("${loop.cardinality.threshold}")

@Value("${activiti.loop.cardinality.threshold:200}")

jesty · 2024-05-29T10:13:09Z

activiti-core/activiti-process-validation/src/main/resources/application.properties

@@ -0,0 +1 @@
+loop.cardinality.threshold=200


specifying the default value, then, we can remove it

…l_tasks_

jesty · 2024-06-03T08:48:08Z

I would prefer review our approach here with @gicappa @igdianov @erdemedeiros, let's have a call next week

Hi @eromano we changed to warning. @gicappa @igdianov @erdemedeiros what do you think?

I asked to add a warning because in my tests I noticed that when we have many parallelel task to create we can face 500 errors because the operation it's going to be really long.

igdianov

@jesty Could add unit tests to make sure there is coverage against future regressions?

igdianov · 2024-06-04T16:21:31Z

activiti-core/activiti-process-validation/pom.xml

@@ -37,5 +37,9 @@
      <artifactId>assertj-core</artifactId>
      <scope>test</scope>
    </dependency>
+      <dependency>


Is it really needed to include the dependency from Spring in the core engine?

igdianov · 2024-06-04T16:23:34Z

...ss-validation/src/main/java/org/activiti/validation/validator/impl/FlowElementValidator.java

-	  }
+    protected static final int ID_MAX_LENGTH = 255;
+
+    @Value("${loop.cardinality.threshold:200}")


It is not a good practice to use Spring value injection. The core validation configuration approach should not rely on Spring field injection to configure parameter limits.

igdianov · 2024-06-04T16:24:44Z

...ss-validation/src/main/java/org/activiti/validation/validator/impl/FlowElementValidator.java

-	protected static final int ID_MAX_LENGTH = 255;
-
-	@Override
-	protected void executeValidation(BpmnModel bpmnModel, Process process, List<ValidationError> errors) {
-		for (FlowElement flowElement : process.getFlowElements()) {
-
-			if (flowElement instanceof Activity) {
-				Activity activity = (Activity) flowElement;
-				handleConstraints(process, activity, errors);
-				handleMultiInstanceLoopCharacteristics(process, activity, errors);
-				handleDataAssociations(process, activity, errors);
-			}
-
-		}
-
-	}
-
-	protected void handleConstraints(Process process, Activity activity, List<ValidationError> errors) {
-		if (activity.getId() != null && activity.getId().length() > ID_MAX_LENGTH) {
-			addError(errors, Problems.FLOW_ELEMENT_ID_TOO_LONG, process, activity,
-					"The id of a flow element must not contain more than " + ID_MAX_LENGTH + " characters");
-		}
-	}
-
-	protected void handleMultiInstanceLoopCharacteristics(Process process, Activity activity, List<ValidationError> errors) {
-		MultiInstanceLoopCharacteristics multiInstanceLoopCharacteristics = activity.getLoopCharacteristics();
-		if (multiInstanceLoopCharacteristics != null) {
-
-			if (StringUtils.isEmpty(multiInstanceLoopCharacteristics.getLoopCardinality())
-	    		&& StringUtils.isEmpty(multiInstanceLoopCharacteristics.getInputDataItem())) {
-
-			  addError(errors, Problems.MULTI_INSTANCE_MISSING_COLLECTION, process, activity,
-	    			"Either loopCardinality or loopDataInputRef/activiti:collection must been set");
-	    }
-
-		}
-	}
-
-	protected void handleDataAssociations(Process process, Activity activity, List<ValidationError> errors) {
-	  if (activity.getDataInputAssociations() != null) {
-	  	for (DataAssociation dataAssociation : activity.getDataInputAssociations()) {
-	  		if (StringUtils.isEmpty(dataAssociation.getTargetRef())) {
-	  			 addError(errors, Problems.DATA_ASSOCIATION_MISSING_TARGETREF, process, activity,
-	  					 "Targetref is required on a data association");
-	  	    }
-	  	}
-	  }
-	  if (activity.getDataOutputAssociations() != null) {
-	  	for (DataAssociation dataAssociation : activity.getDataOutputAssociations()) {
-	  		if (StringUtils.isEmpty(dataAssociation.getTargetRef())) {
-	  			 addError(errors, Problems.DATA_ASSOCIATION_MISSING_TARGETREF, process, activity,
-	  					 "Targetref is required on a data association");
-	  	    }
-	  	}
-	  }


Could you restore original indents to keep the formatting consistent for easier code review?

AkhileshPamidimarthi marked this pull request as ready for review May 10, 2024 14:08

AkhileshPamidimarthi requested review from jesty, mavotto, tom-dal and pmartinezga May 10, 2024 14:08

eromano requested changes May 10, 2024

View reviewed changes

jesty added do-not-merge next-version labels May 14, 2024

eromano removed the next-version label May 22, 2024

jesty requested changes May 27, 2024

View reviewed changes

AkhileshPamidimarthi force-pushed the fix/AAE-22465_Cannot_execute_many_parallel_tasks_ branch from 129183d to c14cc6c Compare May 28, 2024 11:09

AkhileshPamidimarthi added 3 commits May 28, 2024 16:40

AAE-22465 Added logic to validate the cardinality

01d8337

Fixed failing test

6c18c90

Fixed PR comments

c14cc6c

AkhileshPamidimarthi requested a review from jesty May 28, 2024 11:27

jesty requested changes May 28, 2024

View reviewed changes

Extracted the value to application.properties as per the review comment

31e8d2e

AkhileshPamidimarthi requested a review from jesty May 29, 2024 08:22

jesty requested changes May 29, 2024

View reviewed changes

Removed the loop.cardinality.threshold value from Application.properties

5aaf507

AkhileshPamidimarthi requested a review from jesty May 29, 2024 12:57

jesty requested review from igdianov and erdemedeiros June 3, 2024 08:46

Merge branch 'develop' into fix/AAE-22465_Cannot_execute_many_paralle…

9fed036

…l_tasks_

igdianov requested changes Jun 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AAE-22465 Added logic to validate the cardinality #4654

AAE-22465 Added logic to validate the cardinality #4654

AkhileshPamidimarthi commented May 10, 2024 •

edited

eromano left a comment •

edited

jesty May 13, 2024

jesty May 27, 2024

jesty May 28, 2024

jesty May 29, 2024

jesty May 29, 2024

jesty commented Jun 3, 2024

igdianov left a comment

igdianov Jun 4, 2024

igdianov Jun 4, 2024

igdianov Jun 4, 2024

	@Value("${loop.cardinality.threshold}")

	@Value("${activiti.loop.cardinality.threshold:200}")

		@@ -0,0 +1 @@
		loop.cardinality.threshold=200

AAE-22465 Added logic to validate the cardinality #4654

Are you sure you want to change the base?

AAE-22465 Added logic to validate the cardinality #4654

Conversation

AkhileshPamidimarthi commented May 10, 2024 • edited

Description

eromano left a comment • edited

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

jesty commented Jun 3, 2024

igdianov 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

AkhileshPamidimarthi commented May 10, 2024 •

edited

eromano left a comment •

edited