Recompile the router when a route is declared after compilation - #2848
Recompile the router when a route is declared after compilation#2848ericproulx wants to merge 1 commit into
Conversation
The router is built once, when the API is first compiled, and #compile!
memoizes the instance holding it. A route declared after that point was
appended to the endpoint list -- .routes reported it -- but never reached the
router, so requests for it answered 404 and recognize_path returned nil. The
API's own metadata disagreed with what it served.
Whether it bit depended on what had touched the API first:
get('/one'); get('/two') # /two served
api.routes; get('/two') # /two served
Rack::Test.new(api).get('/one'); get('/two') # /two -> 404
api.recognize_path('/one'); get('/two') # /two -> 404
`helpers` and `mount` already call #change! for exactly this reason -- route
declaration was the one mutating DSL call that did not.
The dummy host in routing_spec gains #change!, which Grape::DSL::Routing
already required of it: #mount calls it too, just from a branch that dummy
never reaches.
No boot cost: #change! is an ivar assignment, and compilation is lazy, so
defining 2000 routes takes the same 1.24s either way.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ceb2cc1 to
e619622
Compare
Danger ReportNo issues found. |
|
Closing: the scenario this fixes is not one Grape supports. Checking which shapes actually break on master:
Every ordinary class-body arrangement already works. The only two that break require the API to have been compiled first — that is, mutating a live, already-serving API, which is undocumented and not thread-safe ( That also matches what So this fixed only an unsupported case, and charged O(n²) for it: each declaration discarded the compiled router, so interleaving declaration with serving went 1.56 → 8.79 ms/route between n=25 and n=200. If the Dropped from #2850 as well. |
Summary
The router is built once, when the API is first compiled, and
#compile!memoizes the instance holding it. A route declared after that point was appended to the endpoint list —.routesreported it — but never reached the router, so requests for it answered 404 andrecognize_pathreturned nil. The API's own metadata disagreed with what it served.Whether it bit depended on what had touched the API first:
get('/one')…get('/two')api.routesthenget('/two')get('/two')api.recognize_path('/one'), thenget('/two')get('/two'), then explicitapi.change!In every case
.routesalready listed/two.Approach
Call
#change!when a route is added.helpersandmountalready do this for exactly the same reason — route declaration was the one mutating DSL call that did not, which is why an explicitapi.change!was the existing workaround.The dummy host in
routing_spec.rbgains#change!.Grape::DSL::Routingalready required it of its host:#mountcalls it bare too, just from a branch that dummy never reaches.Perf
The normal shape — declare every route, then serve — is unaffected.
#change!is a single ivar assignment and compilation is lazy, so the calls made while a class body is evaluated cost nothing:Serving 200 requests against a fully-declared API takes 0.075s, unchanged.
Interleaving declaration with serving now costs O(n²), because each declaration discards the compiled router and the next request rebuilds every endpoint so far:
Worth stating plainly rather than burying: on master the same loop takes 0.014s — but only because it is not doing the work. Every one of those requests answers 404; that is the bug. The cost is the price of the routes actually being served, and it is paid only by a pattern that previously did not function.
If that pattern matters enough, the follow-up is an incremental router append rather than a full rebuild on invalidation — a substantially larger change to
Grape::Router, deliberately not attempted here.Backward compatibility
No UPGRADING entry. A route that previously 404'd now answers; nothing could have relied on a declared route being unreachable. APIs defined entirely before first use — effectively all of them — are unaffected, since the route table is identical by the time anything compiles.
Longstanding, not a regression: identical in 3.3.4.
Test plan
api_spec.rbcovering a route added after a request and afterrecognize_path, its visibility torecognize_path, and a guard that earlier routes keep serving; verified 3 fail without thelib/change.🤖 Generated with Claude Code