adomokos / light-service

Series of Actions with an emphasis on simplicity.
MIT License
837 stars 67 forks source link

Add action `expects` validations #227

Closed ni3t closed 2 years ago

ni3t commented 2 years ago

Add validates options to expects and promises

Introduction

This PR introduces the concept of runtime validations of expects and promises keys to LightService.

The functionality introduced in this PR is useful for elucidating hard-to-track-down errors, as well as additional validation when integrating different services. It exposes most of the functionality of ActiveModel::Validations on a LightService::Context and merges it with the expects/promises syntax.

It also reconfigures the default option handler to allow more than one option to be passed to expects.

I also added a couple of validators we use in our system:

Problem Statement

Some errors can be very hard to track down in a complex system, because runtime errors, unless specifically rescued, rarely give good information on the location of the error. Modern error reporting tools are helpful with stack traces, but when developing locally it can be very hard to track it down.

At LoadUp, we commonly nest our services using a pattern we call AsAction - i.e. a service class has a subclass called AsAction, that passes values from the context into the service and is executed as a part of another service using Service::AsAction. When nesting multiple services, it involves a lot of tracing. We have also built hundreds of actions that are reused in many places, and in a long, complicated service, it can be difficult to track what keys are on the context at any one point in time, especially if keys are being added during execution (i.e. Actions::Orders::FindBy::Id, where an order is placed on the context based on order_id in the context.)

Examples

For example, given the following action, it is impossible, without stack tracing, to determine what calls fight! and where it is called.

class AvengerFight
  extend LightService::Action

  expects :avenger

  executed do |c|
    c.avenger.fight!
  end
end

AvengerFight.call(avenger: Avenger.find_by(name: "Tohr"))  # Thor's name is misspelled
# => "NoMethodError (undefined method `fight!' for nil:NilClass")

However, with a simple presence validation, a much more helpful error is produced:

class AvengerFight
  extend LightService::Action

  expects :avenger, validates: { presence: true }

  executed do |c|
    c.avenger.fight!
  end
end

AvengerFight.call(avenger: Avenger.find_by(name: "Tohr"))
# => LightService::InvalidKeysError:
#      AvengerFight:
#        avenger: can't be blank

In practice, this error message can be made even more explicit:

class AvengerFight
  extend LightService::Action

  expects :avenger, validates: { class_name:  Avenger }
  expects :power_level, validates: { presence: true, numericality: { greater_than: 500 } }
  expects :movie_title

  promises :box_office_hit, validates: { presence: true }

  executed do |c|
    c.avenger.fight!(c.power_level)
    if c.movie_title == "Infinity War"
      c.box_office_hit = true
    end
  end
end

AvengerFight.call(avenger: Avenger.find_by(name: "Tohr"), power_level: 100, movie_title: "Thor: The Dark World")
# =>LightService::InvalidKeysError:
#     AvengerFight:
#       avenger: must be an instance of Avenger, got NilClass
#       power_level: must be greater than 500
#       ... and assuming you fix the expects errors ...
#       box_office_hit: can't be blank

Summary

These validations are completely optional, but for where it's needed, can really add value.

Caveats and Alternate Approaches

1. It includes a new dependency, ActiveModel.

I've tried to stick as close as possible to basic ActiveModel::Validation patterns, in an effort to minimize the impact of potential breaking changes in the future to ActiveModel. Also, I assume (and you know what assuming does...) that most of the user base for the gem uses rails, and will already have this gem installed. Lastly, ActiveSupport is already a dependency, so it's not too huge of a jump from there to ActiveModel.

An alternate approach could be to limit the vocabulary supported by this feature and hand-write the validators.

2. It only performs runtime validations

As much as I wish this could happen at compile time, it just isn't possible to validate these in any context but runtime. The way I think of LightService's expects and promises is somewhat akin to a runtime type checker, and this feature fits in a long those lines. However, because it raises errors, unless they are rescued the execution of the service completely halts and things like rollbacks and after_actions are not executed. (This is also a problem with the current implementation of the KeyVerifier.)

An alternate approach could be to, instead of operating in the KeyVerifier, to operate in the executed at the very beginning, allowing the context to be failed and the resulting errors to be propagated in the result object. This might be a better approach, but I thought since this is operating outside the executed block, it made more sense to run the validations at the same time as the key verifier.

Side Note

I'd love to work on adding Ruby 3 types to the gem, and I believe adding these runtime validations can help the compiler produce more explicit types.

adomokos commented 2 years ago

@ni3t - I like the idea behind this PR. 👍

However, I can't introduce another gem dependency to LightService. Take a look at this discussion we just had recently. The conclusion was to remove ActiveSupport if possible and your PR is going against this goal.

If you can make it work without ActiveModel, I'll help you any way I can to merge this.