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

Form: required(:foo).maybe(:max_size?) fails unexpectedly #133

Closed
jodosha opened this issue May 3, 2016 · 9 comments
Closed

Form: required(:foo).maybe(:max_size?) fails unexpectedly #133

jodosha opened this issue May 3, 2016 · 9 comments

Comments

@jodosha
Copy link
Member

jodosha commented May 3, 2016

require 'bundler/setup'
require 'dry-validation'

form = Dry::Validation.Form do
  required(:foo).maybe(max_size?: 3)
end

result = form.call('foo' => %w(a b c))
puts result.success?
puts result.messages[:foo].inspect

__END__

Expected:
  true
  nil

Actual:
  false
  ["is missing", "size cannot be greater than 3"]
GIT
  remote: git://github.com/dry-rb/dry-logic.git
  revision: e649fad58546e8e53a048d310c554ad910466a93

GIT
  remote: git://github.com/dry-rb/dry-types.git
  revision: 394fa8b62966fd398f84b83cc712e5b1355f4af7

GIT
  remote: git://github.com/dry-rb/dry-validation.git
  revision: 59fb0a09c4997db95076b49082dc68445ead4630
@jodosha
Copy link
Member Author

jodosha commented May 3, 2016

Opposite case:

optional(:foo).maybe(max_size?: 3)

result = form.call('foo' => { 'a' => '1', 'b' => '2', 'c' => '3', 'd' => '4' })

__END__

Expected:
  false
  ["size cannot be greater than 3"]

Actual:
  true
  nil

@jodosha
Copy link
Member Author

jodosha commented May 3, 2016

Another case of wrong error message:

required(:foo).maybe(max_size?: 3)

result = form.call('foo' => { 'a' => '1', 'b' => '2', 'c' => '3', 'd' => '4' })

__END__

Expected:
  false
  ["size cannot be greater than 3"]

Actual:
  false
  ["is missing", "size cannot be greater than 3"]

@jodosha
Copy link
Member Author

jodosha commented May 3, 2016

This problem affects min_size? and size? too.

@timriley
Copy link
Member

timriley commented May 3, 2016

Just a note here: looks like this is another case of the maybe macro causing some issues. I see that when required(:foo).required(max_size?: 3) is used (instead of maybe, in your first example), everything works:

form = Dry::Validation.Form do
  required(:foo).required(max_size?: 3)
end

result = form.call('foo' => %w(a b c))
# => #<Dry::Validation::Result output={:foo=>["a", "b", "c"]} messages={}>

@Erol
Copy link

Erol commented May 5, 2016

Pardon me if this is going to be long...

I noticed that this code does not work correctly:

form = Dry::Validation.Form do
  required(:foo) { none? | max_size?(3) }
end

result = form.call('foo' => %w[a b c])
result.success? #=> false

But passing a Symbol key instead of a String key does:

form = Dry::Validation.Form do
  required(:foo) { none? | max_size?(3) }
end

result = form.call(foo: %w[a b c])
result.success? #=> true

I also noticed that if we drop the disjunction (|), passing a String key now works:

form = Dry::Validation.Form do
  required(:foo) { min_size?(2) | max_size?(3) }
end

result = form.call('foo' => %w[a b c])
result.success? #=> true

Going throught the code, I came to this line which behaves differently whenever a disjunction is included in the Schema AST:

https://github.com/dry-rb/dry-validation/blob/master/lib/dry/validation/schema.rb#L184

The result of the call to input_processor[input] does not symbolize keys when an | (or :or in the AST) is included.

module Dry
  module Validation
    class Schema
      def call(input)
        processed_input = input_processor[input]
        puts processed_input.inspect
        Result.new(processed_input, apply(processed_input), error_compiler, hint_compiler)
      end
    end
  end
end

form = Dry::Validation.Form do
  required(:foo) { none? | max_size?(3) }
end

result = form.call('foo' => %w[a b c])

#=> {"foo"=>["a", "b", "c"]}

In the absence of an | or :or, we have:

module Dry
  module Validation
    class Schema
      def call(input)
        processed_input = input_processor[input]
        puts processed_input.inspect
        Result.new(processed_input, apply(processed_input), error_compiler, hint_compiler)
      end
    end
  end
end

form = Dry::Validation.Form do
  required(:foo) { max_size?(3) }
end

result = form.call('foo' => %w[a b c])

#=> {:foo=>["a", "b", "c"]}

I don't have a deep enough understanding yet of both dry-validation and dry-types (which does the actual input processing) to figure out why this is happening. Does someone have an idea?

PS: Yes, I'm a puts debugger 😦

@solnic
Copy link
Member

solnic commented May 7, 2016

I...cannot reproduce this:

require 'dry-validation'

form = Dry::Validation.Form do
  required(:foo).maybe(max_size?: 3)
end

result = form.call(foo: %w(a b c))

puts result.success?
# true
puts result.messages[:foo].inspect
# nil

result = form.call(foo: %w(a b c d))

puts result.success?
# false
puts result.messages[:foo].inspect
# ["size cannot be greater than 3"]

Using latest dry-(v,l,t) from master.

@Erol
Copy link

Erol commented May 7, 2016

Hi @solnic, can you try passing string keys:

form.call('foo' => %w(a b c))

@solnic
Copy link
Member

solnic commented May 7, 2016

@Erol aah yeah, so it's the same issue as described here

@fran-worley
Copy link
Contributor

fran-worley commented May 11, 2016

I've got some more examples of this:

1) Predicates: Excluded From as macro with required with maybe with valid input is successful
     Failure/Error: expect(result).to be_successful
       expected that #<Dry::Validation::Result output={"foo"=>["2", "3", "4"]} messages={:foo=>["is missing", "must not include 1"]}> would be successful
     # ./spec/integration/form/predicates/excludes_spec.rb:239:in `block (6 levels) in <top (required)>'

2) Predicates: Excluded From as macro with required with maybe with invalid type is not successful
     Failure/Error: expect(result).to be_failing ['must not include 1']
       expected that #<Dry::Validation::Result output={"foo"=>{"a"=>"1"}} messages={:foo=>["is missing", "must not include 1"]}> would be failing (["must not include 1"])
     # ./spec/integration/form/predicates/excludes_spec.rb:271:in `block (6 levels) in <top (required)>'

3) Predicates: Excluded From as macro with required with maybe with invalid input is not successful
     Failure/Error: expect(result).to be_failing ['must not include 1']
       expected that #<Dry::Validation::Result output={"foo"=>["1", "2", "3"]} messages={:foo=>["is missing", "must not include 1"]}> would be failing (["must not include 1"])
     # ./spec/integration/form/predicates/excludes_spec.rb:279:in `block (6 levels) in <top (required)>'

4) Predicates: Excluded From as macro with required with maybe with blank input is successful
     Failure/Error: expect(result).to be_successful
       expected that #<Dry::Validation::Result output={"foo"=>[]} messages={:foo=>["is missing", "must not include 1"]}> would be successful
     # ./spec/integration/form/predicates/excludes_spec.rb:263:in `block (6 levels) in <top (required)>'

5)  Predicates: Excluded From as macro with optional with maybe with invalid input is not successful
   Failure/Error: expect(result).to be_failing ['must not include 1']
       expected that #<Dry::Validation::Result output={"foo"=>["1", "2", "3"]} messages={}> would be failing (["must not include 1"])
     # ./spec/integration/form/predicates/excludes_spec.rb:449:in `block (6 levels) in <top (required)>'

In all of these cases perform as I expect when the input is passed in with a symbol key instead of a string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants