Язык Java известен широким применением в бизнесе. Бизнес-процессами необходимо управлять, в чём может помочь платформа Flowable, которая, так сложилось, написана на Java, так ещё и с открытым исходным кодом! А значит, мы можем изучить его с помощью PVS-Studio и попробовать найти ошибки.
Упомянутое в аннотации — это во многом это всё, что я могу сказать о проекте, не начав просто пересказывать написанное в Википедии или на официальном сайте. Потому хорошей идеей будет обратиться к нему: Flowable: Business Process Automation | Low-code | Workflow Automation.
Бизнес значит Enterprise. А Java и Enterprise — известная связка! Следовательно, большое количество строк кода совпадает по длине с эссе. В связи с этим мне пришлось заняться своевольным форматированием для того, чтобы сделать код более читаемым. Имейте в виду, что большая часть представленных фрагментов в оригинале, скорее всего, выглядит иначе.
Чтобы убедиться, что мы работаем с Java, предлагаю посмотреть несколько коротких наименований классов:
Но это не всё. Рекордсменом по длине стал класс UpdateChannelDefinitionTypeAndImplementationForAllChannelDefinitionsCmd. А с тестами побеждает это же наименование с дополнительным суффиксом Test.
Посмотрев на Enterprise наименования, предлагаю заглянуть в кодовую базу. Для этого, согласно традиции, воспользуемся PVS-Studio и попробуем найти самое интересное. Надеваем пиджаки и отправляемся в путь.
Начинаем приключение с тестов. Хотя тесты предназначены для поиска ошибок в логике программы, в них допустить ошибку так же просто, как и в обычном коде. Тесты на тесты не пишут, а вот статический анализатор их может проверить.
Причём про поиск ошибок в Unit-тестах мы писали совсем недавно в отдельной статье! Рекомендую тестировщикам к ознакомлению.
А здесь предлагаю взглянуть на следующий фрагмент:
@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);
}
}Тест проверяет время исполнения сервиса. К сожалению, значение durationInMillis всегда равно нулю, поскольку вычитается getEndTime() из getEndTime(). Соответственно, сам тест, проверяющий, что это значение меньше 1500, всегда будет положительным, ведь 0 < 1500, что легко доказать после прочтения Principia Mathematica.
Исправление крайне простое:
long durationInMillis = historicCaseInstance.getEndTime().getTime() –
historicCaseInstance.getStartTime().getTime();Я перезапустил тест с этим исправлением, и он успешно прошёл. А теперь посмотрим на сообщение анализатора PVS-Studio, которое помогло найти проблему:
V6001 There are identical sub-expressions historicCaseInstance.getEndTime().getTime()' to the left and to the right of the '-' operator. ServiceTaskWithFuturesTest.java 76
Далее на очереди легендарный в финтехе BigDecimal:
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(....);
}
}
}
}Рекомендую в код долго не всматриваться, иначе посмотрит в ответ. Нас интересует работа с уже упомянутым BigDecimal. Для выяснения деталей необходимо обратиться к примечанию в JavaDoc:
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.
В этом же документе указана рекомендация к применению BigDecimal.valueOf(double) или new BigDecimal(String). При этом в указанном фрагменте значение парсят в String, а затем вновь в double. Изменится ли от этого что-то? Предлагаю посмотреть интереса ради:
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());
}Да! Для подробностей предлагаю к изучению JEP-477. Хотя на момент написания статьи этот функционал находится в preview feature, его использование в публикации помогает упростить такой простой фрагмент и не писать public static void main(String[] args). Великосимвольных строк нам хватит и в Enterprise названиях классов.
Получаем следующий вывод:
new BigDecimal(double):
0.1000000000000000055511151231257827021181583404541015625
new BigDecimal(double -> string): 0.1
new BigDecimal(double -> string -> double):
0.1000000000000000055511151231257827021181583404541015625
BigDecimal.valueOf(double): 0.1Что ж, от дополнительных преобразований double в строку и обратно поведение не меняется. В любом случае следует использовать либо valueOf, либо конструктор со String аргументом, если мы хотим получить 0.1 из вводимого 0.1 значения (исходя из названия метода, это обработка input variables).
В результате получаем, что можно не только избавиться от потенциально неожиданного значения, но и устранить лишнюю операцию парсинга String к double:
new BigDecimal(inputVariableValue.toString());Напоследок срабатывание анализатора PVS-Studio:
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;
}
....
}Сразу отмечу, что и в оригинале тело цикла пустое (иначе бы там стояли ....).
Уже встречавшаяся ошибка в других проектах — одно из выражений всегда истинно. Речь о s.charAt(i) != ' ' || s.charAt(i) != '\t'.
Для того, чтобы получить ложный результат оператора ИЛИ, необходимо, чтобы символ был одновременно равен двум разным символам. Что, естественно, невозможно.
В результате получается, что цикл выполняется, пока условие i < s.length() является true, и вывод метода — это всегда индекс конца строки, а не символа пробела.
Предупреждение PVS-Studio:
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");
});
}Небольшой круг, и мы вновь встречаем тесты. И вновь они не падают. Сначала посмотрим на срабатывания анализатора PVS-Studio, а затем попытаемся исправить:
Вероятно, разработчик по некой причине ожидал, что вторым аргументом метода isInstanceOfSatisfying является Function<T, Boolean>. Хотя я не знаю, почему могло быть такое ожидание, на деле же метод принимает Consumer<T>. Возможно, дело в простой невнимательности, и разработчику могло показаться, что метод equals эквивалентен AssertEquals.
Для того, чтобы тест упал, необходимо выбросить исключение. Просто обернём вызовы equals в assertThat(....).isEqualTo(....). Например:
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);
}
}
}
}Здесь нарушенная иерархия приводит к тому, что у объекта childNode родителем является он сам. На необходимую зависимость косвенно указывает строка, следующая сразу после – parentNode.getChildNodes().add(childNote). То есть родитель о ребёнке знает, но не наоборот. Любопытно, существует ли риск при итерации по таким нодам выйти в StackOverflowError?
Сообщение анализатора PVS-Studio о странности:
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;
}Знакомимся с неожиданным break, который сводит количество итераций к единице вне зависимости от содержимого в xmlStreamReader. Возможно, так и задумывалось, но тогда следовало использовать if. Очень странная обработка ресурса.
Небольшое обращение к git позволяет отыскать предыдущую версию метода (он был переименован):
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);
}
}В этой версии вместо break используется return, и вновь без условия. Я думаю, что в этом методе просто решили использовать while, потому что вызывается метод xtr.hasNext().
Вне зависимости от изначальной идеи метода, PVS-Studio справедливо ругается, ведь из-за такого кода возникает слишком большое количество вопросов:
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(....);
}
}
}
}Бессмысленная проверка. newRecipients принимает значение либо forceRecipients, либо recipients. Оба перед этим проверяются на isEmpty.
Бессмысленности прибавляет и то, что цикл for (String t : newRecipients) и так не стал бы итерировать по пустой коллекции.
На такой излишний код указал анализатор PVS-Studio:
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;
}
}Срабатывание PVS-Studio:
V6022 Parameter 'additionalData' is not used inside constructor body. CallbackData.java 29
Несколько теоретический случай. Использование разработчиком указанного конструктора может привести к неприятным последствиям, ведь при передаче в него additionalData это значение не будет записано. Произойдёт неприятное рождение NullPointerException. Например, при использовании метода getAdditionalData(), если ожидается, что он не вернёт 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() может вернуть null. И с учётом, что проверка на null в коде происходит, это ожидается. Чтобы прибавить уверенности, посмотрим на этот метод:
public static ProcessEngineConfigurationImpl getProcessEngineConfiguration() {
return getProcessEngineConfiguration(getCommandContext());
}
public static ProcessEngineConfigurationImpl
getProcessEngineConfiguration(CommandContext commandContext) {
if (commandContext != null) {
return ....
}
return null;
}Может ли commandContext быть null? Продолжаем проход по цепочке вызовов:
public static CommandContext getCommandContext() {
return Context.getCommandContext();
}
public static CommandContext getCommandContext() {
Stack<CommandContext> stack = getStack(commandContextThreadLocal);
if (stack.isEmpty()) {
return null;
}
return stack.peek();
}Если stack пуст, то возвращается null. Пока предлагаю на этом остановиться, проверять возможность получения пустой коллекции может быть трудно.
В результате вызов processEngineConfiguration.getHistoryLevel() рискует обернуться NullPointerException.
Предупреждение анализатора:
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;
}
}Странная проверка на Number с последующим кастом в Integer (а также Long и Double в других классах-наследниках ValuedDataObject). Возможно, разработчик ожидал, что это возымеет эффект, или, подобно примитивным типам, произойдёт соответствующая конвертация. Однако на самом деле исполнение следующей программы закончится ClassCastException:
void main() {
Number integer = 528;
println((Float)integer);
}Результат:
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)Потому, думаю, необходимо пересмотреть происходящее в этих методах.
Предупреждения PVS-Studio:
@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;
}Хотел я здесь найти проблему, где использование if вместо else if привело бы к исполнению последнего else блока, даже если один из блоков if отработал (подобно тому, что я находил в проекте GeoGebra). Однако здесь всё как раз оканчивается else if, а значит, ничего трагичного не случится.
В результате получаем лишь неприятное форматирование кода. Чтобы улучшить читаемость, необходимо вынести if на следующие строки, либо добавить else. Второй вариант может быть лучше, поскольку это позволит избавиться от исполнения нескольких бессмысленных операций процессора.
Срабатывания PVS-Studio:
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);
}
}
....
}Я обнаружил большое количество подобных фрагментов кода. Условие в if и так крайне громоздкое, так ещё и излишнее:
V6007 Expression 'evaluateConditions' is always true. TakeOutgoingSequenceFlowsOperation.java 220
Вход во второе выражение в операторе или происходит только в случае, если evaluateConditions является false. Соответственно, в проверке (evaluateConditions && ...) выражение слева бессмысленно.
На этом пока завершаем экскурс по потенциальным ошибкам в проекте. Можно заметить, что значительная их часть произошла банально из-за невнимательности. По этой же причине они не были замечены и на code review. Не стоит винить разработчиков, ведь размеры одного PR могут вызвать желание никогда его не открывать.
А вот инструменты статического анализа проверяют код вне зависимости от его размера. С помощью PVS-Studio получилось отыскать такие проблемные места. При этом это далеко не всё, что есть в проекте. И не пришлось долго изучать кодовую базу, чтобы сориентироваться в структуре проекта, достаточно было лишь запустить анализатор. А интеграция в процесс разработки так и вовсе позволит сразу находить ошибки в коде.
При этом попробовать PVS-Studio можно совершенно бесплатно :)