Routing priority issue in Grape v2.1.0 when using with rails

[eiskrenkov]

Hello, everyone! When upgrading to grape v2.1.0 we've encountered an issue with routing. I created a sample rails new repo for demo purposes - https://github.com/eiskrenkov/grape-routing-issue-demo

Issue

I'm mounting grape API in rails routes at the top of the file

Rails.application.routes.draw do
  mount Twitter::Api => '/'

  get "up" => "rails/health#show", as: :rails_health_check
end

When i'm doing GET http://localhost:3000/up request in grape v2.0.0 it returns response correctly, but in grape v2.1.1 i'm getting 404 error. It seems like something was changed in route prioritising, if nothing matches grape's routes it doesn't continue the journey

Steps to reproduce

1. (export SHOULD_WORK=true; bundle; bundle exec rails s)

will use grape v2.0.0 and will respond to http://localhost:3000/up correctly

2. (export SHOULD_WORK=false; bundle; bundle exec rails s)

will use grape v2.1.0 and respond with 404

[dblock]

Looks like a regression. Try porting this into a spec in this repo without rails? Then it's easy to bisect down to where it changed.

[ericproulx]

I do confirm, this test in our stack is passing in 2.0.0 but not in 2.1.0. There's is something with the / it works when mounting to /api.

context 'rails mounted' do
  let(:app) do
    require 'rails'
    require 'action_controller/railtie'

    api = Class.new(Grape::API) do
      get('/test_grape'){ 'rails mounted' }
    end

    Class.new(Rails::Application) do
      config.eager_load = true
      config.load_defaults 7.1
      config.api_only = true
      config.consider_all_requests_local = true
      config.hosts << 'example.org'

      routes.append do
        mount api => "/"

        get 'up', to: ->(_env) do
          ['200', {}, ['hello world']]
        end

      end
    end
  end

  before { app.initialize! }

  it 'responds' do
    get '/test_grape'
    expect(last_response).to be_successful
    expect(last_response.body).to eq('rails mounted')
    get '/up'
    expect(last_response).to be_successful
    expect(last_response.body).to eq('hello world')
  end
end

[SageOfTixPaths]

Same problem confirmed. I'm moving the mount definition for my Grape routes to the bottom of the routes.rb file as a temporary fix

[dblock]

Can someone bisect this to a change please?

[eiskrenkov]

@dblock I just did, the test provided by @ericproulx starts failing at 3ae7b7a720911a11a933b1788fc82764fbeaf52c, after this pr got merged

[ericproulx]

I found something related to the X-Cascade. I'm still digging

[ericproulx]

@eiskrenkov are you using rack >= 3 ? While testing, I found that its only happening with Rack >= 3 but maybe I'm wrong

[eiskrenkov]

@ericproulx yes, it's 3.1.3 in both my demo repo and grape's Gemfile.lock on my machine where i did bisect

[eiskrenkov]

Thank you so much for your work guys @dblock @ericproulx <3 Will try the new version once it's out

[dblock]

@eiskrenkov please do try HEAD, github: 'ruby-grape/grape' in Gemfile, appreciate if you could confirm the fix works.

[eiskrenkov]

@dblock sure thing! Just tried it in my project, it helped! Routes that are defined under grape mounting are reachable now. Thank you so much for such a quick fix again!

[ericproulx]

@dblock Until now, I couldn't wrap my head around this bug because obviously we had a test for cascading. While looking at the caller when calling the default response, I figured that rack-test processes the request with a Rack::MockResponse which will lowercase the headers through Rack::Response by design. So, even though we were returning X-Cascade, the last_response.headers would have x-cascade.

Now, when running within a rails app, the default-response is managed by Rails's routing which will look for x-cascade with Rack 3 instead of X-Cascade.

[ericproulx]

A test like this one would have catch it

context 'cascade within another app' do
  subject { last_response.headers }

  let(:api) do
    Class.new(Grape::API) do
      get('/test'){ return_no_content }
    end
  end

  let(:app) do
    test_api = api
    Rack::Builder.app do
      use Rack::Lint
      run test_api.new
    end
  end

  before { get '/' }

  it { is_expected.to include('x-cascade') }
end

Rack::Lint would have raised

Rack::Lint::LintError:
  uppercase character in header name: X-Cascade