Creating custom validators is difficult

[stanhu]

I'm attempting to port a custom validator to Grape v1.3.1:

requires :custom, type: ::API::Validations::Types::CustomObject, desc: 'This is a test'

At first, I tried to take https://github.com/ruby-grape/grape/blob/41ee988c06b87b036d12c4c7247d06b75bfa0b7f/lib/grape/validations/types/file.rb as an example:

module API
  module Validations
    module Types
      class CustomObject
        def coerce(input)
          input
        end

        def coerced?(value)
          value.is_a?(::CustomObject)
        end
      end
    end
  end
end

In Grape v1.1.0, this worked (ignore the coerced_value? vs.coerced? change and lack of inheritance of Virtus::Attribute) because BuildCoercer.create_coercer_instance would just fall through and create a Virtus Attribute: https://github.com/ruby-grape/grape/blob/v1.1.0/lib/grape/validations/types/build_coercer.rb#L71.

In Grape v1.3.1, with the switch to dry-types, all unknown types fall through to PrimitiveCoercer and fails:

1. grape/lib/grape/validations/types/build_coercer.rb

   Line 67 in 812284f

   PrimitiveCoercer.new type, strict

   .
2. This quietly fails validation because super in

   grape/lib/grape/validations/types/primitive_coercer.rb

   Line 42 in 812284f

   super

   eventually calls @coercer[val] in

   grape/lib/grape/validations/types/dry_type_coercer.rb

   Line 30 in 812284f

   @coercer[val]

   .
3. This fails because @coercer is nil.

To get around this, I saw the custom? check appears to look for self.parse:

grape/lib/grape/validations/types.rb

Lines 134 to 135 in 41ee988

	type.respond_to?(:parse) &&
	type.method(:parse).arity == 1

. I ended up with:

module API
  module Validations
    module Types
      class CustomObject
        def self.coerce(input)
          input
        end

        def self.coerced?(value)
          value.is_a?(::CustomObject)
        end

        def self.parse(value)
          value
        end
      end
    end
  end
end

This appears to work for my purposes, but it seems inconsistent (not to mention undocumented) compared to the built-in validators present in Grape.

Note that dry-types has a simple mechanism for checking types: https://dry-rb.org/gems/dry-types/master/custom-types. Ideally, we'd be able to pass in one of these custom types so we don't need all that code above, but this doesn't work either for the same reasons above:

requires :custom, type: DryTypes.Instance(Range), desc: 'This is a test'

@dnesteryuk My questions are:

1. Should the Grape type validations look more consistent between Json, File, and custom ones (e.g. class vs. instance methods, self.parse method)?
2. If so, how can we make them more consistent? Otherwise, is this just a documentation issue?
3. Should Grape handle custom dry-types (e.g. https://dry-rb.org/gems/dry-types/1.2/custom-types) natively?

[dnesteryuk]

@stanhu First of all there is documentation about the required parse method. Probably, we need to update internal classes, a PR is welcome 🙂

I know that Dry-types supports custom types, however, I don't think we need to couple Grape and Dry-types that match. Maintenance of Virtus was dropped, we don't know what might happen to Dry-Types in the future. Also, there are differences in coercing, you might've seen this method, so custom types maintained by Dry-types could bring breaking changes. In general, what can be simpler than

class Color
  attr_reader :value
  def initialize(color)
    @value = color
  end

  def self.parse(value)
    fail 'Invalid color' unless %w(blue red green).include?(value)
    new(value)
  end
end

A simple plain Ruby class which everyone can understand without checking documentation. 😉

[dblock]

You could roll out a grape-dry-types gem that makes the connection, even if trivial.

[stanhu]

@dnesteryuk Ah, thank you! I don't know how I missed that block of documentation. I'll look at improving the internal classes to reflect this.

[stanhu]

@dnesteryuk Actually, the example above doesn't work because coerced? has to return true as well:

grape/lib/grape/validations/types/custom_type_coercer.rb

Line 58 in 8deebb5

return InvalidValue.new unless coerced?(coerced_val)

I think we need to update the documentation to reflect that? Something like:

class Color
  attr_reader :value
  def initialize(color)
    @value = color
  end

  def self.parse(value)
    fail 'Invalid color' unless self.coerced?(value)
    new(value)
  end

  def self.coerced?(value)
    %w(blue red green).include?(value)
  end
end

[stanhu]

I'll revise my statement because it's not right. dry-types has stronger type checking now, so if the parse returns an object other than the type specified in the API, then it will be invalid. For example:

class Color
  attr_reader :value
  def initialize(color)
    @value = color
  end

  def self.parse(value)
    fail 'Invalid color' unless self.parsed?(value)
    new SomeThing(value)
  end

  def self.parsed?(value)
    %w(blue red green).include?(value)
  end
end

That's because the following block comes into play:

grape/lib/grape/validations/types/custom_type_coercer.rb

Lines 96 to 110 in 8deebb5

	if type.respond_to? :coerced?
	type.method :coerced?
	elsif type.respond_to? :parsed?
	type.method :parsed?
	elsif type.respond_to? :call
	# Arbitrary proc passed for type validation.
	# Note that this will fail unless a method is also
	# passed, or if the type also implements a parse() method.
	type
	elsif type.is_a?(Enumerable)
	->(value) { value.respond_to?(:all?) && value.all? { |item| item.is_a? type[0] } }
	else
	# By default, do a simple type check
	->(value) { value.is_a? type }
	end

[dblock]

What do you think we shall we do with this issue, @stanhu?

[stanhu]

@dblock I submitted #2029. Let me know if that makes sense.

Would you mind releasing 1.3.2 soon? Thanks!

[dblock]

Thank you. Opened #2030 for a release, add +1.