dry-rb / dry-schema

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

Struct extension doesn't work with nested structs #464

Closed paul closed 1 year ago

paul commented 1 year ago

Describe the bug

When generating a Dry::Schema from a Struct using the :struct extension, it first converts the input nested Hash into the Struct, then errors that the input isn't the expected hash.

To Reproduce

I added a test case to extensions/struct_spec.rb:

    context "complex struct" do
      let(:struct) do
        Class.new(Dry::Struct) do
          attribute :name, Types::String
          attribute :addresses, Types::Array do
            attribute  :city,   Types::String
            attribute? :street, Types::String
          end
        end
      end

      context "with valid input" do
        let(:input) do
          {foo: {name: "Jane", addresses: [{city: "New York"}]}}
        end

        it "has no errors" do
          expect(schema.(input).errors.to_h).to eq({})
        end
      end
Failures:

  1) struct extension value macro complex struct with valid input has no errors
     Failure/Error: expect(schema.(input).errors.to_h).to eq({})

       expected: {}
            got: {:foo=>{:addresses=>{0=>["must be a hash"]}}}

       (compared using ==)

       Diff:
       @@ -1 +1,2 @@
       +:foo => {:addresses=>{0=>["must be a hash"]}},

     # ./spec/extensions/struct_spec.rb:84:in `block (5 levels) in <top (required)>'

Expected behavior

I'd expect this input to have no errors, since the nested hash satisfies the inner struct's schema. It appears something is converting the Hash into the given Struct, then errors because now that Struct isn't a Hash like its expecting.

I spent some time digging into dry-types and dry-logic, but was unable to pin down exactly where it was attempting to verify the type of the struct was a Hash.

My environment

paul commented 1 year ago

Update: I can make it work by monkey-patching Dry::Struct like this:

module Dry
  class Struct
    def is_a?(klass)
      return true if klass == ::Hash

      super
    end

    def key?(name)
      @attributes.key?(name)
    end
  end
end
paul commented 1 year ago

Overriding Struct#is_a? breaks some other code of mine, so that didn't work.

This does, but obviously isn't the right solution:

  class Struct
    def key?(name)
      @attributes.key?(name)
    end
  end

  Logic::Predicates::Methods.prepend(::Module.new {
    def hash?(input)
      input.is_a?(Dry::Struct) || input.is_a?(Hash)
    end
  })

I was hoping something like this might work, but it breaks some of the other tests:

    # PredicateInferrer::Compiler.send(:alias_method, :visit_struct, :visit_hash)
    PredicateInferrer::Compiler.prepend(::Module.new {
      def visit_struct(_)
        [type?: Dry::Struct]
      end
    })

    # PrimitiveInferrer::Compiler.send(:alias_method, :visit_struct, :visit_hash)
    PrimitiveInferrer::Compiler.prepend(::Module.new {
      def visit_struct(_)
        ::Dry::Struct
      end
    })
  1) struct extension value macro infers predicates from struct
     Failure/Error: expect(result).to be_failing(email: ["is missing", "must be a string"])
       expected that ["must be Dry::Struct"] would be failing ({:email=>["is missing", "must be a string"]})
     # ./spec/extensions/struct_spec.rb:64:in `block (3 levels) in <top (required)>'
paul commented 1 year ago

~Nope, nevermind, that one does weird things when the input is an empty array. It converts the empty array to an empty hash, then complains that the hash is not an array.~

Turns out the empty hash -> empty array has nothing to do with my monkey patch, it happens on main too. Here's a failing test:

      context "with empty nested array" do
        let(:input) do
          {foo: {name: "Jane", addresses: []}}
        end

        it "has no errors" do
          expect(schema.(input).errors.to_h).to eq({})
        end
      end
  1) struct extension value macro complex struct with empty nested array has no errors
     Failure/Error: expect(schema.(input).errors.to_h).to eq({})

       expected: {}
            got: {:foo=>{:addresses=>["must be an array"]}}

       (compared using ==)

       Diff:
       @@ -1 +1,2 @@
       +:foo => {:addresses=>["must be an array"]},

Commenting out this line in the extension fixes it: https://github.com/dry-rb/dry-schema/blob/main/lib/dry/schema/extensions/struct.rb#L17

flash-gordon commented 1 year ago

Thanks for reporting! Can you try this branch out https://github.com/dry-rb/dry-schema/pull/466? It should work

paul commented 1 year ago

@flash-gordon That seems to work for me! It also works for the empty array situation in my previous commit. Should we add that as a test, too?

flash-gordon commented 1 year ago

@paul thanks, I did that. I'll merge the PR soon and cut a release with the fix

flash-gordon commented 1 year ago

Fixed in 1.13.3