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

Rails Routing Contraints with 9.1.3.0 #4124

Closed
creddy opened this Issue Aug 31, 2016 · 7 comments

Comments

Projects
None yet
2 participants
@creddy

creddy commented Aug 31, 2016

Environment

  • jruby 9.1.3.0 (2.3.1) 2016-08-29 a2a3b29 Java HotSpot(TM) 64-Bit Server VM 25.74-b02 on 1.8.0_74-b02 [darwin-x86_64
  • Darwin LAXM00618506A 15.6.0 Darwin Kernel Version 15.6.0: Thu Jun 23 18:25:34 PDT 2016; root:xnu-3248.60.10~1/RELEASE_X86_64 x86_64

Expected Behavior

When a rails app has routing contraints such as:

  controller "home" do
    get "/home" => "home#list", constraints: -> (_request) { false }
    get "/home" => "home#index"
  end

A request to "/home" does not match the constraints for the #list endpoint therefore it passes through to the #index endpoint when under Ruby 2.3.1 and JRuby 9.1.2.0. When under JRuby 9.1.3.0 the same request fails with a routing error.

You can find the repro code here https://github.com/creddy/request_contraints

➜  routing_contraints [jruby-9.1.2.0] (master) jruby -v
jruby 9.1.2.0 (2.3.0) 2016-05-26 7357c8f Java HotSpot(TM) 64-Bit Server VM 25.74-b02 on 1.8.0_74-b02 [darwin-x86_64]
➜  routing_contraints [jruby-9.1.2.0] (master) rspec spec --format documentation

Home routing
  get /home
    returns successfully

Finished in 0.469 seconds (files took 2.64 seconds to load)
1 example, 0 failures
➜  routing_contraints [2.3.1] (master) ruby -v
ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-darwin15]
➜  routing_contraints [2.3.1] (master) rspec spec
.

Finished in 0.27264 seconds (files took 0.93337 seconds to load)
1 example, 0 failures

Actual Behavior

  • In JRuby 9.1.3.0 the constraint is not matched and a routing error is raised instead of falling through to the next match.
➜  routing_contraints [jruby-9.1.3.0] (master) jruby -v
jruby 9.1.3.0 (2.3.1) 2016-08-29 a2a3b29 Java HotSpot(TM) 64-Bit Server VM 25.74-b02 on 1.8.0_74-b02 [darwin-x86_64]
➜  routing_contraints [jruby-9.1.3.0] (master) rspec spec
F

Failures:

  1) Home routing get /home returns successfully
     Failure/Error: get "/home"

     ActionController::RoutingError:
       No route matches [GET] "/home"
     # ./spec/requests/home_spec.rb:6:in `block in (root)'

