Стоит ли бросать кастомные ошибки, если этити не найдено в API?

Question

[maxvinogradov]

Думаю, стоит ли кидать свои собственные эксепшены, если что-то не найдено. С одной стороны, это логично и не будет ошибок от Spring data JPA вроде EntityNotFoundException и куда менее понятных ошибок. Но с другой стороны, если с фронта все правильно настроено, то таких ситуаций и не должно быть. Еще и джава будет конкатенировать стринги, которые я передаю в ошибку, даже если ошибки не произошло.

Я только учусь и не понимаю, вот пример двух вариантов кода.

Первый, меня в нем беспокоит только конкатенация строк.

public VoteDTO getByid(int voteId) {
        Vote vote = voteRepository.getById(voteId);
        Optional<Vote> voteOptional = voteRepository.findById(voteId);
        if(voteOptional.isEmpty()) {
            throw new ApiException("Vote with id " + id  + 
                    "is not in DB");
        }
    return toVoteDto(voteOptional.get());
}

И второй без такой проверки ( Spring будет кидать свои эксепшены, если Entity не найдено, но является ли это проблемой? )

public VoteDTO getByid(int voteId) {
    Vote vote = voteRepository.getById(voteId);
    return toVoteDto(vote);
}

Answer 1

[Орхан Гасанлы]

Добрый день!
Наверное, это зависит от того, как вы в команде договоритесь.
Вы можете выбрасывать кастомные эксепшены, а далее ловить их в Controller или ControllerAdvice и отдавать соответствующее сообщение на клиент с кодом ошибки.
Также можно обернуть entity в Optional и выбрасывать исключение orElseThrow() или возвращать другой объект (orElse()) или новый объект и т.д.

Optional<Vote> voteOptional = voteRepository.findById(voteId);
        if(voteOptional.isEmpty()) {
            throw new ApiException("Vote with id " + id  + 
                    "is not in DB");
        }

Можно же упростить:

Vote vote = voteRepository.findById(voteId).orElseThrow(VoteNotFoundException::new);

Но с другой стороны, если с фронта все правильно настроено, то таких ситуаций и не должно быть.

не нужно надеяться на клиент. Например, человек может сам совершить запрос при помощи Postman и передать некорректное значение.

Comments

[maxvinogradov]

Очень благодарен Вам за ответ, но вынужден уточнить один момент. Как Вы считаете, нужно ли создавать много кастомных эксепшенов или создать самые важные, а остальные пускать через общий класс ApiException?

[Erik Mironov]

maxvinogradov, Каждая внештатная ситуация должна быть обработана. Не нужно надеятся на других. Вероятно, как клиент вы бы не хотели получить размытый ответ об ошибке аля "Error, please try agian", либо что еще хуже получить стек-трейс.

[Орхан Гасанлы]

maxvinogradov,

Как Вы считаете, нужно ли создавать много кастомных эксепшенов

Да, нужно.
Вообще каждое исключение должно быть обработано и на клиент нужно отдать свой код ошибки.

а остальные пускать через общий класс ApiException?

Смотрите какая ситуация. Вот, представьте себе, что вы написали некий REST сервис, который используют разработчики. Вы бросите ApiException, с конкретный кодом (например, код ошибки 100 - неверно указан API ключ). Этот же класс будет отвечать за другое исключение (например, код ошибки 100 - срок действия ключа истек).
И другой разработчик не сможет дифференцировать ошибки, так как у вас одно исключение с одним кодом ошибки - 100.
Другое дело, если у вас 2 исключение:
TokenNotValidException (код ошибки 101) и IncorrectTokenException (код ошибки 100) и уже у исключений разные коды и другой разработчик, который будет работать с вашим API сможет у себя их как-то обработать.

Как отметил коллега - Erik Mironov

Вероятно, как клиент вы бы не хотели получить размытый ответ об ошибке аля "Error, please try agian", либо что еще хуже получить стек-трейс.

Чем специфичнее исключение, тем лучше.

А то иногда встречаешься с API, в которых один ответ на все беды и непонятно, что именно ты сделал неправильно...

[Денис Загаевский]

