Java is famous for its widespread use in enterprise applications. Business processes need to be managed. The Flowable platform can help with that! What's more, the project is written in Java and is open source. So, we can analyze it and search for errors using PVS-Studio.
What's in the foreword is pretty much all I can say about the project without just repeating the information on Wikipedia or the official website. So, this is a good idea to visit it: Flowable: Business Process Automation | Low-code | Workflow Automation.
Business means Enterprise. Java and Enterprise are a well-known bundle! That's why many lines of code can be compared to the length of an essay. This made me bother to slightly format the code to increase its readability. Note that most fragments shown here probably look different in the original code.
To make sure we're really working with Java, look at some short class names:
But wait, there's more. The record holder for length is the UpdateChannelDefinitionTypeAndImplementationForAllChannelDefinitionsCmd class. And when it comes to tests, the same name with the Test suffix wins.
Now that we've seen the Enterprise names, I suggest we move on to the code base. To keep with tradition, we'll use PVS-Studio for this task, and see if we can find the most interesting things. Put on your suit jackets and let's go.
We begin our journey with tests. Although tests are designed to detect bugs in the program logic, it's just as easy to make a mistake in them as it is in regular code. Nobody writes tests for tests, but a static analyzer can check them.
By the way, just recently we've written a separate article about finding bugs in unit tests! I encourage testers to read it.
Here though, let's take a look at the following snippet:
@Test
@CmmnDeployment
public void testExpressionReturnsFuture() {
CountDownLatch latch = new CountDownLatch(2);
TestBeanReturnsFuture testBean = new TestBeanReturnsFuture(latch,
cmmnEngineConfiguration.getAsyncTaskExecutor());
CaseInstance caseInstance = cmmnRuntimeService.createCaseInstanceBuilder()
.caseDefinitionKey("myCase")
.transientVariable("bean", testBean)
.start();
assertCaseInstanceEnded(caseInstance);
if (....) {
HistoricCaseInstance historicCaseInstance = ....;
// Every service task sleeps for 1s,
// but when we use futures it should take less then 2s.
long durationInMillis = historicCaseInstance.getEndTime().getTime() –
historicCaseInstance.getEndTime().getTime();
assertThat(durationInMillis).isLessThan(1500);
}
}The test checks the service execution time. Unfortunately, durationInMillis is always zero because it subtracts getEndTime() from getEndTime(). So, the test that checks whether this value is less than 1500 is always positive because 0 < 1500, which is easy to prove after reading Principia Mathematica.
The fix is quite easy:
long durationInMillis = historicCaseInstance.getEndTime().getTime() –
historicCaseInstance.getStartTime().getTime();I restarted the test with this fix, and it passed. Now let's look at the PVS-Studio analyzer message that helped us find the issue:
V6001 There are identical sub-expressions historicCaseInstance.getEndTime().getTime()' to the left and to the right of the '-' operator. ServiceTaskWithFuturesTest.java 76
Next up is BigDecimal, a legend of fintech:
public class ELExecutionContextBuilder {
....
protected static void preProcessInputVariables(DecisionTable decisionTable,
Map<String, Object> inputVariables) {
....
// check if transformation is needed
for (Map.Entry<String, Object> inputVariable : inputVariables.entrySet()) {
String inputVariableName = inputVariable.getKey();
try {
Object inputVariableValue = inputVariable.getValue();
if (inputVariableValue instanceof LocalDate) {
Date transformedDate = ((LocalDate) inputVariableValue).toDate();
inputVariables.put(inputVariableName, transformedDate);
} else if (inputVariableValue instanceof java.time.LocalDate) {
Date transformedDate =
Date.from(((java.time.LocalDate) inputVariableValue).atStartOfDay()
.atZone(ZoneId.systemDefault())
.toInstant());
inputVariables.put(inputVariableName, transformedDate);
} else if (....) {
BigInteger transformedNumber =
new BigInteger(inputVariableValue.toString());
inputVariables.put(inputVariableName, transformedNumber);
} else if (inputVariableValue instanceof Double) {
BigDecimal transformedNumber =
new BigDecimal((Double) inputVariableValue);
inputVariables.put(inputVariableName, transformedNumber);
} else if (inputVariableValue instanceof Float) {
double doubleValue =
Double.parseDouble(inputVariableValue.toString());
BigDecimal transformedNumber = new BigDecimal(doubleValue); // <=
inputVariables.put(inputVariableName, transformedNumber);
}
} catch (Exception ex) {
throw new FlowableException(....);
}
}
}
}I recommend that you don't gaze into the code for too long, or it will gaze back at you. We're interested in working with the aforementioned BigDecimal. Let's look at the following note in JavaDoc to clarify the details:
The results of this constructor can be somewhat unpredictable. One might assume that writing new BigDecimal(0.1) in Java creates a BigDecimal which is exactly equal to 0.1 (an unscaled value of 1, with a scale of 1), but it is actually equal to 0.1000000000000000055511151231257827021181583404541015625. This is because 0.1 cannot be represented exactly as a double (or, for that matter, as a binary fraction of any finite length). Thus, the value that is being passed in to the constructor is not exactly equal to 0.1, appearances notwithstanding.
The same document recommends using BigDecimal.valueOf(double) or new BigDecimal(String). At the same time, the value in the specified fragment is parsed to String and then back to double. Does it make any difference? Let's check out just for the fun of it:
void main() {
double d = 0.1;
BigDecimal bigDecimal;
bigDecimal = new BigDecimal(d);
println("new BigDecimal(double): \t" + bigDecimal.toPlainString());
bigDecimal = new BigDecimal(String.valueOf(d));
println("new BigDecimal(double -> string): \t" +
bigDecimal.toPlainString());
bigDecimal = new BigDecimal(Double.parseDouble(String.valueOf(d)));
println("new BigDecimal(double -> string -> double): \t" +
bigDecimal.toPlainString());
bigDecimal = BigDecimal.valueOf(d);
println("BigDecimal.valueOf(double): \t" + bigDecimal.toPlainString());
}Yes! For details, see JEP-477. However, at the time of writing, this is still a preview feature. I used it in the article to simplify this already simple fragment and not to write public static void main(String[] args). We have enough wordy strings in the Enterprise class names.
We get the following output:
new BigDecimal(double):
0.1000000000000000055511151231257827021181583404541015625
new BigDecimal(double -> string): 0.1
new BigDecimal(double -> string -> double):
0.1000000000000000055511151231257827021181583404541015625
BigDecimal.valueOf(double): 0.1Well, additional conversions of double to string and back don't change the behavior. In any case, we need to use either valueOf or the constructor with the String argument if we want to get 0.1 from the 0.1 input value (based on the method name, it handles input variables).
As a result, not only do we get rid of a potentially unexpected value, but we also eliminate the redundant String to double parsing operation:
new BigDecimal(inputVariableValue.toString());Lastly, here's the PVS-Studio analyzer warning:
V6068 Constructor call can result in imprecise representation of the initialized value. ELExecutionContextBuilder.java 137
public class CronExpression implements Serializable, Cloneable {
....
protected int findNextWhiteSpace(int i, String s) {
for (; i < s.length() && (s.charAt(i) != ' ' || s.charAt(i) != '\t'); i++) {
}
return i;
}
....
}I'd like to note that the loop body is empty in the original code fragment (otherwise it would have "....").
Here is an error we already encountered in other projects—one of the expressions is always true. We're talking about s.charAt(i) != ' ' || s.charAt(i) != '\t'.
To get a false OR result, a character must be equal to two different characters at the same time. Of course, this is impossible.
The result is that the loop is executed as long as the i < s.length() condition is true, and the method output is always the end of the string index, not the space character.
The PVS-Studio warning:
V6007 Expression 's.charAt(i) != '\t'' is always true. CronExpression.java 963
@Test
void convertJsonToModelKeyWithFixedValue() {
ChannelModel channelModel = readJson(....);
assertThat(channelModel)
.isInstanceOfSatisfying(KafkaOutboundChannelModel.class, model -> {
model.getRecordKey().getFixedValue().equals("customer");
});
}
@Test
void convertJsonToModelKeyWithDelegateExpression() {
ChannelModel channelModel = readJson(....);
assertThat(channelModel)
.isInstanceOfSatisfying(KafkaOutboundChannelModel.class, model -> {
model.getRecordKey().getDelegateExpression().equals("${test}");
});
}
@Test
void convertJsonToModelKeyWithEventField() {
ChannelModel channelModel = readJson(....);
assertThat(channelModel)
B.isInstanceOfSatisfying(KafkaOutboundChannelModel.class, model -> {
model.getRecordKey().getEventField().equals("customer");
});
}We do a short loop and meet the tests again. Once again, they aren't falling. Let's first look at the warnings that the PVS-Studio analyzer issues, then we'll fix them:
The developer probably expected the second argument of the isInstanceOfSatisfying method to be Function<T, Boolean> for some reason. I have no idea why there might have been such an expectation, but in reality, the method takes Consumer<T>. Maybe the problem is simple carelessness, and developers may have thought that the equals method was equivalent to AssertEquals.
For the test to fail, we need an exception. Let's just wrap the equals calls in assertThat(....).isEqualTo(....). Here's an example:
assertThat(model.getRecordKey().getEventField()).isEqualTo("customer");public class ProcessExecutionLogger {
protected void internalPopulateExecutionTree(....) {
if (parentMapping.containsKey(parentNode.getId())) {
for (....) {
....
childNode.setParentNode(childNode); // <=
parentNode.getChildNodes().add(childNode);
internalPopulateExecutionTree(childNode, parentMapping);
}
}
}
}The childNode object is a parent of itself due to the broken hierarchy here. The necessary dependency is indirectly indicated by the line that immediately follows: parentNode.getChildNodes().add(childNote). So, the parent is aware of the child but not the other way around. I'm wondering if there's a risk when iterating over such nodes to reach a StackOverflowError?
Here's the PVS-Studio analyzer message about this anomaly:
V6100 An object is used as an argument to its own method. Consider checking the first actual argument of the 'setParentNode' method. ProcessExecutionLogger.java 107
protected String getTargetNameSpace(XMLStreamReader xmlStreamReader) {
String targetNameSpace = null;
try {
while (xmlStreamReader.hasNext()) {
try {
xmlStreamReader.next();
} catch (Exception e) {
LOGGER.debug("Error reading XML document", e);
throw new DmnXMLException("Error reading XML", e);
}
targetNameSpace = xmlStreamReader.getNamespaceURI();
break;
}
} catch (XMLStreamException e) {
LOGGER.error("Error processing DMN document", e);
throw new DmnXMLException("Error processing DMN document", e);
}
return targetNameSpace;
}Let's dig into an unexpected break that reduces the number of iterations to one regardless of the content in xmlStreamReader. That may have been the intention, but then the devs should've used if. This is a very strange resource handling.
A small reference to git enables finding the previous version of the method (it has been renamed):
protected boolean isDMN12(XMLStreamReader xtr) {
try {
while (xtr.hasNext()) {
try {
xtr.next();
} catch (Exception e) {
LOGGER.debug("Error reading XML document", e);
throw new DmnXMLException("Error reading XML", e);
}
return DMN_12_TARGET_NAMESPACE.equals(xtr.getNamespaceURI());
}
return false;
} catch (XMLStreamException e) {
LOGGER.error("Error processing DMN document", e);
throw new DmnXMLException("Error processing DMN document", e);
}
}This version uses return instead of break, and there's no condition again. The developers probably just decided to use while in this method because the xtr.hasNext() method is called.
Regardless of the original method concept, PVS-Studio correctly issues a warning, because such code raises too many questions:
V6037 An unconditional 'break' within a loop. DmnXMLConverter.java 187
protected void addRecipient(MimeMessage message,
Collection<String> recipients,
Message.RecipientType recipientType) {
if (recipients == null || recipients.isEmpty()) {
return;
}
Collection<String> newRecipients = recipients;
Collection<String> forceRecipients = defaultsConfiguration.forceTo();
if (forceRecipients != null && !forceRecipients.isEmpty()) {
newRecipients = forceRecipients;
}
if (!newRecipients.isEmpty()) {
for (String t : newRecipients) {
try {
message.addRecipient(recipientType, createInternetAddress(t));
} catch (MessagingException e) {
throw new FlowableMailException(....);
}
}
}
}This is a pointless check because newRecipients takes the value of either forceRecipients or recipients. Both are checked for isEmpty beforehand.
It's also pointless because the for (String t : newRecipients) loop wouldn't iterate over an empty collection anyway.
The PVS-Studio analyzer pointed out this redundant code:
Expression '!newRecipients.isEmpty()' is always true. JakartaMailFlowableMailClient.java 194
public class CallbackData {
protected String callbackId;
protected String callbackType;
protected String instanceId;
protected String oldState;
protected String newState;
protected Map<String, Object> additionalData = null;
public CallbackData(String callbackId, String callbackType,
String instanceId, String oldState,
String newState, Map<String, Object> additionalData) {
this.callbackId = callbackId;
this.callbackType = callbackType;
this.instanceId = instanceId;
this.oldState = oldState;
this.newState = newState;
}
....
public Map<String, Object> getAdditionalData() {
return additionalData;
}
public void setAdditionalData(Map<String, Object> additionalData) {
this.additionalData = additionalData;
}
}The PVS-Studio warning:
V6022 Parameter 'additionalData' is not used inside constructor body. CallbackData.java 29
This is a rather hypothetical case. A developer's use of the specified constructor can have unpleasant consequences, because if they pass additionalData to it, the value won't be written. A nasty NullPointerException will occur, for example, when using the getAdditionalData() method if it's not expected to return null.
protected void endAllHistoricActivities(....) {
ProcessEngineConfigurationImpl processEngineConfiguration = CommandContextUtil
.getProcessEngineConfiguration();
if (!processEngineConfiguration.getHistoryLevel()
.isAtLeast(HistoryLevel.ACTIVITY)) {
return;
}
List<HistoricActivityInstanceEntity> historicActivityInstances = ....;
Clock clock = processEngineConfiguration.getClock();
for (var historicActivityInstance : historicActivityInstances) {
historicActivityInstance.markEnded(deleteReason, clock.getCurrentTime());
// Fire event
FlowableEventDispatcher eventDispatcher = null;
if (processEngineConfiguration != null) { // <=
eventDispatcher = processEngineConfiguration.getEventDispatcher();
}
if (eventDispatcher != null && eventDispatcher.isEnabled()) {
seventDispatcher.dispatchEvent(....),
processEngineConfiguration.getEngineCfgKey()); // <=
}
}
}CommandContextUtil.getProcessEngineConfiguration() can return null. So, since the null check is done in the code, it is expected. To be sure, let's look at this method:
public static ProcessEngineConfigurationImpl getProcessEngineConfiguration() {
return getProcessEngineConfiguration(getCommandContext());
}
public static ProcessEngineConfigurationImpl
getProcessEngineConfiguration(CommandContext commandContext) {
if (commandContext != null) {
return ....
}
return null;
}Can commandContext be null? We continue through the call chain:
public static CommandContext getCommandContext() {
return Context.getCommandContext();
}
public static CommandContext getCommandContext() {
Stack<CommandContext> stack = getStack(commandContextThreadLocal);
if (stack.isEmpty()) {
return null;
}
return stack.peek();
}If stack is empty, null is returned. It can be difficult to check whether we can get an empty collection, so let's stop here for now.
So, the call of processEngineConfiguration.getHistoryLevel() can result in NullPointerException.
The PVS-Studio warning:
V6060 The 'processEngineConfiguration' reference was utilized before it was verified against null. TerminateEndEventActivityBehavior.java 166, TerminateEndEventActivityBehavior.java 179
public class IntegerDataObject extends ValuedDataObject {
@Override
public void setValue(Object value) {
if (value instanceof String &&
!StringUtils.isEmpty(((String) value).trim())) {
this.value = Integer.valueOf(value.toString());
} else if (value instanceof Number) {
this.value = (Integer) value; // <=
}
}
@Override
public IntegerDataObject clone() {
IntegerDataObject clone = new IntegerDataObject();
clone.setValues(this);
return clone;
}
}Here we've got a strange check for Number with a following casting to Integer, as well as to Long and Double in other ValuedDataObject subclasses. Perhaps the developer expected this to have some effect, or, as with primitive types, there would be an appropriate conversion. However, the following program execution actually ends with a ClassCastException:
void main() {
Number integer = 528;
println((Float)integer);
}Here's the result:
Exception in thread "main" java.lang.ClassCastException:
class java.lang.Integer cannot be cast to class java.lang.Float
(java.lang.Integer and java.lang.Float
are in module java.base of loader 'bootstrap')
at Exception.main(Exception.java:3)So, I think we need to reconsider what's going on in these methods.
The PVS-Studio warning:
@Override
protected DmnElement convertXMLToElement(XMLStreamReader xtr,
ConversionHelper conversionHelper) throws Exception {
DecisionService decisionService = new DecisionService();
decisionService.setId(xtr.getAttributeValue(null, ATTRIBUTE_ID));
decisionService.setName(xtr.getAttributeValue(null, ATTRIBUTE_NAME));
decisionService.setLabel(xtr.getAttributeValue(null, ATTRIBUTE_LABEL));
boolean readyWithDecisionService = false;
try {
while (!readyWithDecisionService && xtr.hasNext()) {
xtr.next();
if (xtr.isStartElement() &&
ELEMENT_OUTPUT_DECISION.equalsIgnoreCase(xtr.getLocalName())) {
DmnElementReference ref = new DmnElementReference();
ref.setHref(xtr.getAttributeValue(null, ATTRIBUTE_HREF));
decisionService.addOutputDecision(ref);
} if (xtr.isStartElement() &&
ELEMENT_ENCAPSULATED_DECISION
.equalsIgnoreCase(xtr.getLocalName())) {
DmnElementReference ref = new DmnElementReference();
ref.setHref(xtr.getAttributeValue(null, ATTRIBUTE_HREF));
decisionService.addEncapsulatedDecision(ref);
} if (xtr.isStartElement() &&
ELEMENT_INPUT_DATA.equalsIgnoreCase(xtr.getLocalName())) {
DmnElementReference ref = new DmnElementReference();
ref.setHref(xtr.getAttributeValue(null, ATTRIBUTE_HREF));
decisionService.addInputData(ref);
} else if (xtr.isEndElement() &&
getXMLElementName().equalsIgnoreCase(xtr.getLocalName())) {
readyWithDecisionService = true;
}
}
} catch (Exception e) {
LOGGER.warn("Error parsing output entry", e);
}
return decisionService;
}I wanted to find an issue where the if statement instead of else if would cause the last else block execution even if one of the if blocks worked out. The case is similar to what I found in the GeoGebra project. However, everything here ends with else if, which means that nothing tragic can happen.
As a result, all we get is unpleasant code formatting. To improve readability, we can move if to the next lines or add else. The second option may be better, as it avoids performing multiple pointless CPU operations.
The PVS-Studio warning:
protected void leaveFlowNode(FlowNode flowNode) {
....
// Determine which sequence flows can be used for leaving
List<SequenceFlow> outgoingSequenceFlows = new ArrayList<>();
for (SequenceFlow sequenceFlow : flowNode.getOutgoingFlows()) {
String skipExpressionString = sequenceFlow.getSkipExpression();
if (!SkipExpressionUtil.isSkipExpressionEnabled(
skipExpressionString, sequenceFlow.getId(),
execution, commandContext)
) {
if (!evaluateConditions // <=
|| (evaluateConditions && // <=
ConditionUtil.hasTrueCondition(sequenceFlow, execution) &&
(defaultSequenceFlowId == null ||
!defaultSequenceFlowId.equals(sequenceFlow.getId())))) {
outgoingSequenceFlows.add(sequenceFlow);
}
} else if (flowNode.getOutgoingFlows().size() == 1 ||
SkipExpressionUtil.shouldSkipFlowElement(
skipExpressionString, sequenceFlow.getId(), execution, commandContext)
) {
....
outgoingSequenceFlows.add(sequenceFlow);
}
}
....
}I found a lot of similar code fragments. The condition in if is extremely cumbersome and redundant as it is:
V6007 Expression 'evaluateConditions' is always true. TakeOutgoingSequenceFlowsOperation.java 220
Entering the second expression in the or statement occurs only if evaluateConditions is false. So, the expression on the left is meaningless in the (evaluateConditions && ...) check.
Let's wrap up our tour on potential project bugs for now. You can see that a lot of them were caused by simple carelessness. For the same reason, the devs didn't see them during code review. The size of a single PR can make you never want to open it again, so let's not blame the developers.
Static analysis tools, on the other hand, check code regardless of its size. PVS-Studio helped us find those problematic fragments. Although, that's not all the project has in store. I didn't have to study the code base for a long time to navigate through the project structure, I just ran the analyzer. While full integration into the development process will enable you to immediately detect errors in the code.
Also, you can try PVS-Studio for free :)