обработка ошибок
От: MadHuman Россия  
Дата: 07.04.22 16:16
Оценка:
Всем привет!
хватит про политику, давайте про полезное)

кейс. есть функция GetThing, внутри проверяется её аргумент на валидность. если что не так — генерится ArgumentException (AE). пока всё ок.
теперь её надо заюзать например внутри хандлера рест-апи ендпойнта. хочется чтоб если извне пришедший аргумент для неё кривой — вернуть 400.

можно сделать кэтч для AE, но это может оказаться AE из более глубоких недр (то есть быть реальной ошибкой и не про наш аргумент который извне).
вводить спец класс ексепшина? это решит проблему — можно кэтчить именно его, но кажется как-то не айс на каждый подобный кейс вводить спец класс ексепшина... или норм?

ещё вариант — в кэтче как-то понимать что AE именно изнутри самой GetThing а не глубже... как-то такое можно?...

и ещё вариант — у AE можно задать paramName и тогда в кэтче ловим и если имя параметра наше, то так и опознаём. уже лучше, но всё равно не 100%, остается слабая вероятность что из недр и имя параметра совпало.

было подобное? какое решение находили?..
Re: обработка ошибок
От: Sinclair Россия http://corp.ingrammicro.com/Solutions/Cloud.aspx
Дата: 07.04.22 17:13
Оценка: 7 (2) +3
Здравствуйте, MadHuman, Вы писали:
MH>кейс. есть функция GetThing, внутри проверяется её аргумент на валидность. если что не так — генерится ArgumentException (AE). пока всё ок.
MH>теперь её надо заюзать например внутри хандлера рест-апи ендпойнта. хочется чтоб если извне пришедший аргумент для неё кривой — вернуть 400.
Так делать не надо.
400 означает, что неверны не вообще любые аргументы чего угодно, а конкретно аргументы, переданные клиентом.
То есть хэндлер рест-апи должен проверить аргументы, и только если они в порядке, ехать дальше.
Исключения за пределами первичной валидации — это уже 500.
Покажите ваш код контроллера, который хэндлит рест апи. Какое место там занимает GetThing?
Уйдемте отсюда, Румата! У вас слишком богатые погреба.
http://rsdn.org/File/5743/rsdnaddict.GIF
Re[2]: обработка ошибок
От: MadHuman Россия  
Дата: 07.04.22 17:29
Оценка:
Здравствуйте, Sinclair, Вы писали:

S>Здравствуйте, MadHuman, Вы писали:

MH>>кейс. есть функция GetThing, внутри проверяется её аргумент на валидность. если что не так — генерится ArgumentException (AE). пока всё ок.
MH>>теперь её надо заюзать например внутри хандлера рест-апи ендпойнта. хочется чтоб если извне пришедший аргумент для неё кривой — вернуть 400.
S>Так делать не надо.
S>400 означает, что неверны не вообще любые аргументы чего угодно, а конкретно аргументы, переданные клиентом.
ну да. кейс как раз и такой. клиент передал неверное значение аргумента.

S>То есть хэндлер рест-апи должен проверить аргументы, и только если они в порядке, ехать дальше.

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


S>Покажите ваш код контроллера, который хэндлит рест апи. Какое место там занимает GetThing?

кода нет, я думаю как его написать. примерно так

//рабочая функция. размещена в другом месте от хэндлера
public static object GetThing(string arg1){

  if ( some condition for arg1) {
     throw new ArgumentException("arg1 invalid");
  }
  //do work
}


//хэндлер для апи ендпойнта
public static object ProcApiRequest(string arg1, string arg2){

  try {
     var result1 = GetThing(arg1);
     var result2 = DoSomeOtherWork(result1);
     return MakeOutPut(result2);
  } catch (ArgumentException ae){
     return new HttpError(400, ae.Message); 
  }
}
Re: обработка ошибок
От: Xander Zerge Россия www.zerge.com
Дата: 07.04.22 17:51
Оценка:
Здравствуйте, MadHuman, Вы писали:

MH>было подобное? какое решение находили?..


