Dry::Validation::Contract reports an error for coercible integer strings, but only when a hash field fails validation

[elohanlon]

Describe the bug

Hello. I'm running into an issue where a Dry::Validation::Contract reports an error for coercible integer strings, but only when a hash field fails validation. I've included some rspec tests below that demonstrate a few successful validation cases and one unexpected failure case .

To Reproduce

describe 'contract test' do
  class TestContract < Dry::Validation::Contract
    params do
      required(:outer_key).schema do
        required(:str_field).value(:string)
        required(:integer_field).value(:integer)
        required(:integer_field2).value(:integer)
        required(:hash_field).value(:hash)
      end
    end
  end

  let(:contract) { TestContract.new }

  # This test passes
  it 'does not return any errors when provided with valid params (and allows coercable string integers in integer fields)' do
    expect(
      contract.call({
        outer_key: {
          str_field: 'abc',
          integer_field: '1',
          integer_field2: '2',
          hash_field: {'key' => 'val'},
        }
      }).errors.to_h
    ).to be_empty
  end

  # This test passes
  it 'returns the expected error when a non-integer value is provided to an integer field' do
    expect(
      contract.call({
        outer_key: {
          str_field: 'abc',
          integer_field: 'NOT AN INTEGER',
          integer_field2: '2',
          hash_field: {'key' => 'val'},
        }
      }).errors.to_h
    ).to eq({ outer_key: { integer_field: ["must be an integer"] } })
  end

  # This test fails and I get THREE errors rather than ONE expected error
  it 'returns the expected error when a non-hash value is provided to a hash field' do
    expect(
      contract.call({
        outer_key: {
          str_field: 'abc',
          integer_field: '1',
          integer_field2: '2',
          hash_field: 'NOT A HASH',
        }
      }).errors.to_h
    ).to eq({ outer_key: { hash_field: ["must be a hash"] } })

    # Instead of one error, we get these three:
    # {
    #   outer_key:
    #   {
    #     hash_field: ["must be a hash"]
    #     integer_field: ["must be an integer"],
    #     integer_field2: ["must be an integer"],
    #   }
    # }

    # This is unexpected because the failing integer fields didn't fail with valid coercible
    # string values in the earlier test.
  end

  # Unlike the above test, the test below passes when the integer fields are provided as integers
  # rather than coercible strings
  it 'returns the expected error when a non-hash value is provided to a hash field' do
    expect(
      contract.call({
        outer_key: {
          str_field: 'abc',
          integer_field: 1,
          integer_field2: 2,
          hash_field: 'NOT A HASH',
        }
      }).errors.to_h
    ).to eq({ outer_key: { hash_field: ["must be a hash"] } })
  end
end

Expected behavior

Since string values of '1' and '2' don't fail validation in my earlier tests, I would have expected those values to continue to be coerced properly in the failing test. The validation failure on the hash field seems to interfere with the integer validation.

Is it possible that I'm doing something wrong? Or is this a bug? Thank you for taking a look.

My environment

• Affects my production application: NO (not deployed to production, just new code under development)
• Ruby version: Ruby 2.6.4
• OS: macOS 10.15.7

[flash-gordon]

you should be using required(:outer_key).hash do

[elohanlon]

@flash-gordon I tried this and I'm still getting the same error:

required(:outer_key).hash do
  required(:str_field).value(:string)
  required(:integer_field).value(:integer)
  required(:integer_field2).value(:integer)
  required(:hash_field).value(:hash)
end

But this appears to solve the problem:

required(:outer_key).schema do
  required(:str_field).value(:string)
  required(:integer_field).value(:integer)
  required(:integer_field2).value(:integer)
  required(:hash_field).hash
end

Is that the correct usage? (required(:hash_field).hash)

I want to validate that the parameter supplied to hash_field is a hash, but I don't want to run any deeper validation rules on that hash.

[solnic]

Is that the correct usage? (required(:hash_field).hash)

For now, yes, but in 2.0.0 it won't work. value(:hash) should work, so this is a bug. I'm assigning this to 2.0.0 milestone, because one of the biggest priorities in this release will be to clean up the way types are handled. After the clean up, fixing bugs like this one will be much simpler than it would be now. It's also possible that the clean up will fix some of the bugs as a nice side-effect.

[elohanlon]

Thanks, @solnic !

[mereghost]

This seems to be an issue on dry-schema itself. The ValueCoercers get kinda funky when you have a value (or filled for that matter) of :hash and this leads to the later failures are basically no changes get applied and the rules don't verify in the end.

[mereghost]

So after some diving into this, it relates more into dry-types. Let me see if I can explain what's happening:

Dry::Types::Schema#resolve_safe goes on to evaluate each key and its type. For :hash fields we get #<Dry::Types[Constructor<Hash< meta={required: false, maybe: false}> fn=Dry::Types::Coercions::Params.to_hash>]> which seems correct.

The implementation for the method is in Dry::Types::Coercions::Params.to_hash that delegates the coercion to the supplied block by calling yield.

At this point in execution the block is defined in Constructor#call_safe which returns to Schema#call_safe

What seem to cause the issue is two-fold: some of the blocks yield a value that's being ignored up in the call chain and also those 2 blocks force an early return.

I'm trying to work around the breakage that results from this (2 specs on dry-types and 1 in dry-schema) as I have this inkling that this issue is the root of many of the nested schema bugs that were reported.

[solnic]

@mereghost wow that's some major investigation right there! 🙇🏻

[elohanlon]

@mereghost Thank you for looking into this!