Skip to content

Instantly share code, notes, and snippets.

@davydovanton
Last active August 11, 2023 13:33
Show Gist options
  • Save davydovanton/a8f3cab112e43cfafbcbbfae2472f6c7 to your computer and use it in GitHub Desktop.
Save davydovanton/a8f3cab112e43cfafbcbbfae2472f6c7 to your computer and use it in GitHub Desktop.
Рефакторинг сервис объекта с монадами и AR

https://t.me/pepegramming

Сегодня попался рельсовый код, в котором используются монады, сервисы и прочее. Решил сделать обзор с объяснением того, что в коде не нравится и что можно исправить.

Данный разбор основан только на личном опыте и избегает попытку написать самый идеальный код на свете. К сожалению пошарить ссылку на код не могу, потому что автор попросил опубликовать анонимно.

Исходные данные

Главная операция, которая вызывается из контроллера выглядит следующим образом:

module Excursion
  module Services
    class JoinUser < Service
      def initialize(params, record: ,
        input_validator: Users::Validators::NewUser.new,
        user_exists_check: Users::Services::CheckExists,
        user_creator: Users::Services::Create
      )
        @params = params
        @input_validator = input_validator
        @record = record
        @user_creator = user_creator
        @user_exists_check = user_exists_check
      end

      def call
        validated_params = yield input_validate
        yield business_validate(validated_params.to_h)
        save(validated_params.to_h)
      end

      private

      attr_reader :params, :record, :input_validator, :user_creator, :user_exists_check

      def input_validate
        input_validator.call(params[:group]).to_monad
      end

      def business_validate(validated_params)
        return Failure(:already_exists) if record.users.where(validated_params).exists?
        Success()
      end

      def save(validated_params)
        user_check_result = user_exists_check.new(validated_params[:email]).call
        if user_check_result == Success(:user_not_exists)
          user = yield user_creator.new(validated_params).call

          record.users << user
          return Success(:created)
        end

        record.users << user_check_result.value!

        Success(:created)
      end
    end
  end
end

Два сервиса зависимостей, которые используются в этом примере выглядят так:

module Users
  module Service
    class CheckExists < Service
      def initialize(email, model: User)
        @email = email
        @model = model
      end

      def call
        return Success(model.find_by(email: email)) if model.where(email: email).exists?
        Success(:user_not_exists)
      end

      private

      attr_reader :email, :model
    end
  end
end

module Users
  module Services
    class Create < Service
      def initialize(params,
        input_validator: Users::Validators::NewUser.new,
        model: User
      )
        @params = params
        @input_validator = input_validator
        @model = model
      end

      def call
        yield input_validate
        save
      end

      private

      attr_reader :params, :model, :input_validator

      def input_validate
        input_validator.call(params).to_monad
      end

      def save
        Try() { model.create!(params) }
      end
    end
  end
end

Проблемы которые нашел

Контекст > реализация

