Skip to content
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

Allow direct reuse of dry-validation contracts in actions #454

Merged
merged 11 commits into from
Sep 17, 2024

Conversation

timriley
Copy link
Member

@timriley timriley commented Sep 2, 2024

Follow on from #453, and allow dry-validation contract classes to be provided directly to actions, in two ways.

First, via the contract class method, instead of expecting Hanami::Action::Params subclasses:

class MyAction < Hanami::Action
  contract MyContract # a Dry::Validation::Contract subclass
end

Second, via a contract dependency of the action:

# Either manually
class MyAction < Hanami::Action
end

action = MyAction.new(contract: MyContract.new)
# Or in Hanami apps, via Deps
class MyAction
  include Deps[contract: "my_contract"]
end

This is an altogether more flexible arrangement, since it will allow contracts to be used across both actions and other non-action classes within Hanami apps.

With this PR, we "soft deprecate" the idea of using Hanami::Action::Params subclasses as the way to provide reusable validation contracts to actions. We do this by updating Hanami::Actions.contract such that it does not support being given Hanami::Action::Params subclasses. Given that .contract is the new and full-powered incarnation of action validation, this hopefully will nudge people in the right direction.

We also have #455 for us to consider a proper deprecation of all direct Hanami::Action::Params usage, including subclasses being given to Action.params.

Implementation-wise, here's how we've done this:

  • Add a contract_class setting to Hanami::Action
  • Have Action.params and Action.contract set this contract_class config
  • In Action#initialize, initialize the contract_class and assign it to @contractbecause we initialize the contract at this point, rather than at the time of assignment, it means we support contracts with their own auto-injected dependencies!
  • When actions are called, pass @contract into the created instance of Hanami::Action::Params
  • Update Hanami::Action::Params to validate its params via this contract, or when none is given (i.e. when the action itself has no user-supplied contract), via an internal default contract that permits all params and deep symbolizes the keys

That's it! Along the way, we've also been able to simplify things:

  • Stop needing to store a custom Hanami::Action::Params subclass (as params_class) for every action with a contract defined (instead we store the contract_class and pass it into a standard params object, as described above)
  • Merge Hanami::Action::Params and Hanami::Action::BaseParams, while still ensuring that Params on its own can still work when the hanami-validations gem is not bundled
  • Remove the need for a "base contract" that ensures the _csrf_token param is permitted. This would have been an issue with reusable user-defined contracts, since users will want to inherit from Dry::Validation::Contract directly, not a base contract from hanami-controller. To account for this, update the CSRFProtection module so that it looks for the _csrf_token in the raw, non-validated params instead.

Not only does this change simplify internals, it also offers a net reduction in lines of code! 😄

@timriley timriley force-pushed the validation-contract-reuse branch from 85599da to bd39600 Compare September 2, 2024 14:13
This way we don’t need to have a base validator contract to permit it.
Now that our CSRF protection module pulls the _csrf_token directly from the raw params, we don’t need a vase validator, and can use dry-validation contracts directly
@timriley timriley force-pushed the validation-contract-reuse branch from bd39600 to 919ff62 Compare September 2, 2024 14:23
@timriley timriley self-assigned this Sep 2, 2024
@timriley timriley force-pushed the validation-contract-reuse branch from 919ff62 to 9f6ab88 Compare September 3, 2024 11:18
@timriley timriley marked this pull request as ready for review September 6, 2024 13:29
I think it’s better we use consistent terminology throughout the codebase.
@timriley timriley requested a review from alassek September 7, 2024 03:37
@timriley
Copy link
Member Author

timriley commented Sep 7, 2024

@alassek Going to ping you for a review here, since I headed in this direction after your recent prompting. How does feel to you? Does it give you what you'd hoped for?

@timriley
Copy link
Member Author

Merging this one now. We can always make further adjustments later.

@timriley timriley merged commit 0a7a746 into main Sep 17, 2024
8 checks passed
@timriley timriley deleted the validation-contract-reuse branch September 17, 2024 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant