Шукаємо помилки за допомогою Google Error Prone. Частина 2
Всім привіт. Я Сергій Моренець, розробник, тренер, викладач, спікер та технічний письменник, хочу поділитися з вами своїм досвідом роботи з такою технологією як Google Error Prone. У першій частині я розповів про те, як додати цю технологію у свій проєкт і привів кілька прикладів знайдених проблем. У цій частині я продовжу свою розповідь і поділюся власним bug checker.
Сподіваюся, що ця стаття буде корисною для всіх, хто хоче більше дізнатися про те, як зменшити кількість помилок у своїх проєктах, підвищити якість коду, зробити його більш компактним і сучасним.
Розбираємо попередження
Продовжуємо розбирати попередження в консолі:
[MissingCasesInEnumSwitch] Non-exhaustive switch; either add a default or handle the remaining cases: NOT_DONE (see https://errorprone.info/bugpattern/MissingCasesInEnumSwitch)
Воно говорить про те, що ми не вказали всі значення перерахувань у switch:
switch (status) {
case SEND_CONTINUE -> facesContext.responseComplete();
case SEND_FAILURE -> facesError("Authentication failure");
case SUCCESS -> ec.redirect("index.xhtml");
}
В принципі цього і не потрібно, тому що ми не повертаємо дані із switch. Але по-хорошому варто додати хоча б блок default.
Ще одне зауваження:
[BadImport] Importing nested classes/static methods/static fields with commonly-used names can make code harder to read, because it may not be clear from the context exactly which type is being referred to. Qualifying the name with that of the containing class can make the code clearer. Here we recommend using qualified class: RetryConfig. (see https://errorprone.info/bugpattern/BadImport) Did you mean 'RetryConfig.Builder<Object> builder = RetryConfig.custom().retryExceptions(ConsulException.class, TimeoutException.class);'?
Код виглядає досить безневинно:
Builder<Object> builder = RetryConfig.custom().retryExceptions(ConsulException.class, TimeoutException.class);
Але в Error Prone є своя думка про те, що Builder — це загальновживане слово, а Builder — статичний клас у класі RetryConfig. Тому краще явно вказувати його батька. Але тут замість слідування пораді можна зробити ще елегантніше:
var builder = RetryConfig.custom().retryExceptions(ConsulException.class, TimeoutException.class);
Щоправда, може виникнути питання. А які слова бібліотека вважає загальновживаними? Якщо заглянути до коду класу BadImport, то тут можна знайти і назви класів:
static final ImmutableSet<String> BAD_NESTED_CLASSES = ImmutableSet.of( "Builder", "BuilderFactory", "Callback", "Class", "Enum", "Factory", "Type", "Key", "Id", "Provider");
І назви ідентифікаторів:
private static final ImmutableSet<String> BAD_STATIC_IDENTIFIERS = ImmutableSet.of( "builder", "create", "copyOf", "from", "getDefaultInstance", "INSTANCE", "newBuilder", "newInstance", "of", "valueOf");
Наступна претензія:
[StaticMockMember] @Mock members of test classes shouldn't share state between tests and preferably be non-static (see https://errorprone.info/bugpattern/StaticMockMember) Did you mean 'Transformer transformer;'?
на те, що в тесті статичні поля не повинні бути моком, тому що в такому випадку вони зберігають стану між тестами і порушують їх ізольованість:
@Produces @Mock static Transformer transformer;
Однак у цьому конкретному випадку Error Prone не має рації, тому що не помітив анотацію @Produces, яка вимагає, щоб поле було статичним.
Наступне зауваження говорить про те, що ми зловживали десь copy-paste:
[InvalidParam] Parameter name `city` is unknown. Did you mean user? (see https://errorprone.info/bugpattern/InvalidParam) Did you mean '* @param user'?
Тому що скопіювали код, не змінивши назву аргументу:
public interface UserService {
/**
* Saves(creates or modifies) specified user instance
* @param city
*/
void save(User user);
Ще одне попередження:
[MissingOverride] login implements method in UserFacade; expected @Override (see https://errorprone.info/bugpattern/MissingOverride) Did you mean '@Override @PostMapping(path = "login", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE)'?
Говорить про те, що ми в класі UserController реалізували інтерфейс, але забули поставити анотацію @Override:
@PostMapping(path = "login", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE)
public UserDTO login(@Valid @RequestBody LoginDTO loginDTO) {
Наступне зауваження досить об’ємне:
[StringCaseLocaleUsage] Specify a `Locale` when calling `String#to{Lower,Upper}Case`. (Note: there are multiple suggested fixes; the third may be most appropriate if you're dealing with ASCII Strings.)
(see https://errorprone.info/bugpattern/StringCaseLocaleUsage)
Did you mean 'return new Identifier(name.getText().toUpperCase(Locale.ROOT), name.isQuoted());' or 'return new Identifier(name.getText().toUpperCase(Locale.getDefault()), name.isQuoted());' or 'return new Identifier(Ascii.toUpperCase(name.getText()), name.isQuoted());'?
І говорить про те, що ми не вказали локаль для переведення рядка у верхній регістр:
private Identifier toUpperCase(Identifier name) {
if (name == null) {
return null;
}
return new Identifier(name.getText().toUpperCase(), name.isQuoted());
}
Спершу може здатися, що Error Prone чіпляється, оскільки метод toUpperCase() у класі String викликає себе значенням Locale.getDefault():
public String toUpperCase() {
return toUpperCase(Locale.getDefault());
}
Тобто в принципі так само, як хоче bug checker. Але з іншого боку методи toUpper-Case/toLowerCase працюють по-різному в залежності від локалі. Тому гарною практикою буде явно вказати локаль, що ми зробимо:
return new Identifier(name.getText().toUpperCase(Locale.getDefault()), name.isQuoted());
Більше того, є пропозиція оголосити методи без аргументів deprecated. Хоча в нашому випадку це нічого не змінить, тому що ми використовуємо ASCII-символи для назв ідентифікаторів.
Наступне зауваження найсерйозніше з усіх зустрінутих і фактично є свідченням нашої помилки:
[ArgumentSelectionDefectChecker] The following arguments may have been swapped: 'dto.getApartment()' for formal parameter 'zipCode', 'dto.getHouseNo()' for formal parameter 'street', 'dto.getStreet()' for formal parameter 'houseNo', 'dto.getZipCode()' for formal parameter 'apartment'. Either add clarifying `/* paramName= */` comments, or swap the arguments if that is what was intended (see https://errorprone.info/bugpattern/ArgumentSelectionDefectChecker) Did you mean 'station.setAddress(new Address(/* zipCode= */dto.getApartment(), /* street= */dto.getHouseNo(), /* houseNo= */dto.getStreet(), /* apartment= */dto.getZipCode()));' or 'station.setAddress(new Address(dto.getZipCode(), dto.getStreet(), dto.getHouseNo(), dto.getApartment()));'?
Якщо проаналізувати тип Address:
@Embeddable
public record Address(@Column(name = "ZIP_CODE", length = 10) String zipCode,
@Column(name = "STREET", length = 32) String street, @Column(name = "HOUSE_NO", length = 16) String houseNo,
@Column(name = "APARTMENT", length = 16) String apartment) {
}
То можна переконатися, що першим полем (і аргументом конструктора) йде zipCode, а при створенні цього об’єкта значення передаються у зворотному порядку:
station.setAddress(new Address(dto.getApartment(), dto.getHouseNo(), dto.getStreet(), dto.getZipCode()));
Тут має бути досить серйозна перевірка, щоб виявити таку проблему, потрібно віддати належне Error Prone. Тож перепишемо код так:
station.setAddress(new Address(dto.getZipCode(), dto.getStreet(), dto.getHouseNo(), dto.getApartment()));
Ще одне зауваження досить цікаве:
[JavaTimeDefaultTimeZone] LocalDateTime.now() is not allowed because it silently uses the system default time-zone. You must pass an explicit time-zone (e.g., ZoneId.of("America/Los_Angeles")) to this method.
(see https://errorprone.info/bugpattern/JavaTimeDefaultTimeZone)
Did you mean 'setCreatedAt(LocalDateTime.now(ZoneId.systemDefault()));'?
В більшості випадків розробники викликають метод now() у LocalDateTime без будь-яких аргументів (часового поясу). А це означає, що час прив’язується до time zone вашого сервера. І з цим можуть бути проблеми, якщо ви зберігатимете дані в БД або передаватимете їх в якусь іншу систему, де може бути зовсім інший часовий пояс. У цьому відмінність LocalDateTime від ZonedDateTime, де зберігається і часовий пояс, і time offset. Тому краще явно вказувати time zone.
Ще одне попередження досить своєчасне:
[JavaUtilDate] Date has a bad API that leads to bugs; prefer java.time.Instant or LocalDate. (see https://errorprone.info/bugpattern/JavaUtilDate)
10 років тому в Java 8 з’явилася бібліотека Java Time, після чого використовувати Date/Calendar у новому коді стало поганою практикою. На жаль, досі є бібліотеки, які не перейшли на новий API і все ще вимагають Date. Тут нічого не можна зробити.
Ще одне зауваження:
] [StringCharset] StringCharset (see https://errorprone.info/bugpattern/StringCharset) Did you mean 'resp.getOutputStream().write(response.body().getBytes(UTF_8));'?
Говорить про те, що не варто використовувати рядки як кодування (charsets):
resp.getOutputStream().write(response.body().getBytes("UTF-8"));
Java вже має відповідні константи в класі StandardCharsets, тому перепишемо код як:
resp.getOutputStream().write(response.body().getBytes(StandardCharsets.UTF_8));
Ще одне попередження, важливе для асинхронних операцій:
[FutureReturnValueIgnored] Return value of methods returning Future must be checked. Ignoring returned Futures suppresses exceptions thrown from the code that completes the Future.
(see https://errorprone.info/bugpattern/FutureReturnValueIgnored)
Did you mean 'var unused = kafkaTemplate.send("payments", order.getId(), event);' or to remove this line?
Коли ви викликаєте код, який повертає Future/CompletableFuture:
kafkaTemplate.send("payments", order.getId(), event);
То завершення роботи цього рядка не означає, що повідомлення успішно відправлено та доставлено. Можливо, воно ще навіть не почало вирушати. Тому тут варто додати щось на кшталт:
var future = kafkaTemplate.send("payments", order.getId(), event);
future.whenComplete((sendResult,ex) -> {});
Але в даному конкретному випадку це не має особливого сенсу, тому що ми в тестах використовуємо Awaitility і перевіряємо, що вся операція завершилася успішно:
Awaitility.await("Verifying that order status has changed to PAYED")
.failFast("Order " + order.getId() + " must exist and has status COMPLETED/PAYED",
() -> Optional.ofNullable(ticketService.findOrder(order.getId()))
.map(item -> item.isCancelled() || item.isInError()).orElse(false))
Є попередження, до яких потрібно ставитись із граничною обережністю, наприклад:
[UnusedVariable] The field 'value' is never read. (see https://errorprone.info/bugpattern/UnusedVariable) Did you mean to remove this line?
Незважаючи на свій просунутий евристичний аналіз, Error Prone не може заглянути в таємниці Reflection API і здогадатися, чи використовується деяке поле таким чином, чи ні. З іншого боку, це привід мінімізувати використання Reflection API і звертатися до даних звичних способів.
Інтеграція з Gradle
Тепер спробуємо використати Error Prone через Gradle. Для цих цілей є окремий плагін від Java-спільноти.
Спочатку додамо плагін:
plugins {
id("net.ltgt.errorprone") version "4.0.1"
}
І потім саму залежність:
errorprone("com.google.errorprone:error_prone_core:2.30.0")
На відміну від Maven-плагіна, тут немає необхідності явно передавати компілятору спеціальні опції, це робить сам плагін. Він підтримує і передає всі опції компілятора. Єдине, що потрібно налаштувати — виключити другорядні попередження, як ми це зробили з Maven-плагіном.
import net.ltgt.gradle.errorprone.errorprone
tasks.compileJava {
options.errorprone.disable("EmptyBlockTag")
}
Запускаємо збирання: gradle build, яка виводить ті самі зауваження, що і при Maven-збиранні.
Додаємо свій bug checker
Залишилося одне питання, яке було поставлене на самому початку. Чи можна додати свій bug checker, і якщо так, то як?
- Створити окремий проєкт, до якого додати новий клас у пакеті com.google.errorprone.bugpatterns. Цей клас має успадкувати базовий клас BugChecker. Додати до нього необхідну логіку.
- Покрити новий код тестами.
- Проімпортувати новий проєкт у своєму проєкті та новий bug checker буде підключений автоматично.
У базі Error Prone більше 700 bug patterns, причому вони стосуються тільки JDK і не перевіряють будь-які Java-технології. Цікаво спробувати створити свій bug checker, який, наприклад, перевіряє конфігурацію для Spring Framework. Візьмемо такий приклад:
@Configuration
public class AppConfig {
@Bean
public ProductService productService() {
return new ProductService();
}
}
Це абсолютно коректне оголошення бина в Spring Framework, але в останні роки намітилася тенденція не писати модифікатор доступу для методу productService (або вказувати private). Справа в тому, що модифікатори доступу мають сенс для коду, який використовує чи викликає розробник. Метод productService() викликає сам Spring Framework, тому тут доречно знизити доступ до package/private, щоб якийсь програміст не викликав випадково цей метод.
Тому створимо новий проєкт SpringBugChecker, додамо потрібні залежності:
<dependencies> <dependency> <groupId>com.google.errorprone</groupId> <artifactId>error_prone_annotation</artifactId> <version>2.30.0</version> </dependency> <dependency> <groupId>com.google.errorprone</groupId> <artifactId>error_prone_check_api</artifactId> <version>2.30.0</version> </dependency> <dependency> <groupId>com.google.auto.service</groupId> <artifactId>auto-service-annotations</artifactId> <version>1.1.1</version> </dependency> <dependency> <groupId>org.springframework</groupId> <artifactId>spring-context</artifactId> <version>6.1.12</version> <scope>test</scope> </dependency> <dependency> <groupId>org.junit.jupiter</groupId> <artifactId>junit-jupiter</artifactId> <version>5.11.0</version> <scope>test</scope> </dependency> <dependency> <groupId>com.google.errorprone</groupId> <artifactId>error_prone_test_helpers</artifactId> <version>2.30.0</version> <scope>test</scope> </dependency> </dependencies>
Може виникнути питання. А навіщо нам тут Spring Framework? Він був доданий для використання в тестах, тому що Error Prone не просто парсить Java-код як текст, але вона його компілює. І якщо ми використовуємо в коді анотацію @Bean, то, відповідно, і Spring-залежності повинні бути в classpath. Тепер налаштуємо плагін для компіляції:
<build> <plugins> <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-compiler-plugin</artifactId> <version>3.13.0</version> <configuration> <annotationProcessorPaths> <path> <groupId>com.google.auto.service</groupId> <artifactId>auto-service</artifactId> <version>1.1.1</version> </path> </annotationProcessorPaths> </configuration> </plugin> </plugins> </build>
Навіщо нам потрібен Google Auto Service? Це Google-проєкт, який дозволяє декларативно реєструвати класи, які використовують технологію ServiceLoader для пошуку певних реалізацій. Приступимо до реалізації. Писати свій bug checker не так просто, тому що в ньому потрібно не лише аналізувати вміст Java-коду, але й добре розумітися на тому API, який надає Error Prone. Я рекомендую подивитися на вже готові bug checkers, тому що рішення «в лоб» може зайняти надто багато часу. Ось що в нас вийшло:
@AutoService(BugChecker.class)
@BugPattern(name = "SpringBeanDeclaration", summary = "Spring bean declaration (@Bean) should not be public", severity = BugPattern.SeverityLevel.WARNING)
public class BeanDeclarationChecker extends BugChecker implements BugChecker.MethodTreeMatcher {
private static final String SPRING_BEAN_ANNOTATION = "org.springframework.context.annotation.Bean";
private static final Matcher<MethodTree> springBeanDeclaration =
allOf(
methodHasVisibility(MethodVisibility.Visibility.PUBLIC),
hasAnnotation(SPRING_BEAN_ANNOTATION),
not(Matchers.hasModifier(Modifier.ABSTRACT)),
not(Matchers.hasModifier(Modifier.STATIC)),
not(methodReturns(VOID_TYPE)));
@Override
public Description matchMethod(MethodTree tree, VisitorState state) {
if (tree.getBody() == null) {
return NO_MATCH;
}
if (springBeanDeclaration.matches(tree, state)) {
Set<Modifier> badModifiers = intersection(tree.getModifiers().getFlags(), Set.of(Modifier.PUBLIC));
return buildDescription(tree)
.addFix(removeModifiers(tree.getModifiers(), state, badModifiers)
.orElseThrow(() -> new IllegalStateException("Invalid bug checker configuration. No public modifier in the method: "
+ tree.getName())))
.build();
}
return Description.NO_MATCH;
}
}
Розберемо його по порядку:
- @AutoService потрібний для реєстрації цього класу через ServiceLoader.
- springBeanDeclaration — це набір (композиція) умов, які перевіряються при аналізі нашого методу. Метод має бути публічний, не абстрактний, не статичний, мати анотацію @Bean і не void як тип, що повертається.
- matchMethod — метод, який викликається для аналізу кожного методу у наших класах. Якщо у нашого методу немає тіла, то жодних змін не потрібно.
- Далі ми перевіряємо сигнатуру методу на наявність умов із пункту 2 і якщо все збігається, то дістаємо з методу його публічний модифікатор.
- І наприкінці генеруємо зміни як видалення цього модифікатора (тобто слова public).
Тепер займемося тестами. Додамо новий клас BeanDeclarationCheckerTest:
class BeanDeclarationCheckerTest {
private final BugCheckerRefactoringTestHelper testHelper =
BugCheckerRefactoringTestHelper.newInstance(BeanDeclarationChecker.class, getClass());
Тут дуже зручно використовувати спеціальний TestHelper від Error Prone для більш компактної вказівки очікуваного та реального результату. Тепер додамо два тести, які перевіряють, що наш bug checker не повинен відпрацювати, якщо немає анотації @Bean або тип повертається void:
@Test
void matchMethod_emptyBody_noMatch() {
final String classBody = """
public class AppConfig {
public void doSomething() {
}
}
""";
testHelper
.addInputLines("in/AppConfig.java",
classBody)
.addOutputLines(
"out/AppConfig.java",
classBody)
.doTest();
}
@Test
void matchMethod_noReturnValue_noMatch() {
final String classBody = """
public class AppConfig {
public void doSomething() {
int i = 2;
}
}
""";
testHelper
.addInputLines("in/AppConfig.java",
classBody)
.addOutputLines(
"out/AppConfig.java",
classBody)
.doTest();
}
Зверніть увагу, що потрібно щоразу передавати назву файлу/класу та його тип (in або out) як рядок у спеціальному форматі. Потім додамо тест на той випадок, коли потрібні зміни:
@Test
void matchMethod_springBean_replacementAdvised() {
testHelper
.addInputLines("in/AppConfig.java",
"""
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
@Configuration
public class AppConfig {
@Bean
public String createBean() {
return "";
}
}
""")
.addOutputLines(
"out/AppConfig.java",
"""
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
@Configuration
public class AppConfig {
@Bean
String createBean() {
return "";
}
}
""")
.doTest();
}
Запускаємо тести. Усі вони проходять успішно. Перебираємо проєкт, і в папці tar-get/classes/META-INF/services знаходимо файл com.google.errorprone.bugpatterns.BugChecker з таким вмістом:
bug.checker.spring.BeanDeclarationChecker
Саме його і використовуватиме Google Error Prone для пошуку сторонніх bug checkers. Тепер додаємо залежність (spring-bugchecker) у конфігурацію для maven-compiler-plugin у наших основних проєктах:
<annotationProcessorPaths> <path> <groupId>com.google.errorprone</groupId> <artifactId>error_prone_core</artifactId> <version>2.30.0</version> </path> <path> <groupId>error.prone</groupId> <artifactId>spring-bugchecker</artifactId> <version>1.0</version> </path> </annotationProcessorPaths>
Додаємо найпростішу Spring-конфігурацію:
@Configuration
public class AppConfig {
@Bean
public String sampleBean() {
return "";
}
}
І запускаємо збірку mvn clean install:
[WARNING] AppConfig.java:[10,19] [SpringBeanDeclaration] Spring bean declaration (@Bean) should not be public
(see https://errorprone.info/bugpattern/SpringBeanDeclaration)
Did you mean 'String sampleBean() {'?
Ну а якщо ми прибираємо модифікатор public в sampleBean(), то попередження більше не показується.
Висновки
Підсумовуючи, можна сказати, що інтеграція з Error prone не викликає труднощів. Причому для Gradle-проєкту вона набагато простіша, ніж для Maven. Завдяки великій (>700) базі bug patterns ця бібліотека дозволяє знайти помилки, які важко визначити при code review або запуску тестів. Конфігурація дозволяє змінити рівень критичності будь-якого bug pattern (error, warning) чи вимкнути його. Якщо ви не хочете вручну виправляти помилки, можна запустити збірку в режимі patching і буде згенерований файл error-prone.patch з усіма змінами.
Про те, що ця бібліотека допомагає виправляти помилки, свідчить і той факт, що її автор компанія Google збільшує винагороду за знайдені помилки у своїх продуктах. Якщо сума збільшується, значить помилок поменшало і їх важче знайти.
Ви також можете додати свій bug checker/pattern та використовувати його в будь-якому своєму проєкті.
Немає коментарів
Додати коментар Підписатись на коментаріВідписатись від коментарів