Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid evaluating the same route twice. #1356

Closed
wants to merge 1 commit into from

Conversation

dblock
Copy link
Member

@dblock dblock commented Apr 7, 2016

The exact matching route may be evaluated multiple times if it yields a 406 or something like that. Maybe we can do this more elegantly?

@namusyaka
Copy link
Contributor

How about this patch?

diff --git lib/grape/router.rb lib/grape/router.rb
index bb6f830..98426ee 100644
--- lib/grape/router.rb
+++ lib/grape/router.rb
@@ -41,7 +41,8 @@ module Grape

     def call(env)
       with_optimization do
-        identity(env) || rotation(env) { |route| route.exec(env) }
+        response, route = identity(env)
+        response || rotation(env, route)
       end
     end

@@ -54,26 +55,28 @@ module Grape
     private

     def identity(env)
-      transaction(env) do |input, method, routing_args|
+      route = nil
+      response = transaction(env) do |input, method, routing_args|
         route = match?(input, method)
         if route
           env[Grape::Env::GRAPE_ROUTING_ARGS] = make_routing_args(routing_args, route, input)
           route.exec(env)
         end
       end
+      [response, route]
     end

-    def rotation(env)
-      transaction(env) do |input, method, routing_args|
-        response = nil
-        routes_for(method).each do |route|
-          next unless route.match?(input)
-          env[Grape::Env::GRAPE_ROUTING_ARGS] = make_routing_args(routing_args, route, input)
-          response = yield(route)
-          break unless cascade?(response)
-        end
-        response
+    def rotation(env, exact_route = nil)
+      response = nil
+      input, method, routing_args = *extract_required_args(env)
+      routes_for(method).each do |route|
+        next if exact_route == route
+        next unless route.match?(input)
+        env[Grape::Env::GRAPE_ROUTING_ARGS] = make_routing_args(routing_args, route, input)
+        response = route.exec(env)
+        break unless cascade?(response)
       end
+      response
     end

     def transaction(env)

I believe we should two search methods should be put separately.

@dblock
Copy link
Member Author

dblock commented Apr 11, 2016

That is totally fine too. Go ahead and replace my PR with that or just do it part of #1355.

@namusyaka namusyaka mentioned this pull request Apr 11, 2016
@dblock
Copy link
Member Author

dblock commented Apr 11, 2016

Closing in favor of #1359.

@dblock dblock closed this Apr 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants