Allow Dry::Validation::Contract rule definitions in Hanami::Action

[danhealy]

For a Validatable Hanami::Action (including hanami-validations), calls to rule inside the Hanami::Action are passed through to the Dry::Validation::Contract class via the Hanami::Action::Params class.

class Event < Hanami::Action
  params do
    required(:start_date).value(:date)
    required(:end_date).value(:date)
  end

  # Rules must be defined after the params schema.
  rule(:start_date, :end_date) do
    if start_date > end_date
      base.failure('event cannot end before it starts')
    end
  end

  def handle(req, *)
    halt 400 unless req.params.valid?
    # ...
  end
end

Reversing the order of params and rule will raise an exception, this is the same constraint that Dry::Validation::Contract has.

#421

[parndt]

@danhealy FYI, looks like main is the same as 2.1.x (0 files changed) so I've updated the PR's base base branch to main (which is why there are now 4 merge commits which would disappear if you rebased from main). Hope this is helpful!

[parndt]

@danhealy This patch works exceedingly well for me! Previously I was using this great code from @katafrakt https://github.com/katafrakt/palaver/blob/d1a31fec7dba6638ec798c419cecf35ce826f28e/lib/palaver/action.rb#L18-L25 but I've been able to remove that and all of my contract do wrappers and use what I had written inline instead.

[danhealy]

Thank you for the feedback @parndt ! I removed the extra guard and rebased on main

[parndt]

@danhealy awesome, I pushed a fix for rubocop offenses to your branch so that CI will not fail on that.

[parndt]

@jodosha @timriley how do you want to handle this? It would be good to get this in for 2.1 but I'll leave that up to you. Either way we should changelog it?

[danhealy]

I'll fix the remaining style issues

[parndt]

This works really well in my app!

[parndt]

I'll fix the remaining style issues

nice, your fixes mirror exactly how I was going to do it.

[parndt]

Woohoo ✅ CI 🎉

[timriley]

Thanks so much for this, @danhealy! Got it done by conference end, amazing stuff 😍

I'm just about to enter into various metal tubes for 24h, so I'll look at this once I'm back early next week.

[timriley]

@danhealy Thanks again for making this! So it turns out in America you don't have to go through customs before leaving the country, which means I have a bit of bonus computer time! I won't be able to go through the code in detail yet, but do have one thought here for you consideration:

After seeing this in totality, I wonder if a better API for this would be (building from your example):

params do
  required(:start_date).value(:date)
  required(:end_date).value(:date)
end

rules do
  rule(:start_date, :end_date) do
    if start_date > end_date
      base.failure('event cannot end before it starts')
    end
  end
end

In this way, we just have two clear, well-contained blocks for action params validation: rules and params. This means they also mirror each other in terms of having pluralised names that each contain one or more of the items they describe (individual param schema entries and individual rules, respectively).

In such an approach, I think we'd just take the block passed to rules and eval that in the context of the contract class. In this way, I think it could make the dry-validation integration simpler and more flexible, since inside rules gives you the ability to do whatever else you want inside that validation contract context.

Now, if we did take this approach, it does make me wonder if contract might be a better name for this method (despite the positive aspects I noted about rules before): it'd make it clearer that it's working with the context of the contract class, and it could also work where it's the only block you use, if that's your preference, e.g.

contract do
  params do
    required(:start_date).value(:date)
    required(:end_date).value(:date)
  end

  rule(:start_date, :end_date) do
    if start_date > end_date
      base.failure('event cannot end before it starts')
    end
  end
end

Having contract eval straight into a contract class might also open up interesting avenues where users want to use different schema types, e.g. json from https://dry-rb.org/gems/dry-validation/1.10/schemas/. Though I think we'd want to test what that looks like in practice before deciding if that's something we want to allow or discourage.

As you know from our earlier discussions, @danhealy, I think it's worth us thoroughly exploring all our API design options here. As someone who's been active in this space now, I'm interested in your thoughts on the above!

And thanks again for your code contributions — having these changes be real and concrete and right here in front of our eyes is going to help so much for us to get this much needed feature done :)

In the meanwhile, I'll continue to think on this. Will get back to you again when I'm (a) in Australia, and (b) have any further developed thoughts to add.

--

@parndt — I love your enthusiasm here. 😉 Technically, we've frozen 2.1, and our attention should be on making sure any fixes to existing features are squared away before considering other things.

For this feature, as you can see from my thoughts above, I don't think it's so cut and dry and ready to go that we can just drop this in before the release. I want to make sure we give ourselves the time to arrive at the most helpful design. In addition, @jodosha's attention has been on our assets fixes and I don't want to rush him with this one either. ☺️

If you have thoughts about my musings above, though, Phil, I'm definitely keen to hear them too!

[parndt]