После валидации аргумента работаю внутри try-catch, при поимке исключения выбрасываю новое (типа InvalidOperation) с пойманным в InnerException.
Серёжа Новиков,
программист
Re[3]: обработка ошибок
От: gandjustas Россия http://blog.gandjustas.ru/
Дата: 07.04.22 23:21
Оценка: 5 (1) +1
Здравствуйте, MadHuman, Вы писали:

MH>ну да. кейс как раз и такой. клиент передал неверное значение аргумента.

Тогда нужно отфутболить запрос, не доходя даже до контроллера
В современном asp.net для этого очень много средств, смотри https://docs.microsoft.com/en-us/aspnet/core/mvc/models/validation?view=aspnetcore-6.0
Если не хватает атрибутов модели, то используй IValidatableObject

S>>То есть хэндлер рест-апи должен проверить аргументы, и только если они в порядке, ехать дальше.

MH>необязательно. задачу по проверке аргумента можно делегировать GetThing (т.к. она там внутри лучше знает что и как проверять).
Обязательно. То что ты предлагаешь это "проектирование снизу вверх", что почти всегда является антипаттерном.

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

MH>и в хэндлере её дернуть — можно
И нужно, а по возможности вынести в атрибут валидации или в реализацию IValidatableObject.Validate

MH>но ... нехотелось бы.

Ты сам создал ограничения и с ними борешься. Ради чего?


MH>примерно так


MH>
MH>//рабочая функция. размещена в другом месте от хэндлера
MH>public static object GetThing(string arg1){

MH>  if ( some condition for arg1) {
MH>     throw new ArgumentException("arg1 invalid");
MH>  }
MH>  //do work
MH>}


MH>//хэндлер для апи ендпойнта
MH>public static object ProcApiRequest(string arg1, string arg2){

MH>  try {
MH>     var result1 = GetThing(arg1);
MH>     var result2 = DoSomeOtherWork(result1);
MH>     return MakeOutPut(result2);
MH>  } catch (ArgumentException ae){
MH>     return new HttpError(400, ae.Message); 
MH>  }
MH>}
MH>


А чем это принципиально лучше, чем:
public static object GetThing(string arg1){
  //do work
}


//хэндлер для апи ендпойнта
public static object ProcApiRequest(string arg1, string arg2){
  if ( some condition for arg1) {
    this.ModelState.AddModelError(nameof(arg1),"Condition failed");
  }
  if ( some other condition for arg2) {
    this.ModelState.AddModelError(nameof(arg2),"Condition failed");
  }

  if(!this.ModelState.IsValid) {
    return BadRequest(this.ModelState);
  }

  var result1 = GetThing(arg1);
  var result2 = DoSomeOtherWork(result1);
  return MakeOutPut(result2);
}

?
Re: обработка ошибок
От: vaa https://www.youtube.com/playlist?list=PLtrvASfI1KW7VOYRKjglcagQzWLoxlncl
Дата: 08.04.22 01:30
Оценка: +1
Здравствуйте, MadHuman, Вы писали:

MH>Всем привет!

MH>хватит про политику, давайте про полезное)
MH>было подобное? какое решение находили?..

Как уже можно было убедится вариантов масса, выбирай любой.
еще можно без спец искл.
(bool, string) ValidateArgOne(string arg)
{
    .....
    return (false, "Ошибка");
}

Вообще о валидацию много копий сломано,
но общая идея: парсить, а не валидировать
или еще как принцип "Making illegal states unrepresentable"
т.е. для arg1 и arg2 если они логически связаны создать valueobject и в конструкторе проверить валидность, а дальше уже передать нормальный объект
или в нем сделать статик метод подобный тому что выше и сделать конструктор приватным, чтобы не выкидывать исключений из конструктора.
Re[4]: обработка ошибок
От: MadHuman Россия  
Дата: 08.04.22 05:25
Оценка:
Здравствуйте, gandjustas, Вы писали:


G>А чем это принципиально лучше, чем:

G>
G>public static object GetThing(string arg1){
G>  //do work
G>}


