javajub Jun 8 at 13:41

Этот код на Spring дают на java-собесе в ТБанке и Альфе. Найдёте все 8 багов за 15 минут?

Medium

12 min

10K

Java * IT career

Review

+14

Comments 14

OlegIct Jun 8 at 14:18

Первая — catch (Exception e) глотает абсолютно всё, включая OutOfMemoryError-обёртки

можно поподробнее что имеете в виду? это же не Throwable

mrsantak Jun 8 at 14:22

Формулировка задания и ожидаемые ответы как-то не соответствуют друг другу.

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

Тут прям большой акцент на том, что искать нужно только баги, которые вызывают проблему на проде. И первым же пунктом история про Autowired vs constructor injection, которая не является багой и не вызывает проблем на проде. Это уже скорее стилевая проблема, а такие проблемы довольно неоднозначные. Если уж в них закапываться, то тут вам и отсутствие интерфейса, и Map в качестве body, и error reporting в случае всяких bad request’ов никакой, и метрик нет, и т.д. В общем, тут только на таких проблемах легко за 8 можно выйти.

Ну и ожидать, что кандидат прочитает ваши мысли и догадается, что AntifraudClient - это хождение в соседний сервис по http - это, имхо, такое себе. Точно так же можно и NotificationService заподозрить за хождением в соседний сервис. Тем более, что к интерфейсу AntifraudClient возвращающему “OK” строкой в принципе есть вопросики.

Про кеш вообще бред какой-то, ибо это не кеш, а непонятно что - в него пишут, но не читают. Его так-то просто удалить нужно, а не на кофеин менять.

В целом - неплохо, но разбор оставляет ощущение, что ожидалось, что кандидат должен мысли интервьюера прочитать.

atyumentsev Jun 8 at 18:57

А еще:

этот кэш для каждого инстанса свой,
на api отсутствует версионность,
полное отсутствие контроля входных данных на уровне контроллера,
Сам контроллер, по-хорошему, должен проверять авторизацию, валидировать входные данные, выделять тред на выполнение запроса, но не содержать в себе бизнес-логику,
В примерах упущены фигурные скобки на однострочные блоки кода,
Неспецифический RuntimeException для весьма понятной ошибки. В той же кибане проще будет собирать статистику по специфичным ошибкам,
… и так далее.

В целом - все не так плохо, можно довольно быстро поправить, причем стандартные линтеры (Sonar, Qodana) отловят заметную часть этих проблем.

Бывает, дают такие примеры, что аж руки опускаются, насколько все запутано.

alexander-java Jun 8 at 16:09

А что-то аналогичное есть в виде книги?

alexander-java Jun 8 at 19:45

И, в целом, про банковский домен и обеспечение надёжности?

svz Jun 8 at 19:01

Ощущение, что написано пополам с нейронкой. Сам по себе набор ошибок подобран неплохо. Есть вопросы к реализации.

Я правильно понимаю, что транзакция в create() никогда не откатится, потому что обёртка реагирует на исключения, а вы их все проглотили? Это не упоминается в тексте, хотя кажется серьёзным дефектом.

sendNotification() по неймингу выглядит как отправка сообщения во внешний канал, где транзакция в принципе бессмысленна, а не запись в бд.

Про кеш упомянули - непонятно зачем он и как вообще используется.

Набор ошибок хороший, пример стоит доработать.

alexander-java Jun 8 at 19:48

Насчёт транзакции в create. Исключение может кинутся при попытке зафиксировать транзакцию. Это будет в прокси объекте, и, соответственно, обработчик исключений в этом методе не будет задействован.

Snaret Jun 9 at 00:52

Спасибо за код. Если это действительно из собесов, то тут поле не паханное))

Каюсь, сначала нашел не то что вы указали)

Но предлагаю в список добавить и эти (сори не читал комментарии может уже что-то есть):

Отсутствие версионирования (/v1/payments) - API без версии ломает обратную совместимость при любом изменении. Клиенты, завязанные на этой версии, перестанут работать. Вообще показывает способность к расширению

ResponseEntity<?>
сырой дженерик теряет информацию о типе тела ответа. В Swagger/OpenAPI это превратится в object, клиенты не смогут сгенерировать типизированные модели

@RequestBody Map<String, Object>
теряется контракт API. Нет @Valid, нет документации полей, нет контроля типов. Любое изменение структуры ломается в рантайме ClassCastException

new Date()
java.util.Date - мутабельный, устаревший класс, не рекомендуется к использованию хотя и допустимф

p.setStatus("PENDING")
строковый литерал для статуса - это нонсенс. Опечатка ("PENING") не будет поймана компилятором, думаю выводы сделаете сами)

