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

Issues versioning serializers with Grape 1.4 master #55

Closed
nhinze opened this issue Jul 13, 2016 · 17 comments
Closed

Issues versioning serializers with Grape 1.4 master #55

nhinze opened this issue Jul 13, 2016 · 17 comments
Labels

Comments

@nhinze
Copy link

nhinze commented Jul 13, 2016

I'm building an API with:

  • Ruby 2.2
  • Rails 4.2.6
  • grape-gem 0.16.2
  • active_model_serializers-gem 0.10.2
  • grape-active_model_serializers-gem (1.4 from master)

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
Copy link
Member

drn commented Jul 13, 2016

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
Copy link
Author

nhinze commented Jul 14, 2016

@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
Copy link
Member

drn commented Jul 14, 2016

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
Copy link
Member

drn commented Jul 15, 2016

@nhinze can you try your project against master?

@drn drn added the bug? label Jul 15, 2016
@nhinze
Copy link
Author

nhinze commented Jul 15, 2016

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
Copy link
Author

nhinze commented Jul 15, 2016

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
Copy link
Member

drn commented Jul 15, 2016

@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
Copy link
Author

nhinze commented Jul 15, 2016

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
Copy link
Member

drn commented Jul 16, 2016

@nhinze - you can try #58 out. I tested it against your project. Thanks for giving me access to it.

@nhinze
Copy link
Author

nhinze commented Jul 18, 2016

@drn this works well. Thank You!

@drn
Copy link
Member

drn commented Jul 21, 2016

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
Copy link
Member

drn commented Jul 24, 2016

@nhinze #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

@drn drn closed this as completed Jul 24, 2016
@nhinze
Copy link
Author

nhinze commented Jul 25, 2016

@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
Copy link
Member

drn commented Jul 25, 2016

Hey @nhinze - I forgot to include handling for collections in that last 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
Copy link
Member

drn commented Jul 26, 2016

Okay @nhinze - #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
Copy link
Author

nhinze commented Jul 26, 2016

Thanks. It works now.

@drn
Copy link
Member

drn commented Jul 26, 2016

Perfect, thanks for following up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants