Simplify handling of validation input args

[timriley]

Instead of needing to introspect method params in order to allow a subset of keyword args to be passed through alongside those validated by a contract, put in place a simpler set of rules around how input args are handled:

• Validated input is the last positional arg, if any are given
• When no positional args are given, validated input is the keyword args in full

This allows us to remove param introspection altogether, while still giving the user a range of flexible param signatures. They can be any combination of e.g. def call(id, input, notify: false), or just def call(input) or def call(**input) in the simplest cases.

As part of this change, fix an error arising from this extension that made .call raise an ArgumentError when no arguments were given. This should be allowed when the operation’s #call method itself doesn’t require arguments.

This is an alternative approach I'm proposing to @aaronmallen's #46.

[aaronmallen]

Mi gusta

[alassek]

I have two concerns with this approach. After the great keyword argument migration of 2.7/3.0, this pattern feels really awkward to me:

update_user.call(123, {name: "Alice", admin: true}, notify: true)

It's not very clear from looking at this without context why admin: true is inside the hash, and notify: true is outside. And looking at the Operation class itself won't answer this question, it's implicit behavior tucked away in the framework. Don't you think this is likely to become a major footgun for people?

A pretty common pattern in my codebase (about a quarter of operations) use the following pattern:

# Authorize a pending authorization code by associating it with an account and scope.
#
# @param client [Entities::AuthorizationCode] the pending authorization code to authorize
# @param account_id [String] the ID of the account to associate with the authorization code
# @param scope [Array<String>] the scopes to associate with the authorization code
#
# @return [Success(Entities::AuthorizationCode)]
#
# @return [Failure[:type, [Symbol, Object]]] if any parameters are invalid
# @return [Failure[:invalid, Entities::AuthorizationCode]] if the client is not eligible for authorization
# @return [Failure[:authz, Response]] principal failed authorization check
# @return [Failure[:http, Response]] if there was an error communicating with the authz service
# @return [Failure[:persistence, ROM::SQL::Error]] if there was an error saving the authorized client
def call(client, account_id:, scope: [])

Our operations are data-oriented, so generally there is a primary argument which is the struct provided by a repository, followed by kwargs for things we are doing with it.

One of the top reasons why I'm interested in adopting Dry::Operation is the validation DSL. So I'm curious how we could provide a simple way to customize this behavior. I think what we're missing is an explicit way to define how the validation contract is applied to the arguments.

[katafrakt]

After reading @alassek's comment, I agree with him. It feels a bit weird to distinguish between params and non-params arguments. To me name: 302 should be as much of an error as notify: "nobody", as both should be a dev error at this point - even passing wrong argument from the code or letting non-validated data to leak into the operation.

[timriley]

@alassek & @katafrakt, thank you both for sharing your thoughts here, this is so valuable!

I take your point, and with this in mind I think it does make sense to have the contract validate the entire input without exception.

My hope when trying for something subtler was to allow for e.g. sharing a contract that validates the structure of the entity that the operation is working upon, while still allowing operation-only args to work alongside it.

For those operation-only args, I feel like there's still merit in allowing them to work outside the validation contract, because if they're to be supplied by the programmer only, there's no way for them to be correctable by the end user. So if the validation errors are being presented back to the user, they'll see things that don't make any sense to them.

I'd rather get a hard exception if a programmer supplies an invalid argument in that way, but I can understand how it might be nice to give users either approach.

What do you think?

Anyway, I'll have a go at re-working this PR, possibly moving it closer to the original handling, where named kwargs remain permissible outside the contract, hopefully without as much complexity as #46.

Definitely keen for continued feedback, please keep it coming!

[timriley]

What about this?

# validates the `attrs:` kwarg only
contract for: :attrs do
  # ...
end

# validates all kwargs as a whole
contract do
  # ...
end

And we'd leave positional args fully alone.

Simple clear API, covers both use cases, no params introspection required.

[alassek]

@timriley I like that proposal a lot more, I think it solves the problem at hand and it provides the explicitness that I'm looking for. I think if we're going to implicitly validate anything, targeting the kwargs makes a lot more intuitive sense over positional, since dry-schema presupposes key-value shape.

Presumably, the :for argument could also accept a list of kwargs

contract for: %i[attrs options] do
  # ...
end

[katafrakt]

I like contract for:. In fact this is something I have been thinking about, but as usual you came up with much nicer API.

[timriley]

Thanks @alassek and @katafrakt for your thoughts! I'm happy the direction feels good to you :)

@alassek — I hadn't actually considered giving a list of kwargs to for:. This is intriguing! But I want to play out some scenarios with you to better understand this.

So, for the single-arg form, I was expecting to pass that arg as a whole value to the validation contract. For example:

• Given contract for: :attrs
• When attrs: {name: "Jane"} is given as the operation's args
• Then {name: "Jane"} will be sent to the contract

For the multiple-args form, how would you handle it? I would anticipate it working like this this:

• Given contract for: [:attrs, :options]
• When attrs: {name: "Jane"}, options: {notify: true} are given as the operation's args
• Then {attrs: {name: "Jane"}, options: {notify: true}} will be sent to the contract

The (potential) issues with this:

• It's not consistent with the single-arg form, because now we're also sending the enclosing keys to the contract as well as their values.
• If we decided not to send the enclosing keys, then I don't see this being particularly helpful.

But perhaps this difference in behaviour might turn out to be beneficial? Because it opens up a third use case: validating a named subset of the operation's kwargs. If we thought about the case of them not being nested hashes, then it starts to feel more sensible:

• Given contract for: [:name, :notify]
• When name: "Jane", notify: true, another: "hi" are given as the operation's args
• Then {name: "Jane", notify: true} will be sent to the contract

What do you think?

This would end up being something that we include in our docs, so we have clear examples of each form.

[timriley]

Come to think of it, maybe it's best we avoid having one option play double duty. We could possibly use mutually exclusive options for this:

contract for: :single_key_here for when you want to pass a single value only, and perhaps contract keys: %i[key names here] for when you want to pass a subset of the keyword args, with the keys included?