maxvinogradov, не нужно использовать исключения в бизнес-логике. Что-то не найдено - это не нештатная ситуация.

[maxvinogradov]

Денис Загаевский, а если не найдено то, что должно там быть. Значит, неправильно посылаются запросы с фронта, значит, - нештатная ситуация. Я правильно мыслю?

[Денис Загаевский]

maxvinogradov, на мой взгляд, неправильный запрос с фронта это типичная ситуация. Ты должен отдать правильный http код, и опционально тело.
Для сравнения - консольная программа, ты в неё даёшь неправильные входные параметры, она тебе нормально отвечает, что ты дурак, а не падает с исключением.

[Орхан Гасанлы]

Денис Загаевский,

на мой взгляд, неправильный запрос с фронта это типичная ситуация. Ты должен отдать правильный http код, и опционально тело.

Так по идее именно это и происходит.
Выбрасывается исключение, которое обрабатывается на уровне контроллера (Controller) или ControllerAdvice, а затем отдается соответствующий http код с телом на клиент.

Для сравнения - консольная программа, ты в неё даёшь неправильные входные параметры, она тебе нормально отвечает, что ты дурак, а не падает с исключением.

Приложение и не падает с исключением. Исключение выбрасывается, а далее на клиент возвращается ошибка с http кодом и сообщением, что именно не так.

Вот, пример:

@ControllerAdvice
public class DefaultExceptionHandler {

  @ExceptionHandler(PointNotFoundException.class)
  public ResponseEntity<info.md7.routing_gh.exception.ErrorMessage> handlePointNotFoundException(PointNotFoundException ex) {
    String errorMessage = ex.getMessage();
    return new ResponseEntity<>(new info.md7.routing_gh.exception.ErrorMessage(200, errorMessage), HttpStatus.NOT_FOUND);
  }

  @ExceptionHandler(PointOutOfBoundsException.class)
  public ResponseEntity<info.md7.routing_gh.exception.ErrorMessage> handlePointOutOfBoundsException(PointOutOfBoundsException ex) {
    String errorMessage = ex.getMessage();
    return new ResponseEntity<>(new info.md7.routing_gh.exception.ErrorMessage(201, errorMessage), HttpStatus.BAD_REQUEST);
  }

  @ExceptionHandler(PointDistanceExceededException.class)
  public ResponseEntity<info.md7.routing_gh.exception.ErrorMessage> handlePointDistanceExceededException(PointDistanceExceededException ex) {
    String errorMessage = ex.getMessage();
    return new ResponseEntity<>(new info.md7.routing_gh.exception.ErrorMessage(202, errorMessage), HttpStatus.BAD_REQUEST);
  }

}

[Орхан Гасанлы]

а если не найдено то, что должно там быть. Значит, неправильно посылаются запросы с фронта, значит, - нештатная ситуация. Я правильно мыслю?

Смотрите, есть ошибки на стороне клиента, а также на стороне сервера.
Если клиент передал некий id, который отсутствует в БД, то это ошибка на стороне клиента. Отвечаете пользователю http запросом с кодами 4хх. Например, 404.
Если пользователь стучится туда, куда нельзя (нет прав), то например 403 и т.д.
Другое дело, что пользователь отправляет POST запрос на создание некой записи и при создании возникает ошибка (серверная). Тогда отвечаете запросом с кодом http 5хх. Например, 500 internal server error.
Но чтобы дать более специфичный ответ - выбрасываете исключение, которое затем на уровне контроллера или controllerAdvice при помощи ExceptionHandler обрабатываете и отдаете на клиент уже http ошибку с соответствующим кодом и сообщение (Например, пользователь не найден или еще что...). Иногда помимо http ошибки также отдают свой кастомный код внутри сообщения.
Например,
http ошибка 404
Сообщение - Пользователь не найден
Код ошибки - 101 и т.д.

(Посмотрите код выше для наглядности).

Answer 2

[Руслан .]

Своей класс исключения имеет смысл бросать, если у вас для этого класса описан хэндленр, который сформиует правильный по бизнесу http ответ например.
Вот тут описано как это делать
https://spring.io/blog/2013/11/01/exception-handli...