Finished in 0.411 seconds (files took 2.57 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/requests/home_spec.rb:5 # Home routing get /home returns successfully

Thank you

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 31, 2016

Member

Thank you for the repro! Looking into it now.

Member

headius commented Aug 31, 2016

Thank you for the repro! Looking into it now.

@headius headius added this to the JRuby 9.1.4.0 milestone Aug 31, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 31, 2016

Member

Well I can reproduce, so that's good. Removing the constrained route or moving it second allows /home to work (obviously). Changing the lambda to a proc has no effect.

Member

headius commented Aug 31, 2016

Well I can reproduce, so that's good. Removing the constrained route or moving it second allows /home to work (obviously). Changing the lambda to a proc has no effect.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 31, 2016

Member

After a bit of hunting I managed to see that it's basically registering the list-constrained route twice.

find_routes on 9.1.3.0:

/home
{:controller=>"home", :action=>"list"}
#<ActionDispatch::Journey::Route:0x5ad50b02>
/home
{:controller=>"home", :action=>"list"}
#<ActionDispatch::Journey::Route:0x5ad50b02>

find_routes on 9.1.2.0:

/home
{:controller=>"home", :action=>"list"}
#<ActionDispatch::Journey::Route:0x4c8abec7>
/home
{:controller=>"home", :action=>"index"}
#<ActionDispatch::Journey::Route:0x6b86826a>
Member

headius commented Aug 31, 2016

After a bit of hunting I managed to see that it's basically registering the list-constrained route twice.

find_routes on 9.1.3.0:

/home
{:controller=>"home", :action=>"list"}
#<ActionDispatch::Journey::Route:0x5ad50b02>
/home
{:controller=>"home", :action=>"list"}
#<ActionDispatch::Journey::Route:0x5ad50b02>

find_routes on 9.1.2.0:

/home
{:controller=>"home", :action=>"list"}
#<ActionDispatch::Journey::Route:0x4c8abec7>
/home
{:controller=>"home", :action=>"index"}
#<ActionDispatch::Journey::Route:0x6b86826a>
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 31, 2016

Member

And after a bit more hunting, we have our reduced repro:

$ jruby -e "x = [1,2]; x.sort_by!(&:to_s); p x"
[1, 1]

The sort_by! call we're interested in in Rails is in action_dispatch/journey/router.rb around line 107.

And the fix is equally trivial:

diff --git a/core/src/main/java/org/jruby/specialized/RubyArrayTwoObject.java b/core/src/main/java/org/jruby/specialized/RubyArrayTwoObject.java
index d38bb89..e24f6df 100644
--- a/core/src/main/java/org/jruby/specialized/RubyArrayTwoObject.java
+++ b/core/src/main/java/org/jruby/specialized/RubyArrayTwoObject.java
@@ -252,7 +252,7 @@ public class RubyArrayTwoObject extends RubyArraySpecialized {

         if (origArr.size() == 2) {
             car = origArr.eltInternal(0);
-            cdr = origArr.eltInternal(0);
+            cdr = origArr.eltInternal(1);
             return this;
         }

Sigh.

Member

headius commented Aug 31, 2016

And after a bit more hunting, we have our reduced repro:

$ jruby -e "x = [1,2]; x.sort_by!(&:to_s); p x"
[1, 1]

The sort_by! call we're interested in in Rails is in action_dispatch/journey/router.rb around line 107.

And the fix is equally trivial:

diff --git a/core/src/main/java/org/jruby/specialized/RubyArrayTwoObject.java b/core/src/main/java/org/jruby/specialized/RubyArrayTwoObject.java
index d38bb89..e24f6df 100644
--- a/core/src/main/java/org/jruby/specialized/RubyArrayTwoObject.java
+++ b/core/src/main/java/org/jruby/specialized/RubyArrayTwoObject.java
@@ -252,7 +252,7 @@ public class RubyArrayTwoObject extends RubyArraySpecialized {

         if (origArr.size() == 2) {
             car = origArr.eltInternal(0);
-            cdr = origArr.eltInternal(0);
+            cdr = origArr.eltInternal(1);
             return this;
         }

Sigh.

@headius headius closed this in 0c4f987 Aug 31, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 31, 2016

Member

I will be adding a flag to 9.1.4.0 to turn off packed arrays in case we have other issues, but we at least have this one fixed!

Member

headius commented Aug 31, 2016

I will be adding a flag to 9.1.4.0 to turn off packed arrays in case we have other issues, but we at least have this one fixed!

@creddy

This comment has been minimized.

Show comment
Hide comment
@creddy

creddy Aug 31, 2016

👍 Thank you for addressing this (and so quickly!)

creddy commented Aug 31, 2016

👍 Thank you for addressing this (and so quickly!)

headius added a commit that referenced this issue Aug 31, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 31, 2016

Member

@creddy Given the severity of the failure, we'll be pushing a 9.1.4.0 release tomorrow.

cc @enebo

Member

headius commented Aug 31, 2016

@creddy Given the severity of the failure, we'll be pushing a 9.1.4.0 release tomorrow.

cc @enebo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment