Еще немного о валидации в ASP.NET

[Tantrido]

Круто!

[mayorovp]

Func<Type, object, bool> — ужасный тип для сервиса. Надо либо напрямую обращаться к DbContext в атрибуте — либо заводить именованный делегат или интерфейс для сервиса.

Кстати, я бы в таких случаях использовал не столько валидатор, сколько биндер. Потому что раз уж все равно обращения к базе не избежать — почему бы не вытащить из нее сразу всю сущность и не поместить в модель?

Как-то так в идеале должно выглядеть:

public class MoveProductParam
{
   [ResolveEntity]
   public Product Product {get; set; }

   [ResolveEntity]
   public Category Category {get; set; }
}

[marshinov]

Func<Type, object, bool> — ужасный тип для сервиса. Надо либо напрямую обращаться к DbContext в атрибуте — либо заводить именованный делегат или интерфейс для сервиса.

Не факт, что вы не захотите делать это через Dapper или еще как-то. Завист от проекта.
Func<Type, object, bool> — пример. Какую зависимость использовать — вам решать.

Байндер такой у меня тоже завалялся. Он сильно старый с reflection'ом и его еще допилить нужно. Только в виде идеи:

    public class EntityModelBinder : DefaultModelBinder
    {
        public override object BindModel(ControllerContext controllerContext, ModelBindingContext bindingContext)
        {
            var model = base.BindModel(controllerContext, bindingContext);
            if (model == null || !(model is EntityBase)) return null;

            var props = ((EntityBase) model).GetDependentProperties();

            var dbContext = DependencyResolver.Current.GetService<DbContext>();
            foreach (var propertyInfo in props)
            {
                var id = controllerContext.HttpContext.Request[propertyInfo.Name];
                if (string.IsNullOrEmpty(id)) continue;
                
                var fkValue = dbContext.Set(propertyInfo.PropertyType).Find(int.Parse(id));
                if (fkValue == null) continue;

                propertyInfo.SetValue(model, fkValue, null);
                if (bindingContext.ModelState[propertyInfo.Name].Errors.Any())
                {
                    bindingContext.ModelState[propertyInfo.Name].Errors.Clear();
                }
            }

            return model;
        }
    }

[mayorovp]

Если я захочу делать через Dapper вместо EF — мне придется переписывать одинаковый объем кода независимо от того где я буду этот код держать — в валидаторе или в сервисе.

Сервис может понадобиться если я захочу иметь возможность свободно переключаться между Dapper и EF… но, исходя из моего опыта, это очень глупая идея: подобные общие интерфейсы обычно наследуют плохие стороны обоих решений.

Что же до вашего EntityModelBinder — он делает слишком многое. Я имел в виде простой поиск сущности по Id. Точно то же самое что вы делаете в валидаторе — только вместо Any использовать First.

[mayorovp]

UPD: значение лучше брать из ValueProvider, а не из Query

[mediarium]

Возможно Я не понял, какую именно проблемы вы пытаетесь решить, но весь этот код не требуется если использовать FOREIGN KEY Constraints при проектировании структуры БД.

[unsafePtr]

И пользователь получит SQL ошибку. Лучше вернуть человекоподобную ошибку что бы клиент знал что делать.

[mediarium]

О какой SQL ошибке идёт речь? При правильной настройке FOREIGN KEY (с DELETE/UPDATE) описанная в статье ситуация в принципе не возможна, т.к. за гарантированным наличием/отсутствием данных в парах будет следить сама БД.

[mayorovp]

Каким образом она за этим будет следить? Разве не выкидывая ошибку в ответ на некорректные операции?

[mediarium]

Ещё раз спрашиваю — о какой SQL ошибке идёт речь?

[mayorovp]

О вот этой:

Конфликт инструкции INSERT с ограничением FOREIGN KEY "FK_Vehicle_Request". Конфликт произошел в базе данных "RDSv9", таблица "dbo.Request", column 'RequestID'.

Так вот: пользователь такого сообщения видеть не должен.

[mediarium]

Э-эм, а как она утечёт пользователю, если ваш код обязан проверять результат операции вставки данных в БД? Вы же наверняка про UPSERT + транзакции знаете :)

В статье ведь описана ситуация проверки связки данных двух таблиц через третью? Если данные (как у автора) уже есть в БД, то FOREIGN KEY будут автоматически обновлять/удалять зависимости и все описанные проверки в принципе не требуются.

[mayorovp]

Итак, ваш код проверил результат вставки данных в БД. Проверка показала что произошла ошибка.

Что вы скажете пользователю?

[mediarium]

Очевидно то, что “данные не удалось сохранить”.

Лучше расскажите как вы объясните пользователю, что у вас Product в БД привязаны к несуществующим Category, а Category содержат несуществующие Product?

[mayorovp]

А откуда у меня в базе возьмутся несогласованные данные? Или вы думаете, что валидация — это замена внешним ключам, а не дополнение к ним?

[mediarium]

Вот и мне интересно откуда у автора в БД взялись несогласованные данные, для которых требуется обязательная дополнительная валидация ключей “кочующая из одного файла в другой” :)

