dry-rb / dry-schema

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

Unexpected behaviour with array of hashes #410

Open jmortlock opened 2 years ago

jmortlock commented 2 years ago

Describe the bug

Reusing a schema directly in the the array macro produces an unexpected exception when passed bad data.

To Reproduce

NestedHash = Dry::Schema.Params {
  required(:code).filled(:string)
}

Parent = Dry::Schema.Params {
 required(:nested).array(NestedHash)
}

## Exception
result = Parent.call(nested: [[]])
/vendor/ruby/3.0.0/gems/dry-logic-1.2.0/lib/dry/logic/predicates.rb:25:in `key?': undefined method `key?' for []:Array (NoMethodError)

If you correctly pass it in an array of hashes it will happily work

Expected behavior

Should work the same as the long form expression; or not work at all.

Parent = Dry::Schema.Params {
  required(:nested).array(:hash).each(NestedHash)
}
result = Parent.call(nested: [[]])

My environment

Running the latest and greatest versions of dry-schema (1.9.2)

solnic commented 2 years ago

What about required(:nested).array(:hash, NestedHash)?

jmortlock commented 2 years ago

Yes, this syntax also works in my scenario.

solnic commented 2 years ago

OK so this is because passing a schema to array(...) doesn't assume a hash type check. It's tempting to say this should be the default though because that's your natural assumption when you want to apply a schema to array member.

jmortlock commented 2 years ago

I guess can a schema by anything but a hash? if not than i think setting :hash would be a sensible default.

Judging by the exception; dry-schema itself thinks that a schema should be a hash.

solnic commented 2 years ago

Yes it assumes a hash but that's just because applying a schema doesn't have hash? check applied by default, there's a reason for this - predicates must be granular because otherwise you'd lose the ability to perform fine-grained checks. ie maybe you want to check if something is a hash, has 3 keys and then apply a detailed schema (for whatever reason).

Anyhow, that's why we have macro methods that combine complex checks for you :) Like I said, passing an instance of a schema could assume you want hash? check as well. Looks like it's not consistent with the #call API though, because if you ie do schema.call(nil) it would not crash and return a failure that defined keys are missing.

I need to dig into this more to figure out a good solution, but it sounds like a 2.0.0 task, because it may potentially be a breaking change.