Feature/action contracts

[krzykamil]

#441

This allows to use contact do blocks, known from dry-validation gem, in Hanami Actions (as per examples in the spec).

It is largely based on Params class, but there is less code in the Contract, since the major difference is that, Contract, delegates the work to dry-validations (Params does that too, but much later, after doing some more stuff, and it builds the schema, not the full contract, limiting the usage of rule blocks etc).

The return is still put into request.params and generally behaves the same in the end, giving more options in validations.

There is an important note, left in the code comments

# 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.

The contract usage possibility should be added to the documentation, I think right after this bit: https://guides.hanamirb.org/v2.1/actions/parameters/#validations-at-the-http-layer but that is up for discussion.

Any comments would be appreciated, as this code base was completely new to me, and at moments a bit overwhelming. Not sure if I did a good job with the documentation too.

[timriley]

Thanks @krzykamil, I'll check this out soon!

[solnic]

Thanks for working on this! There are a couple of things to figure out before we can merge this in.

[solnic]

Can't we just do @_validator = Dry::Validation::Contract.build(&blk) here and remove validations method?

[krzykamil]

You're right, it was pointless.

Thanks for the review (and while I am at it - for your OS work, that I've used extensively over the years and greatly admired). Applied all changes now.

[solnic]

Thanks @krzykamil 😃

[timriley]

This is looking good, @krzykamil!

I've had a read through of the code here and left a couple of notes.

Next step for me is to pull it down and have a proper explore and play. I'll get back to you again as soon as I can.

[timriley]

Just noting that while we allow existing schemas to be passed in directly, the new code here does not support this for contracts.

Given how contracts are instances, and may also have their own dependencies, I think this is a reasonable decision, and we can always choose to reevaluate it later.

[krzykamil]

I think I understand what you want here, but I am not sure how would this look in terms of DSL.

Lets say I have a NewUserContract defined somewhere already, and I want to use it in my new action.

Should I use Deps to include this contract? Then how do I use it in the action class?
Or should I just NewUserContract do and use that in the contract method as an argument, or pass it to the block?

Asking because while using contracts as instances is not new to me, I am not sure how to implement reusability of those contracts on a class level?

I would gladly do it if I get a little bit of push in the right direction :)

[alassek]

Since a preexisting contract should have a container key, it would be nice to reference it by key. But that presents the problems you've described.

I think passing the contract class as an argument could be supported without much effort, but since contracts are not composable this presents a problem that params does not: passing a class would be mutually incompatible with passing a block.

Nevertheless, I still think passing a contract by class name would be useful. Otherwise if I want to share contract logic between multiple Actions or elsewhere, I must implement it multiple times.

[timriley]

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 of Hanami::Action::Contract will become the params object that we provide to the handle method as part of the request.

Am I understanding this correctly?

If so, since this params object can now be an instance of either Hanami::Action::Contract or Hanami::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, and contract do in others. So this means that across a single app, the user will get different kinds of params instances, but that user should expect those objects to behave identically.

Right now Hanami::Action::Params implements a few things that the new Hanami::Action::Contract does not:

• #raw
• #error_messages
• #deconstruct_keys
• #get, #dig (via BaseParams)
• #each (via BaseParams)

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:

1. We could expand the Hanami::Action::Contract so that it has the same interface as Params and BaseParams
2. Or I wonder if there's a way we can have a single class provide this "params" interface to the user, and have it be provided the "validation" object (which is either a dry-validation contract or a dry-schema schema)? This way we can stick with a single class for params and not have to maintain two parallel implementations.

I think I like option (2) better. But I'd be keen for your thoughts!

[solnic]

Option 2 sounds much better

[krzykamil]

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.

[timriley]

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.

[krzykamil]

@timriley I've pushed the changes in the last two commits.

The idea is:

Common functionality/interface can be divided into two modules:

1. Validations handling
2. Base functions

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

[krzykamil]

Just noticed that over the weekend the spec suit run and failed. I'll fix the spec today or tomorrow, this spec was ok locally so might be hard to debug, but should be done soon.

Edit: done in latest commit

[timriley]

Thanks @krzykamil! I've started looking at this again, btw! I want to explore whether there's an alternative approach we can take for providing the single user-exposed params interface. I'll try and get something back to you soon.