А строки не обязательно конкатенировать, можно использовать StringBuilder или String.format() например.

Comments

[Борис Животное]

А разве в Java строки конкатенируются, если этот код не выполняется?

[Руслан .]

Борис Животное, конечно нет.
Я просто набросал несколько вариантов как можно сформировать строку без конкатенаций.

[Борис Животное]

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

Он пишет:

Еще и джава будет конкатенировать стринги, которые я передаю в ошибку, даже если ошибки не произошло.

Answer 3

[BorLaze]

Стоит ли бросать кастомные ошибки

Да. Фронту совершенно не нужно знать, что там и как на уровне репозитария организовано.

меня в нем беспокоит только конкатенация строк

Так не конкатенируй.

Используй String.format()

throw new ApiException(String.format("Vote with id %d is not in DB", voteId));

либо, если ApiException твой, спрячь эту логику вообще в конструктор

public ApiException(String format, Object... parameters) {
    ...
}

и бросай исключение как

throw new ApiException("Vote with id %d is not in DB", voteId);

Comments

[Борис Животное]

Вы читали в чём его проблема? Он думает, что конкатенация выполняется всегда, даже если условие не срабатывает.

Он написал:

Еще и джава будет конкатенировать стринги, которые я передаю в ошибку, даже если ошибки не произошло.

И как здесь поможет String.format и сокрытие конкатенации или String.format в конструктор, который, вероятно, ещё и тяжелее, чем обычная конкатенация? Я пишу потому что мне стало интересно, что никто не понимает проблему автора конкретно про конкатенацию.

[BorLaze]

КМК, его больше беспокоит вопрос custom exception vs JPA exception, чем конкатенация...

А по поводу ее он беспокоится зря - пока дело до конкретной строки не дойдет, никаких операций выполняться не будет. Мне думается, ТС просто слегка запутался и распространил ошибку вызова логгера а-ля log.debug("Looking for Vote with id " + id) (вот тут, да, склейка строки будет выполняться всегда, вне зависимости от текущего уровня логирования) на любой код вообще.

Answer 4

[Денис Загаевский]

беспокоит только конкатенация строк

...

кидает исключение в бизнес-логике

Л - Логика.

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

Comments

[Антон Воскобоев]

А вот Боб Мартин говорит, что лучше бросить исключение, чем вернуть null. А насчет затратности, преждевременная оптимизация - зло.

[Денис Загаевский]

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

[Антон Воскобоев]

Денис Загаевский, не построенную на исключениях, а бросающую их, когда это уместно. EntityNotFoundException ведь идиоты придумали, ага?

надо вернуть нормально описанный результат.

Это какой, например?

[Денис Загаевский]

Антон Воскобоев, например, sealed class(когда их завезут в джаву), или его симуляцию до того. Either и Optional(nullable) тоже придумали идиоты, ну.

[Антон Воскобоев]

Денис Загаевский,

sealed class(когда их завезут в джаву)

Вот когда/если завезут, тогда и рассмотрим.

Either и Optional(nullable)

Не всегда удобны.

Можно еще добавить Null object, но и исключения часто являются нормальным вариантом.

[BorLaze]

Не нужно использовать исключения для бизнес-логики! Ненайденая сущность - это не исключительная, а штатная ситуация.

Кроме случаев, когда она ДОЛЖНА быть, но ее не обнаруживается.

Например, в дереве не найденный потомок - это да, нормально.
Но вот не найденный родитель - это уже фейл (ессно, если это не корень).

[Денис Загаевский]

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

[maxvinogradov]

Денис Загаевский, прошу прощения, что повторяю вопрос, если исключение бросается, когда, допустим, фронт пытается изменить объект, которого в базе не существует, то это же не штатная ситуация? У исключения быть должны. Правильно я понимаю?
Обычный поиск объекта и т.п. исключения не кидает.

[Денис Загаевский]

maxvinogradov, сильно зависит от того, кто имеет доступ к этому апи. Если любой, то это потенциальный ддос. Если закрыто, то обсуждаемо.