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

Array elements are coerced before array constructor is run #351

Closed ojab closed 3 years ago

ojab commented 3 years ago

Describe the bug

require 'dry-schema'
require 'json'

field_schema = Dry::Schema.define do
  required(:string_field).filled(:string)
end

array_type = Dry.Types().Array(field_schema).constructor do |input|
  p input
  input.map { |value| JSON.parse(value, {symbolize_names: true}) }
end

schema = Dry::Schema.define do
  required(:schema_field).filled(array_type)
end

p schema.call({ schema_field: [{ string_field: '1' }, { string_field: '2' }].map(&:to_json) })

To Reproduce

Run the example

[{}, {}]
#<Dry::Schema::Result{:schema_field=>[{}, {}]} errors={:schema_field=>{0=>{:string_field=>["is missing"]}, 1=>{:string_field=>["is missing"]}}}>

so strings from the input was coerced to empty hashes and array constructor is receiving this empty hashes instead of input strings.

Expected behavior

Input strings are passed to the constructor.

My environment

ojab commented 3 years ago

Why do we even have KeyMap & stuff, can't we just use constructors :thinking:?

solnic commented 3 years ago

Why do we even have KeyMap & stuff, can't we just use constructors 🤔?

How exactly? KeyMaps are used to provide basic info about keys and are used by the KeyValidator feature too, so it's a bit more than to just process keys. I mean, I'd love to see if we could just re-use dry-types for this too...something worth exploring in 2.0.0.

ojab commented 3 years ago

exactly is the hard part :sweat_smile:

I don't have the full picture about KeyMap usage, but IMHO conceptually dry-schema/dry-validation is just pimp my Types::Hash with some batteries/creation DSL, but implementation is different, which makes working with all that stuff more complicated.

Probably I'm missing something, but KeyValidator looks like Types::Hash.strict, key+value coercer looks like constructor, filter schema looks like Types::X.constrained, dry-v rules look like custom predicates and so on.

Reusability/flexibility is hurt because of that, we can't just

EMAIL_ADDRESS_SCHEMA = Dry::Schema.define do
  required(:email).filled(:string)
  optional(:name).filled(:string)

  rule(:email) { validate_email(value) }
end

MY_SCHEMA = Dry::Schema.define do
  required(:email_address).filled(EMAIL_ADDRESS_SCHEMA)
end

and have reusable email address schema with validation included.

An I'm not even talking about increased cognitive load due to multiple implementations doing the same thing. As a user I don't understand why API can't be

schema.strict.with_key_transform(&:to_sym) do
  required(:email).filled(:string)
    .construtor(&:strip)
    .fallback('foo')
    .default('bar')
    .constrained(min_size: 3)

  # just convert it into kinda anonymous predicate and add to `:email` type
  rule(:email) { complex_validation ) 
end

with each macro just returning underlying type, so docs can be go read about dry-types and here are the goodies that we have on top of it.

Looks like a rant and, again, I suppose I'm missing a lot of things here.

flash-gordon commented 3 years ago
  required(:email).filled(:string)
    .construtor(&:strip)
    .fallback('foo')
    .default('bar')
    .constrained(min_size: 3)

For the record, this doesn't look appealing to me. Stuffing everything into a single thing with the hope you can somehow pull this off wrt checks, coercing, message generation, and performance is a loooong shot. If I come across a library with such an API I'd move along :)

Yeah I know dry-types is a flexible gem, this doesn't mean it should be used for everything, it's not supposed to. But it's a general comment, not to discourage particular attempts.

solnic commented 3 years ago

Yes I agree with Nikita. There are a lot of similarities and it's often tempting to reuse some abstraction but it's much more nuanced. I know you can have a very fluid DSL where you just chain things together freely (there are folks who implemented similar libs based on this idea) but it's not gonna work well for schema definitions. That's the main purpose here. Personally, I'm still not happy with how custom types are handled and to be honest, I don't know if I would like to use custom constructors with schemas anyway. For me transforming data input before schema can be applied is still an unsolved problem, even though you can already do it to a large extent using existing APIs. This stuff is hard 🙂 I will get back to this topic eventually, maybe during 2.0.0 work.