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