From c532d648eea16a08e4e7aeab2ef4bfe8a8985962 Mon Sep 17 00:00:00 2001 From: Joakim Antman Date: Fri, 26 Jun 2020 23:49:01 +0300 Subject: [PATCH] Populate the env route information when calling the generated allow_headers endpoints --- CHANGELOG.md | 1 + lib/grape/api/instance.rb | 48 +++++++++++++---------------- lib/grape/router.rb | 35 +++++++++++---------- spec/grape/api_spec.rb | 65 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa43c2cb80..c50d10928d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ * [#2067](https://github.com/ruby-grape/grape/pull/2067): Coerce empty string to nil for all primitive types except String - [@petekinnecom](https://github.com/petekinnecom). * [#2064](https://github.com/ruby-grape/grape/pull/2064): Fix Ruby 2.7 deprecation warning in `Grape::Middleware::Base#initialize` - [@skarger](https://github.com/skarger). * [#2072](https://github.com/ruby-grape/grape/pull/2072): Fix `Grape.eager_load!` and `compile!` - [@stanhu](https://github.com/stanhu). +* [#2076](https://github.com/ruby-grape/grape/pull/2076): Make route information available for hooks when the automatically generated endpoints are invoked - [@anakinj](https://github.com/anakinj). * Your contribution here. ### 1.3.3 (2020/05/23) diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index 922ce58bc6..e8f8d85fb3 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -192,37 +192,15 @@ def cascade? # will return an HTTP 405 response for any HTTP method that the resource # cannot handle. def add_head_not_allowed_methods_and_options_methods - routes_map = {} - - self.class.endpoints.each do |endpoint| - routes = endpoint.routes - routes.each do |route| - # using the :any shorthand produces [nil] for route methods, substitute all manually - route_key = route.pattern.to_regexp - routes_map[route_key] ||= {} - route_settings = routes_map[route_key] - route_settings[:pattern] = route.pattern - route_settings[:requirements] = route.requirements - route_settings[:path] = route.origin - route_settings[:methods] ||= [] - if route.request_method == '*' || route_settings[:methods].include?('*') - route_settings[:methods] = Grape::Http::Headers::SUPPORTED_METHODS - else - route_settings[:methods] << route.request_method - end - route_settings[:endpoint] = route.app - end - end - + versioned_route_configs = collect_route_config_per_pattern # The paths we collected are prepared (cf. Path#prepare), so they # contain already versioning information when using path versioning. # Disable versioning so adding a route won't prepend versioning # informations again. without_root_prefix do without_versioning do - routes_map.each_value do |config| - methods = config[:methods] - allowed_methods = methods.dup + versioned_route_configs.each do |config| + allowed_methods = config[:methods].dup unless self.class.namespace_inheritable(:do_not_route_head) allowed_methods |= [Grape::Http::Headers::HEAD] if allowed_methods.include?(Grape::Http::Headers::GET) @@ -241,6 +219,25 @@ def add_head_not_allowed_methods_and_options_methods end end + def collect_route_config_per_pattern + all_routes = self.class.endpoints.map(&:routes).flatten + routes_by_regexp = all_routes.group_by { |route| route.pattern.to_regexp } + + # Build the configuration based on the first endpoint and the collection of methods supported. + routes_by_regexp.values.map do |routes| + last_route = routes.last # Most of the configuration is taken from the last endpoint + matching_wildchar = routes.any? { |route| route.request_method == '*' } + { + options: {}, + pattern: last_route.pattern, + requirements: last_route.requirements, + path: last_route.origin, + endpoint: last_route.app, + methods: matching_wildchar ? Grape::Http::Headers::SUPPORTED_METHODS : routes.map(&:request_method) + } + end + end + # Generate a route that returns an HTTP 405 response for a user defined # path on methods not specified def generate_not_allowed_method(pattern, allowed_methods: [], **attributes) @@ -252,7 +249,6 @@ def generate_not_allowed_method(pattern, allowed_methods: [], **attributes) end not_allowed_methods = supported_methods - allowed_methods return if not_allowed_methods.empty? - @router.associate_routes(pattern, not_allowed_methods: not_allowed_methods, **attributes) end diff --git a/lib/grape/router.rb b/lib/grape/router.rb index 26c7f1a0d8..f2b8cf2416 100644 --- a/lib/grape/router.rb +++ b/lib/grape/router.rb @@ -99,37 +99,34 @@ def transaction(env) response = yield(input, method) return response if response && !(cascade = cascade?(response)) - neighbor = greedy_match?(input) + last_neighbor_route = greedy_match?(input) - # If neighbor exists and request method is OPTIONS, + # If last_neighbor_route exists and request method is OPTIONS, # return response by using #call_with_allow_headers. - return call_with_allow_headers( - env, - neighbor.allow_header, - neighbor.endpoint - ) if neighbor && method == Grape::Http::Headers::OPTIONS && !cascade + return call_with_allow_headers(env, last_neighbor_route) if last_neighbor_route && method == Grape::Http::Headers::OPTIONS && !cascade route = match?(input, '*') - return neighbor.endpoint.call(env) if neighbor && cascade && route + + return last_neighbor_route.endpoint.call(env) if last_neighbor_route && cascade && route if route response = process_route(route, env) return response if response && !(cascade = cascade?(response)) end - !cascade && neighbor ? call_with_allow_headers(env, neighbor.allow_header, neighbor.endpoint) : nil + return call_with_allow_headers(env, last_neighbor_route) if !cascade && last_neighbor_route + + nil end def process_route(route, env) - input, = *extract_input_and_method(env) - routing_args = env[Grape::Env::GRAPE_ROUTING_ARGS] - env[Grape::Env::GRAPE_ROUTING_ARGS] = make_routing_args(routing_args, route, input) + prepare_env_from_route(env, route) route.exec(env) end def make_routing_args(default_args, route, input) args = default_args || { route_info: route } - args.merge(route.params(input)) + args.merge(route.params(input) || {}) end def extract_input_and_method(env) @@ -160,9 +157,15 @@ def greedy_match?(input) @neutral_map.detect { |route| last_match["_#{route.index}"] } end - def call_with_allow_headers(env, methods, endpoint) - env[Grape::Env::GRAPE_ALLOWED_METHODS] = methods.join(', ').freeze - endpoint.call(env) + def call_with_allow_headers(env, route) + prepare_env_from_route(env, route) + env[Grape::Env::GRAPE_ALLOWED_METHODS] = route.allow_header.join(', ').freeze + route.endpoint.call(env) + end + + def prepare_env_from_route(env, route) + input, = *extract_input_and_method(env) + env[Grape::Env::GRAPE_ROUTING_ARGS] = make_routing_args(env[Grape::Env::GRAPE_ROUTING_ARGS], route, input) end def cascade?(response) diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index 43abeffbe6..8b3af2710f 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -816,6 +816,71 @@ class DummyFormatClass end end + describe 'when hook behaviour is controlled by attributes on the route ' do + before do + subject.before do + error!('Access Denied', 401) unless route.options[:secret] == params[:secret] + end + + subject.namespace 'example' do + before do + error!('Access Denied', 401) unless route.options[:namespace_secret] == params[:namespace_secret] + end + + desc 'it gets with secret', secret: 'password' + get { status(params[:id] == '504' ? 200 : 404) } + + desc 'it post with secret', secret: 'password', namespace_secret: 'namespace_password' + post {} + end + end + + context 'when HTTP method is not defined' do + let(:response) { delete('/example') } + + it 'responds with a 405 status' do + expect(response.status).to eql 405 + end + end + + context 'when HTTP method is defined with attribute' do + let(:response) { post('/example?secret=incorrect_password') } + it 'responds with the defined error in the before hook' do + expect(response.status).to eql 401 + end + end + + context 'when HTTP method is defined and the underlying before hook expectation is not met' do + let(:response) { post('/example?secret=password&namespace_secret=wrong_namespace_password') } + it 'ends up in the endpoint' do + expect(response.status).to eql 401 + end + end + + context 'when HTTP method is defined and everything is like the before hooks expect' do + let(:response) { post('/example?secret=password&namespace_secret=namespace_password') } + it 'ends up in the endpoint' do + expect(response.status).to eql 201 + end + end + + context 'when HEAD is called for the defined GET' do + let(:response) { head('/example?id=504') } + + it 'responds with 401 because before expectations in before hooks are not met' do + expect(response.status).to eql 401 + end + end + + context 'when HEAD is called for the defined GET' do + let(:response) { head('/example?id=504&secret=password') } + + it 'responds with 200 because before hooks are not called' do + expect(response.status).to eql 200 + end + end + end + context 'allows HEAD on a GET request that' do before do subject.get 'example' do