Всем привет!
хватит про политику, давайте про полезное)
кейс. есть функция GetThing, внутри проверяется её аргумент на валидность. если что не так — генерится ArgumentException (AE). пока всё ок.
теперь её надо заюзать например внутри хандлера рест-апи ендпойнта. хочется чтоб если извне пришедший аргумент для неё кривой — вернуть 400.
можно сделать кэтч для AE, но это может оказаться AE из более глубоких недр (то есть быть реальной ошибкой и не про наш аргумент который извне).
вводить спец класс ексепшина? это решит проблему — можно кэтчить именно его, но кажется как-то не айс на каждый подобный кейс вводить спец класс ексепшина... или норм?
ещё вариант — в кэтче как-то понимать что AE именно изнутри самой GetThing а не глубже... как-то такое можно?...
и ещё вариант — у AE можно задать paramName и тогда в кэтче ловим и если имя параметра наше, то так и опознаём. уже лучше, но всё равно не 100%, остается слабая вероятность что из недр и имя параметра совпало.
Здравствуйте, MadHuman, Вы писали: MH>кейс. есть функция GetThing, внутри проверяется её аргумент на валидность. если что не так — генерится ArgumentException (AE). пока всё ок. MH>теперь её надо заюзать например внутри хандлера рест-апи ендпойнта. хочется чтоб если извне пришедший аргумент для неё кривой — вернуть 400.
Так делать не надо.
400 означает, что неверны не вообще любые аргументы чего угодно, а конкретно аргументы, переданные клиентом.
То есть хэндлер рест-апи должен проверить аргументы, и только если они в порядке, ехать дальше.
Исключения за пределами первичной валидации — это уже 500.
Покажите ваш код контроллера, который хэндлит рест апи. Какое место там занимает GetThing?
Уйдемте отсюда, Румата! У вас слишком богатые погреба.
Здравствуйте, 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);
}
}
Здравствуйте, 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);
}
Вообще о валидацию много копий сломано,
но общая идея: парсить, а не валидировать
или еще как принцип "Making illegal states unrepresentable"
т.е. для arg1 и arg2 если они логически связаны создать valueobject и в конструкторе проверить валидность, а дальше уже передать нормальный объект
или в нем сделать статик метод подобный тому что выше и сделать конструктор приватным, чтобы не выкидывать исключений из конструктора.
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 тоже норм.
не всегда хорошо чтоб каждое место использование функции вынуждало её менять/рефакторить.
Здравствуйте, 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?
Если сценарий использования меняется, тоти сама функция меняется. Это называется "проектирование сверху вниз"
Здравствуйте, 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 ошибка?
G>3) Хотим чтобы счета при конструировании были существующими. G>Стоит ли в данном случае изнутри БЛ кидать ArgumentException? Нужно ли в таком случае выдавать 400 клиенту? Или это уже 500 ошибка?
Если ошибка в бухгалтерии, то это уже не ArgumentException, а лучше бросать специализированное исключение. И тогда это уже 400, а не 500.
ArgumentException подходит только для самых простых проверок, типа должен прийти емейл, а неожиданно пришёл какой-то мусор. Такие неожиданные исключения можно трактовать наравне с аналогичными исключениями из третьесторонних библиотек и выдавать пользователю 500.
Здравствуйте, MadHuman, Вы писали:
MH>можно сделать кэтч для AE, но это может оказаться AE из более глубоких недр (то есть быть реальной ошибкой и не про наш аргумент который извне). MH>вводить спец класс ексепшина? это решит проблему — можно кэтчить именно его, но кажется как-то не айс на каждый подобный кейс вводить спец класс ексепшина... или норм?
Норм. Если есть куча легаси — можно оба подхода использовать, и спецкласс, и набор well-known исключений.
Здравствуйте, Sinclair, Вы писали:
S>Так делать не надо. S>400 означает, что неверны не вообще любые аргументы чего угодно, а конкретно аргументы, переданные клиентом. S>То есть хэндлер рест-апи должен проверить аргументы, и только если они в порядке, ехать дальше. S>Исключения за пределами первичной валидации — это уже 500.
Гладко было на бумаге. Валидация, зараза, может быть тоже размазана по нескольким слоям. Часть валидации, к примеру, отрабатывается стандартным биндером на основании DataAnnotation, часть в коде контроллера, а часть — в бизнес-логике (к примеру, потому что провалидировать можно только выполнив половину работы).
Здравствуйте, yenik, Вы писали:
Y>ArgumentException подходит только для самых простых проверок, типа должен прийти емейл, а неожиданно пришёл какой-то мусор. Такие неожиданные исключения можно трактовать наравне с аналогичными
Почему 500? 400 означает, что проблема с переданными данными, 500 что это внутренняя проблема сервиса, на которую клиент повлиять не в состоянии. В описанном кейсе проблема именно в переданных данных.
Здравствуйте, gandjustas, Вы писали:
G>Что мешает просто в отдельный метод вынести? Или даже в отдельный класс?
К примеру, для проверки могут потребоваться промежуточные данные, которые затем используются для собственно вычислений. Если выдрать в отдельный класс, то либо эти внутренние данные вылезут аж в контроллер, что резко увеличит объем бойлерплейта, либо тебе придется делать одну и ту же работу дважды.
Ну и отдельный эротичный кейс это когда есть два сервиса, один из которых зовет другой и возвращает его результат наружу с минимальными изменениями (к примеру, просто обогащая данными). Чего и куда будем выносит в этом случае?
Y>>ArgumentException подходит только для самых простых проверок, типа должен прийти емейл, а неожиданно пришёл какой-то мусор. Такие неожиданные исключения можно трактовать наравне с аналогичными
НС>Почему 500? 400 означает, что проблема с переданными данными, 500 что это внутренняя проблема сервиса, на которую клиент повлиять не в состоянии. В описанном кейсе проблема именно в переданных данных.
Согласен. Лично я вообще против выдачи клиенту 500. Если исключение в UI или BL, то лучше его логировать и выдавать 400. Так разработчик хотя бы может предполагать, что исключение в его коде. А 500 — это может быть веб-сервер гавкнулся, или прокси-сервер гавкнулся, или приложение вообще не стартануло нормально.
G>Стоит ли в данном случае изнутри БЛ кидать ArgumentException? Нужно ли в таком случае выдавать 400 клиенту? Или это уже 500 ошибка?
Это общая проблема, если ловим конкретное исключение, то можно 400, если другое 500.
в корке можно возвращать record Result () { record Ok(r):Result(); record Error(exn)}