Skip to content

Commit 82c7cd2

Browse files
ericproulxclaude
andauthored
Compare the base API, not to_s, when refreshing a mounted app (#2788)
`refresh_already_mounted` deduplicates endpoints for the "same" mounted app by comparing `endpoint.mounted_app.to_s == app.to_s`. That only works by accident: `mount` turns every mounted Grape API into a throwaway `mount_instance` (a fresh `Class.new(@base_parent)` per mount), so two mounts of the same API are distinct objects, and `Grape::API::Instance` delegates `to_s` to its `@base` (the original API). The comparison is really "same base API", done through a string proxy -- brittle (two APIs that happen to share a name collide) and unclear. Expose the base API via a reader and compare it by identity instead. Plain Rack apps have no base and are mounted as-is, so they fall back to object identity. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 1d94591 commit 82c7cd2

4 files changed

Lines changed: 28 additions & 1 deletion

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
* [#2792](https://github.com/ruby-grape/grape/pull/2792): Move `Grape::Path` to `Grape::Router::Pattern::Path` and add a `Grape::Router::Pattern.build` factory (`Grape::Path` kept as a deprecated constant) - [@ericproulx](https://github.com/ericproulx).
1818
* [#2785](https://github.com/ruby-grape/grape/pull/2785): Make route `params` a first-class endpoint input and split `Grape::Router::Route#params` into `#params` (declared definitions) and `#params_for(input)` (extracted values) - [@ericproulx](https://github.com/ericproulx).
1919
* [#2786](https://github.com/ruby-grape/grape/pull/2786): Make route `requirements` and `anchor` explicit keyword arguments and first-class endpoint inputs instead of opaque `route_options` keys - [@ericproulx](https://github.com/ericproulx).
20+
* [#2788](https://github.com/ruby-grape/grape/pull/2788): Compare the base API instead of `to_s` when refreshing a mounted app - [@ericproulx](https://github.com/ericproulx).
2021
* Your contribution here.
2122

2223
#### Fixes

lib/grape/api/instance.rb

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ class << self
2727

2828
def_delegators :@base, :to_s
2929

30+
attr_reader :base
31+
3032
def base=(grape_api)
3133
@base = grape_api
3234
grape_api.instances << self

lib/grape/dsl/routing.rb

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ def mount(mounts, *opts)
152152
refresh_already_mounted = opts.any? ? opts.first[:refresh_already_mounted] : false
153153
if refresh_already_mounted && !endpoints.empty?
154154
endpoints.delete_if do |endpoint|
155-
endpoint.mounted_app.to_s == app.to_s
155+
same_mounted_app?(endpoint.mounted_app, app)
156156
end
157157
end
158158

@@ -294,6 +294,18 @@ def refresh_mounted_api(mounts, *opts)
294294
mount(mounts, *opts)
295295
end
296296

297+
# Two mounts refer to the same app when they share the same base Grape
298+
# API. +mount+ turns every mounted Grape API into a throwaway
299+
# +mount_instance+ (a fresh +Class.new+ per mount), so object identity
300+
# never holds across mounts; comparing the base is the real signal.
301+
# Plain Rack apps have no base and are mounted as-is, so they fall back
302+
# to object identity.
303+
def same_mounted_app?(mounted, app)
304+
return mounted.base.equal?(app.base) if mounted.respond_to?(:base) && app.respond_to?(:base)
305+
306+
mounted.equal?(app)
307+
end
308+
297309
# Execute first the provided block, then each of the
298310
# block passed in. Allows for simple 'before' setups
299311
# of settings stack pushes.

spec/grape/api_spec.rb

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3665,6 +3665,18 @@ def self.call(object, _env)
36653665
expect { subject.mount app => '/thing' }
36663666
.to change { subject.endpoints.count }.by(1)
36673667
end
3668+
3669+
it 'does not confuse two distinct apps that share a name' do
3670+
other = Class.new(described_class) { get('/y') { 'y' } }
3671+
# Force a #to_s collision to prove the refresh keys off the base API
3672+
# identity rather than the string representation.
3673+
allow(app).to receive(:to_s).and_return('SameName')
3674+
allow(other).to receive(:to_s).and_return('SameName')
3675+
3676+
subject.mount app => '/thing'
3677+
expect { subject.mount({ other => '/thing' }, refresh_already_mounted: true) }
3678+
.to change { subject.endpoints.count }.by(1)
3679+
end
36683680
end
36693681

36703682
context 'mounting an API' do

0 commit comments

Comments
 (0)