-
-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/action contracts #451
Changes from 7 commits
dd319d4
47b719a
bb742e1
4e052b8
c64c741
ef832c6
59f1216
9debfdb
56081d2
9a264e1
7733380
3f26e7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,139 @@ | ||
# frozen_string_literal: true | ||
|
||
module Hanami | ||
class Action | ||
# A wrapper for defining validation rules using Dry::Validation. This class essentially | ||
# wraps a Dry::Validation::Contract and acts as a proxy to actually use Dry gem | ||
# | ||
# Defined via the `contract` block in an action class. | ||
# Although more complex domain-specific validations, or validations concerned with things such as uniqueness | ||
# are usually better performed at layers deeper than your HTTP actions, Contract still provides helpful features | ||
# that you can use without contravening the advice form above. | ||
# | ||
# @since 2.2.0 | ||
class Contract | ||
# A wrapper for the result of a contract validation | ||
# @since 2.2.0 | ||
# @api private | ||
class Result < SimpleDelegator | ||
# @since 2.0.0 | ||
# @api private | ||
def output | ||
__getobj__.to_h | ||
end | ||
krzykamil marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# @since 2.0.0 | ||
# @api private | ||
def messages | ||
__getobj__.errors.to_h | ||
end | ||
krzykamil marked this conversation as resolved.
Show resolved
Hide resolved
|
||
end | ||
|
||
# @attr_reader env [Hash] the Rack env | ||
# | ||
# @since 2.2.0 | ||
# @api private | ||
attr_reader :env | ||
|
||
# Define a contract for the given action | ||
# | ||
# @param blk [Proc] the block to define the contract, including [Params] as a contract schema and connected rules | ||
# | ||
# @since 2.2.0 | ||
# @api private | ||
# | ||
# @example | ||
# class Create < Hanami::Action | ||
# contract do | ||
# params do | ||
# required(:birth_date).value(:date) | ||
# end | ||
# rule(:birth_date) do | ||
# key.failure('you must be 18 years or older to register') if value > Date.today - 18.years | ||
# end | ||
# end | ||
# | ||
# def handle(req, *) | ||
# halt 400 unless req.contract.call.errors.empty? | ||
# # ... | ||
# end | ||
# end | ||
def self.contract(&blk) | ||
validations(&blk || -> {}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we just do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, it was pointless. Thanks for the review (and while I am at it - for your OS work, that I've used extensively over the years and greatly admired). Applied all changes now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @krzykamil 😃 |
||
end | ||
|
||
# @since 2.2.0 | ||
# @api private | ||
class << self | ||
def validations(&blk) | ||
@_validator = Dry::Validation::Contract.build(&blk) | ||
end | ||
|
||
attr_reader :_validator | ||
end | ||
|
||
# Initialize the contract and freeze it. | ||
# | ||
# @param env [Hash] a Rack env or an hash of params. | ||
# | ||
# @return [Hash] | ||
# | ||
# @since 2.2.0 | ||
# @api public | ||
def initialize(env) | ||
@env = env | ||
@input = Hanami::Action::ParamsExtraction.new(env).call | ||
validation = validate | ||
@params = validation.to_h | ||
@errors = Hanami::Action::Params::Errors.new(validation.messages) | ||
freeze | ||
end | ||
|
||
attr_reader :errors | ||
|
||
# Returns true if no validation errors are found, | ||
# false otherwise. | ||
# | ||
# @return [TrueClass, FalseClass] | ||
# | ||
# @since 2.2.0 | ||
# | ||
def valid? | ||
errors.empty? | ||
end | ||
|
||
# Serialize validated params to Hash | ||
# | ||
# @return [::Hash] | ||
# | ||
# @since 2.2.0 | ||
def to_h | ||
validate.output | ||
end | ||
alias_method :to_hash, :to_h | ||
krzykamil marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
attr_reader :result | ||
|
||
# Returns the value for the given params key. | ||
# | ||
# @param key [Symbol] the key | ||
# | ||
# @return [Object,nil] the associated value, if found | ||
# | ||
# @since 2.2.0 | ||
# @api public | ||
def [](key) | ||
@params[key] | ||
end | ||
|
||
private | ||
|
||
# @since 2.2.0 | ||
def validate | ||
Result.new( | ||
self.class._validator.call(@input) | ||
) | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
# frozen_string_literal: true | ||
|
||
require "rack/request" | ||
|
||
module Hanami | ||
class Action | ||
# since 2.2.0 | ||
# @api private | ||
class ParamsExtraction | ||
def initialize(env) | ||
@env = env | ||
end | ||
|
||
def call | ||
_extract_params | ||
end | ||
|
||
private | ||
|
||
attr_reader :env | ||
|
||
def _extract_params | ||
result = {} | ||
|
||
if env.key?(Action::RACK_INPUT) | ||
result.merge! ::Rack::Request.new(env).params | ||
result.merge! _router_params | ||
else | ||
result.merge! _router_params(env) | ||
env[Action::REQUEST_METHOD] ||= Action::DEFAULT_REQUEST_METHOD | ||
end | ||
|
||
result | ||
end | ||
|
||
def _router_params(fallback = {}) | ||
env.fetch(ROUTER_PARAMS) do | ||
if (session = fallback.delete(Action::RACK_SESSION)) | ||
fallback[Action::RACK_SESSION] = Utils::Hash.deep_symbolize(session) | ||
end | ||
|
||
fallback | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
# frozen_string_literal: true | ||
|
||
require_relative "params" | ||
require_relative "contract" | ||
|
||
module Hanami | ||
class Action | ||
|
@@ -17,6 +18,12 @@ module Validatable | |
# @since 0.3.0 | ||
PARAMS_CLASS_NAME = "Params" | ||
|
||
# Defines the contract base class | ||
# | ||
# @api private | ||
# @since 2.2.0 | ||
CONTRACT_CLASS_NAME = "Contract" | ||
|
||
# @api private | ||
# @since 0.1.0 | ||
def self.included(base) | ||
|
@@ -102,7 +109,14 @@ def params(klass = nil, &blk) | |
klass = const_set(PARAMS_CLASS_NAME, Class.new(Params)) | ||
klass.class_eval { params(&blk) } | ||
end | ||
@params_class = klass | ||
end | ||
|
||
# @since 2.2.0 | ||
# @api public | ||
def contract(&blk) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just noting that while we allow existing schemas to be passed in directly, the new code here does not support this for contracts. Given how contracts are instances, and may also have their own dependencies, I think this is a reasonable decision, and we can always choose to reevaluate it later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I understand what you want here, but I am not sure how would this look in terms of DSL. Lets say I have a NewUserContract defined somewhere already, and I want to use it in my new action. Should I use Deps to include this contract? Then how do I use it in the action class? Asking because while using contracts as instances is not new to me, I am not sure how to implement reusability of those contracts on a class level? I would gladly do it if I get a little bit of push in the right direction :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since a preexisting contract should have a container key, it would be nice to reference it by key. But that presents the problems you've described. I think passing the contract class as an argument could be supported without much effort, but since contracts are not composable this presents a problem that Nevertheless, I still think passing a contract by class name would be useful. Otherwise if I want to share contract logic between multiple Actions or elsewhere, I must implement it multiple times. |
||
klass = const_set(CONTRACT_CLASS_NAME, Class.new(Contract)) | ||
klass.class_eval { contract(&blk) } | ||
@params_class = klass | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I trace along the new logic in the PR:
In an action where a user defines a contract with
contract do
, then an instance ofHanami::Action::Contract
will become theparams
object that we provide to thehandle
method as part of therequest
.Am I understanding this correctly?
If so, since this
params
object can now be an instance of eitherHanami::Action::Contract
orHanami::Action::Params
, I think we want to make sure they have the same interface.Why is this? I think it would be perfectly acceptable for a user within a single app to have
params do
in some action classes, andcontract do
in others. So this means that across a single app, the user will get different kinds ofparams
instances, but that user should expect those objects to behave identically.Right now
Hanami::Action::Params
implements a few things that the newHanami::Action::Contract
does not:#raw
#error_messages
#deconstruct_keys
#get
,#dig
(viaBaseParams
)#each
(viaBaseParams
)We need to make these consistent before we can be done with this PR.
I reckon we have a couple of options to consider here:
Hanami::Action::Contract
so that it has the same interface asParams
andBaseParams
I think I like option (2) better. But I'd be keen for your thoughts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option 2 sounds much better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well going with option 1 we would have easier time to change either of the implementation if there is ever a need for it, change of approach.
But for now it makes no sense to "guess" that it might happen, so I like option 2 better too.
I'll try to implement it in the next couple of days.
Thanks for this I completly missed those few methods I did wanted them to have interface as close as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @krzykamil! Looking forward to seeing how you go, and of course I'm happy to provide any feedback you might need along the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timriley I've pushed the changes in the last two commits.
The idea is:
Common functionality/interface can be divided into two modules:
So I created two modules, included in BaseParams and ContractClass (not in Params since it inherits from BaseParams). One of them includes the conversion methods like to_h, and getters like dig, get [] etc. Those are the base functions.
The other module handles the validations parts, extracting params, handling and parsing errors, the more specific stuff connected to Dry::Validation. It also sets the same way to initialize both Params and Contract classes.
This left both Contract and Params classes as rather slim
Params only has
self._base_rules
self.params(&blk)
Those two class methods left. Contract respectiely has self.contract but also some extra stuff that Params class gets from the hanami validations gem, that I could not get around.
Also take note of the _base_rules, I did not implement an adequate method for Contract class, as I wasn't sure if we should have that there and if yes, what is the best way to do it (would appreciate a suggestion)
Anyway, let me know if this approach is OK, what else can be modified and improved.
I've copied a lot of specs from Params specs, to ensure their interfaces are indeed the same and they react similarly to similar operations