helpers injection pollutes Grape::Middleware::Error

[etehtsea]

This commit introduces very strange issues in our projects:

1. helpers are including to the middleware which is global to all code that use Grape. So if you have two helpers with the same name you are in trouble.

   >> auth = session(Auth)
   => #<Rack::Test::Session:0x007fb6b582abe0 @headers={}, @env={}, @rack_mock_session=#  <Rack::MockSession:0x007fb6b582ac30 @app=Auth, @after_request=[], @default_host="example.org", @last_request=nil, @last_response=nil>, @default_host="example.org">
   >> auth.get('/').body
   => "auth"
   >> $sessions.each { |s| s.get('/').body }; nil
   => nil
   >> auth.get('/').body
   => "API"
   

   Upsy daisy!

2. Historically in order to not copy pasting format/helpers and other to each endpoint we have Base endpoint like in example from which we subclass all other. After this commit helpers are injecting over and over and ancestors list grows from ~11 to N * endpoints + 11.

   >> Grape::Middleware::Error.ancestors.count
   => 10
   >> session(Auth).get('/')
   >> Grape::Middleware::Error.ancestors.count
   => 10
   >> $sessions.each { |s| s.get('/') }; nil
   => nil
   >> Grape::Middleware::Error.ancestors.count
   => 210
   

   Possibly this is not an issue by itself and could be fixed by moving helpers to the root class where all other endpoints are mounted, but I could not get it to work (helpers behaviour inconsistency #1418).

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'grape', '>= 0.15.0'
  gem 'rack-test'
  gem 'test-unit'
  gem 'pry'
end

require 'grape'
require 'rack/test'
require 'test/unit'

class Auth < Grape::API
  helpers do
    extend Grape::API::Helpers

    params :foo do
      optional :ololo
    end

    def respond_error!
      error!(format_err_msg, 401)
    end

    def format_err_msg
      'auth'
    end
  end

  rescue_from RuntimeError do |_|
    respond_error!
  end

  get do
    raise 'auth error'
  end
end

class Base < Grape::API
  def self.inherited(subclass)
    super

    subclass.instance_eval do
      rescue_from RuntimeError do |_|
        respond_error!
      end

      helpers do
        def format_err_msg
          'API'
        end

        def respond_error!
          error!(format_err_msg, 401)
        end
      end
    end
  end
end

def session(c)
  Rack::Test::Session.new(Rack::MockSession.new(c))
end

def new_endpoint
  Class.new(Base).tap do |k|
    k.get { raise 'error' }
  end
end

$endpoints = Array.new(100) { new_endpoint }
$sessions = $endpoints.map { |e| session(e) }

[dblock]

Good hunting, this looks like a real problem.

[namusyaka]

Good point, thanks @etehtsea for reporting the issue!

[dblock]

Closed via #1421, @etehtsea would you try HEAD please?

[etehtsea]

@dblock despite of some other breaking changes, this fix seems to work.

[dblock]

@etehtsea What are you seeing wrt breaking changes that you didn't expect? Are those described in UPGRADING?

[etehtsea]

The one that wasn't mention in README is #1390. It changes #middleware method output.

Two other are:

• rescue_handlers was moved from .namespace_stackable to .namespace_reverse_stackable (Exceptions aren't rescued in a predictable order #1405)
• We've already have declared_params helper throughout codebase so this PR breaks almost every grape related spec (Fix logic for include_parent_namespaces option #1415)

This is not issues with the grape itself, but as for me it looks like lack of private/public API separation (the same for grape-entity). As a result, every minor improvement can break project based on grape/grape-entity.

P.S. I don't mean anything negative, just wanted to give a little feedback.

[dblock]

Thanks. I think even for internals like these we should document them since at least someone relies on it. Would you like to PR some updates to UPGRADING @etehtsea?