[HttpPost]
public IActionResult Test(SomeParam param)
{
if (!ModelState.IsValid)
{
return View(param);
// return Json({success: false, state: ModelState});
}
dbContext.UpdateData(param);
return RedirectToAction("index");
// return Ok({success: true});
}
Этот код можно улучшить:
- вынести валидацию из тела метода и избавиться от дублирования
if (!ModelState.IsValid) - вернуть код ответа 422
Вынесем валидацию в ActionFilter
Авторизация в ASP.NET MVC настраивается с помощью атрибутов. Сделаем по аналогии и объявим атрибут для валидации:
public enum ValidationResult
{
View,
Json
}
public class ValidationFilterAttribute: ActionFilterAttribute
{
private readonly ValidationResult _result;
public ValidationFilterAttribute(ValidationResult result
= ValidationResult.Json)
{
_result = result;
}
public override void OnActionExecuting(ActionExecutingContext context)
{
if (!context.ModelState.IsValid)
{
if (_result == ValidationResult.Json)
{
context.Result = new ValidationFailedResult(context.ModelState);
}
else
{
context.Result = ((Controller)context.Controller).View(
context.ActionArguments.Values.First());
ValidationFailedResult.SetStatusCodeAndHeaders(
context.HttpContext);
}
}
}
}Добавим код ответа сервера и дополнительную информацию
На stackoverflow обсуждался вопрос «какой код возвращать при ошибке валидации». Семейство 4** выглядит наиболее подходящим. 422 — уже используется Ruby из коробки. ASP.NET MVC не предлагает best practice на этот счет. Не вижу причин не привести в соответствие с Ruby:
internal class ValidationFailedResult: JsonResult
{
public ValidationFailedResult(ModelStateDictionary modelState)
: base(modelState.Select(x => new
{
x.Key,
ValidationState = x.Value.ValidationState.ToString(),
x.Value.Errors
}).ToList())
{
}
public override void ExecuteResult(ActionContext context)
{
base.ExecuteResult(context);
SetStatusCodeAndHeaders(context.HttpContext);
}
internal static void SetStatusCodeAndHeaders(HttpContext context)
{
context.Response.StatusCode = 422;
context.Response.Headers.Add("X-Status-Reason", "Validation failed");
}
}Используем атрибут
ValidationFilterAttribute можно использовать на - методе контроллера
- контроллере
- глобально
Остается только разделить контроллеры, возвращающие
View от Json. Этого можно добиться, создав два базовых класса или добавив соглашение в атрибут, например проверять наличие api в namespace контроллера.Примеры кода приведены для ASP.NET MVC Core. Для ASP.NET MVC придется создать два набора атрибутов для пространства именMvcиHttp, соответственно.
Комментарии (38)
shai_hulud
09.01.2018 20:25binding property list задается только статически (там есть вроде как теоретический путь создать его и динамически но мой вывод 1) только поверх системного кеширования, 2) массивным переписыванием кода, в общем, будем считать что нет методы, тем более что ее действительно нет как реализации кем-то раз написаной). Последствия: хочешь создавать контроллер из модели не через T4 а через run time генерирование (с исп. Execution trees) — вынужден отказываться от Model Binding'ов, брать HttpRequest и по нему сгенерированной функцией строить класс.
А можно подробнее? В вашем комментарии есть несколько ключевых слов «t4, Execution trees» которые вызывают интерес. Но суть я не уловил. По этому вопросы:
> binding property list
Что это?
>хочешь создавать контроллер из модели не через T4 а через run time генерирование
почему не реализовать свой controller selector и свои контроллеры (со своими динамическими экшонами итд) по динамической модели данных?
RomanPokrovskij
09.01.2018 21:59binding property list: msdn.microsoft.com/en-us/library/system.web.mvc.bindattribute(v=vs.118).aspx это который [Bind(«Prop1,Prop2»)]
почему не реализовать свой controller selector и свои контроллеры (со своими динамическими экшонами итд) по динамической модели данных?
На сколько я понимаю IHttpControllerSelector в Core нет, есть другое. Но вообще мне (пока) не нужна его фунциональность. Я готов сжится с таким способом (на большие перестройки не хватает времени).
public class DashboardLine: Controller{ public static meta = new Meta<DashboardLine>(....); private static Task<IActionResult> Edit(); private static Task<IActionResult> EditConfirmed(); public DashboardLine(){ Edit=meta.GenerateEdit(this); EditConfirmed=meta.GenerateEditСonfirmed(this); } public async Task<IActionResult> Edit() { return await Edit(); } [HttpPost, ActionName(nameof(Edit)), ValidateAntiForgeryToken] public async Task<IActionResult> EditFormData() { return await EditConfirmed(); } }
Тут есть аттрибуты роутинга, пусть остаются. Но уже нет model binding. К этому есть два умных вопроса 1) как такой контроллер тестировать 2) как тут обратится к ASP IoC. Вопросы правильные на них есть ответы но я хочу просто показать что типичный T4 контроллер может быть заменен и далее развит подобным способом.
unsafePtr
09.01.2018 23:54Возможно вам await в реализации метода не нужен.
RomanPokrovskij
10.01.2018 12:36Спасибо, я тоже так думаю. Правда это означает убрать и async у контроллера. Но по скольку я плохо представляю как там в ASP TaskSchedule устроен, вдруг он смотрит что Task создан именно что в контроллере и делает что-то специфическое то вот так взять и отказаться от async у контроллера без тестирования боязно напороться на какую-нибудь дырявую абстракцию.
Прочитаю статью — выношу решение.
RomanPokrovskij
10.01.2018 12:11Как жаль что на хабре нельзя изменять комментарии…
Поправляю.
public class DashboardLine: Controller{ public static meta = new Meta<DashboardLine>(..здесь можно обратится к EF модели..); private Func<Task<IActionResult>> Edit; private Func<Task<IActionResult>> EditConfirmed; public DashboardLine(){ Edit=meta.GenerateEdit(this); EditConfirmed=meta.GenerateEditСonfirmed(this); } public async Task<IActionResult> Edit() { return await Edit(); } [HttpPost, ActionName(nameof(Edit)), ValidateAntiForgeryToken] public async Task<IActionResult> EditFormData() { return await EditConfirmed(); } }
Те куски кода которые нужно закешировать и потом использовать уже скомпилированными компилирются при инициализации static meta.shai_hulud
10.01.2018 12:23+1Ну, концепцию я понял. В моем MVC 2-3(я не на коре) можно сделать полностью динамический контроллер со своей диспечеризацией. Либо сделать Route со своим Handler'ом и там уже и контроллера нет.
btw. там реально async/await не нужен, можно прямо вернуть Task.RomanPokrovskij
10.01.2018 13:10Спасибо за совет. Динамический роутинг хорошо — но руки не доходят. Как и проверить можно ли снять async await у контроллера (боюсь дырявых абстракций у ASP TaskSchedule) завидую тем кто не боится (потому что понимает глубже).
shai_hulud
10.01.2018 13:37+1А тут собственно дело обстоит так.
Доreturn await Edit();будет поток исполнения ASP.NET MVC (шедуллер от типа хостинга зависит).
После await будет тот поток исполнения который предоставит Awaitable объект. В случае Task это будет тот же поток в котором завершилась работа, либо ThreadPool в случаеTASK_STATE_THREAD_WAS_ABORTEDилиThread.CurrentThread.ThreadState == ThreadState.AbortRequestedилиTaskCreationOptions.RunContinuationsAsynchronously.
Если заменить это на просто return Edit() для вызывающего метода ничего не изменится (т.е. поток/контекст исполнения для него будет хаотичным и неизвестным). Скорее всего внутри он сам чекает на нужный контекст исполнения и переключается на ожидаемый.
marshinov Автор
10.01.2018 14:17А если выставить ConfigureAwait = false ASP.NET будет всегде брать из ThreadPool?
shai_hulud
10.01.2018 15:45Да я вверху ошибся, забыл что он по дефолту захватывает контекст.
То что я написал вверху относится к .ConfigureAwait(false).
С его отсутствием или .ConfigureAwait(true) будет шедулинг в текущем SynchronizationContext либо в текущем TaskScheduler либо в дефолтном TaskScheduler(ThreadPool).
Получается 'await Edit();' возвращает исполнение в тот контекст в которм был до await. Вот тут я не уверен, но ASP.NET MVC на это пофиг.RomanPokrovskij
10.01.2018 17:21+1Core: blog.stephencleary.com/2017/03/aspnetcore-synchronization-context.html
Прочитав стало яснее, вероятности какого-нибудь супер хитрого поведения TaskSchedule теперь не опасаюсь.
marshinov Автор
10.01.2018 12:36Не совсем понятно, что там за проблема с T4. Если не хочется писать контроллеры, то можно переопределить фабрику и
использовать HandleUnknownAction. Чем этот вариант не подходит?RomanPokrovskij
10.01.2018 12:52Нет никакой проблемы с T4 — тоже метод, (кстати не понятно зачем и в T4 шаблон вставлять атрибут вместо вызрва прямого кода, все же генерируется).
Однако у генерирования ран тайм и T4 разные предназначения.
Ран тайм — можно улучшать поведение десятка однотипных контроллеров без труда.
T4 — одноразовая генерация (практика показывает только так), получили бойлерплейт остальные изменения только руками.
Есть и различия.
В ран-тайм нет возни со сгенерированными файами исходного кода, нет возни с файлами шаблонов, нет T4 — другого языка.
С T4 все быстрее, цель «сгенерировать один раз», не накладывает больших обязательств на гибкость, читаемость и конфигурироемость кода. Нет притензии на новый уровень абстракции (а у программиста есть травмирующий опыт от новых уровней абстракций).
hVostt
10.01.2018 09:42+2Какой тип ответа возвращать, лучше всего определять по заголовкам. Ещё можно смотреть, не является ли запрос AJAX. Как-то не очень хорошо выглядит определение по имени неймспейса, хоть это и похоже на convention, всё же это костыль :)
marshinov Автор
10.01.2018 10:37Заголовки — хороший вариант. К сожалению бывают противные клиенты, которые их не отправляют или отправляют не те. Я привёл пример с явным указанием, что возвращать — json или view как раз потому что не смог подобрать достаточно интуитивного и удобного соглашения. У кого-то может быть другая ситуация, поэтому упомянул и привёл пример в какую сторону можно подумать.
Teo_van_KOT
10.01.2018 10:50+1А что делать, когда в модели есть Select? Вот как быть, когда надо презаполнить справочник? Уже не первый раз вижу подобные решения, но не понимаю, как решить такой вопрос.
marshinov Автор
10.01.2018 12:38Вы что-то такое имеете в виду?
public class Param { public int SomeEntityId { get;set; } // нужно проверить, что сущность с таким id существует }
focuspocus
10.01.2018 18:56Добавить в атрибут параметр — метод, заполняющий модель для вьюхи?
marshinov Автор
10.01.2018 19:05После вашего комментария понял, что имел в виду Teo_van_KOT. Вопрос про <select>, тот что дропндаун, а не LINQ. Ваш ответ подходит, но потащит DependencyResolver.Current в тело атрибута. Есть риск организовать сильную связанность кода.
Видел еще такую альтернативу: можно <select>'ы грузить ajax'ом с фронта. В момент рендеринга View просто <input type=«hidden» class=«dropdown»> делать, а потом на фронте заменять.marshinov Автор
10.01.2018 19:22Поправка. Теперь есть ServiceFilter, просто пробрасываем через IOC зависимости.
Magic_B
10.01.2018 14:59+1Автор, конечно, молодец! НО! Не забывайте, что еще один экземпляр, в данном случае, — фильтр затормозит обработку запроса (поковыряйте исходники MVC). Я фильтры, вообще, не использую. Проще в Вашем случае, раз уж Вы пошли путем упрощения кода, сделать базовый контроллер, где определить метод (пример):
protected TResult IsModelStateIsValid<TModel, TResult>(Func<TModel, TResult> ifModelIsValidFunc) where TResult : IActionResultmarshinov Автор
10.01.2018 15:21Там все достаточно оптимизировано. В Core по крайней мере:
// Execute the filter factory to determine which static filters can be cached. var filters = CreateUncachedFiltersCore(filterProviders, actionContext, allFilterItems); // Cache the filter items based on the following criteria // 1. Are created statically (ex: via filter attributes, added to global filter list etc.) // 2. Are re-usable for (var i = 0; i < staticFilterItems.Length; i++) { var item = staticFilterItems[i]; if (!item.IsReusable) { item.Filter = null; } } return new FilterFactoryResult(staticFilterItems, filters);
Если хочется перформанса тогда можно выкинуть MVC из пайплайна, заменить на Handler / Middleware. В MVC в любом случае будет осуществляться проверка фильтров. Можете поделиться профайлингом на сколько дополнительный фильтр замедляет выполнение?Magic_B
10.01.2018 15:23Дело, даже не в оптимизации, а дополнительном коде (вызовы и экземпляры). Я для себя открыл AspRazorPages! MVC забыт! :)
Magic_B
10.01.2018 15:37Насчет, бенчмарка. Тут смысла мерить нету. Разница будет в десяток-другой вызовов и нескольких экземплярах. Но т.к. мы говорим о веб-приложении, то (моя позиция) необходимо минимизировать любые расходы… И когда вся совокупность будет учтена, получится разница в производительности. А, вообще, пустой запрос в ASP.NET MVC (Release режим) занимает 5 мс, тогда как в AspRazorPages или при обработке через Handler / Middleware, естественно < 1 мс. Таким образом, избавившись от MVC, мы сэкономим 5 мс на каждом запросе. Но, не будем, забывать, что если остальной код не оптимизирован, то и разговаривать не о чем...
marshinov Автор
10.01.2018 15:52Я пишу:
Если хочется перформанса тогда можно выкинуть MVC из пайплайна, заменить на Handler / Middleware
Вы пишите:
AspRazorPages или при обработке через Handler / Middleware, естественно < 1 мс.
Здесь мы согласны.
Но т.к. мы говорим о веб-приложении, то (моя позиция) необходимо минимизировать любые расходы…
Здесь позволю не согласиться. Перформанс — действительно важный критерий качества ПО, но есть еще удобство сопровождения и стоимость разработки / поддержки. Возможно вы и бизнес по разному оцениваете стоимость этих 4мс. Кроме того кроме tfb нужно еще смотреть на throughput. По этой логике нужно срочно все переписать на Netty :) Я бы не был так категоричен в высказываниях.Magic_B
10.01.2018 16:02+1В свое время смотрел эти бенчмарки, и Netty смотрел тоже! :) Вы не менее категорично восприняли. :)
но есть еще удобство сопровождения и стоимость разработки / поддержки
Тут, к сожалению, любые новые технологии или подходы проиграют… И я со своими наработками проиграю. Но и ладно! Когда-нибудь же эти технологии и патерны зайдут в продакшн.
RomanPokrovskij
10.01.2018 18:00Тут, к сожалению, любые новые технологии или подходы проиграют… Но и ладно!
Вот у меня «ладно» и не получается, поскольку хотя я и работаю один (фрилансер, а если бы приходилось через консервативных «архитектов», «сеньеров» и «тим лидов» пробиваться было бы еще нервней) то все равно выходить к людям с новыми идеями и подходами хочется и конструктива хочется, а тебе говорят «перехочется, ты проиграл, минус».marshinov Автор
10.01.2018 18:02Конструктив всегда на стороне бизнеса в данном случае: сколько разработчиков знает ASP.NET MVC? А сколько WebSharper? Кто будет развивать и поддерживать проект, когда ведущий поедет в отпуск?
RomanPokrovskij
10.01.2018 18:40Если считать за честную монету аппеляцию к бизнесу то безусловно новый подход должен делать неважным вопрос «сколько разработчиков» т.е. быть производительным на новом уровне (грубо: меньше кода в два раза, при ясной абстракции).
Но мой опыт говорит, что «бизнес патриотизм» прибежище черти знает кого. Да и бесконечно длинна очередь желающих доказать что они сильнее всех любят бизнес на словах. Люби бизнес на деле — будь фрилансером :)
RomanPokrovskij
Скажу крамольное: весь ASP Model Binding — тупик (да и потоковый json сериалайзер тоже). Аргументирую: binding property list задается только статически (там есть вроде как теоретический путь создать его и динамически но мой вывод 1) только поверх системного кеширования, 2) массивным переписыванием кода, в общем, будем считать что нет методы, тем более что ее действительно нет как реализации кем-то раз написаной). Последствия: хочешь создавать контроллер из модели не через T4 а через run time генерирование (с исп. Execution trees) — вынужден отказываться от Model Binding'ов, брать HttpRequest и по нему сгенерированной функцией строить класс.
Это работает. Показываешь. Фидбек же будет такой — а как же мои атрибутики модел биндинга?
Есть и правильные вопросы, но из-за атрибутов Asp это такой box — что когда надо out of the box дверь находят немногие. В общем я против атрибутов, слишком часто их выставляют каким-то сверхзнанием, слишком многие люди воспринимают фокус за достижение. А «на самом деле» (простите) атрибут это наведение тумана.
Замену
if (!ModelState.IsValid)return View(param);
этим
[ValidationFilter]не считаю достижением, а считаю шагом в сторону. Шагом вперед считал бы поиск регулярного в коде и составление всего метдоа Action динамически (при первом вызове) чему атрибуты не просто мешают — делают не возможным.
unsafePtr
Ментор мой тоже не любит аттрибуты, и предпочитает выносить в базовый контроллер вот такие вот проверки. И уже в самом начале метода вызывать метод из базового класса.
Таким образом у нас метод становиться статическим, и не надо шаманить с аттрибутами.
marshinov Автор
У вас есть 3 контроллера. У первого методы get и post, у второго — get и put. У третьего — put и post. Все методы требуют валидации. Что будете в базовый класс выносить? ThrowIfModelIsInvalid? Это boilerplate, придётся везде его писать. Кроме этого, ошибка валидации — не исключительная ситуация. Нужно возвращать результат, а не кидаться эксепшнами.
unsafePtr
Можно возвращать HttpMessageException со статус кодом и описанием. Сосбтвенно ради чего этот тип исключения и был создан. У нас принято все 4xx ошибки возвращать через исключение. Тут зависит от похода, и как мы относимся к ошибкам в бизнес-логике. Я лишь привел пример, метод для валидации можете назвать по другому и реализовать по своему
Скажите, а почему бы не вынести это в базовый контроллер? Одна строчка аттрибута или одна строчка в начале метода? В обоих случая если забудете написать, то валидация работать не будет.
hVostt
Как уже сказали, и сказали правильно, логическая ошибка не является исключительной ситуацией. Специальное исключение введено как раз для исключительных ситуаций, для случаев, когда невозможно вернуть результат, это может быть и неправильно спроектированный класс и множество других ситуаций. Но пользоваться исключениями для абсолютно нормального процесса проверки ввода пользователя — это big mistake, и недопустимо в общем случае. Нельзя так делать.
hVostt
Атрибут можно бросить на весь контроллер, на базовый контроллер или даже зарегистрировать глобально. Также атрибут можно навесить динамически по соглашениям и проверять различные условия. Всё это выглядит правильно и намного выгоднее, чем ThrowIfModelIsInvalid(model) — это крайне кривой и отвратительный костыль, тем более он бросает исключения, что противоречит всем best practics и рекомендациям. Обычно исключения, это то, что сломалось и это надо чинить. Тут же обычная рядовая валидация.
AxisPod
Видимо ваш ментор не знает для чего используются исключения. Даже в MSDN указано «Represents errors that occur during application execution.», а каким образом неправильно введённые данные являются ошибкой исполнения приложения?
focuspocus
AxisPod hVosttunsafePtr использование исключений — это одна из многих холиварных тем. например, никого не смущают ArgumentNullException или InvalidArgumentException и тп. исключения — это инструмент по управлению поведением приложения отличного от валидного/основного. если брать rest, при тех же ошибках в запросе обычно возвращается не 200 и {IsError=true}, а 4хх. исключения удобны, когда разные проверки скрыты в слоях сервисов и вызовов, чтобы каждый метод не анализировал результат выполнения вложенного, если и так понятно, что продолжать дальше нельзя. бросил свой тип исключения, в обработчике, например, глобальном поймал, залогировал и вернул соответствующий response клиенту. все
ходы записаныошибки анализируются и логируются в одном месте довольно компактным кодом, откуда бы они не прилетели; отдаются в том формате, в котором клиент это поймет. лепота. и, касательно валидации входных парамеров, ActionFilter выглядит лучше, чем ThrowIfModelIsInvalid. имхо.