OpenAPI Generator через призму статического анализатора

• Введение
• Не закрыли поток
• Потеряли инкремент
• Алло, бизнес? Да-да, деньги!
• Логировали, логировали, да не вылогировали
• Неправильное использование Optional до добра не доводит
• Повторки
• Снова про сравнение строк
• Шило на шило
• Аккуратнее с null
• Конец

Хотите посмотреть на ошибки и подозрительные моменты в Java коде? Потерянный инкремент, пустое регулярное выражение, странная санитизация строк — всё это и не только в open source проекте OpenAPI Generator.

Введение

В первую очередь мне хотелось бы представиться и рассказать новому читателю нашего блога о том, что это у нас за формат статей такой.

В подобных статьях мы рассказываем про то, как запускаем статический анализатор PVS-Studio на каком-либо популярном open source проекте и после показываем интересные срабатывания. И в особых случаях через призму некоторых срабатываний рассказываем об интересных аспектах языка, на котором проект написан.

На рассмотрении у нас OpenAPI Generator. Это open source проект, задача которого — автоматическая генерация клиентских библиотек, серверных заглушек, документации и файлов конфигурации на основе спецификации OpenAPI в формате JSON или YAML.

Одна из основных целей проекта — помогать в реализации подхода API-First (или же Contract-First). В рамках этого подхода при разработке клиент-серверных систем сначала формируется контракт взаимодействия между сервисами и клиентами, описанный в спецификации. Далее на основе этого контракта автоматически генерируются клиентские библиотеки и серверные заглушки, после чего разработчики реализуют бизнес-логику соответствующих компонентов. Для работы с Java доступны плагины для сборочных систем Gradle и Maven. Ну а со списком языков и технологий, для которых OpenAPI Generator поддерживает кодогенерацию, можно ознакомиться здесь.

Проект является достаточно популярным: на момент написания статьи у него чуть больше 25000 звёзд на GitHub.

Для проверки мы взяли последнюю версию — v7.19.0. Предлагаю перейти к срабатываниям.

Не закрыли поток

Начнём статью с рассказа о незакрытом файловом потоке. Перед вами фрагмент кода, на котором анализатор выдал срабатывание:

@Override
public File write(
  Map<String, Object> data, 
  String template, 
  File target
) throws IOException {
  ....
  } else {
    InputStream is;
    try {
      String fullTemplatePath = getFullTemplateFile(template);
      is = getInputStream(fullTemplatePath);
    } catch (TemplateNotFoundException ex) {
      is = new FileInputStream(Paths.get(template).toFile());  // <=
    }
    return writeToFile(
             target.getAbsolutePath(), 
             IOUtils.toByteArray(is)
           );
  }
}

Предупреждение PVS-Studio: V6127 The 'is' Closeable object is not closed. This may lead to a resource leak. TemplateManager.java 177

В блоке catch создаётся файловый поток is. Затем в return-выражении его передают в метод IOUtils.toByteArray и... на этом всё. После выхода из метода ссылка на этот открытый файловый поток теряется, т. е. пропадает возможность его закрыть. Как следствие, этот ресурс будет занят до тех пор, пока до удерживающего его объекта не доберётся сборщик мусора. Поскольку это процесс недетерминированный, есть вероятность утечки файлового дескриптора.

Возможно, вы могли подумать, что поток закрывается в IOUtils.toByteArray, но нет.

Для корректной работы с ресурсами существует конструкция try-with-resources, и в аналогичных ситуациях стоит использовать её.

P.S. А ещё, начиная с Java 11, Paths.get считается устаревшим вариантом. Вместо него лучше использовать Path.of.

Потеряли инкремент

Один из базовых теоретических вопросов, с которым сталкивается каждый, кто только начинает учить Java (или другой похожий ЯП), — в чём отличие операции пост-инкремента от пре-инкремента? Следующий фрагмент кода как раз про это:

private String patchPropertyName(
  CodegenModel model, 
  CodegenProperty property, 
  String value, Set<String> composedPropertyNames
) {
  ....
  if (composedPropertyNames != null) {
    String tmpName = name;
    long count = model.allVars.stream()
                              .map(v -> v.name)
                              .filter(n -> n.equals(tmpName))
                              .count() + composedPropertyNames.stream()
                              .filter(n -> n.equals(tmpName))
                              .count();
    if (count > 0) {
      name = name + count++;  // <=
    }
    composedPropertyNames.add(name);
  }
  return name;
}

Предупреждение PVS-Studio: V6123 Modified value of the operand is not used after the increment operation. AbstractCSharpCodegen.java 780

Думаю, вы поняли, в чём дело. Здесь к строке name хотят добавить некое посчитанное значение count. Но, прибавляя count, его увеличивают оператором пост-инкремента, и это значение дальше нигде не используется. То есть, эта операция пост-инкремента не имеет смысла.

Это может быть следствием банальной опечатки и приводить к некорректным результатам. Даже с такими простыми вещами стоит быть очень внимательным.

Алло, бизнес? Да-да, деньги!

@Override
public String toModelName(final String name) {
  // obtain the name from modelNameMapping directly if provided
  if (modelNameMapping.containsKey(name)) {
    return modelNameMapping.get(name);
  }
  String result = sanitizeName(name);
  // remove dollar sign
  result = result.replaceAll("$", ""); // <=
  ....
}

Предупреждение PVS-Studio: V6059 An odd use of a special character in a regular expression. Possibly, it was intended to be escaped. AbstractJuliaCodegen.java 313

И по комментарию над выделенной строкой, и по самому коду понятно, что планировалось удалить символы $ из строки. Но здесь нас ждёт один неприятный момент: этого не произойдёт.

Метод replaceAll принимает первым параметром регулярное выражение и $ — специальный символ, который обозначает конец регулярного выражения. То есть мы получим регулярное выражение, которым ищут буквально ничего.

Чтобы это поправить, необходимо правильно экранировать спецсимвол с помощью \\. В таком случае исправленный вариант будет выглядеть так:

result = result.replaceAll("\\$", ""); // <=

К слову, этот фрагмент кода связан с генерацией для языка программирования Julia.

Логировали, логировали, да не вылогировали

....
LOGGER.warn(
  "Unknown default value for var {0} in class {1}",
  var.name, 
  cm.classname
);
....

Предупреждение PVS-Studio: V6046 Incorrect format. A different number of format items is expected. Arguments not used: 1, 2. ScalaPlayFrameworkServerCodegen.java 407

В проекте в качестве логгера используется slf4j, у которого подстановка значений в строку осуществляется через пустые фигурные скобки {}. Здесь же использовали формат подстановки из C#, а поэтому, как вы могли догадаться, нужные значения в сообщение лога не подтянутся.

В проекте также присутствуют аналогичные места. Разница в том, что в нём уже использовали символы подстановки, как в String.format или же в printf из C:

....
LOGGER.error(
  "Generated field number is "
  "in reserved range (19000, 19999) for %s, %d",
  name, 
  fieldNumber
);
....

Предупреждение PVS-Studio: V6046 Incorrect format. A different number of format items is expected. Arguments not used: 1, 2. ProtobufSchemaCodegen.java 1076

Хотя и здесь используется тот же slf4j.

Ну и помимо неправильных символов подстановки, мы в проекте столкнулись ещё и простыми опечатками. Здесь в логгер забыли добавить необходимый параметр:

....
LOGGER.error(
  "Type {} not yet supported in openapi-normalizer " + 
  "to process OpenAPI 3.1 spec with multiple types."
);
....

Предупреждение PVS-Studio: V6046 Incorrect format. A different number of format items is expected. Missing arguments: 1. OpenAPINormalizer.java 1857

А здесь возможен как лишний плейсхолдер в самом сообщении, так и отсутствующий параметр:

....
List<String> newArgs = new ArrayList<>();
....
String newArg = String.join(" ", newArgs);
LOGGER.trace("new arg {} {}", newArg);
formattedArgs.add(newArg);
....

