Java: ревью AuthorSearchService и статистики поиска

Условие

Прочитайте код и подсветите ошибки

@Component public class AuthorSearchService {

@Autowired
private AuthorsRepository authorsRepository;
@Autowired
private StatisticsRepository statisticsRepository;

private AlertRestClient arc = new AlertRestClient();

// В query может быть как полностью ФИО, так и часть имени, например "Вадим Панов" или "панов"
@Transactional
public List<Author> search(String query) {
    List<Author> authors = authorsRepository.findByNameContainingIgnoreCase(query);
    Statistics s = statisticsRepository.findById(query).orElse(null);
    if (s == null) s = new Statistics(query);
    s.setNumbers(s.getNumbers() + 1);
    statisticsRepository.save(s);
    if (s.getNumbers() > 1000 && authors.size() > 1000) {
        System.out.println("too popular search with too much data, sending an alert...");
        arc.send(query, s.getNumbers(), authors.size());
    }

    return authors;
}

}

@Entity @Data public class Author { @Id @GeneratedValue private Long id;

private String name;

@OneToMany(mappedBy = "author")
private List<Book> books;

public Author(String name) {
    this.name = name;
}

}

Ответ

В представленном коде есть несколько проблем и потенциальных улучшений:

1. Инициализация AlertRestClient

   • AlertRestClient arc = new AlertRestClient(); создаётся вручную, а не через Spring. Лучше внедрять через @Autowired или конструктор, чтобы использовать преимущества DI и облегчить тестирование.

2. Отсутствие конструктора без параметров в Author

   • В JPA сущности должен быть публичный или защищённый конструктор без параметров. В классе Author есть только конструктор с параметром name, что может привести к ошибкам при загрузке из базы.

3. Потенциальная проблема с транзакционностью и конкурентностью

   • Метод search помечен @Transactional, но при обновлении статистики:

     Statistics s = statisticsRepository.findById(query).orElse(null);
     if (s == null) s = new Statistics(query);
     s.setNumbers(s.getNumbers() + 1);
     statisticsRepository.save(s);
     

     Если несколько потоков одновременно выполняют этот код с одним и тем же query, может возникнуть состояние гонки, и счётчик numbers будет некорректным. Лучше использовать атомарные операции или блокировки.

4. Использование findById(query) для Statistics

   • Предполагается, что query — это строка, а findById обычно ищет по первичному ключу. Если query не является ID, это может быть ошибкой. Нужно проверить, что Statistics действительно использует query как ID.

5. Отсутствие проверки на null у authors

   • Метод findByNameContainingIgnoreCase должен возвращать список, но если вдруг вернёт null, последующие вызовы вызовут NPE. Лучше использовать Optional или проверять на null.

6. Логирование через System.out.println

   • Для продакшн-кода лучше использовать логгер (например, SLF4J), а не System.out.println.

7. Отсутствие аннотации @NoArgsConstructor в Author

   • Если используется Lombok, стоит добавить @NoArgsConstructor для JPA.

Пример исправления и улучшения инициализации AlertRestClient:

@Component
public class AuthorSearchService {

    @Autowired
    private AuthorsRepository authorsRepository;
    @Autowired
    private StatisticsRepository statisticsRepository;
    
    @Autowired
    private AlertRestClient arc;

    @Transactional
    public List<Author> search(String query) {
        List<Author> authors = authorsRepository.findByNameContainingIgnoreCase(query);
        Statistics s = statisticsRepository.findById(query).orElse(null);
        if (s == null) s = new Statistics(query);
        s.setNumbers(s.getNumbers() + 1);
        statisticsRepository.save(s);
        if (s.getNumbers() > 1000 && authors.size() > 1000) {
            // Использовать логгер
            System.out.println("too popular search with too much data, sending an alert...");
            arc.send(query, s.getNumbers(), authors.size());
        }

        return authors;
    }
}

@Entity
@Data
@NoArgsConstructor
public class Author {
    @Id
    @GeneratedValue
    private Long id;

    private String name;

    @OneToMany(mappedBy = "author")
    private List<Book> books;

    public Author(String name) {
        this.name = name;
    }
}

Также стоит рассмотреть использование оптимистической блокировки или атомарных запросов для обновления статистики.