dry-rb / dry-schema

Coercion and validation for data structures
https://dry-rb.org/gems/dry-schema
MIT License
425 stars 109 forks source link

Composing Schemas Raises NoMethodError in Validation Failures #342

Closed nbibler closed 2 years ago

nbibler commented 3 years ago

Describe the bug

Composing complex Schemas appears to throw a NoMethodError: undefined method 'to_or' for #<Array:0x0> in a failure case rather than simply return an object with populated validation errors/failures.

This was found while using:

Using bundler 2.1.4
Using dry-equalizer 0.3.0
Using dry-initializer 3.0.4
Using concurrent-ruby 1.1.8
Using dry-inflector 0.2.0
Using ice_nine 0.11.2
Using dry-core 0.5.0
Using dry-configurable 0.12.0
Using dry-logic 1.1.0
Using dry-container 0.7.2
Using dry-types 1.5.0
Using dry-schema 1.6.0
Using dry-struct 1.4.0
Using dry-validation 1.6.0

To Reproduce

#!/usr/bin/env ruby
# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'
  gem 'dry-equalizer'
  gem 'dry-schema'
  gem 'dry-struct'
  gem 'dry-types'
  gem 'dry-validation'
end

MatchingQuestion = Dry::Schema.Params do
  required(:attributes).hash do
    required(:value).value(:array, :filled?).each(:str?, format?: /\A\d+\Z/)
  end
  required(:relationships).hash do
    required(:question).hash do
      required(:data).hash do
        required(:type).value(:str?, eql?: 'matching')
      end
    end
  end
end

MultipleChoiceQuestion = Dry::Schema.Params do
  required(:attributes).hash do
    required(:value).value(:array, :filled?).each(:str?, format?: /\A\d+\Z/)
  end
  required(:relationships).hash do
    required(:question).hash do
      required(:data).hash do
        required(:type).value(:str?, eql?: 'multiple-choice')
      end
    end
  end
end

Question = Dry::Schema.Params do
  required(:data).hash(MatchingQuestion | MultipleChoiceQuestion)
end

def validate(data)
  p Question.(data).errors.to_h
rescue
  p $!.inspect
end

# Good data, no errors/exceptions
validate(data: {attributes: {value: ['1', '2']}, relationships: {question: {data: {type: 'matching'}}}})
validate(data: {attributes: {value: ['1', '2']}, relationships: {question: {data: {type: 'multiple-choice'}}}})

# Bad data, raises NoMethodError exceptions rather than generates errors
validate(data: {attributes: {value: []}, relationships: {question: {data: {type: 'unknown'}}}})
validate(data: {attributes: {value: []}, relationships: {question: {data: {type: 'matching'}}}})
validate(data: {attributes: {value: []}, relationships: {question: {data: {type: 'multiple-choice'}}}})

This generates the following output, where the "bad data" examples above raise a NoMethodError rather than a return a failed validation result:

{}
{}
"#<NoMethodError: undefined method `to_or' for #<Array:0x00007f9521f56a50>\nDid you mean?  to_ary\n               to_s\n               to_a\n               to_h>"
"#<NoMethodError: undefined method `to_or' for #<Array:0x00007f9521f2da60>\nDid you mean?  to_ary\n               to_s\n               to_a\n               to_h>"
"#<NoMethodError: undefined method `to_or' for #<Array:0x00007f9521f07f40>\nDid you mean?  to_ary\n               to_s\n               to_a\n               to_h>"

Expected behavior

The validation should not raise a NoMethodError exception. It should populate the errors of the returned object with Messages about the validation failures.

My environment

nbibler commented 3 years ago

A similar, but slightly different NoMethodError occurs with a less complex Schema combination:

#!/usr/bin/env ruby
# frozen_string_literal: true

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'dry-equalizer'
  gem 'dry-schema'
  gem 'dry-struct'
  gem 'dry-types'
  gem 'dry-validation'
end

Matching = Dry::Schema.Params do
  required(:type).value(:str?, eql?: 'matching')
end

MultipleChoice = Dry::Schema.Params do
  required(:type).value(:str?, eql?: 'multiple-choice')
end

Question = Dry::Schema.Params do
  required(:data).hash do
    required(:relationships).hash do
      required(:question).hash do
        required(:data).hash(Matching | MultipleChoice)
      end
    end
  end
end

def validate(data)
  p Question.(data).errors.to_h
rescue
  p $!.inspect
end

validate(data: {relationships: {question: {data: {type: 'unknown'}}}})
validate(data: {relationships: {question: {data: {type: 'matching'}}}})
validate(data: {relationships: {question: {data: {type: 'multiple-choice'}}}})

The output is:

"#<NoMethodError: undefined method `path' for #<Array:0x00007fbc3b23e840>>"
{}
{}
solnic commented 3 years ago

Thanks for reporting this. This feature is still experimental but the plan is to improve it, fix any known issues and have it Ready-Ready for 2.0.0.

nbibler commented 3 years ago

@solnic: Yeah, I saw the giant WARNING banner on the composing schemas page. πŸ˜„ The idea and approach looked so nice I wanted to try it on an existing validator to see how it behaved and what, if any, of it worked. After a couple of hours of toying with it, I kept getting various incarnations of the errors identified in this ticket.

solnic commented 3 years ago

@nbibler I'll look into it. It's also possible this is a duplicate of...some...other issue πŸ˜…

nbibler commented 3 years ago

@solnic: Thanks! No particular rush from me. There are certainly other, existing ways in these libraries to validate complicated and dynamic schemas (see Rules).

The composition of schemas looks like a nice way to simplify some of the Rule complexity I've seen in existing applications. It likely also provides an easier way to unit test the validations (smaller schemas are easier to more robustly test) and get better error/failure messages (Rules tend to boil down to a singular key.failure for some combination of conditionals which is less granular than the built-in predicate messaging). But, not having composable schemas today isn't a πŸ”₯ problem.

solnic commented 3 years ago

@nbibler thanks, I appreciate you sharing this with me πŸ˜„ Composing schemas (and also contracts on the dry-validation side) is the biggest thing we want to have in 2.0.0 so stay tuned. Testing it out early is very helpful too, so thank you!

aleksandra-stolyar commented 3 years ago

ran into same issue today using dry-validation. NoMethodError: undefined method 'path' for #<Dry::Schema::Message::Or::MultiPath:0x0000557e8515bcc0> from /usr/local/bundle/gems/dry-schema-1.6.2/lib/dry/schema/result.rb:115:in 'block in error?'

my schema:

EmailType = Dry::Schema.Params do
  ....
end

NicknameType = Dry::Schema.Params do
  required(:nickname).filled(:string, max_size?: 200)
  required(:password).filled(:string, min_size?: 8, max_size?: 128)
end

class SomeContract
  params do
    required(:creds).hash(EmailType | NicknameType)
  end
end

tried to pass only nickname without password and got this error

contract = SomeContract.new
contract.call(input)
solnic commented 3 years ago

@aleksandra-stolyar yes it's using the same implementation. Fixing it here will fix it there.

jstoup111 commented 3 years ago

Any chance the above PR will get merged with a new release? I know it's experimental, but if we have a reasonable fix can it get pushed?

solnic commented 2 years ago

Fixed via #366

edgarjs commented 2 years ago

Thanks for fixing this. Do you know in which version will it be out?

solnic commented 2 years ago

@edgarjs 1.8.1