Предупреждение PVS-Studio: V6046 Incorrect format. A different number of format items is expected. Missing arguments: 2. SpringCodegen.java 1140

Неправильное использование Optional до добра не доводит

private static CodegenParameter pathParamForName(
  CodegenOperation op, 
  String pathParam
) {
  final CodegenParameter 
    param = op.pathParams.stream()
                         .filter(p -> p.paramName.equals(pathParam))
                         .findFirst()
                         .get();
  if (param == null) {
    throw new RuntimeException("Bug: path param " + pathParam + " not found");
  }
  return param;
}

Предупреждение PVS-Studio: V6036 The value from the uninitialized 'op.pathParams.stream().filter(p -> p.paramName.equals(pathParam)).findFirst()' optional is used. ScalaCaskServerCodegen.java 1021

Метод Stream.findFirst возвращает Optional-объект. И вот тут кроется ошибка.

Вместо того, чтобы получить Optional-объект и посмотреть, хранится ли в нём нужное значение с помощью isPresent, его распаковывают с помощью get. В случае, если Optional пуст, до проверки и выброса Runtime-исключения с собственным сообщением мы не дойдём. А всё потому, что:

public T get() {
  if (value == null) {
      throw new NoSuchElementException("No value present");
  }
  return value;
}

Глобально Optional для того и создан, чтобы аккуратно работать с возможно существующими, а возможно и нет значениями. Но здесь же этой аккуратностью пренебрегают. Сам Optional позволяет сделать всё то, что хотели авторы изначально, но более конвенционально и лаконично:

private static CodegenParameter pathParamForName(
  CodegenOperation op, 
  String pathParam
) {
  return op.pathParams.stream()
                      .filter(p -> p.paramName.equals(pathParam))
                      .findFirst()
                      .orElseThrow(() -> new RuntimeException(
                              "Bug: path param " + pathParam + " not found"
                      ));
}

Повторки

В проекте есть достаточно много подобных фрагментов кода:

public Swift6ClientCodegen() {
  ....
  defaultIncludes = new HashSet<>(
          Arrays.asList(
                  "Data",
                  "Date",
                  "URL", // for file
                  "UUID",
                  "Array",
                  "Dictionary",
                  "Set",
                  "Any",       // <=
                  "Empty",
                  "AnyObject",
                  "Any",       // <=
                  "Decimal")
  );
  ....
}

Предупреждение PVS-Studio: V6033 An item with the same key '"Any"' has already been added. Swift6ClientCodegen.java 190

Конкретно в этом проекте вряд ли это серьёзная ошибка, приводящая к страшным и неочевидным падениям, но всё же я хочу сказать про неё пару слов.

По нашему опыту, в некоторых случаях подобные ошибки могут свидетельствовать о том, что в коллекцию добавили не то значение, которое хотели. Часто вместо нужного значения добавляется то, что пишется схожим образом, или то, что не правится после copy-paste. К примеру, ошибка из проекта GeoGebra.

Снова про сравнение строк

Ошибка, с которой мы встречаемся особенно часто. Фрагмент кода:

@Getter private String contentType;
@Getter private String style;
....
public boolean equals(Object o) {
  if (this == o) return true;
  if (o == null || getClass() != o.getClass()) return false;
  CodegenEncoding that = (CodegenEncoding) o;
  return contentType == that.getContentType() && // <=
         Objects.equals(headers, that.getHeaders()) &&
         style == that.getStyle() &&            // <=
         explode == that.getExplode() &&
         allowReserved == that.getAllowReserved() &&
         Objects.equals(vendorExtensions, that.vendorExtensions);
}

Предупреждения PVS-Studio:

V6013 Strings 'contentType' and 'that.getContentType()' are compared by reference. Possibly an equality comparison was intended. CodegenEncoding.java 50

V6013 Strings 'style' and 'that.getStyle()' are compared by reference. Possibly an equality comparison was intended. CodegenEncoding.java 52

И в каждой из статей мы расписываем, почему так лучше не делать. Делали мы это настолько часто, что создали специальный раздел в терминологии. Если объяснять фрагмент в двух словах, то результаты таких сравнений могут быть недетерминированными. Всё зависит от того, как будет определена строка, которую мы используем в методе equals. Для того, чтобы результат такой проверки был детерминирован, строка всегда должна находиться в String Pool. Такое поведение в рамках всего приложения достаточно тяжело гарантировать.

Шило на шило

Снова рассмотрим фрагмент, где в строке заменяют символы:

@Override
public String toDefaultValue(Schema p) {
  ....
  } else if (ModelUtils.isStringSchema(p)) {
    if (p.getDefault() != null) {
      String defaultValue = String.valueOf(p.getDefault());
      if (defaultValue != null) {
        defaultValue = defaultValue.replace("\\", "\\\\")
                .replace("'", "\'");                      // <=
        ....
      }
    }
  }
  ....
}

Предупреждение PVS-Studio: V6009 Function 'replace' receives an odd argument. The '"'"' argument was passed several times. AbstractPythonPydanticV1Codegen.java 191

Трудно понять, что хотел сделать автор. Возможно, планировалось, что формируемая строка будет экранировать символ ', но, спойлер, этого не произойдёт. Чтобы в итоговой строке заменить ' на \', необходимо поправить вызов метода replace:

.replace("'", "\\\\'");

Тогда из строки 'aaa' мы получим строку \'aaa\'. Сейчас же строка 'aaa' остаётся без изменений.

Но если не это было задачей, то тогда мы имеем дело со странным рудиментарным кодом.

Аккуратнее с null

Теперь обратим внимание на возможные разыменования null. Первый фрагмент:

@Override
public String toOneOfName(
  List<String> names, 
  Schema composedSchema
) {
  Map<String, Object> exts = null;
  if (composedSchema != null) {               // <=
    exts = composedSchema.getExtensions();
  }
  if (exts != null && exts.containsKey(X_ONE_OF_NAME)) {
    return (String) exts.get(X_ONE_OF_NAME);
  }
  List<Schema> schemas = ModelUtils.getInterfaces(composedSchema); // <=
  ....
}

Судя по контексту, приходящий в метод параметр composedSchema может быть null. При этом мы передаём его в метод getIntefaces, а там он сразу разыменовывается:

public static List<Schema> getInterfaces(Schema composed) {
  if (composed.getAllOf() != null && !composed.getAllOf().isEmpty()) { // <=
  ....
}

Предупреждение PVS-Studio: V6008 Potential null dereference of 'composedSchema'. RustServerCodegen.java 1422

В проекте есть похожее срабатывание. Давайте перейдём к нему:

@Override
public int compareTo(ModelDepend second) {
  ....
  if (   depend != null 
      && depend.size() != (second.depend == null 
                                            ? 0 
                                            : second.depend.size()) // <=
  ) {
    //LOGGER.debug("Compare " + name + " with " + second.name + "=D"
    //        + (depend.size() - second.depend.size()));
    return depend.size() - second.depend.size(); // <=
  }
  //LOGGER.debug("Compare " + name + " with " + second.name + "=<name>");
  return name.compareTo(second.name);
}

Предупреждение PVS-Studio: V6008 Potential null dereference of 'second.depend'. AbstractAdaCodegen.java 895

В условии есть такой фрагмент:

second.depend == null ? 0 : second.depend.size()

При этом, если всё условие истинно, в блоке then мы без проверок разыменовываем параметр second.depend. Это также может привести к NPE.

По-хорошему, либо стоит учесть, что second.depend может быть null перед разыменованием, либо убрать путающую проверку сверху.

Конец

На этом наша статья подходит к концу. Обо всех рассмотренных срабатываниях мы оповестим авторов OpenAPI Generator через PR и Issue в их репозитории.

Если у вас есть желание попробовать PVS-Studio на своём C, C++, C#, Java, а в скором времени ещё и JavaScript, TypeScript, Go проекте, сделать это вы можете по этой ссылке.

А если вы владелец open source проекта или студент/преподаватель, у нас есть для вас бесплатная лицензия. Подробности по соответствующим ссылкам.

А на этом всё! До скорых встреч!