!"OK".equals(r)
Не совсем плохо но лучше - !Objects.equals("OK", r), более явно проверяет что r != null

Вызов kafka.send без обработки результата
метод асинхронный, но исключения теряются. Нужно добавить callback или использовать синхронную отправку.

RuntimeException - слишком общее исключение. Оно не говорит вызывающему коду (и глобальному обработчику) о том, что именно пошло не так. Лучше - кастомную

Далее более архитектурная проблема:

@Transactional на методе контроллера и отсутствие сервисного слоя
Когда вы ставите @Transactional на контроллер, контроллер берёт на себя ответственность за управление транзакциями - это нарушает принцип SR
Что ждет с таким подходом:
- нормально не протестировать - придется кучу зависимостей тянуть и все либо мокать либо подымать
- невозможно переиспользовать логику, для написания похожей обработки - новая ручка
- расширение приведет только к росту проблем

asamson Jun 9 at 05:17

Вообще, обычно в контроллере нет такой каши - вся логика должна быть вынесена в сервис. И однобуквенных переменных быть не должно. Ну и так, по мелочи - в @RequestMapping в пути отсутствует версия api

Yuri_BY Jun 9 at 07:35

Вот так ответила локальная gemma-4-12B-it-Q5_K_M:

1. Архитектурные проблемы (High Severity)

А. Проблема "Dual Write" (Консистентность данных)

Почему это баг: Код пытается одновременно обновить базу данных (repo.save), отправить сообщение в Kafka (kafka.send) и отправить уведомление (notifications.send). Эти системы не связаны в единую распределенную транзакцию.
Что произойдет в проде: Если repo.save(p) пройдет успешно, но Kafka упадет или произойдет сетевой сбой при отправке сообщения, платеж будет создан в БД, но система обработки заказов/логистики его не увидит. И наоборот: сообщение уйдет, а транзакция в БД откатится. Это приведет к "фантомным" платежам или потере заказов.
Как исправить: Использовать паттерн Transactional Outbox. Сохраняйте событие (сообщение для Kafka) в ту же БД в рамках одной транзакции с платежом. Отдельный сервис (Relay) будет читать эти события и отправлять их в Kafka.

Б. Самовызов транзакционного метода (Self-invocation)

Почему это баг: Метод sendNotification помечен @Transactional(propagation = Propagation.REQUIRES_NEW), но он вызывается внутри метода create через this.sendNotification(p).
Что произойдет в проде: Spring использует AOP-прокси для управления транзакциями. При вызове через this прокси-объект обходится, и аннотация @Transactional игнорируется. Уведомление будет выполняться в той же транзакции, что и основной метод create. Если уведомление упадет, откатится весь платеж (и наоборот).
Как исправить: Вынести sendNotification в отдельный сервис (например, NotificationServiceили новый NotificationManager) и внедрить его в контроллер.

В. Утечка соединений БД (Connection Pool Exhaustion)

Почему это баг: Аннотация @Transactional стоит на всем методе create. Внутри этого метода вызывается antifraud.check(p). Если внешний сервис антифрода работает медленно (например, 2-5 секунд), соединение с базой данных будет удерживаться всё это время.
Что произойдет в проде: При росте нагрузки количество свободных соединений в пуле (HikariCP) быстро закончится. Система начнет выдавать ошибки ConnectionTimeout, и приложение перестанет принимать новые платежи.
Как исправить: Вынести вызов antifraud.check(p) за пределы транзакции. Сначала проверяем антифрод, и только если он "OK", открываем транзакцию для сохранения в БД.

2. Проблемы многопоточности и памяти (Medium/High Severity)

А. Непотокобезопасный кэш и утечка памяти

Почему это баг: private static Map<UUID, Payment> cache = new HashMap<>();
Что произойдет в проде:
1. Race Condition: HashMap не потокобезопасна. Одновременная запись из разных потоков может привести к бесконечному циклу (зависание CPU) или повреждению структуры данных.
2. Memory Leak: Карта никогда не очищается. С каждым новым платежом память будет забиваться, пока приложение не упадет с OutOfMemoryError.
Как исправить: Использовать распределенный кэш (Redis) или локальный кэш с политикой вытеснения и TTL (например, Caffeine или Guava Cache).

3. Проблемы кода и данных (Medium Severity)

А. Использование double для финансовых операций

Почему это баг: Тип double использует двоичную плавающую запятую, которая не может точно точно выразить десятичные дроби (например, 0.1 + 0.2 != 0.3).
Что произойдет в проде: Накопление микроскопических ошибок при расчетах. В финансовой системе это недопустимо и приведет к расхождениям в балансах.
Как исправить: Использовать BigDecimal с указанием RoundingMode.

Б. Слабая типизация входных данных

Почему это баг: Использование Map<String, Object> body и ручное приведение типов (double) body.get("amount").
Что произойдет в проде: Если клиент пришлет число как целое (Integer) в JSON, body.get("amount")вернет Integer, и попытка привести его к double вызовет ClassCastException (500 ошибка). Также нет валидации (например, отрицательный amount).
Как исправить: Создать DTO класс (например, PaymentRequest) с использованием @Valid и аннотаций @Positive, @NotNull.

В. Некорректное логирование и обработка ошибок

Почему это баг: e.printStackTrace() и return ... body(e.getMessage()).
Что произойдет в проде:
1. printStackTrace() не пишет в стандартные системы сбора логов (ELK, Graylog) и не позволяет фильтровать ошибки.
2. e.getMessage() может содержать конфиденциальные данные (SQL ошибки, пути к файлам, данные БД), что является дырой в безопасности.
Как исправить: Использовать SLF4J (log.error("...", e)), создать @ControllerAdvice для обработки исключений и возвращать клиенту понятный, безопасный код ошибки.

Г. Использование System.out.println

Почему это баг: Это синхронная операция, которая может замедлять работу приложения под нагрузкой.
Что произойдет в проде: Засорение стандартного вывода и потеря возможности управлять уровнем логирования.
Как исправить: Заменить на log.info().

Резюме (Checklist для рефакторинга):

Заменить double на BigDecimal.
Заменить Map<String, Object> на PaymentRequestDTO.
Вынести antifraud.check за пределы @Transactional.
Перенести sendNotification в отдельный сервис.
Удалить static HashMap и заменить на Redis/Caffeine.
Реализовать Transactional Outbox для Kafka.
Заменить System.out и printStackTrace на нормальный логгер.
Добавить валидацию входных данных.

Jun 9, 2026, 10:30 AM

31 tokens/sec

(3208 tokens)

Yuri_BY Jun 9 at 07:45

ibm-granite_granite-4.1-8b-Q6_K_L/ibm-granite_granite-4.1-8b-Q6_K_L нашла 10!

Общие рекомендации для продукта

Export

КатегорияРекомендацияПричинаПример реализацииВалидацияДобавить @Valid и DTO для запросаУстранить неопределенность типов и отсутствие проверок@Valid @RequestBody PaymentRequest bodyЛогированиеЗаменить System.out на SLF4JУлучшить контроль уровней и централизацию логовlogger.info("Large payment: {}", amount);ТранзакцииСделать sendNotificationчастью одной транзакции или очистить кэш при ошибкеИзбежать несогласованного состояния@Transactional на sendNotificationКэшИспользовать Spring Cache или ConcurrentHashMapс TTLУстранить гонки и утечки памяти@Cacheable("payments")ИдемпотентностьТребовать Idempotency-KeyПредотвратить дубликаты платежей@RequestHeader("Idempotency-Key")Обработка исключенийРазделить перехваты на конкретные типыПредоставить точные ошибки клиентамcatch (AntifraudException e) { … }Сообщения KafkaОтправлять структурированный JSON без PIIСоответствовать требованиям безопасностиkafka.send("payments", new PaymentEvent(...))Повторы/Обратный отсчетДобавить Retry и Backoff для асинхронных вызововУстойчивость к временным сетевым сбоям@Retryable(...)МониторингЭкспортировать метрики (создание платежей, успех антифрауда, задержки Kafka)Повысить отказоустойчивость и быстрое реагированиеSpring Actuator, Prometheus

Применение этих исправлений сделает контроллер PaymentController более надежным, масштабируемым, соблюдающим требования безопасности и легко тестируемым в производственной среде.

Jun 9, 2026, 10:42 AM

29 tokens/sec

(3457 tokens)

s366315 Jun 9 at 18:50

if (opt.isPresent()) ... else ... вместо .map().orElseGet()

Придираться к таком эту уже жлобство.

amarkevich Jun 10 at 08:58

У HikariCP по дефолту в пуле двадцать соединений

guestfromEarth Jun 10 at 11:09

Благодарю, набор вполне стандартный и полноценный. Тут вдогонку еще валидация amount на уровне API, Map лучше в ДТО. Еще - сервис уведомлений - черный ящик. Важен контекст. Здесь они отправляются сразу, или кладутся куда-нибудь в Redis, например? Тут может потребоваться его отмена в случае отката, то есть компенсирующая операция.