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: format? misleading error message #131

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

Form: format? misleading error message #131

jodosha opened this issue May 3, 2016 · 3 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(format?: /foo/)
end

result = form.call('foo' => { 'a' => '1' })
puts result.success?
puts result.messages[:foo].inspect

__END__

Expected:
  false
  ["is in invalid format"]

Actual:
  false
  ["is missing", "is in invalid format"]
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
@timriley
Copy link
Member

timriley commented May 3, 2016

This one's slightly more interesting than the other "the maybe macro looks to be causing problems" cases I just commented on.

I have a question before I go any further though: you're testing a format? macro here, but passing it hash data instead of a string. And yet you're expecting expecting the validation to run and return an error like normal, even though there's no way in ruby to run a regex against a hash ;) What's your thinking here?

When I use the required macro (which doesn't have the same problems as we're finding with maybe), I get an exception, which seems like a not unreasonable outcome here. Are you also OK with that, if we can make the maybe case behave in the same way?

form = Dry::Validation.Form do
  required(:foo).required(format?: /foo/)
end

result = form.call('foo' => { 'a' => '1' })
# TypeError: no implicit conversion of Hash into String
# from /Users/tim/.rbenv/versions/2.3.0/lib/ruby/gems/2.3.0/gems/dry-logic-0.2.2/lib/dry/logic/predicates.rb:153:in `match'

@jodosha
Copy link
Member Author

jodosha commented May 4, 2016

@timriley Thanks for your reply!

And yet you're expecting expecting the validation to run and return an error like normal, even though there's no way in ruby to run a regex against a hash ;) What's your thinking here?

With Rack based apps, we can receive three kind of values: string, array or hash. Because we don't have control on incoming data (and we shall never trust it), I'm testing form hijacking here: what happens if I expect a string, but I receive a hash? That's the purpose of the test.

@solnic
Copy link
Member

solnic commented May 7, 2016

This is caused by a bad behavior of dry-types, the input processor in this case fails but dry-v uses a "safe type" which rescues from type errors and...returns original input. That's why you get back original hash and key?(:foo) fails hence is missing error message.

I was aware of this issue, just forgot to report it. We should use a different strategy for dry-types hash schemas that we use for input processing. What we want is a hash schema type which simply tries to process values and leave them as original values even when coercion failed. This concept doesn't exist at all in dry-types yet, we gotta add it. Right now Hash::Schema::Symbolized is used from dry-types, I think adding a new one that will be dedicated for our use case here is a good idea. I'm not sure about the name though ;) Maybe tweaking existing Hash::Schema::Safe and extending it with a way of configuring key coercion would be a better option.

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

3 participants