Support custom types

[rnubel]

This implements #993. Originally, I thought it'd be possible to do this simply by having the custom type extend Virtus::Attribute, but a) it turned out to not be that easy and b) I figured it'd be better to define a pattern for this that isn't directly coupled to Virtus.

So, my change allows you to take any class and coerce parameters into it. All that's required is that the class implement a from_param class method. This name is arbitrary, but I feel it matches up with the intent of the change... totally open to suggestions, though. As a quick example:

class Color
  attr_reader :value
  def initialize(color)
    @value = color
  end
  def self.from_param(value)
    raise "Invalid color" unless %w(blue red green).include?(value)
    new(value)
  end
end

# ...

params do
  required :color, type: Color, default: Color.new('blue')
end
get '/stuff' do
  # params[:color] is already a Color.
  params[:color].value
end

# /bin/sh
$ curl localhost:9292/api/stuff?color=red
"red"

Things to consider:

• If coercion fails, it's hard to tell why. The global rescue inside CoerceValidator#coerce_value hides any errors inside the coercion method with a "is invalid" message. This is probably acceptable for a first pass, though.
• Should my tests be in param_scope_spec or coerce_spec? I'm leaning towards the latter, even though they're currently in the former.
• Is there a better/cleaner approach here than checking for the existence of the from_param method?

[dblock]

Run this "code" through RuboCop, just to keep things consistent, change " to ', add a space between methods, etc.

[dblock]

This is great. Needs a CHANGELOG entry, please.

I am not in love with the name from_param. Can we brainstorm better names? Maybe coerce?

That said, I think the constructor should also be the coercer. This avoids the whole interface problem and allows you to use pretty much any type, no?

class Color
    attr_reader :value
    def initialize(value)
        fail 'Invalid color' unless %w(blue red green).include?(value)
        @value = value
    end
end

[rnubel]

Thanks for the feedback! Using the initializer would be clean, but also a bit limiting. It's fine if you're creating the custom type from scratch for just this purpose, but it would restrict you if you wanted to use an existing class as a type. Suppose you already had a Color class which initializes with R, G, and B values, for example.

Let me know what you think. If we go with the extra-class-method approach, I think coerce is a pretty good name.

[dblock]

It could just be Monday, but I don't see why it wouldn't work with an existing class, care to provide an example? :)

[rnubel]

Maybe I'm misunderstanding, but suppose we already have a Color class used by the rest of our API, like so:

class Color
  def initialize(r, g, b)
    @r, @g, @b = r, g, b
  end
end

The constructor takes three integer arguments, so it couldn't directly be used to coerce the value. However, if we add a ::coerce method like so:

class Color
  def self.coerce(param)
    r, g, b = param.unpack('A2A2A2').map(&:hex)
    new(r, g, b)
  end
end

Then you could hit endpoints like /api/pixel?color=FF0000 and it would parse the color param into the same thing as Color.new(255,0,0).

[dblock]

In the example above you would still have to extend Color with a coerce method that doesn't respect the original wishes of Color, is it worse to rewrite an initializer?

Some library gives you Color:

class Color
  def initialize(r, g, b)
    @r, @g, @b = r, g, b
  end

  def to_s
    "#{@r}|#{@g}|#{@b}"
  end
end

you open it up:

class Color
  alias_method :_initialize, :initialize
  def initialize(* args)
    if args.length == 3
      _initialize(*args)
    elsif args.length == 1
      @r, @g, @b = args.first.unpack('A2A2A2').map(&:hex)
    else
      fail 'Oh no.'
    end
  end
end

And:

2.0.0-p598 :183 >   Color.new(10, 20, 55)
 => #<Color:0x007f8e5595ceb0 @r=10, @g=20, @b=55> 
2.0.0-p598 :184 > Color.new('FFCC11')
 => #<Color:0x007f8e55965718 @r=255, @g=204, @b=17> 

Now that I wrote it I think it's worse.

Ruby actually has a way to deal with this for many times, eg. DateTime. I think the method should be called parse, not coerce, this is how most classes support parsing a String into an instance of themselves.

[rnubel]

Yeah, parse definitely expresses the intent of the method, and aligns with existing Ruby standards. My only concern is that my implementation would then directly set the coercer for types that already respond to parse, like DateTime and Date. Granted, that's all Virtus' coercible gem does anyway, so it should be fine. And, I did a quick check and none of the other common types (Integer, Float, Hash, Array) support the parse method.

I'll push up a change to make it parse instead of from_param. Let me know what you think.

Edit: This was complicated slightly by what I described above; since Date.parse('') raises an error, it needs to be handled more deftly. I felt it best to leave that to Coercible. However, the guard I had to put in to fix that is definitely a bit hacky... I'm kind of leaning towards going back to coerce.

[dblock]

Put back this line below yours ;)

[dblock]

See comments above and squash your commits, please.

[rnubel]

@dblock changes made. I'm not entirely sure the ParameterTypes module is worth it, as-is, so let me know what you think.

Part of the problem is, Grape delegates coercion directly to Virtus, and Virtus' coercion logic is pretty complex -- for example, if you specify a type which has included Virtus.model, Grape will handle it just fine (this is why the "complex type" logic as referenced in #1043 happens to actually work). So, deciding up-front whether or not we'll be able to coerce a given type is almost impossible. It might be a good long-term decision to start enforcing a list of allowed types, in order to decouple Grape from Virtus' inner workings. But that's not for this PR.

[dblock]

I am surprised that RuboCop didn't flag this one, the if would go after the line. Maybe we autoignored it, if you don't mind looking into it for a separate PR?

[rnubel]

What are you referring to? It looks like only a few cops are turned off and I don't think any of them apply here.

[dblock]

I'd have to dig :) I think it should tell you to do converter_options[:coercer] = type.method(:parse) if .... It's really nbd.

[dblock]

This is great. If you want to address any of the above in a future PR, please feel free. Merging.