[mayorovp]

Э, постойте. Кто вам сказал что они у него в БД?

Валидация всеми нормальными программистами всегда делается для входных данных, а не для хранимых.

Конкретно же тот механизм, который тут обсуждается — это встроенное решение по валидации входных моделей в ASP.NET MVC.

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

[mediarium]

Ага, а DbContext для красоты написан :)

Не стоит подменять контекст статьи собственными размышлениями на тему того, о чем Я не писал.

[mayorovp]

Ну разумеется. Чтобы проверить существование чего-то в базе — надо сделать запрос в базу.

Но где вы увидели в коде валидацию данных, загруженных из базы?

[mediarium]

Простите, а вот это что? Разве не валидация данных, загруженных из базы?

if(!dbContext.Products.Any(x => x.Id == par.ProductId))
    return BadRequest("Product not found");

if(!dbContext.Categories.Any(x => x.Id == par.CategoryId ))
    return BadRequest("Category not found");

[mayorovp]

Лол, нет. Это валидация свойств par.ProductId и par.CategoryId, которые являются входными данными.

В процессе валидации используется запрос к базе.

[mediarium]

Вы сами себе противоречите :)

> Но где вы увидели в коде валидацию данных, загруженных из базы?

> В процессе валидации используется запрос к базе.

[mayorovp]

А что если приложение должно уметь работать вообще без бд?

Тогда надо сменить реализацию на что-то, что не использует БД.

Нет смысла пытаться писать код который может использовать любую ORM — потому что такой код сможет работать лишь с общим подмножеством всех OPM, что лишает затею со сменой ORM какого бы то ни было смысла. Аналогично и с БД.

[mayorovp]

Можно и так делать — это не помешает показанному здесь подходу к валидации. Получится как-то так в итоге:

public class MoveProductParam
{
   [EntityId(typeof(IProductRepository))]
   public ProductId {get; set; }

   [EntityId(typeof(ICategoryRepository))]
   public CategoryId {get; set; }
}

[mayorovp]

Ну и что толстого и уродливого в двух строках кода?

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

В противном случае вы заменяете ТолстымУродливыйКонтроллер на ТолстуюУродливуюБизнесЛогику.

[mayorovp]

Зачем вообще нужен контроллер из одной строчки?

Вы практически за бесплатно закладываете фундамент для масштабирования в будущем. Достаточно легко можно будет перенести денормализированные данные в хранилище, оптимизированное под чтение и внедрить CQRS.

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

А масштабирования в будущем может в итоге так и не случиться.

[mayorovp]

Можно покрыть бизнес-логику юнит тестами, что гораздо проще и быстрей чем тесты через хттп запросы.

Кстати, контроллер тоже при желании можно покрыть тестами.

[marshinov]

Вы прыгаете с UI сразу в Persistence слой, минуя бизнес логику.

Ну и что? Какая бизнес-логика в наличие или отсутствии в БД записи? Строго говоря, откуда взялись эти id? Либо у нас ошибка и мы передаем в UI неактуальные данные, либо запрос сфабриковали. Pro100Oleh ниже приводит гораздо более убедительные аргументы и указывает на потенциальные проблемы. Кроме транзакционности добавляются потенциальные лишние запроы к БД. Вот эти проблемы действительно стоят внимания.

Вы практически за бесплатно закладываете фундамент для масштабирования в будущем. Достаточно легко можно будет перенести денормализированные данные в хранилище, оптимизированное под чтение и внедрить CQRS.

Это не так. Если у вас синхронный CRUD, но вы вместо контроллера написали запросы в квейри / командах ничего не изменилось, просто называется иначе. Тот CQRS, что асинхронный обычно требует принципиально иного подхода к проектированию системы. Грег Янг вообще не рекомендует использовать CQRS как top level architecture.

[marshinov]

Тут смешаны слои. Слой UI, слой бизнес-логики и слой хранения данных. На уровне контроллера нельз провалидировать ничего, кроме формата запроса. Наличие всех обязательных полей, соответствие типам и форматам (положительные целые/UUID-строки для идентификаторов, валидный email, etc).
Валидация наличия категорий и товаров — это правило бизнес-логики и место ему в бизнес-слое.

Здесь вы правы. Но если кроме валидации изменить модел-байндинг, то все получается логично: раз в контроллер приходят не DTO, а сразу Entity, то и валидацию следует произвести там же. Конкретно в данном случае я вижу опасность не в перекрытии слоев, а в не стандартности решения. Ну и ошибки придется искать в двух местах.

Биндинг моделей к параметрам контролера прибивает вас гвоздями к фреймворку

Не прибивает. Func<Type, object, bool> и Func<Type, object, object> соответствуют следующему php-коду:

function(Type $type, object obj): bool {
   //...
}

function(Type $type, object obj): obj {
   //...
}

