ruby-grape / grape-active_model_serializers

User active_model_serializers with Grape
MIT License
139 stars 68 forks source link

Issues versioning serializers with Grape 1.4 master #55

Closed nhinze closed 8 years ago

nhinze commented 8 years ago

I'm building an API with:

My JSON serializers work well until I try to introduce API Versioning as described here: https://github.com/ruby-grape/grape-active_model_serializers

Unversioned serializer:

class SignupSerializer < ActiveModel::Serializer
  attributes :id, :comments, :pending
end

Versioned serializer:

module Mobile
  module V1
    class SignupSerializer < ActiveModel::Serializer
      attributes :id, :comments, :pending
    end
  end
end

The error message is:

LoadError (Unable to autoload constant SignupSerializer, expected /Users/username/GitHub/AppName/app/serializers/mobile/v1/signup_serializer.rb to define it):
app/api/mobile/logger.rb:16:in `block in call'
app/api/mobile/logger.rb:15:in `call'

API Organization:

enter image description here

base.rb:

require 'grape-swagger'

module Mobile
  class Dispatch < Grape::API
    mount Mobile::V1::Root
    mount Mobile::V2::Root
    format :json
    route :any, '*path' do
      Rack::Response.new({message: 'Not found'}.to_json, 404).finish
    end
    add_swagger_documentation
  end

  Base = Rack::Builder.new do
    use Mobile::Logger
    run Mobile::Dispatch
  end
end

v1/root.rb:

module Mobile
  module V1
    class Root < Dispatch
      format :json
      formatter :json, Grape::Formatter::ActiveModelSerializers
      version 'v1'
      default_error_formatter :json
      content_type :json, 'application/json'
      use ::WineBouncer::OAuth2

      rescue_from :all do |e|
        eclass = e.class.to_s
        message = "OAuth error: #{e.to_s}" if eclass.match('WineBouncer::Errors')
        status = case 
        when eclass.match('OAuthUnauthorizedError')
          401
        when eclass.match('OAuthForbiddenError')
          403
        when eclass.match('RecordNotFound'), e.message.match(/unable to find/i).present?
          404
        else
          (e.respond_to? :status) && e.status || 500
        end
        opts = { error: "#{message || e.message}" }
        opts[:trace] = e.backtrace[0,10] unless Rails.env.production?
        Rack::Response.new(opts.to_json, status, {
          'Content-Type' => "application/json",
          'Access-Control-Allow-Origin' => '*',
          'Access-Control-Request-Method' => '*',
        }).finish
      end

      mount Mobile::V1::Me
      mount Mobile::V1::Events
      mount Mobile::V1::Signups

      route :any, '*path' do
        raise StandardError, 'Unable to find endpoint'
      end
    end
  end
end

api/v1/signup.rb:

module Mobile
  module V1
    class Signups < Mobile::V1::Root
      include Mobile::V1::Defaults

      resource :signups, desc: 'Operations about the signups' do

        desc "Returns list of signups"
        oauth2 # This endpoint requires authentication

        get '/' do
          Signup.where(pending: false)
        end

      end

    end

  end
end

Any ideas?

drn commented 8 years ago

Hey @nhinze, I did some work on namespacing against ASM v0.10.0+ in this pull. The changes in the pull should address your issue.

Grape-ASM tries to send a namespace key to ActiveModel::Serializer.serializer_for here, but support for that key is no longer supported by ASM as described here. So, grape-ASM has to handle serializer lookup by version explicitly.

nhinze commented 8 years ago

@drn, thanks. I'll wait until its merged into master to try it out.

In my signup.rb above, I use "resource", how do I namespace it? Just namespace right around the "get"? I was trying to get it to work with ams .9.5, but it's giving me the same error.

drn commented 8 years ago

Namespacing is based off of the version option, which is v1 in your example in signup.rb. Looking at it, it looks like the surrounding Mobile module might cause issues as namespacing is being based off the version option exclusively. However, constant resolution might handle it fine if you don't have another top-level namespace with another V1::SignupsSerializer somewhere in it.

I'll get that v0.10 support pull merged onto master today. If it doesn't solve your issue, let me know and we'll work on adding some specs and a fix for your use case.

drn commented 8 years ago

@nhinze can you try your project against master?

nhinze commented 8 years ago

Using grape-active_model_serializers 1.4.0 from https://github.com/ruby-grape/grape-active_model_serializers.git (at master@a48d57c)

I still get this error:

action_controller__exception_caught

nhinze commented 8 years ago

The API structure above is inspired from http://codetunes.com/2014/introduction-to-building-apis-with-grape/ . I also named my first API "Mobile", because this one will interact with my mobile app. There will be a separate API for another backend.

This is my first time building an API and maybe my setup is not quite correct. I'll gladly take any recommendations.

drn commented 8 years ago

@nhinze would you be able to give me access to the project you're experiencing this error with? If so, I can dig into the error. If not, would you be able to write a spec that covers this issue?

Re: the Mobile name, if there won't be another api in the rails application, you can get rid of the mobile namespace. the folders within the api namespace should be used for versioning as described in the README where an file hierarchy is displayed.

nhinze commented 8 years ago

The "mobile" api used to be called " api" and the folder structure was /app/api/api/v1 . I plan to have a second API for another service. The api would not work in /app/api/v1.

I have the project on GitLab. I'll PM you.

drn commented 8 years ago

@nhinze - you can try https://github.com/ruby-grape/grape-active_model_serializers/pull/58 out. I tested it against your project. Thanks for giving me access to it.

nhinze commented 8 years ago

@drn this works well. Thank You!

drn commented 8 years ago

Great! I'll let you know when a fix is merged after I get back from vacation on Thursday. I'll close this issue then

drn commented 8 years ago

@nhinze https://github.com/ruby-grape/grape-active_model_serializers/pull/60 has been merged which includes a fix for your issue, so you can point your project to master until v1.5.0 is released

nhinze commented 8 years ago

@drn tried to use the master branch, but it doesn't work. I get the same error as before. The namespace-inferred-serializer branch works fine.

drn commented 8 years ago

Hey @nhinze - I forgot to include handling for collections in that last pull. https://github.com/ruby-grape/grape-active_model_serializers/pull/61 should fix it after it's merged in. I'll let you know as soon as that's merged in and will do a v1.5.0 after you confirm it's working

drn commented 8 years ago

Okay @nhinze - https://github.com/ruby-grape/grape-active_model_serializers/pull/61 is merged. I tested it successfully against your project. Note that you'll need to add the Mobile:: namespace to V2::SignupSerializer -> Mobile::V2::SignupSerializer for it to work.

nhinze commented 8 years ago

Thanks. It works now.

drn commented 8 years ago

Perfect, thanks for following up