-
-
Notifications
You must be signed in to change notification settings - Fork 7
Add Params extension for automatic input validation with dry-validation #34
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
base: main
Are you sure you want to change the base?
Conversation
5887ecc to
f2fa8fd
Compare
|
I think this is a cool idea, but considering how similar it appears to the Action method, don't you think it should work the same? Specifically, there are two things Action can do that this does not: Re-use an existing Schema: class CreateUser < Operation
include Dry::Operation::Extensions::Params
params UserSchema
endDeclare a Validation contract: class CreateUser < Operation
include Dry::Operation::Extensions::Params
contract do
params do
# ...etc
end
end
end |
|
@alassek accepting a class as an argument is straightforward enough I like that idea. But maybe I'm misunderstanding the difference between a schema and a contract? Aren't we explicitly using the contract functionality here? |
|
@aaronmallen well no, you're instantiating a
|
|
@alassek I guess the question becomes Do we wrap the entire functionality behind the dependency on dry validation or do we support contracts in a follow up PR and extend this extensions functionality that is wrapped in the dependency of dry validation? |
|
@aaronmallen well that is a good question that we should probably discuss with the others. My preference would be to always return a consistent result type, so that it's a |
ed90fc6 to
bb72584
Compare
|
This is cool! I did something similar as an addon for Dry::Transaction. Instead of just An example of it in use: class VerifyUserToken
include ApplicationTransaction
validate do
params do
required(:user_token).filled(type?: UserToken)
required(:session).filled(type?: ActionDispatch::Session)
end
rule(:user_token) do
key.failure("expired token") unless value.issued_at.after?(1.hour.ago)
end
rule(:user_token, :session) do
session_key = values[:session][session_key_name]
token_key = values[:user_token].session_key
if session_key.blank? || !ActiveSupport::SecurityUtils.secure_compare(session_key, token_key)
key.failure("session mismatch")
end
end
end
endJust something to consider. |
bb72584 to
8c6657d
Compare
8c6657d to
e056822
Compare
|
Hey folks, unfortunately I haven’t been closely following Hanami’s direction lately. The extension looks great, and I definitely think that returning a |
|
@waiting-for-dev IMO this doesn't really break separation of concerns for two reasons:
|
|
I use dry-operation and dry-validation extensively with Rails, and I considered creating an extension like this. |
Agree about 1. About 2, I think it's really the same case wherever it's used (e.g. Rails) 🙂 Eager to hear @timriley 's thoughts! |
|
While I recognize it is likely a gross violation of separation of concerns, in practice I found the implementation of it on Transactions to be wildly successful. First of all, it provided almost a "type signature" that described what kwargs a transaction expected, the types of values, and the state it expected them in. Second is that we exposed the validation defined by the DSL as a class method on the Transaction. This allowed external callers decide if they should invoke the Transaction, and if a validation failure should be handled or ignored. To provide an example, let me first describe part of our domain. The application is a messaging app, like the gmail UI, which had "Conversations" that could be "open" or "closed". They could be opened or closed manually by the user, but also as a side effect from a dozen other operations, like a new incoming message would reopen a closed conversation but not try to open an already open one. To perform the OpenConversation task, we had a Transaction that would validate that the conversation was currently closed: class OpenConversation
include ApplicationTransaction
validate do
params do
required(:conversation).filled
required(:user).filled
end
rule(:conversation) do
key.failure("is already open") if value.open?
end
end
step :create_open_event
step :transition_the_state_machine
step :run_any_callbacks
# ...One of the step DSL methods we added was class ReceiveMessage
include ApplicationTransaction
validate do ... end
# Steps to parse the input, create a Message object, locate/create the Conversation, etc...
maybe OpenConversation
# More steps
endThe maybe step is conceptually equivalent to: result = OpenConversation.validator.call(input)
result.failure? ? Success(input) : OpenConversation.new.call(input)It was also beneficial for background jobs:
Overall, while yes it was probably a violation of separation of concerns, the pragmatic utility and convenience we got more than made up for it. |
I agree with you 100% that your user-input should always be sanitized at the outer boundary, but data validation needs to happen at all layers of an application, and that validation should become more constrained the deeper into the system it flows. I've give you an illustrative example: consider an OAuth token endpoint. The different combinations of parameters it must accept is very broad; but when dispatching these requests to different Operations, you'll need much more constrained validation of params for each use-case. This kind of situation is when I have resorted to using Schema and Contracts to validate the public interface of Operations. class Token < Action
include Deps["authn.client_secret", "authn.client_assertion"]
params do
required(:grant_type).filled(:string)
required(:client_id).filled(:string)
required(:resource).value(T::Coercible::URI)
optional(:scope).filled(:string)
optional(:client_secret).filled(:string)
optional(:client_assertion).filled(:string)
optional(:client_assertion_type).filled(:string)
end
def handle(req, res)
authn_result = Failure(:unauthorized)
if req.params in { client_assertion: }
authn_result = client_assertion.(req.params)
elsif req.params in { client_secret: }
authn_resuilt = client_secret.(req.params)
end
halt :unauthorized unless authn_result.success?
# etc
end
end
I would suggest an alternative approach: it should only apply to the public interface But in addition to this, dry-operation should follow the do-notation convention that dry-monds uses of calling |
|
Thanks putting this together, @aaronmallen, and thanks to everyone for all the insightful discussion above! I'm in favour of a feature like this. I think it'll be really useful and I expect a lot of users will want to adopt this. I've used validation contracts in many of my operation objects and I think there's good reason to have them there. I haven't had the time to look closely at the code yet; I've been focused on getting Hanami 2.3 shipped next week and this hasn't left me with any spare time. I'd like to have a closer look at the design and the code before we figure out how to merge this. I agree with @alassek that there may be alternative implementations we want to consider. Thanks again for your input and your patience, folks. Once we do get this in, it'll be a really nice enhancement for our users :) |
|
Thanks for the feedback, folks. I agree this is a great extension 🙏 |
|
@waiting-for-dev any idears as to why I'm having build failures for the active_record extension here? |
@aaronmallen, I think that if you rebase from master everything will be green now 🙂 @timriley fixed it in #35 |
Adds input validation support to operations using dry-validation. This follows the Hanami controller pattern where params and contract methods create anonymous Params classes for validation. Key features: - Automatic validation of operation inputs before execution - Support for both params DSL and full contract API - Params class reusability across operations - Integration with operate_on for custom wrapped methods - Params class inheritance for operation hierarchies - Returns Failure[:invalid_params, errors] on validation failure Includes comprehensive unit tests covering validation logic, method wrapping, params class creation and inheritance, and contract validation with custom rules.
Adds comprehensive integration tests demonstrating real-world usage: - Validating operation inputs with success and failure scenarios - Value coercion according to schema types - Nested structure validation with detailed error reporting - Custom wrapped methods via operate_on - Params class reuse across multiple operations - Contract method with custom validation rules - Params class inheritance from parent classes
Adds documentation for the Params extension to the extensions guide, including: - Overview of input validation with dry-schema - Installation and setup instructions - Basic usage examples showing success and failure cases - Schema class usage for reusing schemas across operations - Integration with custom methods via operate_on - Schema inheritance behavior
e056822 to
2ad1551
Compare
Summary
Adds a new
Paramsextension that brings automatic input validation to operations using dry-validation. Following the Hanami controller pattern, operations can define validation rules viaparamsorcontractmethods that create Params classes to validate and coerce inputs before any operation logic executes, providing fail-fast validation with detailed error messages.Motivation
When building operations, input validation is a common requirement. Currently, users need to manually validate inputs within their operation methods, leading to boilerplate and inconsistent error handling. This extension provides a declarative way to define input contracts that integrate seamlessly with the operation's existing failure handling, inspired by Hanami's controller actions.
Usage
Basic params validation
Reusable Params classes
Custom validation rules with contract
What's Included
Failure[:invalid_params, errors]on validation failure#callby default and custom methods via.operate_onImplementation Details
paramsandcontractmethods create anonymous Params classesDependencies
Adds dry-validation as a development/test dependency (only required when using the extension).