-
-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add contract support to actions #453
Conversation
4b90cb9
to
e30f71a
Compare
…dations code Add `Hanami::Action::Params.contract` as a counterpart to `Hanami::Action::Params.params`, allowing `Hanami::Action.contract` to use this API as opposed to directly setting a class-level ivar. This cleans up the remaining “hacky” aspect of my initial `Hanami::Action.contract` introduction. Secondly, stop using code from `Hanami::Validations` altogether. I always found this code too indirect and hard to follow. Instead, introduce the one bit of important logic (the base validator class permitting the `_csrf_token`) directly into `Hanami::Action::Params`, which means this class is now entirely standalone. This will make it easier to adjust/maintain in future.
397bc60
to
e63ba72
Compare
Should this say
-such +support |
@parndt yes, fixed now |
# @example Concrete class | ||
# require "hanami/controller" | ||
# | ||
# class SignupParams < Hanami::Action::Params |
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.
I'm conflicted about this decision. The reason why I would most likely want to use a concrete class for this is to share validation rules between multiple things.
By basing this on Hanami::Action::Params
, this requires a request env to initialize, which limits its usefulness to controllers only. This excludes non-request sharing, such as Operations or CLI tools.
I wish contracts were composable. If I could merge a preexisting contract into the action validator, this would be moot.
Another way you can share logic is via macros. My preference would be to define an abstract class to contain these macros because I may want to use different ones for different use-cases. But since you hard-code Dry::Validation::Contract
as the base class here, I can only share macros globally. I don't know if that's a problem.
On the other hand, perhaps its reasonable that HTTP-boundary validations are their own thing.
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.
@alassek Thanks for raising this! I agree with you here. If I was building this feature from scratch today, I wouldn't have things run through these Hanami::Action::Params
intermediary classes, and instead have dry-validation contracts be provided directly.
I'd describe this current approach as a vestige of the 1.x-era of Hanami design, where it was much more heavy-handed in "owning the APIs" rather than the lighter touch approach we take for our integration with the dry-rb gems today. It's also worth mentioning that back in those times, dry-validation might have seemed like a slightly less stable target, so the framework builders might've wanted this extra layer as "insulation."
In terms of why I implemented it this way here: I wanted a small, minimally-disruptive change as a first step, something I could merge confidently and use as a launching point for any next steps. I didn't say it out loud, but I did want us to get to the point of supporting dry-validation contracts directly, but I wasn't sure about when would be ideal for that.
Your question motivates me to have a quick run at it as a follow-up and see what might be possible :)
As an additional measure, I'm going to mark the new Hanami::API::Params.contract
method here as @api private
, as part of a "soft deprecation" approach to this direct use of the Params class. I'd much rather have users specify their validations at the action level only, and use contracts directly as a way to reuse it. Let's see how it goes!
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.
I've started this work in #454. I think I have a pretty clear path to getting it done, too. I'll leave another update over in that PR once it's ready!
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.
#454 is ready now :)
Thanks for the mention, glad I was useful again :) |
Build upon the foundation that @krzykamil made in #451 (thank you, @krzykamil, we truly wouldn't have gotten here without you! ❤️) and take a second approach at adding support for dry-validation contracts for
Hanami::Action
params.The goals of this approach:
params
object within actionsHere's how we do it:
Hanami::Action.contract
method as the counterpart to.params
. This is the main point of interaction for users.Hanami::Action::Params.contract
method as the counterpart to.params
. Unlike params, this passes the given block to the top-level of aDry::Validation::Contract
subclass, allowing the user to use all features of dry-validation contracts.@_validator
insideHanami::Action::Params
. This was the missing concept in the previous approaches. This validator will the dry-validation contract created by either theparams
orcontract
methods.Hanami::Validations::Form
insideHanami::Action::Params
. This module was doing very little, at the cost of a lot of (cross-gem!) indirection. It is now replaced by the@_validator
as well as theHanami::Action::Params::Validator
base class that is used for the generated schema or contract, which is what ensures a_csrf_token
param is permitted at all times. Now all the code dealing with param validation is kept all together inside hanami-controller, so it's easier to follow and maintain.That's it!
@krzykamil and @solnic — given you were both active within #451, I'm keen for any feedback you may have here.
@parndt, I'll ping you here too, since I know you've been hanging out for this change :)
I've made a related PR (hanami/validations#230) that removes all the code from hanami-validations and leaves it as a gem to manage the dry-validation dependency only. The code in hanami-validations that we shipped in 2.0 I left entirely as
@api private
, so it is safe to remove. The only remaining interaction with this gem was throughHanami::Action.params
, which continues to work unchanged.I do want to acknowledge one known limitation of the approach I've taken here: dry-validation contracts, unlike dry-schema schemas, are classes that are expected to be initialised, and as such, can receive their own dependencies. Right now, we do not support such use of contracts, because of how they are defined at the class level within actions. Later, we could choose to find a way to support contracts with dependencies, but I think we can safely leave that for the future after unlocking their basic usage with this PR. Users have a workaround in the meantime too, which is to make such contracts actual dependencies of the action, and then run then manually run params through those contracts within the
#handle
methods.Resolves #441, resolves #451, resolves #434