Pull to refresh

Comments 53

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


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


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


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

   [ResolveEntity]
   public Category Category {get; set; }
}
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;
        }
    }
Если я захочу делать через Dapper вместо EF — мне придется переписывать одинаковый объем кода независимо от того где я буду этот код держать — в валидаторе или в сервисе.

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

Что же до вашего EntityModelBinder — он делает слишком многое. Я имел в виде простой поиск сущности по Id. Точно то же самое что вы делаете в валидаторе — только вместо Any использовать First.
UPD: значение лучше брать из ValueProvider, а не из Query
Возможно Я не понял, какую именно проблемы вы пытаетесь решить, но весь этот код не требуется если использовать FOREIGN KEY Constraints при проектировании структуры БД.
И пользователь получит SQL ошибку. Лучше вернуть человекоподобную ошибку что бы клиент знал что делать.
О какой SQL ошибке идёт речь? При правильной настройке FOREIGN KEY (с DELETE/UPDATE) описанная в статье ситуация в принципе не возможна, т.к. за гарантированным наличием/отсутствием данных в парах будет следить сама БД.
Каким образом она за этим будет следить? Разве не выкидывая ошибку в ответ на некорректные операции?
Ещё раз спрашиваю — о какой SQL ошибке идёт речь?

О вот этой:


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

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

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

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

Что вы скажете пользователю?
Очевидно то, что “данные не удалось сохранить”.

Лучше расскажите как вы объясните пользователю, что у вас Product в БД привязаны к несуществующим Category, а Category содержат несуществующие Product?
А откуда у меня в базе возьмутся несогласованные данные? Или вы думаете, что валидация — это замена внешним ключам, а не дополнение к ним?
Вот и мне интересно откуда у автора в БД взялись несогласованные данные, для которых требуется обязательная дополнительная валидация ключей “кочующая из одного файла в другой” :)
Э, постойте. Кто вам сказал что они у него в БД?

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

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

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

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

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

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");

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


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

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

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

> В процессе валидации используется запрос к базе.
UFO just landed and posted this here
А что если приложение должно уметь работать вообще без бд?

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


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

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

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

   [EntityId(typeof(ICategoryRepository))]
   public CategoryId {get; set; }
}
UFO just landed and posted this here
Ну и что толстого и уродливого в двух строках кода?

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

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

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


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

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


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

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

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

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

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

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

Это не так. Если у вас синхронный CRUD, но вы вместо контроллера написали запросы в квейри / командах ничего не изменилось, просто называется иначе. Тот CQRS, что асинхронный обычно требует принципиально иного подхода к проектированию системы. Грег Янг вообще не рекомендует использовать CQRS как top level architecture.
UFO just landed and posted this here
Тут смешаны слои. Слой 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-контейнере.
UFO just landed and posted this here
Я о байндинге параметров контролера, когда идентификаторы в хттп-запросе автоматически превращаются в сущности из слоя хранения данных.

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


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

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


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

UFO just landed and posted this here
То есть чтобы вдруг не пришлось переписывать завтра — надо тратить столько же усилий сразу же? :)
UFO just landed and posted this here
Интерфейсы, классы, защитные проверки, тесты — все это тоже код, в вашем варианте его намного больше.
UFO just landed and posted this here
Тоесть вы пишите без тестов?

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


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

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

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

Семантика валидации БЛ одинаковая: нет такой сущности. А вот код ответа — разный. Если вы возвращаете результат валидации из бизнес-слоя, скажем NotFoundResult, дополнительно обрабатываете в контроллере, зная контекст запроса?
UFO just landed and posted this here
UserNotFound, ProductNotFound, OrderCantBePaid, ProductOutOfStock — это типы исключений или возвращаемых из CommandHandler значений?
UFO just landed and posted this here
UFO just landed and posted this here
Стоит заметить что ваше решение плохо работает когда надо прикрутить транзакции. А имя вашего класса MoveProductParam как бы подразумевает что транзакция бы не помешала. Можно бы усложнить велосипед и прикрутить транзакцию к веб контексту, но в голове уже возникает троллейбус из хлеба. Но что делать когда нужно несколько транзакций на один реквест?
Правильно заметили выше — намешаны слои. Если уж так хочеться — делайте авто-валидацию или модел-байдинг по ид из БД уже в бизнес слое не завязываясь на веб АПИ. Тут тоже можно прикрутить свой велосипед через рефлекшн и атрибуты.
Стоит заметить что ваше решение плохо работает когда надо прикрутить транзакции. А имя вашего класса MoveProductParam как бы подразумевает что транзакция бы не помешала.

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

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

Да прикрутил уже. Бывают простые случаи, когда нужно сделать много CRUD'а и не хочится делать DTO ради двух-трех полей, которые мапятся на foreign-ключи. Хотя, может их проще сразу гнать на Update в базу через Dapper. В любом случае, вариант с атрибутом скорее экспериментальный и не пертендует на истину в последней инстанции.
А не лучше было бы просто перехватывать ошибку от БД и уже разбирая ее выводить какой-то юзер меседж, если уж так хочется. Хотя не понятно зачем. Юзер обычно в таких случаях должен получить какой-то дженерик меседж да и все.
Мне — не лучше. Предпочитаю возвращать осмысленные коды ответов. Возвращение осмысленных кодов имеет преимущества по usability, соответствию стандартам и простоте отладки.
Sign up to leave a comment.

Articles