G>//хэндлер для апи ендпойнта
G>public static object ProcApiRequest(string arg1, string arg2){
G>  if ( some condition for arg1) {
G>    this.ModelState.AddModelError(nameof(arg1),"Condition failed");
G>  }
G>  if ( some other condition for arg2) {
G>    this.ModelState.AddModelError(nameof(arg2),"Condition failed");
G>  }

G>  if(!this.ModelState.IsValid) {
G>    return BadRequest(this.ModelState);
G>  }

G>  var result1 = GetThing(arg1);
G>  var result2 = DoSomeOtherWork(result1);
G>  return MakeOutPut(result2);
G>}
G>

G>?

тем что, логика проверки аргумента arg1 довольно сложна (это не одна строчка для if как в упрощённом примере) и специфична именно для GetThing, и там уже реализована, тк функция используется не только для хандлера рест-апи ендпойнта.

в предлагаемом вами варианте логика проверки перенесена из GetThing в ProcApiRequest. это было бы ок, если она совсем простая, но она не совсем простая.
проверка внутри GetThing органична и реюзает часть внутренней логики. не так просто её взять и перенести.
и минус — в GetThing проверка всё равно нужно, т.к. GetThing не только для ProcApiRequest.

можно вынести логику валидации в отдельную функцию и реюзать и в GetThing и в ProcApiRequest, так будет уже лучше.
но было ощущение что кэтчем эксепшина из GetThing тоже норм.
не всегда хорошо чтоб каждое место использование функции вынуждало её менять/рефакторить.
Re[5]: обработка ошибок
От: gandjustas Россия http://blog.gandjustas.ru/
Дата: 08.04.22 07:04
Оценка: +2
Здравствуйте, MadHuman, Вы писали:

MH>


MH>тем что, логика проверки аргумента arg1 довольно сложна (это не одна строчка для if как в упрощённом примере) и специфична именно для GetThing, и там уже реализована, тк функция используется не только для хандлера рест-апи ендпойнта.

Что мешает просто в отдельный метод вынести? Или даже в отдельный класс?
Я так понимаю, что основной это "уже там реализована",но это аргумент слабый, так как с современными средствами рефакторинга перемещение кода довольно простое, а других положительных эффектов нет


MH>в предлагаемом вами варианте логика проверки перенесена из GetThing в ProcApiRequest. это было бы ок, если она совсем простая, но она не совсем простая.

Это неважно, рефакторинг extract method выполняется легко.

MH>проверка внутри GetThing органична и реюзает часть внутренней логики. не так просто её взять и перенести.

Это ограничение, которое ты сам себе создал. Кроме того непонятно почему такая нетривиальная логика кидает в итоге ArgumentException, который по сути usage, а не runtime exception.

MH>и минус — в GetThing проверка всё равно нужно, т.к. GetThing не только для ProcApiRequest.

Вовсе не факт. У тебя в каждом методе АПИ своя валидация.

Чтобы руками не дублировать во пользуйся model binding и встроенной валидацией

MH>можно вынести логику валидации в отдельную функцию и реюзать и в GetThing и в ProcApiRequest, так будет уже лучше.

MH>но было ощущение что кэтчем эксепшина из GetThing тоже норм.
MH>не всегда хорошо чтоб каждое место использование функции вынуждало её менять/рефакторить.
А какая разница где код править, в котроллере или GetThing?

Если сценарий использования меняется, тоти сама функция меняется. Это называется "проектирование сверху вниз"
Re[2]: обработка ошибок
От: gandjustas Россия http://blog.gandjustas.ru/
Дата: 08.04.22 09:30
Оценка:
Здравствуйте, vaa, Вы писали:

vaa>Вообще о валидацию много копий сломано,

vaa>но общая идея: парсить, а не валидировать
vaa>или еще как принцип "Making illegal states unrepresentable"
vaa>т.е. для arg1 и arg2 если они логически связаны создать valueobject и в конструкторе проверить валидность, а дальше уже передать нормальный объект
vaa>или в нем сделать статик метод подобный тому что выше и сделать конструктор приватным, чтобы не выкидывать исключений из конструктора.