contract do is what I had before (for many months now, via https://github.com/katafrakt/palaver/blob/d1a31fec7dba6638ec798c419cecf35ce826f28e/lib/palaver/action.rb#L18-L25) and in practice what this meant is that I had a further abstraction over the top of the regular params do and had to redeclare optional(:_csrf_token).maybe(:str?) (I noticed that otherwise any param that wasn't declared inside contract do got wiped, including CSRF protection). Maybe this was an implementation detail of the way that it was being done, or maybe not, but I wanted to note it. 😄

Personally, I quite like params do followed by any number of rule blocks, but open to perspectives too. I guess I don't see the point in adding more abstractions to learn when these concepts exist already and can be hooked into - via DRY. It's also backwards compatible rather than having to wrap any existing definition of params do inside contract do to get this functionality.

[timriley]

Yeah, I should make this clear: I would like to keep params do working no matter what else we do. We must do this anyway, to preserve API stability across 2.x. And if you have an action that does not need any rules, then params do is still the most straightforward way of declaring your params schema.

The reason I'm not so keen (at this point, at least!) on having many individual rule blocks declared directly in the class body of actions is that it suggests that the "rule" behavior is a feature built into the action itself, when the reality is that it's actually just a light-touch integration with extra features offered via dry-validation contracts.

IMO, the fact that this is using dry-validation is not just a minor implementation detail: we should be making this clear to users, for many reasons:

• so they know why that gem will need to be present
• they'll likely want to no look up dry-validation docs to learn about all the different features they can exercise (I don't think it makes sense for hanami to entirely duplicate those)

In this way, I like the rules wrapper block because it is a stronger hint that the rules in aggregate are a part of a combined thing, which we can make clear is externally-powered by dry-validation itself. This also provides a contrast to eg. the class-level before callbacks and the like, which are, in fact, intrinsic behaviors of Action.

[danhealy]

I think I prefer the current implementation for the following reasons:

• Brevity, this implementation avoids the doubly nested rules do .. rule do (and similarly, without having to nest params do)
• DSL matches that of Dry::Validation::Contract to the extent that a future simple integration via include Dry::Validation::Contract is compatible.

My complaints with the current implementation:

• Too tightly coupled with the implementation details of Dry::Validation::Contract while not providing much additional benefit. That complaint would go away if it is a stated goal and the expectation to the user is clear that Hanami::Action is directly mixing Dry::Validation::Contract, over the current more abstract behavior of hanami-validations.
• It feels awkward to use params as a receiver for an instance of Dry::Validation::Contract, rather than something more obvious like contract.

Maybe the current DSL + an alternative, explicit receiver for a contract instance would be the best balance?

I don't have a good conceptual judgement about the balance of the ease of use versus potentially surprising behavior of Hanami::Action behaving as Dry::Validation::Contract only sometimes because of the gem inclusion, but I think that allowing some but not all of Dry::Validation::Contract's DSL is probably not right.

Also, my opinions here include a caveat that they are based on abstract feelings rather than actual usage.

[jodosha]

Hey @danhealy, thanks for this enhancement. 👏

TL;DR: Here's my proposal:

• Keep params working as it is.
• Introduce contract with params and rule support.

---

SemVer compatibility

We need to keep params to work as it they are because of Semantic Versioning.

Why contract?

Reason 1: Because we can forward the block as it is to dry-validation.

For Hanami 3, we can consider deprecating params in favor of contract only.
This move would reduce the public API surface of hanami-controller, delegating to dry-validation the entire contract handling.

Reason 2: Because rule on its own is meaningless.
While params has a distinct connotation, the developers may wonder, "What the heck is a rule?".
It can be a rule for accepting or rejecting MIME Types or an authorization rule... You name it.

But if we wrap it into contract, then the context is explicit.

---

cc: @timriley @parndt

[timriley]

Thanks for checking this out and sharing your preference, @jodosha :)

As @danhealy knows from our (in person! so cool) discussions on this, I didn't have one strong opinion about the exact shape of the API for this, so having you share your thoughts here helps a lot.

I'm happy to go with your suggestion. It's straightforward and provides a single integration point: "contract" builds or accepts a Dry::Validation::Contract subclass. That's easy to explain to users, and it will keep the integration with the external dry-validation gem as simple as possible.

The only downside is that for actions that don't need a contract rules, we get this double nesting, which isn't the ideal experience:

contract do
  params do
    required(:start_date).value(:date)
    required(:end_date).value(:date)
  end
end

An option here could be to keep params do within Action so that cases like this can just use that directly, rather than contract, even though leaving two similar methods in place (for 3.0 or beyond) does muddy the water a bit. In the meantime, however, we have to keep both, so this could be a good opportunity for us to learn from experiences in the wild.

(Another option here could be to rejig dry-validation so that it doesn't need a nested params block like that, but that's an idea for another day!)

@danhealy — since I think we have a clear path now, would you be up for adjusting this PR? Thanks again! ❤️

[parndt]

Cool - please let me know when you'd like me to test it out with my app 😄

We're changing this implementation

[parndt]

Is this still the direction we're headed?

[timriley]

@parndt Yes indeed, I'd be happy to review a revised solution here.

[danhealy]

Sorry for letting this sit so long! The proposal by @jodosha is reasonable, supporting the syntax will be a bit more substantial of a change because it would first depend on modification to hanami-validations.