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

Unexpected behaviour with array of hashes #410

Open
jmortlock opened this issue Jun 22, 2022 · 5 comments
Open

Unexpected behaviour with array of hashes #410

jmortlock opened this issue Jun 22, 2022 · 5 comments

Comments

@jmortlock
Copy link

jmortlock commented Jun 22, 2022

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
Copy link
Member

solnic commented Jun 23, 2022

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

@jmortlock
Copy link
Author

Yes, this syntax also works in my scenario.

@solnic
Copy link
Member

solnic commented Jun 24, 2022

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
Copy link
Author

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
Copy link
Member

solnic commented Jun 25, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants