Problem with hash predicate logic in array field

[fijemax]

Describe the bug

Problem during schema validation: On the prerequisites field of the example I want validate that this array contain only hash describe by the previous schemas. Every schema contain a name with a specifig value. When all hashes match with "pictures" or "scan_barcode" everything is ok. But when an I put anything else in the name field of a hash the errors method on result return an exception

To Reproduce

This is my code:

class Pictures < Dry::Schema::Params
  define do
    required(:name).filled(:string, eql?: 'pictures')
  end
end

class ScanBarcode < Dry::Schema::Params
  define do
    required(:name).filled(:string, eql?: 'scan_barcode')
  end
end

class MissionActionTypeSchema < Dry::Schema::Params
  define do
    optional(:id).filled(:string)
    required(:previous_mission_status_type_reference).filled(:string)
    required(:next_mission_status_type_reference).filled(:string)
    optional(:mission_event_type_references).array(:str?)
    optional(:hidden).maybe(:bool)
    optional(:prerequisites).value(:array).each do
      schema(Pictures.new|ScanBarcode.new)
    end
  end
end



s = MissionActionTypeSchema.new
res = s.call({ id: '1', previous_mission_status_type_reference: 'a', next_mission_status_type_reference: 'b', prerequisites: [{name: 'pictu'}]})
res.errors

Expected behavior

I think in normal case res.errors should return an error relative to the index of hash who fail to correpsond of one of the schemas.

My environment

• Using dry-schema 1.5.5
• Affects my production application: YES
• Ruby version: ruby 2.4.0
• OS: ubuntu 18.04

[solnic]

This fixes the crash but it's hard to say whether it can be treated as THE fix:

diff --git lib/dry/schema/message/or.rb lib/dry/schema/message/or.rb
index f8d4736..c73fcc1 100644
--- lib/dry/schema/message/or.rb
+++ lib/dry/schema/message/or.rb
@@ -15,14 +15,20 @@ module Dry
           msgs = [left, right].flatten
           paths = msgs.map(&:path)
 
-          if paths.uniq.size == 1
-            SinglePath.new(left, right, messages)
+          if left.is_a?(Array)
+            if right.is_a?(Array) && paths.uniq.size > 1
+              MultiPath.new(left, right)
+            else
+              left
+            end
           elsif right.is_a?(Array)
             if left.is_a?(Array) && paths.uniq.size > 1
               MultiPath.new(left, right)
             else
               right
             end
+          elsif paths.uniq.size == 1
+            SinglePath.new(left, right, messages)
           else
             msgs.max
           end

Then with your example slightly tweaked:

require 'dry/validation'

class Pictures < Dry::Schema::Params
  define do
    required(:name).filled(:string, eql?: 'pictures')
  end
end

class ScanBarcode < Dry::Schema::Params
  define do
    required(:name).filled(:string, eql?: 'scan_barcode')
  end
end

class MissionActionTypeSchema < Dry::Schema::Params
  define do
    optional(:id).filled(:string)
    required(:previous_mission_status_type_reference).filled(:string)
    required(:next_mission_status_type_reference).filled(:string)
    optional(:mission_event_type_references).array(:string)
    optional(:hidden).maybe(:bool)
    optional(:prerequisites).array(:hash, Pictures.new | ScanBarcode.new)
  end
end

s = MissionActionTypeSchema.new
res = s.call({ id: '1', previous_mission_status_type_reference: 'a', next_mission_status_type_reference: 'b', prerequisites: [{name: 'pictu'}]})
puts res.errors.to_h.inspect

# {:prerequisites=>{0=>{:name=>["must be equal to pictures"]}}}

It's tricky because producing "OR" messages coming from two different schemas requires special processing. You need to gather all errors, see which paths are the same and then convert them into "Message::Or".

So, the patch fixes the crash but it doesn't seem to be the final solution.

[fijemax]

Hello, thank you for answer and reactivity,
This seems good in this case but I need to complexify prerequisites schemas as following:

require 'dry/validation'

class Pictures < Dry::Schema::Params
  define do
    required(:name).filled(:string, eql?: 'pictures')
    required(:params).filled(:hash) do
      required(:number).filled(:integer, gt?: 0)
    end
  end
end

class ScanBarcode < Dry::Schema::Params
  define do
    required(:name).filled(:string, eql?: 'scan_barcode')
    required(:params).filled(:hash) do
      required(:missions).filled(:string, included_in?: %w[all all_previous all_next previous next unknown])
      required(:mission_type).filled(:string, included_in?: %w[mission rest departure arrival])
      required(:statuses).array(:string)
      required(:presentation_order).filled(:string, included_in?: %w[reverse none normal])
    end
  end
end


class MissionActionTypeSchema < Dry::Schema::Params
  define do
    optional(:id).filled(:string)
    required(:previous_mission_status_type_reference).filled(:string)
    required(:next_mission_status_type_reference).filled(:string)
    optional(:mission_event_type_references).array(:string)
    optional(:hidden).maybe(:bool)
    optional(:prerequisites).array(:hash, Pictures.new | ScanBarcode.new)
  end
end

s = MissionActionTypeSchema.new
res = s.call({ id: '1', previous_mission_status_type_reference: 'a', next_mission_status_type_reference: 'b', prerequisites: [{name: 'pictures', params: {number: 8} }]})
puts res.errors.to_h.inspect

and I got an error with following messages:

whereas hash corresponds well to a Pictures

[fijemax]

Maybe this idea have already thread but why don't implement something like @JsonSubTypes from the Jackson java library to treat this case ?

[solnic]

@fijemax I'm not familiar with this library, how would this help?

[madejejej]

@fijemax could you see if this patch works for you? #387

It fixes a few problems I've encountered, but I also wouldn't call it a proper fix. In my opinion, it might be better to rearchitect all of Dry::Schema::Message::Or::*, so that it actually quacks like Dry::Schema::Message.

I'm not a regular contributor, so my intuition might not be best here though.