Dry::Types::Constructor(URI) triggers :uri? predicate with bad arity

[doriantaylor]

The recent addition of the uri? predicate to Dry::Logic::Predicates has the unfortunate side effect of crashing when a Dry::Types::Constructor is based off of URI (or presumably /(?:.*::)?URI$/; as will be explained momentarily):

Exception: ArgumentError: uri? predicate arity is invalid
--
0: /usr/local/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/predicate.rb:75:in `ensure_valid'
1: /usr/local/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/trace.rb:104:in `method_missing'
2: /usr/local/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/trace.rb:52:in `block in evaluate_predicates'
3: /usr/local/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/trace.rb:44:in `each'
4: /usr/local/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/trace.rb:44:in `flat_map'

The following code will reproduce the error given dry-schema 1.5.6 and dry-logic 1.0.8:

require 'dry/schema'

module WTF
  module Types
    include Dry::Types()
    URI = Types.Constructor(::URI) { |x| ::URI.parse x }
  end

  Schema = Dry::Schema.Params { required(:base).value Types::URI }
end

Diagnosis

It appears that what is happening is the Types::URI object is being handed off to an instance of Dry::Types::PredicateInferrer like so:

Dry::Types::PredicateInferrer.new[Types::URI]
# => [:uri?]

…which it appears to do via Dry::Schema::Macros::DSL#extract_type_spec. The :uri? predicate is picked because of line 49 in dry-types/predicate_inferrer.rb, which looks like:

[TYPE_TO_PREDICATE.fetch(type) { :"#{type.name.split('::').last.downcase}?" }]

…which if I understand correctly will turn the class name URI into the symbol :uri? given that it is not found in the hash. Since this predicate takes two arguments, Dry::Schema::Predicate#ensure_valid raises an exception because this invocation of :uri? has been queued without its other argument (an array of URI schemes), and to my knowledge there is no (documented) way to get additional arguments into predicates that have been queued through the inferrer mechanism.

Remedies

I see a number of possible remedies for this situation; the simplest perhaps being to overwrite the uri? predicate in the application with a method of arity 1. My only problem with this is it is not clear via the documentation where the predicate is actually being invoked and thus where it must be overridden.

Another solution would be to modify Dry::Types::PredicateInferrer#infer_predicate to ignore matches on predicates with arities greater than 1. Ostensibly the most involved solution would be to change (or document?) the interface so the additional arguments can be passed into multi-argument predicates when those predicates have been inferred.

Remarks

This bug was particularly difficult to track down. The behaviour spans three modules (dry-schema, dry-types, dry-logic), which is fine—it is DRY after all—but perhaps some additional signposts or an architectural overview would be helpful. The ultimate raise in ensure_valid was particularly ambiguous, down to the choice of ArgumentError: in addition to the terse error message, it wasn't clear initially where this error was coming from. My inclination here would be to create a subclass of ArgumentError (say, PredicateParamError) to signal the peculiar case that a given predicate—which has yet to be invoked—has not been bundled with its requisite parameters. Saying something explicit in the error message like predicate :foo? takes parameters; see docs at … would also be helpful.

The overarching approach to mapping class names to predicates, moreover, has the potential to produce fragile and surprising results. Consider:

module Hmm
  class Key
    # this should trigger INVALID_PREDICATES
  end

  module Types
    include Dry::Types()

    Key = Types.Constructor(Key)
  end

  Schema = Dry::Schema.Params do
    required(:key).value Types::Key
  end
end

…sure enough:

Exception: Dry::Schema::InvalidSchemaError: key? predicate cannot be used in this context
--
0: /usr/local/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/trace.rb:96:in `method_missing'
1: /usr/local/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/trace.rb:52:in `block in evaluate_predicates'
2: /usr/local/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/trace.rb:44:in `each'
3: /usr/local/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/trace.rb:44:in `flat_map'
4: /usr/local/lib/ruby/gems/2.7.0/gems/dry-schema-1.5.6/lib/dry/schema/trace.rb:44:in `evaluate_predicates'

