search

dry-rb / dry-transaction

Business transaction DSL
http://dry-rb.org/gems/dry-transaction
MIT License
468 stars 55 forks source link

Re-allow Hash-like parameters to be passed to steps #136

Closed trammel closed 3 years ago

trammel commented 3 years ago

Previous commit 49c8dc24cf8e826c5771ddce5ec631f7952354a1 fixed a ruby 2.7 (https://github.com/dry-rb/dry-transaction/issues/132) / ruby 3.0 issue (https://github.com/dry-rb/dry-transaction/issues/135) , where you are no longer allowed to pass hash structures as the last argument to a method.

However, this fix affected other types of objects, that may inherit from Hash, and behave in a very similar way to Hash, specifically HashWithIndifferentAccess.

HashWithIndifferentAccess is used as the default object type for Rails parameters, and as such, is often passed to validation steps, as a HashWithIndifferentAccess type. The underlying validation code is often written with that implicit expectation, and may use strings or symbols as keys. This breaks, when the parameter is transformed in to a simple Hash.

While, it would probably be ideal to be able to go back, and remove HashWithIndifferentAccess from validators, and tighten the expectations to explicitly use strings or symbols, this fix was a breaking change to validators build with Rails parameters in mind.

This commit adds a test with a simple HashWithIndifferentAccess like test class, and demonstrates typical validation code. Without the included fix, these tests fail on current master (49c8dc24cf8e826c5771ddce5ec631f7952354a1) but pass on the commit before the fixes (69e886005ceff48a8f917011ceaf1bf2b90d03e9). The included fix limits the scope of the previous fix to only apply explicitly to Hash instances, leaving "hash-like" objects untouched.

timriley commented 3 years ago

Thanks @trammel for the great explanation and the code fix! This seems like a sensible adjustment to me.

timriley commented 3 years ago

@trammel actually, one request before I merge this: could you put a comment above the ruby_27_last_arg_hash? method succinctly explaining what it does, and why the instance_of? check is particularly important (instead of kind_of?/is_a?) –I think this will be very helpful to anyone who works on this part of the code in future.

trammel commented 3 years ago

Added a comment. It's not exactly brief, but it explains the why :grinning:

timriley commented 3 years ago

Thanks!

timriley commented 3 years ago

Released in 0.13.2!