Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow custom coercion failure messages #1203

Closed
wants to merge 1 commit into from

Conversation

zuk
Copy link

@zuk zuk commented Nov 14, 2015

Currently if something goes wrong inside a custom param type's .parse method, the only feedback in the resulting validation failure response is a generic "____ is invalid" message. This pull request allows custom coercion failure messages by having the .parse method return an InvalidValue initialized with the failure message.

For example:

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

  def self.parse(value)
    if  %w(blue red green).include?(value)
      new(value)
    else
      Grape::Validations::Types::InvalidValue.new "is not a valid color"
    end
  end
end

# ...

params do
  requires :color, type: Color, default: Color.new('blue')
end

get '/stuff' do
  # params[:color] is already a Color.
  params[:color].value
end

zuk pushed a commit to zuk/grape that referenced this pull request Nov 14, 2015
@dblock
Copy link
Member

dblock commented Nov 14, 2015

Great, can you squash these commits please?

@zuk zuk force-pushed the custom-coercion-failure-message branch from 756e9ff to fa6b90d Compare November 15, 2015 00:46
@zuk
Copy link
Author

zuk commented Nov 15, 2015

Ok done. Not sure what's up with those failed Rubinius builds in Travis.

@@ -11,7 +11,12 @@ def validate_param!(attr_name, params)
if valid_type?(new_value)
params[attr_name] = new_value
else
fail Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message_key: :coerce
bad_value = new_value
if bad_value.is_a?(Types::InvalidValue) && !bad_value.message.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not in love with this. We're now explicitly checking what kind of exception we got. Any better suggestions?

@zuk
Copy link
Author

zuk commented Nov 15, 2015

I think there's a bigger problem here. Coercion and validation take separate paths but are really converging towards the same thing (i.e. we'll eventually end up needing full-fledged validation inside .parse). But this feels too monumental for a casual contributor to take on. Another way to do this would be to give custom types an optional .validate method that behaves like a regular custom validator, and when present, gets called prior to coercion. I considered doing it this way but thought the current proposal would be more likely to get pulled since it leverages the exiting BadValue type (which already seems a bit out of place... its acts like an exception, but isn't...)

Custom param types can now report why coercion failed in their .parse
method by returning an InvalidValue initialized with an error message.
For example:

  Grape::Validations::Types::InvalidValue.new "is too short"
@zuk zuk force-pushed the custom-coercion-failure-message branch from 242ecea to ce9f494 Compare July 2, 2016 18:06
@dblock
Copy link
Member

dblock commented Aug 12, 2017

I encountered something related in #1675. I think our validation + coercion needs some serious revisiting. Just thinking out loud.

@zuk
Copy link
Author

zuk commented Aug 15, 2017

I agree :) We've been using our own fork of Grape with the above change, periodically rebasing to origin. It works, but it's pretty hacky. There's probably a bigger architectural issue starting to surface here, but like I said before, it feels like too much for me to try to take on.

@zuk zuk closed this Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants