ruby-grape / grape

An opinionated framework for creating REST-like APIs in Ruby.
http://www.ruby-grape.org
MIT License
9.88k stars 1.22k forks source link

Grape::Exceptions::UnknownValidator #1178

Open gottfrois opened 8 years ago

gottfrois commented 8 years ago

I have the following files:

# app/controllers/api/v2/base.rb
module Api
  module V2
    class Base < Grape::API
      mount V2::Jobs
    end
  end
end

# app/controllers/api/v2/jobs.rb
module Api
  module V2
    class Jobs < Grape::API
      resources :jobs do
        params do
          requires :id, type: String, uuid: true
        end
        get ':id' do
        end
      end
    end
  end
end

# app/controllers/api/v2/validators/uuid.rb
module Api
  module V2
    module Validators
      class Uuid < ::Grape::Validations::Base

        REGEX = %r{[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}}

        def validate_param!(attr_name, params)
          unless (params[attr_name] =~ REGEX).zero?
            fail ::Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: 'must be a valid UUID'
          end
        end

      end
    end
  end
end

If I want the above to work, I have to manually require the validator class in base.rb file, like so:

require_relative './validators/uuid'

which result in throwing warnings:

app/controllers/api/v2/validators/uuid.rb:6: warning: already initialized constant Api::V2::Validators::Uuid::REGEX
app/controllers/api/v2/validators/uuid.rb:6: warning: previous definition of REGEX was here

since rails will automatically require the validator as well. Any idea why grape is not able to pick up the custom validator? Looks like a require order issue or something. Any clue?

Thanks

towanda commented 8 years ago

Yes, It seems like a require order issue, how are you loading grape in rails?

gottfrois commented 8 years ago

nothing fancy, i have grape in the gemfile and have this in my route:

mount Api::V2::Base => '/'
dm1try commented 8 years ago

Any idea why grape is not able to pick up the custom validator?

grape doesn't have an autoloading mechanism for custom validators (PR are welcomed).

In your case you can use require_dependency

# app/controllers/api/v2/base.rb
require_dependency 'api/v2/validators/uuid'

But I should explain why this happens. First, how does grape load buit-in validators?

In short, grape validators are "auto-registered" only if the validator file is explicity loaded.

Second, Rails autoloading relies on #const_missing to load constants. Obviously, the constant will be autoloaded only if we have its reference in the code.

In your case: Api::V2::Base const is specified in routes.rb => loads base.rb V2::Jobs in base.rb => loads jobs.rb Api::V2::Validators::Uuid is not specified in jobs.rb, only the short name and this is why rails doesn't load your custom validator.

P.S. as you can see from the explanation some :scream: things can be done:

# app/controllers/api/v2/base.rb

Api::V2::Validators::Uuid # instead of require_dependency 'api/v2/validators/uuid'

module Api
  module V2
gottfrois commented 8 years ago

Thanks for the great explanation, makes perfect sense now. I'm guessing we could improve things on grape by looking up for the constant when we hit the :uuid value in validator parameters?

dm1try commented 8 years ago

@gottfrois this sounds reasonable, but I think we should register all available validators(bullt-in/custom) before a hitting anything(validators_paths helper would be helpfull to register a bunch of validators from different directories).

Also I'm personally don't like the implementation using inherited callback(the validator code must be eager loaded for this).

ok, the 3th solution :)

    class Base < Grape::API
      Grape::Validations.register_validator("uuid", Api::V2::Validators::Uuid) # by design it will be called twice
    end   

@dblock any thoughts?)

gottfrois commented 8 years ago

Since grape params are declarative, we could simply add a constant lookup strategy in place that will be trigger when the ruby file is loaded. Am I missing anything? We don't have to look for the constant when someone hits the endpoint.

Also I'm personally don't like the implementation using inherited callback(the validator code must be eager loaded for this).

Yeah sounds like an easy solution. Ideally you would look/require only used constants.

dm1try commented 8 years ago

Since grape params are declarative, we could simply add a constant lookup strategy in place that will be trigger when the ruby file is loaded. Am I missing anything?

You should be able to resolve the constant nesting in that place (from :uuid to Api::V2::Validators::Uuid).

dm1try commented 8 years ago

@gottfrois take a look on the referenced issue, I've implemented some part

kevinelliott commented 7 years ago

What is the status of this? Has there been any progress on this validator issue (specifically with :uuid) in the last 2 years?

dblock commented 7 years ago

@kevinelliott Doesn't look like it, feel free to PR.

koushinrin commented 7 years ago

😢 Run into this bug(?) using grape in rails. Still not fixed.

loicginoux commented 4 years ago

I have an issue related to this. I had one private api where I loaded all my api validators at the root of the grape api file:

Dir[File.dirname(__FILE__) + '/validators/*.rb'].each {|file| require file }

Now I have to create a second public api on the same rails app. I did the same to load all my public api validators on the root of the public api file. The issue is that some validators have the same name and conflicting, for example category_id_validation below will raise either a MyApi::V1::Exceptions or a MyApiPublic::V1::Exceptions ex:

class CategoryIdValidation < Grape::Validations::Base
  include MyApi::V1::Helpers::Authentication

  def validate(request)
    if request.params[:category_id].present?
      unless current_user(request).categories.map(&:id).include?(request.params[:category_id])
        raise MyApi::V1::Exceptions::ParamsIdNotExist, "category #{request.params[:category_id]} does not exist"
      end
    end
  end
end

It would be good to be able to load manually validators for each API so that we can have scoped validators and don't have name conflicts.

The only solution I can think of for now is having to rename validators on the second api like public_api_category_id_validation to not conflict with first validators from the other api, which is not great...

dblock commented 4 years ago

It would be good to be able to load manually validators for each API so that we can have scoped validators and don't have name conflicts.

agreed

aesyondu commented 3 years ago

Has anyone figured out how to use this with Rails 6/zeitwerk?