Убрал общие слова из именования объектов, сервисов и данных. Как пример, в изначальном варианте есть строчка save(validated_params.to_h) (Excursion::Services::JoinUser#call), как оказалось - это запись пользователя на экскурсию, т.е. с точки зрения кода - это сохранение чего-то в базе, но с точки зрения логики это совершенно другая операция. Если использовать общее именование - придется каждый раз думать зачем этот код, а если использовать описание процесса - появляется контекст, в котором понять реализацию проще, чем по реализации понять контекст. Поэтому переименовал этот шаг в enroll.

Избыточность сервис объектов

В изначальном примере можно увидеть 2 глобальные проблемы:

  1. Зависимость от кучи зависимостей
  2. Оба сервиса реализуют логику работы с базой данных (проверка существования пользователя и пользователя сохранение в базе).

Большой соблазн сделать 2 отдельных сервиса и положить эту логику туда, как сделал автор оригинального примера. К сожалению, можно заметить, что такой подход приводит к усложнению бизнес логики, яркий пример - две строчки из Excursion::Services::JoinUser#save:

def save(validated_params)
  user_check_result = user_exists_check.new(validated_params[:email]).call
  # вот тут приходится делать страшную проверку с использованием монады
  
  # так же, сама монада самоисключающий случай и вне контекста логики не понятно
  # почему отсутствие пользователя успех
  if user_check_result == Success(:user_not_exists)
    # в данном случае приходится использовать DO notations во вложенном методе,
    # таким образом появляется вложенность, что приводит к усложению data flow
    user = yield user_creator.new(validated_params).call

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

Общие слова

В примере много общих слов, которые увеличивают конгнетивную нагрузку на человека, который этот код читает. Я говорю о record, user_exists_check, validated_params, business_validate, input_validate, params без контекста не понятно, что это такое, потому что терминология общая. В качестве примера: прочтите название двух сервисов и попробуйте ответить себе, что каждый из них делает:

  • Excursion::Services::JoinUser
  • Excursion::Services::EnrollUser

Стейт класса и микс параметров и зависимостей

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

def initialize(params, record:, # данные
  input_validator: Users::Validators::NewUserContract.new, # зависимость
  user_exists_check: Users::Services::CheckExists, # зависимость
  user_creator: Users::Services::Create # зависимость
)

def initialize(email, model: User)
  @email = email  # данные
  @model = model # зависимость
end

def initialize(params, # данные
  input_validator: Users::Validators::NewUserContract.new, # зависимость
  model: User # зависимость
)

В этом нет проблем до тех пор, пок не придется тестировать код и использовать тяжелособирающиеся зависимости (например rom). Так же, каждый вызов сервиса приведет к новой аллокации объектов, но это не так критично. Намного критичнее завязка локики на внутренний стейт объекта. Во первых, сервис перестает быть чистым(мутация данных приведет к багам), во вторых, поддержка кода становится сложнее, так как аргументы могут подсказать детали реализации. В качестве примера сравните два определения функций:

  • def save(validated_params)
  • def save(record, validated_params)

В первом случае первая мысль приходящая в голову - параметры сохраняются в базе и создается новый объект модели. Во втором случае понимаешь, что сохраняются данные для конкретной записи. Проблема в том, что это не выдуманный пример. Если посмотреть на оригинал - save действительно использует record который является внутренним стейтом объекта. Но на самом деле record - это просито инстанс модели excursion.

Изоляция цепочек

Распространный подход в рельсе: цепочки методов в моделе, которые сложно тестировать. Я говорю о случаях record.users.where(payload).exists? и подобных ему. Проблема заключается в том, что для такого кода сложно написать юнит тест, поэтому приходиться либо писать сложный мок, либо использовать интеграционное тестирование (на любой тест ходить в базу). В каждом из случаев возникают проблемы: моки сложно поддерживать и понять. А походы в базу увеличивают время прохождения тестов на минуты и даже часы.

Конечный результат

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

module Excursion
  module Services
    class EnrollUser < Core::Service
      attr_reader :validator

      def initialize(validator: Users::Validators::NewUserContract.new)
        @validator = validator
      end

      def call(excursion, params)
        paylaod = yield validate_raw_data(params)
        yield attender_of_excursion?(excursion, payload)

        user = yield find_or_create_by_email(payload[:email])

        Success(excursion.enroll(user))
      end

      def attender_of_excursion?(excursion, payload)
        excursion.attender?(payload) ? Failure(:attender_of_excursion) : Success()
      end

      def find_or_create_by_email(payload)
        Try() { User.find_or_create(payload) }.to_result
      end
    end
  end
end

class Excursion
  def attender?(payload)
    users.where(payload).exists?
  end

  def enroll(user)
    self.users << user
    self
  end
end

class User
  def self.find_or_create(payload)
    find_by_email(payload[:email]) || create!(payload)
  end
end

UPD

  1. спасибо @likeath, в последнем варианте User должен инжектиться в сервис, т.е. конструктор сервиса должен выглядеть так:
module Excursion
  module Services
    class EnrollUser < Core::Service
      attr_reader :model, :validator

      def initialize(model: User, validator: Users::Validators::NewUserContract.new)
        @model = model
        @validator = validator
      end
@ptiforka
Copy link

ptiforka commented May 8, 2020

не идеальны -> не идеален
В части Конечный результат

@Yegorov
Copy link

Yegorov commented May 8, 2020

локики -> логики В разделе Стейт класса и микс параметров и зависимостей

@kudrin
Copy link

kudrin commented May 9, 2020

Смущает то, что по БЛ завязана на формат входных данных. Если измениться формат params придется переписать не только сам сервис, но и возможно класс User.

Мне еще не понятно, что выдаст API в случае ошибки заполнения формы?
Если просто: {"error":"invalid"}, то ок.
Если {"error": {"email":"is empty"}}, то как клиенту отличить ошибку валидации от ошибки БЛ {"error":"attender_of_excursion"}.

# тут видимо нужно просто передать payload
user = yield find_or_create_by_email(payload[:email])

# тут возможно использвать не весь payload а только email, т.к. он является уникальным ключом
def attender?(payload)
  users.where(payload).exists?
end

@davydovanton
Copy link
Author

@kudrin привет, да. такой вариант предполагает зависимость от схемы входящих данных. В каких-то случаях я использую мапперы (или дто для каких-то случаев), что бы подогнать данные из одного слоя в другой (например из транспорта в бизнес логику, как тут: https://github.com/davydovanton/rubyjobs.dev/blob/master/apps/web/controllers/vacancies/create.rb#L16-L17). Проблема тут в том, что не всегда это надо и в целом, это уже индивидуальные интересы/привычки команды.

про ошибки: тут сложнее, потому что формат сообщений будет отличаться. Обычно можно просто ловить все что не соответствует Failure(Symbol) и считать что там ошибка валидации (проблема плагина для dry-validation который to_result делает)

@kudrin
Copy link

kudrin commented May 10, 2020

@davydovanton привет. Да, без понимания как этот сервис будет использоваться - сложно понять, что будет меняться, что нет, и на что тратить время. Как клиент будет обрабатывать ошибки.

При желании более явно выделить бизнес слой от технического - я бы делал форму, зависимую от валидатора. Она же приводит типы. Оборачивает, если надо, в value object'ы. И реализует интерфейс request model. Ошибки заполнения формы это один http code. Ошибка исполнения - другой.

@davydovanton
Copy link
Author

@kudrin

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

а можешь пример с псевдокодом показать как бы ты это сделал?

@kudrin
Copy link

kudrin commented May 12, 2020

Пусть форма используется следующим образом

form = Excursion::Forms::NewUser.new(
  validator: Users::Validators::NewUserContract.new,
)

form.fill(data) # => nil
form.valid?     # => true
form.email # => '[email protected]'
form.first_name # => 'John'
form.last_name  # => 'Doe'
form.birthdate  # => #<Date: 1970-01-01 >
form.errors     # => {}

Вопрос, как подчеркнуть, что форма соответствует Interfaces::NewUser. На данном этапе я бы это явно не показывал, ограничившись тестами.

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

Meme

  require 'time'
  
  # - Данный интерфейс приводит типы или делает проверки?
  # - Кря-кря
  module Interfaces
    module NewUser
      attr_reader :email, :first_name, :last_name, :birthday
      
      def email=(val)
         @email = String(val)
      end

      def first_name=(val)
        @first_name = String(val)
      end

      def last_name=(val)
        @first_name = String(val)
      end

      def birthday=(val)
        @birthday = case val
                    when Date   then val
                    when String then Date.strptime(val)
                    else raise ArgumentError, "Wrong type for birthday #{val.class.name}"
                    end
      end
    end
  end

Мне в целом нравится sorbet и steep, особенно steep, но оба сырые, и не стали стандартами de-facto. И с большей вероятностью при их использовании придется переписывать код. Ждем 3ки.

Или тебе больше интересно как бы я делал Service через LunaPark?

@pirj
Copy link

pirj commented Apr 22, 2021

Мне лично не понятно, почему attender_of_excursion? выдаёт Failure, и в каком именно случае. При разных применениях можно считать "успехом" тот факт, что кто-то attends (при погрузке участников в автобус, например) или наоборот (повторный enroll).

@pirj
Copy link

pirj commented Apr 22, 2021

def call(excursion, params)
  paylaod = yield validate_raw_data(params)
  yield attender_of_excursion?(excursion, payload)
  user = yield find_or_create_by_email(payload[:email])

Хотелось бы посмотреть что там за блок, с которым вызывается этот call. Блок принимает три последовательных вызова, причём:

1й на входе принимает результат validate_raw_data (подозреваю, что Success и что-то в нём), а на выходе выдаёт хеш с атрибутами.

2й на входе имеет boolean (судя по ? в attender_of_excursion?), хотя по факту Success/Failure. А на выходе - ничего.

3й на входе имеет Success/Failure, и на выходе разворачивает Success для получения завёрнутого в него user.
А если Failure? Например, потому что find_by_email || create! упали с исключением (race condition, например). Где обработка ошибки?

Есть подозрение, что сложность этого блока зашкаливает.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment