В прошлый раз я перенес часть императивного кода в атрибут. Есть еще одна проверка, кочующая из одного файла в другой:

public class MoveProductParam
{
   public ProductId {get; set; }

   public CategoryId {get; set; }
}

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

Мы достойны лучшего
public class MoveProductParam
{
   [EntityId(typeof(Product))]
   public ProductId {get; set; }

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

Добавим Атрибут для валидации


Метод IsValid у ValidationAttribute перегружен. Нам потребуется вторая перегрузка, чтобы дотянуться до IOC-контейнера. Для простоты я не стал вводить дополнительный интерфейс и просто пытаюсь получить делегат, который по типу и Id вернет булево значение: есть такая сущность или нет.

public class EntityIdAttribute: ValidationAttribute  
{
	private readonly Type _entityType;

	public EntityIdAttribute(Type entityType)
	{
		_entityType = entityType;
	}

        protected override ValidationResult IsValid(object value,
            ValidationContext validationContext)
        {
            var checker = validationContext
                .GetService(typeof(Func<Type, object, bool>))
                as Func<Type, object, bool>
                ?? throw new InvalidOperationException(
                    "You must register Func<Type, object, bool>");

            return checker(_entityType, value)
                ? ValidationResult.Success
                : new ValidationResult($"Entity with id {value} is not found");
        }
}

Регистрируем делегат


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

services.AddScoped<Func<Type, object, bool>>(x =>
{
	bool Func(Type t, object o)
	{
		var dbContext = x.GetService<DbContext>();
		return dbContext.Find(t, o) != null;
	};

	return Func;
});

Собираем вместе


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

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

[Validate]
public class ProductController: Controller
{
   public IActionResult Move(MoveProductParam par)
   {
      // Ваша логика здесь.
      // Этот код выполнится, только если ProductId и CategoryId есть в БД
      // Все проверки стали декларативными
   }
}

Да, атрибут можно забыть поставить, тогда при передаче несуществующего id мы упадем с NRE или ArgumentException, если вы используете защитное программирование. Такую ошибку будет очень легко диагностировать и исправить. Если MoveProductParam используется более чем в одном месте валидация применится везде, где этот параметр используется. Вы не забудете добавить проверку повторно.

UPD. Развиваем идею. Добавляем ModelBinder


mayorovp подсказал в комментариях, что можно пойти дальше и сделать ModelBinder. Чтобы можно было так:

public class MoveProductParam
{
   [ModelBinder(typeof(EntityModelBinder))]
   public Product Product{get; set; }

   [ModelBinder(typeof(EntityModelBinder))]
   public Category Category{get; set; }
}

Сказано — сделано. К сожалению в метод Find нельзя передать строку из запроса. Нужно знать тип первичного ключа. Скорее всего эта информация доступна из контекста бд, но я так глубоко не копал, поэтому вытащил тип свойства Id. Чтобы было побыстрее добавил TypeAccessor из FastMember.

public class EntityModelBinder: IModelBinder
{
	private readonly Func<Type, object, object> _getter;

	public EntityModelBinder(Func<Type, object, object> getter)
	{
		_getter = getter;
	}

	public Task BindModelAsync(ModelBindingContext bindingContext)
	{
		var value = bindingContext.ActionContext
			.HttpContext
			.Request
			.Query[bindingContext.ModelName]
			.FirstOrDefault();

		if (value == null)
		{
			bindingContext.ModelState
                             .AddModelError(bindingContext.ModelName,
                             "Id for \"bindingContext.ModelName\" is null");
			return Task.CompletedTask;
		}

		try
		{
			var typeAccessor = TypeAccessor
                            .Create(bindingContext.ModelType);
			
			// Не все называеют Id так.
                        // Лучше покопаться в конфигурации EF, чтобы вытащить тип PK
			// Если кто подскажет в комментариях где это буду благодарен

			var id = Convert.ChangeType(value,
                            typeAccessor.GetMembers()
                                .First(y => y.Name == "Id")
                                .Type);
			
                        var result = _getter(bindingContext.ModelType, id);
			bindingContext.Result = ModelBindingResult.Success(result);
		}
		catch (Exception e)
		{
			bindingContext.ModelState.AddModelError(
                            bindingContext.ModelName, e.Message);
		}
		
		return Task.CompletedTask;
	}

//Сигнатура делегата только в качестве примера. Может быть другой интерфейс
// с явной семантикой
services.AddScoped<Func<Type, object, object>>(x =>
{
	object Func(Type t, object o)
	{
		var typeAccessor = TypeAccessor.Create(t);
		var dbContext = x.GetService<DemoContext>();
		return dbContext.Find(t, o);
	};
	
	return Func;
});

Осталось реализовать IModelBinderProvider, чтобы назначить этот байндер на все Entity из контекста.

Комментарии (53)


