From 2bbb1bd6234b79ca881769d4f3ee44d760283e25 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Thu, 10 Sep 2026 15:26:21 +0200 Subject: [PATCH] Look for a 405 neighbour only on paths the method has no route on When the routes for a request's method all miss its path, `#neighbours` walks `@union` -- one greedy route per path -- for a route to answer 405 with. A 404 therefore walks two unions: its method's, then every path. Most of that second walk cannot succeed. `collect_route_config_per_pattern` groups routes by `pattern_regexp` and hands each group's greedy route the pattern those routes share, so once the union for a method has missed a path, the greedy route of every path that method has a route on has missed it too. `compile!` now works out, per method, the greedy routes of the paths it has no route on and compiles those into a union of their own; after a miss, only that one is walked. In an API where every path answers GET, a GET leaves nothing to walk. The subset is built from the same members as `@union`, in the same order, so a path resolves to the same greedy route either way and the 405 and its Allow header are unchanged. A method with no routes of its own covers no path and still walks all of `@union` -- which is also what auto-OPTIONS does, unless OPTIONS routes are declared. Methods that leave the same paths uncovered, PUT and DELETE on a member say, share one union. And once a route has matched and cascaded the neighbour is never read, so it is no longer looked up. A subset of `@union` numbers its groups differently from `@union` itself, so each neighbour union carries the group of each of its routes rather than reading `regexp_capture_group`. Measured on Ruby 4.0.6 without YJIT, median of 5 interleaved rounds against an extract of master, on a CRUD-shaped API: GET and POST on each collection, GET, PUT and DELETE on each member, varied resource names, prefix + path version + `format :json`. At 10 / 100 / 250 resources: GET 404 +74.6% +97.9% +93.5% POST 404 +24.3% +40.4% +31.3% POST 405 +5.8% +27.7% +21.3% POST gains less because it has no route on the member paths, which therefore stay candidates. A route hit from the middle of the union and OPTIONS, both unchanged paths, stay within +/-5%. Boot, full API instance build: +5.1%, +8.5%, +5.8%, and +0.9% at 500 resources. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 1 + lib/grape/router.rb | 58 ++++++++++++++++++++++++++++-- spec/grape/router_spec.rb | 74 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d2c1a6870..2fdf54ae1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ * [#2915](https://github.com/ruby-grape/grape/pull/2915): Update rubocop to 1.90.0 and rubocop-performance to 1.27.0 - [@ericproulx](https://github.com/ericproulx). * [#2918](https://github.com/ruby-grape/grape/pull/2918): Skip the dry-types round trip when a value already is the declared type - [@ericproulx](https://github.com/ericproulx). * [#2917](https://github.com/ruby-grape/grape/pull/2917): Read path captures out of the router's union match instead of re-running the route's pattern - [@ericproulx](https://github.com/ericproulx). +* [#2920](https://github.com/ruby-grape/grape/pull/2920): Look for a 405 neighbour only on paths the request's method has no route on - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 4.0.0 (2026-09-07) diff --git a/lib/grape/router.rb b/lib/grape/router.rb index a83cd2023..8b04b7948 100644 --- a/lib/grape/router.rb +++ b/lib/grape/router.rb @@ -6,17 +6,19 @@ def initialize @neutral_map = [] @neutral_regexes = [] # Plain hashes with no auto-vivifying default: a lookup for an HTTP method - # that has no routes must not insert a key. `compile!` freezes both maps, + # that has no routes must not insert a key. `compile!` freezes the maps, # so request-time reads never mutate shared state (see #match? / #rotation). @map = {} @optimized_map = {} + # Per HTTP method, the greedy routes still worth trying once that method's + # own routes have missed a path (see #compile_neighbour_map). + @neighbour_map = {} end def compile! return if @compiled @union = resolve_capture_groups(Regexp.union(@neutral_regexes), @neutral_map) - @neutral_regexes = nil # Compiled from the routes actually registered rather than from # Grape::HTTP_SUPPORTED_METHODS. A route declared with any other verb # (`route :purge, '/cache'`) is accepted at definition time and is @@ -28,8 +30,11 @@ def compile! optimized_map = routes.map.with_index { |route, index| route.to_regexp(index) } @optimized_map[method] = resolve_capture_groups(Regexp.union(optimized_map), routes) end + compile_neighbour_map + @neutral_regexes = nil @map.freeze @optimized_map.freeze + @neighbour_map.freeze @compiled = true end @@ -104,7 +109,9 @@ def transaction(input, method, env) # +method+ have declined. +cascaded+ says whether any of them matched: the # auto-OPTIONS and 405 answers are only right when none did. def neighbours(input, method, env, response, cascaded) - last_neighbor_route = greedy_match?(input) + # Only ever read while nothing for +method+ has matched, so once a route + # has -- and cascaded -- there is no neighbour to look for. + last_neighbor_route = neighbour_match?(input, method) unless cascaded # If last_neighbor_route exists and request method is OPTIONS, # return response by using #include_allow_header. @@ -207,6 +214,51 @@ def greedy_match?(input) @union.match(input) { |m| @neutral_map.detect { |route| m[route.regexp_capture_group] } } end + # The greedy route for +input+ once the routes for +method+ have all missed + # it, from the ones #compile_neighbour_map left to try. A method with no + # routes of its own covers no path, so it still walks the whole of @union. + def neighbour_match?(input, method) + return greedy_match?(input) unless @map.key?(method) + + union, groups = @neighbour_map[method] + union&.match(input) { |m| groups.find { |_route, group| m[group] }&.first } + end + + # After a miss, #neighbours looks for a greedy route to answer 405 with. + # Walking the whole of @union for it would cost a 404 a second walk over + # every path, and most of that walk cannot succeed: a greedy route shares + # its pattern with the routes it was collected from (see + # API::Instance#collect_route_config_per_pattern), so once the union for + # +method+ has missed a path, the greedy route of every path +method+ has a + # route on has missed it too. What is left to try is the paths +method+ has + # no route on -- in an API where every path answers GET, none at all for a + # GET. + # + # Built from the same members as @union, in the same order, so a path + # resolves to the same greedy route either way. Methods that leave the same + # paths uncovered -- PUT and DELETE on a member, say -- share one union. + def compile_neighbour_map + unions = {} + @map.each do |method, routes| + covered = routes.to_set(&:pattern_regexp) + uncovered = @neutral_map.each_index.reject { |index| covered.include?(@neutral_map[index].pattern_regexp) } + @neighbour_map[method] = unions[uncovered] ||= neighbour_union(uncovered) unless uncovered.empty? + end + end + + # The union of the greedy routes at +indices+ of @neutral_map, and the + # group each of them occupies in it: a subset of @union numbers its groups + # differently, so the ones #resolve_capture_groups recorded do not apply. + def neighbour_union(indices) + union = Regexp.union(@neutral_regexes.values_at(*indices)) + named_captures = union.named_captures + groups = indices.to_h do |index| + route = @neutral_map[index] + [route, named_captures.fetch(route.regexp_capture_index).first] + end + [union, groups.freeze].freeze + end + def cascade?(response) response && response[1]['X-Cascade'] == 'pass' end diff --git a/spec/grape/router_spec.rb b/spec/grape/router_spec.rb index 8352f841a..98d65d1fc 100644 --- a/spec/grape/router_spec.rb +++ b/spec/grape/router_spec.rb @@ -179,4 +179,78 @@ def response_body expect(body['params']).to eq('name' => '123') end end + + # When no route for the request's method matches its path, the router answers + # 405 if a route for another method does, and 404 otherwise. It only tries the + # paths the method has no route on, since the others share a pattern with a + # route that has just missed -- so these cover a path with no route for the + # method, a covered path whose pattern overlaps an uncovered one, and methods + # with no routes at all, for which every path is still a candidate. + describe 'answering a path no route for the method matches' do + let(:app) do + Class.new(Grape::API) do + format :json + get('/items') { 'index' } + post('/items') { 'create' } + get('/items/:id') { 'show' } + put('/items/:id') { 'update' } + delete('/items/:id') { 'destroy' } + # Digits only, so /users/me misses it and reaches the POST-only path. + get('/users/:id', requirements: { id: /\d+/ }) { 'user' } + post('/users/me') { 'me' } + end + end + + it 'answers 405 from a path the method has no route on' do + post '/items/1' + + expect(last_response.status).to eq(405) + expect(last_response.headers['Allow']).to eq('OPTIONS, GET, PUT, DELETE, HEAD') + end + + it 'answers 405 from an uncovered path when a covered one overlaps it' do + get '/users/me' + + expect(last_response.status).to eq(405) + expect(last_response.headers['Allow']).to eq('OPTIONS, POST') + end + + it 'answers 405 to a method that has no routes at all' do + patch '/items/1' + + expect(last_response.status).to eq(405) + expect(last_response.headers['Allow']).to eq('OPTIONS, GET, PUT, DELETE, HEAD') + end + + it 'answers OPTIONS for a path' do + options '/items/1' + + expect(last_response.status).to eq(204) + expect(last_response.headers['Allow']).to eq('OPTIONS, GET, PUT, DELETE, HEAD') + end + + it 'answers 404 to a path no route matches' do + get '/nothing/here' + expect(last_response.status).to eq(404) + + post '/nothing/here' + expect(last_response.status).to eq(404) + end + + context 'when every path has a route for the method' do + let(:app) do + Class.new(Grape::API) do + get('/items') { 'index' } + get('/items/:id') { 'show' } + delete('/items/:id') { 'destroy' } + end + end + + it 'answers 404 to a path no route matches' do + get '/nothing/here' + + expect(last_response.status).to eq(404) + end + end + end end