dry-rb / dry-schema

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

The unexpected behavior of the validation of object with array type attribute that contains custom object #379

Closed dawlib closed 2 years ago

dawlib commented 2 years ago

Describe the bug

The unexpected behavior of the validation is shown in the example below.

To Reproduce

require 'dry/schema'

module Types
  include Dry::Types()
end

class MyItem < Dry::Schema::JSON
  define do
    required(:name).filled(Types::String.enum(*%w[b]))
    optional(:time).filled(:time)
  end
end

class MyList < Dry::Schema::JSON
  define do
    required(:list).value(:array).each { MyItem.new }
  end
end

item = { 'name' => 'b', 'time' => Time.now.iso8601 }

MyItem.new.call(item).failure? # => false
MyList.new.call('list' => [item]).failure? # => true

MyList.new.call('list' => [item])
# => #<Dry::Schema::Result{:list=>[{"name"=>"b", "time"=>"2021-11-16T14:14:18+01:00"}]} errors={:list=>{0=>{:name=>["is missing"]}}} path=[]>

Expected behavior

I would expect to receive false as a result of the command below:

puts MyList.new.call('list' => [item]).failure? # => false

My environment

flash-gordon commented 2 years ago

@dawlib is syntax like .each { MyItem.new } described somewhere? Looks unfamiliar

madejejej commented 2 years ago

@flash-gordon not per-se .each { MyItem.new }, but a similar syntax is described in one of the examples here https://dry-rb.org/gems/dry-schema/1.5/nested-data/#nested-code-array-code

schema = Dry::Schema.Params do
  required(:people).value(:array, min_size?: 1).each do
    hash do
      required(:name).filled(:string)
      required(:age).filled(:integer, gteq?: 18)
    end
  end
end

errors = schema.call(people: []).errors

puts errors.to_h.inspect
# => {
#   :people=>["size cannot be less than 1"]
# }
esparta commented 2 years ago

IMO even tho looks similar it's not, @madejejej

The DSL on the example is calling an internal procedure on every element of the array and interpreted as that, in your case the .each { MyItem.new } is just yielding an object tha - as the current behavior of the DS- is not interpreted as such. Looks like a new feature the maintainers would need to be agree on incorporate - and support.

It would be nice to pitch the idea in the Dry-rb forums.

flash-gordon commented 2 years ago

It's a misuse of the API, it shouldn't have worked in the first place. You already can share schemas, just don't use blocks

class MyItem < Dry::Schema::Params
  define do
    required(:name).filled(Types::String.enum(*%w[b]))
    optional(:time).filled(:time)
  end
end

class MyList < Dry::Schema::Params
  define do
    required(:list).value(:array).each(MyItem.new)
  end
end

item = { 'name' => 'b', 'time' => Time.now.iso8601 }

[1] pry(main)> MyList.new.call('list' => [item])
=> #<Dry::Schema::Result{:list=>[{:name=>"b", :time=>2021-12-01 15:41:25 +0300}]} errors={} path=[]>