  1. Tantrido
    17.01.2018 17:32
    +1

    Круто!


  1. mayorovp
    17.01.2018 18:04
    +1

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


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


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


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


    1. marshinov Автор
      17.01.2018 19:34

      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;
              }
          }


      1. mayorovp
        17.01.2018 19:47

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

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

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


      1. mayorovp
        17.01.2018 21:56

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


  1. mediarium
    17.01.2018 18:05

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


    1. unsafePtr
      17.01.2018 22:13

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


      1. mediarium
        18.01.2018 02:37

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


        1. mayorovp
          18.01.2018 06:28

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


          1. mediarium
            18.01.2018 10:17

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


            1. mayorovp
              18.01.2018 10:23
              +1

              О вот этой:


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

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


              1. mediarium
                18.01.2018 10:36

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

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


                1. mayorovp
                  18.01.2018 10:55

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

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


                  1. mediarium
                    18.01.2018 11:24

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

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


                    1. mayorovp
                      18.01.2018 12:29

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


                      1. mediarium
                        18.01.2018 12:33

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


                        1. mayorovp
                          18.01.2018 12:44

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

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

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

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


                          1. mediarium
                            18.01.2018 12:52
                            -1

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

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


                            1. mayorovp
                              18.01.2018 12:58

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

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


                              1. mediarium
                                18.01.2018 13:10

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

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


                                1. mayorovp
                                  18.01.2018 13:34
                                  +1

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


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


                                  1. mediarium
                                    18.01.2018 14:01
                                    -1

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

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

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


  1. oxidmod
    18.01.2018 09:50

    Я может чего-то не понимаю в этих ваших шарпах, но для меня этот подход попахивает нехорошим.

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

    Биндинг моделей к параметрам контролера прибивает вас гвоздями к фреймворку. Возможно в мире шарпа нет альтернатив, но на пыхе мне приходилось интегрировать магазины с разными фреймворками и даже с системами без фреймворков (а давайте добавим страничку, где будем продавать товары онлайн… на бложик, накиданный на коленке 10 лет назад, ага...). Если бизнес логика не завязана на конкретный фреймворк — это сделать несложно.

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


    1. mayorovp
      18.01.2018 10:20

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

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


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


      1. oxidmod
        18.01.2018 10:28

        Просто нужно писать бизнес-логику не завязанную ни на фреймворк, ни на орм.
        Пример с хранилищем. В бизнес слое есть IProductRepository. В слое инфраструктуры лежит DbProductRepository и InMemoryProductRepository. Все сервисы в бизнес-слое завязаны на IProductRepository, а уж какую реализацию им передашь — дело десятое. Более того, можно всю бизнес-логику написать до того как вообще определишься с хранилищем, фреймворком и т.д.


        1. mayorovp
          18.01.2018 10:35

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

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


          1. oxidmod
            18.01.2018 13:38

            Не совсем. Проверка существования перед перемещением — это правило бизнес-логики и должно проверяться в доменном сервисе, а не в контроллере. Вы прыгаете с UI сразу в Persistence слой, минуя бизнес логику. Собственно последний шаг к ТолстымУродливымКонтроллерам — прямо здесь же в контролере написать что-то типа

            product.Category.removeProduct(product);
            
            category.addProduct(product);
            


            1. mayorovp
              18.01.2018 13:43

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

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

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


              1. oxidmod
                18.01.2018 13:54

                Зачем ждать критическую массу?
                Реализовать хотя бы Command не так сложно.
                Это сразу дает ряд преимуществ:
                1. Можно покрыть бизнес-логику юнит тестами, что гораздо проще и быстрей чем тесты через хттп запросы.
                2. Разработка становиться ориентированной на use cases. Обработка каждого кейса строго изолирована. Беглого взгляда на список команд достаточно, что бы понять на что способна система.
                3. Вот тут можно применить байндинг реквеста на объект команды, в котором и будет зашита валидация. Что все идентификаторы и данные на месте, что они в правильном формате. А в контроллерах будет минимум логики:

                commandHandler.execute(command);
                

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


                1. mayorovp
                  18.01.2018 13:59

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


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

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


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


                1. mayorovp
                  18.01.2018 14:02

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

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


            1. marshinov Автор
              18.01.2018 14:40
              +1

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

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

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

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


              1. oxidmod
                18.01.2018 16:05

                1. CQRS не обязан быть асинхронным
                2. Если вам не нужен CQRS — не используйте, но бизнес логике все равно место в бизнес-слое, а не в контроллерах

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


    1. marshinov Автор
      18.01.2018 11:53

      Тут смешаны слои. Слой 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-контейнере.


      1. oxidmod
        18.01.2018 13:44

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


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

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


        1. mayorovp
          18.01.2018 13:49

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

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


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

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


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


          1. oxidmod
            18.01.2018 14:02

            Я о том, что БЛ не должна зависеть от фреймворка или от ОРМ или любой другой либы.
            Ну условно, завтра в секьюрити компоненте фреймворка найдут критическую уязвимость или упретесь в производительность EF и нужно будет переписать на чистый sql. Вам придется во всех местах, где опираетесь на сторонние классы — переписывать.
            Потому что у вас зависимость от реализации, а не интерфейса.

            Я не призываю отказаться от фреймворка или ОРМ. Я призываю соблюдать границы между слоями.


            1. mayorovp
              18.01.2018 14:11

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


              1. oxidmod
                18.01.2018 14:15

                Cколько «столько»? Кода даже по сути будет столько же, только размещаться будет на разных слоях.


                1. mayorovp
                  18.01.2018 14:21

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


                  1. oxidmod
                    18.01.2018 14:33

                    Тоесть вы пишите без тестов?
                    В моем случае для БЛ нужно писать юнит тесты, которые пишутся быстрей и проще, чем end-to-end тесты. Их может быть больше, но все они небольшие и очень быстро гонятся на CI. TDD становится не обузой (опять тесты писать), а инструментом. Хотите верьте, хотите нет, но 90% задач я делаю не запуская написанный код вручную, только гоняя тесты.

                    Интерфейсы нужны на границах слоев в основном. Да, они нужны для стратегий, визиторов и других сервисов, имеющих множественные имплементации, но там их можно выделить уже тогда, когда они реально станут нужны. И это будет рефакторинг в бизнес-логике, который никак не отразится на слое UI. Там по прежнему будет commandHandler.execute(command);

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


                    1. mayorovp
                      18.01.2018 14:47

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

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


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

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


                      1. oxidmod
                        18.01.2018 16:11

                        Вам не кажется странным пытаться совершить бизнес действие с заведомо неверными входными данными?
                        Это валидация разного уровня, и ошибки разного типа.
                        В ответ на кривой запрос веб приложение должно отдать «Bad request», а при попытке совершить некорректное бизнес действие — «Conflict», «Not found» или другой подходящий ответ в зависимости от ситуации.


                        1. marshinov Автор
                          18.01.2018 20:27

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

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


                          1. oxidmod
                            19.01.2018 00:01

                            Не совсем. У меня есть ряд общих ошибок: UserNotFound / ProductNotFound, которые летят из репы. OrderCantBePaid, ProductOutOfStock, которые могут летать из определенных сервисов. Но эти ошибки не вылетают в контроллер, потому что отсутсвие юзера может быть как 404, так и 422 в разных запросах. Так что все потенциальные ошибки ловятся хендлером команды и в случае возникновения заворачиваются в новую. Например при попытке поместить в корзину несуществующий товар или в корзину несуществующего юзера вылетит в контроллер AddToBasketError, которая в свою очередь уже сконвертится в код 422 и боди типа такого

                            {
                                "message": "Product can't be added to basket.",
                                "logref": "...",
                                "_embedded": {
                                    "errors": [{
                                            "message": "User #123 doesn't exist.",
                                    }]
                                }
                            }
                            


                            1. marshinov Автор
                              19.01.2018 10:41

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


                              1. oxidmod
                                19.01.2018 11:09

                                Это исключения. CommandHandler всегда возвращает успешный результат.


                                1. marshinov Автор
                                  19.01.2018 12:20

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


                                  1. oxidmod
                                    19.01.2018 12:54
                                    +1

                                    Я не использую эксепшены для control flow.

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


  1. Pro100Oleh
    18.01.2018 12:33
    +1

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


    1. marshinov Автор
      18.01.2018 14:48

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

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

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

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


  1. pshhpshh
    18.01.2018 17:27

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


    1. marshinov Автор
      19.01.2018 10:43

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