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

Improve safety guarantees and coercion logic #309

Merged
merged 17 commits into from Apr 9, 2019

Conversation

3 participants
@flash-gordon
Copy link
Member

commented Mar 30, 2019

This diligently re-makes coercion logic to raise an error on failed coercion. Any invalid input now raises an exception by default:

# before 
[1] pry(main)> Dry::Types['params.integer'].('foo')
=> "foo"
# after
[1] pry(main)> Dry::Types['params.integer'].('foo')
Dry::Types::CoercionError: invalid value for Integer(): "foo"

The semantics of nominal types was changed to be completely typecheck-free, both .try and .call methods for nominal types will succeed on any input. For now, I added Nominal#coerce and Nominal#try_coerce implementing old behavior of call and try but maybe we should instead add Nominal#strict and suggest using .strict.call and .strict.try instead.

Now, Type#call takes an optional block which will be called on failed coercion:

[2] pry(main)> Dry::Types['params.integer'].('foo') { 'oops' }
=> "oops"

For constructor types coercion result will be passed to the block:

[3] pry(main)> Dry::Types['params.integer'].constrained(gteq: 18).('17') { |x| x }
=> 17

"Safe" types are now defined as simple as:

class Safe
  # ...
  def call(input)
    type.(input) { |output = input| output }
  end
end

Calling .safe recursively makes underlying types safe. This prevents raising and catching exceptions internally and makes things faster (unfortunately, not for all cases, see below):

[4] pry(main)> Dry::Types['params.hash'].schema(age: 'params.integer').safe
=> #<Dry::Types[Safe<Constructor<Schema<keys={age?: Safe<Constructor<Nominal<Integer> fn=Dry::Types::Coercions::Params.to_int>>}> fn=Dry::Types::Coercions::Params.to_hash>>]>

Calling .safe removes constraints because they become useless:

[1] pry(main)> Dry::Types['hash'].schema(age: 'integer')
=> #<Dry::Types[Constrained<Schema<keys={age: Constrained<Nominal<Integer> rule=[type?(Integer)]>}> rule=[type?(Hash)]>]>
[2] pry(main)> Dry::Types['hash'].schema(age: 'integer').safe
=> #<Dry::Types[Safe<Schema<keys={age?: Nominal<Integer>}>>]>

User-defined coercions can yield a block which must be called on failed coercion (if passed, otherwise Dry::Types::CoercionError must be thrown). An example from built-in coercions:

      def to_nil(input, &block)
        if input.nil? || empty_str?(input)
          nil
        elsif block_given?
          yield
        else
          raise CoercionError.new("#{input.inspect} is not nil")
        end
      end

Note that due to some ruby limitations the signature of a callable object yielding a block must have a block parameter (here &block but the name is not relevant).


I want to merge this PR because it's getting out of hand but some things are yet to be done:

  • Rename "Safe" to something more suitable. I like "lax" but we'll see what works best.
  • Improve errors handling. What I did here was a temporary "solution" and it has to be cleaned up.
  • Performance improvements. For no real reason safe types were slowed down in some cases. I know how to improve the performance to the level that beats all previous records but it's a separate thread of work.
  • Redefine valid? in terms of call(input) { return false }; true.

I expect the result of this to be a clean design and really performant type checks and coercions. I already improved dry-logic and I hope together with this set of changes the next versions of dry-types and dry-struct will be up to multiple times faster.

@solnic solnic added this to ⚙️ In Progress in dry-validation 1.0.0 Mar 30, 2019

@flash-gordon flash-gordon force-pushed the reword-safe-types branch from 14b851f to c161bcb Mar 31, 2019

flash-gordon added some commits Mar 30, 2019

Make all coercions not safe by default
This is the first step towards #291
Now all coercion types raise an error when failing.
I also made nominal type completely check-less. I'm not sure if this is the final decision. Probably, we'll need to extend the type interface to add sane behavior to nominal types, as in `try_coerce` or something like that.
Change interface of #call to yield an optional block on failed coercion
This allows for using CPS to avoid object allocations. It will improve performance when you don't care about errors.
Improve perfomance of failing case for schemas
I'm yet to prove my statements :)

@flash-gordon flash-gordon force-pushed the reword-safe-types branch from c161bcb to abedb9f Mar 31, 2019

flash-gordon added some commits Mar 31, 2019

Remove constraints from safe types
They are effectless so it's safe to throw them away

@flash-gordon flash-gordon force-pushed the reword-safe-types branch from e844ca6 to 3c45a2a Apr 3, 2019

flash-gordon added some commits Apr 4, 2019

Cover coercible types with specs checking for constraints
The new version will raise errors by default, we want to reject all invalid input unless types are marked as "safe" (the word "safe" will also be changed to something else with more clear semantics).

@flash-gordon flash-gordon marked this pull request as ready for review Apr 5, 2019

@flash-gordon flash-gordon requested review from solnic and timriley Apr 5, 2019

@flash-gordon flash-gordon self-assigned this Apr 5, 2019

@flash-gordon flash-gordon added this to the 0.16.0 milestone Apr 5, 2019

@timriley
Copy link
Member

left a comment

@flash-gordon This change looks good and your plans sound good 👍

dry-validation 1.0.0 automation moved this from ⚙️ In Progress to 👌🏻Approved Apr 8, 2019

@solnic

solnic approved these changes Apr 9, 2019

@flash-gordon flash-gordon merged commit 6b68645 into master Apr 9, 2019

4 of 5 checks passed

codeclimate 30 issues to fix
Details
codeclimate/diff-coverage 96% (80% threshold)
Details
codeclimate/total-coverage 96%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

dry-validation 1.0.0 automation moved this from 👌🏻Approved to ✅ Done Apr 9, 2019

@flash-gordon flash-gordon deleted the reword-safe-types branch Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.