This is because of line 95 in trace.rb, looking for the derivation of the class name :key? which has been listed as invalid. As with the introduction of the :uri? predicate did with my code, there is potential for future predicates colliding with class names given to Dry::Types::Constructor. As such I apologize if I have potentially burdened you with an architectural rethink, and that my ignorance of the inner workings of dry-rb precludes me of being much more help than this. Nevertheless if pointed in the right direction I can hopefully do more.

[solnic]

Thanks again for such a detailed report. I'll get to it eventually unless somebody beats me to it first.

[doriantaylor]

What kind of solution are you aiming for?

• Skip over inferred predicates with arity != 1
• Add (document?) some kind of mechanism for getting extra arguments into inferred predicates
• Something else?

Can you tell me where I can add a dummy uri? predicate in here so I can get this package running again in the interim?

[flash-gordon]

@solnic @doriantaylor IMO the correct solution would be removing this fallback from dry-types https://github.com/dry-rb/dry-types/blob/3fac200005fccc546386281af27dff11de45bdb1/lib/dry/types/predicate_inferrer.rb#L49
I don't know why it was added, surely it wasn't me (I ported this code from dry-schema or dry-validation). This fallback uses reflection based on the class name, it's absolutely counter-intuitive in my view. Besides, it fails when you pass an anonymous class to the inferred. I would even call this a bug, not a feature, and remove straight away if you're ok with this :)

[flash-gordon]

I mean I doubt anyone uses this intentionally

[doriantaylor]

Yeah, hey, if it's not intentional then I doubt anybody will miss it…

Question: would it make sense to be able to do something like:

Foo = Types.Constructor(::Foo) do |input|
  Foo.new(coerce_foo input)
end.predicate do |raw|
  validate_foo raw
end

…and then have the validation predicate attached to the type itself?

[solnic]

@flash-gordon @doriantaylor the intention was to support arbitrary types with a primitive that has a corresponding predicate type-check, it's obviously buggy and naive so it should be fixed, rather than removed (especially that removing it would break people's code). I believe a simple check if there's a valid corresponding predicate will be enough.

[solnic]

@doriantaylor btw for the time being you can pin dry-logic to the version that doesn't have uri? predicate

[flash-gordon]

Fixed via dry-rb/dry-types#414
Fun fact, this feature worked (as in really worked as planned) for classes with names:

Filled
Bool
Number
Empty
Odd
Even
True
False
UUID_V1
UUID_V2
UUID_V3
UUID_V4
UUID_V5

I don't think it has a lot of usage :)

[solnic]

@flash-gordon we probably want to make this fix in dry-schema 1.6.0 because it should depend on latest dry-types that may require some people to turn on the old behavior. WDYT? (assuming I understand dry-rb/dry-types#414 correctly 🙂)

[flash-gordon]

@solnic like cut a release with a bumped dependency and add an entry to the changelog?

[solnic]

@flash-gordon yes because if somebody pulls in latest dry-types w/o us bumping dry-schema, it could be a nasty surprise. If we make it more explicit through dep update + new release of dry-schema, things will be more obvious for the users.

[flash-gordon]

So, the final solution will be:

1. If you happen to depend on class name-based inference in any way, it'll throw an exception after updating to 1.6.0.
2. You can turn off inference explicitly with

   Dry::Schema::PredicateInferrer::Compiler.infer_predicate_by_class_name false

   It'll make the following code work:

   module Types
     include Dry::Types()
     URI = Types.Constructor(::URI, ::URI.method(:parse))
   end
   
   Dry::Schema.Params { required(:base).value(Types::URI) }

3. Or you can restore the previous behavior with:

   Dry::Schema::PredicateInferrer::Compiler.infer_predicate_by_class_name true

   Keep in mind, this option will be removed in dry-types 2.0.

[solnic]

@flash-gordon sounds like a really good plan!

[doriantaylor]

when are you going to release this?

[flash-gordon]

[flash-gordon]

I mean it's basically ready. I'll deploy it to production today and release gems tomorrow (if nothing comes up).