dry-rb / dry-schema

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

Dry::Schema::Params.info throws exception when using `maybe` #326

Open Grasseh opened 4 years ago

Grasseh commented 4 years ago

Describe the bug

Using a maybe in a schema breaks the info option to generate an AST-parsed hash with the following stack-trace:

irb(main):007:0> p schema.info
Traceback (most recent call last):
       16: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:53:in `block in visit_set'
       15: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:46:in `visit'
       14: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:46:in `public_send'
       13: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:73:in `visit_implication'
       12: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:73:in `each'
       11: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:74:in `block in visit_implication'
       10: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:46:in `visit'
        9: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:46:in `public_send'
        8: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:86:in `visit_key'
        7: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:46:in `visit'
        6: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:46:in `public_send'
        5: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:73:in `visit_implication'
        4: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:73:in `each'
        3: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:74:in `block in visit_implication'
        2: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:46:in `visit'
        1: from /home/steve/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/extensions/info/schema_compiler.rb:46:in `public_send'
NoMethodError (undefined method `visit_not' for #<Dry::Schema::Info::SchemaCompiler:0x0000559b2ed0d6e8>)
Did you mean?  visit_set
               visit_and
               visit_key

To Reproduce

Open a terminal with irb or whatever environment you use. This uses the example code for maybe found here: https://dry-rb.org/gems/dry-schema/1.5/optional-keys-and-values/

require 'dry/schema'
Dry::Schema.load_extensions(:info)
schema = Dry::Schema.Params do
  required(:email).filled(:string)
  optional(:age).maybe(:integer, gt?: 18)
end
p schema.info

Expected behavior

I'd expect an hash to be produced without exception, just like the following does when the maybe is replaced by a filled: {:keys=>{:email=>{:required=>true, :type=>"string"}, :age=>{:required=>false, :type=>"integer"}}}

Your environment

solnic commented 4 years ago

Thanks for the report. With the maybe method though, the expected type is ["nil", "integer"] in this case, required key in the meta data means the key is optional, it's not about the value under that key. This is a bit tricky because the AST as we have it now, doesn't clearly specify that we're dealing with "a maybe value". It's represented like this:

[:key, [:phone, [:implication, [[:not, [:predicate, [:nil?, [[:input, Undefined]]]]], [:predicate, [:str?, [[:input, Undefined]]]]]]]]

This translates to "if the value is not nil, then it must be a string". Which is great, but in case of the info plugin I think it's ambiguous because you may have other negated predicates (represented as [:not, ...] nodes) that are regular predicates without the special "maybe" meaning.

I think this should be properly addressed in 2.0.0 once we extend the AST with additional meta-data that could be used here, otherwise we could only fix it for the maybe case while breaking it for any negated predicate.

carbroj commented 3 years ago

Thanks for the report. With the maybe method though, the expected type is ["nil", "integer"] in this case, required key in the meta data means the key is optional, it's not about the value under that key. This is a bit tricky because the AST as we have it now, doesn't clearly specify that we're dealing with "a maybe value". It's represented like this:

[:key, [:phone, [:implication, [[:not, [:predicate, [:nil?, [[:input, Undefined]]]]], [:predicate, [:str?, [[:input, Undefined]]]]]]]]

This translates to "if the value is not nil, then it must be a string". Which is great, but in case of the info plugin I think it's ambiguous because you may have other negated predicates (represented as [:not, ...] nodes) that are regular predicates without the special "maybe" meaning.

I think this should be properly addressed in 2.0.0 once we extend the AST with additional meta-data that could be used here, otherwise we could only fix it for the maybe case while breaking it for any negated predicate.

@solnic It seems this has been closed for a little bit, are we still planning on addressing this case for 2.0.0?

solnic commented 3 years ago

@carbroj yes this will be done eventually. Marking as "help wanted" for now.