Вместо них можно инжектировать сервисы / репозитории / что угодно. Для примера я использовал просто делегат (функцию). Кстати, dbContext уже реализует репозиторий, поэтому на практике эта абстракция в .NET смысловой нагрузки не несет: IQueryable — протекающая абстракция by design, потому что все выражения может разобрать только in-memory реализация, которая убивает весь смысл интерфейса.

Биндинг моделей к параметрам контролера прибивает вас гвоздями к фреймворку

Не прибивает. В популярных ORM на .NET достаточно давно используются POCO. Т.е. — сущности — это просто классы. Они не зависят от фреймворка.

зы. Ну и от dbContext коробит. А что если приложение должно уметь работать вообще без бд? Или использовать редис как хранилище? Получается мы снова жестко прибиты к EF.

Обратите внимание, dbContext не инжектится в атрибут. Я работаю с делегатом. Конкретная релизация настривается в IOC-контейнере.

[mayorovp]

Я о байндинге параметров контролера, когда идентификаторы в хттп-запросе автоматически превращаются в сущности из слоя хранения данных.

Так превращайте их автоматически в сущности доменной модели или в сущности слоя бизнес-логики.

И, тут я не уверен, этот байндинг часть фреймворка или часть языка? Если часть фреймворка, то не будет работать с другим без перепиливания.

Если вы пишите приложение так, что в любой момент можете сменить фреймворк или вовсе обойтись без оного — то зачем вам вообще фреймворк?

Глупо не использовать возможности фреймворка только потому что вы боитесь использовать возможности выбранного вами же фреймворка.

[mayorovp]

То есть чтобы вдруг не пришлось переписывать завтра — надо тратить столько же усилий сразу же? :)

[mayorovp]

Интерфейсы, классы, защитные проверки, тесты — все это тоже код, в вашем варианте его намного больше.

[mayorovp]

Тоесть вы пишите без тестов?

Разумеется, я не пишу тесты на несуществующие классы. И еще не пишу тесты на тривиальный код.

Но в моем случае валидация типов\форматов будет в конструкторе команды, а валидация бизнес-логики в хендлере этой команды, а не скопом в контроллере.

То есть получить все ошибки пользователь сможет только со второй попытки? Пока не исправит формат данных — ошибки в логике показаны не будут?

[marshinov]

Кстати, интересная тема: коды ответов. Если мы запрашиваем GET: /category/5 и такой категории нет принято возвращать 404 или 410. А вот если POST: { categoryId: 5, someData: ...} я бы возвращал 422, чтобы четко показать: такой метод существует, но шлешь ты мне хрень.

Семантика валидации БЛ одинаковая: нет такой сущности. А вот код ответа — разный. Если вы возвращаете результат валидации из бизнес-слоя, скажем NotFoundResult, дополнительно обрабатываете в контроллере, зная контекст запроса?

[marshinov]

UserNotFound, ProductNotFound, OrderCantBePaid, ProductOutOfStock — это типы исключений или возвращаемых из CommandHandler значений?

[marshinov]

А вот эти аргументы можете прокомментировать? Считаете, что ничего страшного: exceptions for control flow?

[Pro100Oleh]

Стоит заметить что ваше решение плохо работает когда надо прикрутить транзакции. А имя вашего класса MoveProductParam как бы подразумевает что транзакция бы не помешала. Можно бы усложнить велосипед и прикрутить транзакцию к веб контексту, но в голове уже возникает троллейбус из хлеба. Но что делать когда нужно несколько транзакций на один реквест?
Правильно заметили выше — намешаны слои. Если уж так хочеться — делайте авто-валидацию или модел-байдинг по ид из БД уже в бизнес слое не завязываясь на веб АПИ. Тут тоже можно прикрутить свой велосипед через рефлекшн и атрибуты.

[marshinov]

Стоит заметить что ваше решение плохо работает когда надо прикрутить транзакции. А имя вашего класса MoveProductParam как бы подразумевает что транзакция бы не помешала.

Вы имеете в виду случай, когда валидация проходит, а за оставшиеся 100мс кто-то успевает удалить продукт / категорию с таким id? Если да, то эта тема отдельной большой статьи.

Правильно заметили выше — намешаны слои. Если уж так хочеться — делайте авто-валидацию или модел-байдинг по ид из БД уже в бизнес слое не завязываясь на веб АПИ. Тут тоже можно прикрутить свой велосипед через рефлекшн и атрибуты.

Да прикрутил уже. Бывают простые случаи, когда нужно сделать много CRUD'а и не хочится делать DTO ради двух-трех полей, которые мапятся на foreign-ключи. Хотя, может их проще сразу гнать на Update в базу через Dapper. В любом случае, вариант с атрибутом скорее экспериментальный и не пертендует на истину в последней инстанции.

[pshhpshh]

А не лучше было бы просто перехватывать ошибку от БД и уже разбирая ее выводить какой-то юзер меседж, если уж так хочется. Хотя не понятно зачем. Юзер обычно в таких случаях должен получить какой-то дженерик меседж да и все.

[marshinov]

Мне — не лучше. Предпочитаю возвращать осмысленные коды ответов. Возвращение осмысленных кодов имеет преимущества по usability, соответствию стандартам и простоте отладки.