-
-
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
Closed
Closed
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
dd319d4
First basic version - prototype
krzykamil 47b719a
More details in documentation and slight refactors
krzykamil bb742e1
fix constant name
krzykamil 4e052b8
Second version, that aligns contract more with params
krzykamil c64c741
remove debug statements
krzykamil ef832c6
Some missing doc statements, remove useless code
krzykamil 59f1216
Fix rubocop offenses
krzykamil 9debfdb
Better names for methods, remove obsolete extraction
krzykamil 56081d2
Use ivar in to_h implementaion
krzykamil 9a264e1
Extract common interfrace of Params and Contract
krzykamil 7733380
Final small changes
krzykamil 3f26e7c
Fix confusion with #raw
krzykamil File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
# 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 | ||
include Hanami::Action::RequestParams::ActionValidations | ||
include Hanami::Action::RequestParams::Base | ||
# 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 to_h | ||
__getobj__.to_h | ||
end | ||
krzykamil marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# This method is called messages not errors to be consistent with the Hanami::Validations::Result | ||
# | ||
# @return [Hash] the error messages | ||
# | ||
# @since 2.0.0 | ||
# @api private | ||
def messages | ||
__getobj__.errors.to_h | ||
end | ||
krzykamil marked this conversation as resolved.
Show resolved
Hide resolved
|
||
end | ||
|
||
# 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) | ||
@_validator = Dry::Validation::Contract.build(&blk) | ||
end | ||
|
||
# @since 2.2.0 | ||
# @api private | ||
class << self | ||
attr_reader :_validator | ||
end | ||
|
||
private | ||
|
||
# @since 2.2.0 | ||
def validate | ||
Result.new( | ||
self.class._validator.call(@input) | ||
) | ||
end | ||
end | ||
end | ||
end |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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