Я тут задумался о реальном примере
Предположим веб-сервис перевода между счетами в банке
Принимаем на вход
record Transaction(string From, string To, decimal Amount)


1) Хотим чтобы номера счетов соответствовали формату, а сумма была положительная, это легко делается атрибутами:
record Transaction([RegularExpression(...)]string From, [RegularExpression(...)]string To, [RangeAttribute(...)]decimal Amount)


2) Хотим чтобы номера счетов были разными
record Transaction([RegularExpression(...)]string From, [RegularExpression(...)]string To, [RangeAttribute(...)]decimal Amount): IValidatableObject
{
    public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
    {
        if (From == To)
        {
            yield return new ValidationResult("From and To are same", new[] { nameof(From), nameof(To) });
        }
    }
}


3) Хотим чтобы счета при конструировании были существующими.
Это атрибутами уже не сделаешь, да IValidatableObject в данном случае так себе решении, так как надо в базу сходить.
Более того проверка счетов уже часть БЛ, и полученные атрибуты счета будут использоваться в дальнейшем для логики перевода.
То есть примерно тот случай, про который пишет топикстартер.

Стоит ли в данном случае изнутри БЛ кидать ArgumentException? Нужно ли в таком случае выдавать 400 клиенту? Или это уже 500 ошибка?
Re[3]: обработка ошибок
От: yenik  
Дата: 08.04.22 11:23
Оценка:
G>3) Хотим чтобы счета при конструировании были существующими.
G>Стоит ли в данном случае изнутри БЛ кидать ArgumentException? Нужно ли в таком случае выдавать 400 клиенту? Или это уже 500 ошибка?

Если ошибка в бухгалтерии, то это уже не ArgumentException, а лучше бросать специализированное исключение. И тогда это уже 400, а не 500.
ArgumentException подходит только для самых простых проверок, типа должен прийти емейл, а неожиданно пришёл какой-то мусор. Такие неожиданные исключения можно трактовать наравне с аналогичными исключениями из третьесторонних библиотек и выдавать пользователю 500.
Re: обработка ошибок
От: Ночной Смотрящий Россия  
Дата: 08.04.22 11:38
Оценка: +1
Здравствуйте, MadHuman, Вы писали:

MH>можно сделать кэтч для AE, но это может оказаться AE из более глубоких недр (то есть быть реальной ошибкой и не про наш аргумент который извне).

MH>вводить спец класс ексепшина? это решит проблему — можно кэтчить именно его, но кажется как-то не айс на каждый подобный кейс вводить спец класс ексепшина... или норм?

Норм. Если есть куча легаси — можно оба подхода использовать, и спецкласс, и набор well-known исключений.
... << RSDN@Home 1.3.17 alpha 5 rev. 62>>
Re[2]: обработка ошибок
От: Ночной Смотрящий Россия  
Дата: 08.04.22 11:38
Оценка:
Здравствуйте, vaa, Вы писали:

vaa>еще можно без спец искл.


Это самый глупый вариант.
... << RSDN@Home 1.3.17 alpha 5 rev. 62>>
Re[2]: обработка ошибок
От: Ночной Смотрящий Россия  
Дата: 08.04.22 11:38
Оценка: +2
Здравствуйте, Sinclair, Вы писали:

S>Так делать не надо.

S>400 означает, что неверны не вообще любые аргументы чего угодно, а конкретно аргументы, переданные клиентом.
S>То есть хэндлер рест-апи должен проверить аргументы, и только если они в порядке, ехать дальше.
S>Исключения за пределами первичной валидации — это уже 500.

Гладко было на бумаге. Валидация, зараза, может быть тоже размазана по нескольким слоям. Часть валидации, к примеру, отрабатывается стандартным биндером на основании DataAnnotation, часть в коде контроллера, а часть — в бизнес-логике (к примеру, потому что провалидировать можно только выполнив половину работы).
... << RSDN@Home 1.3.17 alpha 5 rev. 62>>
Re[4]: обработка ошибок
От: Ночной Смотрящий Россия  
Дата: 08.04.22 11:38
Оценка: +1
Здравствуйте, yenik, Вы писали:

Y>ArgumentException подходит только для самых простых проверок, типа должен прийти емейл, а неожиданно пришёл какой-то мусор. Такие неожиданные исключения можно трактовать наравне с аналогичными


Почему 500? 400 означает, что проблема с переданными данными, 500 что это внутренняя проблема сервиса, на которую клиент повлиять не в состоянии. В описанном кейсе проблема именно в переданных данных.
... << RSDN@Home 1.3.17 alpha 5 rev. 62>>
Re[6]: обработка ошибок
От: Ночной Смотрящий Россия  
Дата: 08.04.22 11:38
Оценка: +2
Здравствуйте, gandjustas, Вы писали:

G>Что мешает просто в отдельный метод вынести? Или даже в отдельный класс?


К примеру, для проверки могут потребоваться промежуточные данные, которые затем используются для собственно вычислений. Если выдрать в отдельный класс, то либо эти внутренние данные вылезут аж в контроллер, что резко увеличит объем бойлерплейта, либо тебе придется делать одну и ту же работу дважды.
Ну и отдельный эротичный кейс это когда есть два сервиса, один из которых зовет другой и возвращает его результат наружу с минимальными изменениями (к примеру, просто обогащая данными). Чего и куда будем выносит в этом случае?
... << RSDN@Home 1.3.17 alpha 5 rev. 62>>
Re[5]: обработка ошибок
От: yenik  
Дата: 08.04.22 11:45
Оценка:
Y>>ArgumentException подходит только для самых простых проверок, типа должен прийти емейл, а неожиданно пришёл какой-то мусор. Такие неожиданные исключения можно трактовать наравне с аналогичными

НС>Почему 500? 400 означает, что проблема с переданными данными, 500 что это внутренняя проблема сервиса, на которую клиент повлиять не в состоянии. В описанном кейсе проблема именно в переданных данных.


Согласен. Лично я вообще против выдачи клиенту 500. Если исключение в UI или BL, то лучше его логировать и выдавать 400. Так разработчик хотя бы может предполагать, что исключение в его коде. А 500 — это может быть веб-сервер гавкнулся, или прокси-сервер гавкнулся, или приложение вообще не стартануло нормально.
Re[6]: обработка ошибок
От: Ночной Смотрящий Россия  
Дата: 08.04.22 11:50
Оценка:
Здравствуйте, yenik, Вы писали:

Y>Согласен. Лично я вообще против выдачи клиенту 500.


Ну что то же надо выдать, если у тебя внутренние проблемы. К примеру, БД слегка упала.
... << RSDN@Home 1.3.17 alpha 5 rev. 62>>
Re[3]: обработка ошибок
От: vaa https://www.youtube.com/playlist?list=PLtrvASfI1KW7VOYRKjglcagQzWLoxlncl
Дата: 08.04.22 11:51
Оценка:
Здравствуйте, Ночной Смотрящий, Вы писали:

НС>Здравствуйте, vaa, Вы писали:


vaa>>еще можно без спец искл.


НС>Это самый глупый вариант.


А если bool TryParse ?
Re[3]: обработка ошибок
От: vaa https://www.youtube.com/playlist?list=PLtrvASfI1KW7VOYRKjglcagQzWLoxlncl
Дата: 08.04.22 11:54
Оценка:
Здравствуйте, gandjustas, Вы писали:


G>Стоит ли в данном случае изнутри БЛ кидать ArgumentException? Нужно ли в таком случае выдавать 400 клиенту? Или это уже 500 ошибка?


Это общая проблема, если ловим конкретное исключение, то можно 400, если другое 500.
в корке можно возвращать record Result () { record Ok(r):Result(); record Error(exn)}
Re[7]: обработка ошибок
От: yenik  
Дата: 08.04.22 11:59
Оценка:
Y>>Согласен. Лично я вообще против выдачи клиенту 500.

НС>Ну что то же надо выдать, если у тебя внутренние проблемы. К примеру, БД слегка упала.


Разные проблемы — разные коды. Если БД прилегла, то можно выдать 503. Но лучше не выдавать тупо 500.
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.