Avoid multiple mount pollution

[alexanderadam]

Soooo, I might have missed something here but the way I see this is, that PR #1893 introduced the behaviour seen in #2576.

And the problem to be tackled was different than what was implemented.
According to the Rack Spec, a Rack app only needs to be

a Ruby object that responds to call. It takes exactly one argument, the environment (representing an HTTP request) and returns a non-frozen Array of exactly three elements: the status, the headers, and the body (representing an HTTP response).

Therefore having Grape::API.call delegating to base_instance.call should be enough.

Howeveeeeeer, when using Rack::Test with def app returning a Grape::API class directly (e.g., API::V1::Ping), tests would naturally fail with 404.

So to me this looks like the fix with d29eb79 was a bit weird and introduced that bug in #2576. 🤔

But I could also be totally wrong with my understanding here.
And I'm therefore open to any corrections and learnings.

PS: I'm looking for a new adventure in case anybody is looking to hire or work with a Ruby/Rails/Crystal dev
PPS: would you be so kind and add the hacktoberfest-accepted label to this issue in case you find that PR helpful? 🥺

[dblock]

@alexanderadam Thanks! Fix CI/CHANGELOG and I'll take a close look - @ericproulx wdyt?

[alexanderadam]

@alexanderadam Thanks! Fix CI/CHANGELOG

done 🙂

[dblock]

This looks correct to me. Let's ask @bogdan and @myxoh to also look at this as they authored the linked issues/PRs.

[bogdan]

That's fine to me if it fixes the issue. The test looks perfectly right.

[ericproulx]

Looks good to me 👍 thank you

[ericproulx]

@alexanderadam could you mount your api multiple times and try with a configuration.

[dblock]

@alexanderadam see my comment on breaking up specs, too, please.

[alexanderadam]

@alexanderadam could you mount your api multiple times and try with a configuration.

I don't have a production API (the ticket creator is @bogdan — and I also think it would be good if @bogdan could check whether this fixes it 😉 ) but I created a test script doing the two variants where we always send call to base_instance and the other one where we also send it to mounted_instances.first:

require 'grape'
require 'rack/mock'

module BuggyBehavior
  def call(env)
    (never_mounted? ? base_instance : mounted_instances.first).call(env)
  end
end

Grape::API.singleton_class.prepend(BuggyBehavior)

api1 = Class.new(Grape::API) do
  format :json
  version 'v1', using: :path
  resource(:system) { get(:ping) { { message: 'pong' } } }
end

parent1 = Class.new(Grape::API) { namespace('/api') { mount api1 } }
parent2 = Class.new(Grape::API) { mount api1 }

status_via_parent1, * = parent1.call(Rack::MockRequest.env_for('/v1/api/system/ping', method: 'GET'))
status_via_parent2, * = parent2.call(Rack::MockRequest.env_for('/v1/system/ping', method: 'GET'))
status_direct, * = api1.call(Rack::MockRequest.env_for('/v1/system/ping', method: 'GET'))

puts "Without fix via parent1 => /v1/api/system/ping => #{status_via_parent1} #{status_via_parent1 == 200 ? 'All good' : 'Sad server noises'}"
puts "Without fix via parent2 => /v1/system/ping => #{status_via_parent2} #{status_via_parent2 == 200 ? 'All good' : 'Sad server noises'}"
puts "Without fix directly => /v1/system/ping => #{status_direct} #{status_direct == 200 ? 'All good' : 'Sad server noises - polluted'}"

api2 = Class.new(Grape::API) do
  format :json
  version 'v1', using: :path
  resource(:system) { get(:ping) { { message: 'pong' } } }
end

def api2.call(env) = base_instance.call(env)

parent3 = Class.new(Grape::API) { namespace('/api') { mount api2 } }
parent4 = Class.new(Grape::API) { mount api2 }

status_via_parent3, * = parent3.call(Rack::MockRequest.env_for('/v1/api/system/ping', method: 'GET'))
status_via_parent4, * = parent4.call(Rack::MockRequest.env_for('/v1/system/ping', method: 'GET'))
status_direct2, * = api2.call(Rack::MockRequest.env_for('/v1/system/ping', method: 'GET'))

puts "With fix via parent1 => /v1/api/system/ping => #{status_via_parent3} #{status_via_parent3 == 200 ? 'All good' : 'Sad server noises'}"
puts "With fix via parent2 => /v1/system/ping => #{status_via_parent4} #{status_via_parent4 == 200 ? 'All good' : 'Sad server noises'}"
puts "With fix directly => /v1/system/ping => #{status_direct2} #{status_direct2 == 200 ? 'All good' : 'Sad server noises'}"

Its output looks like this:

Without fix via parent1 => /v1/api/system/ping => 200 All good
Without fix via parent2 => /v1/system/ping => 200 All good
Without fix directly => /v1/system/ping => 404 Sad server noises - polluted
With fix via parent1 => /v1/api/system/ping => 200 All good
With fix via parent2 => /v1/system/ping => 200 All good
With fix directly => /v1/system/ping => 200 All good

@alexanderadam see my comment on breaking up specs, too, please.

✔️ </done>

[bogdan]

I tried to test it with my project, but this patch branch is not compatible with grape-swagger:

ruby-grape/grape-swagger#965

[dblock]

@alexanderadam Do we capture all the scenarios from your example in tests? if not please add

@bogdan removal of namespace_stackable was unrelated, are we good merging this PR?

[bogdan]

@bogdan removal of namespace_stackable was unrelated, are we good merging this PR?

I can not test the issue fixed in my project. I guess I was asked to do it:

I also think it would be good if @bogdan could check whether this fixes it 😉

[alexanderadam]

I can not test the issue fixed in my project. I guess I was asked to do it:

Yes, it would have made me feel better.
But I guess it doesn't matter. 😉

@alexanderadam Do we capture all the scenarios from your example in tests? if not please add

I don't know. Do you have something particular in mind?
Or does anyone else have something in mind?

[dblock]

I'm merging this, thanks everyone. Looks like someone will followup on ruby-grape/grape-swagger#965.