Regression in URL generation in Rails 7.1

[adrianthedev]

Hi,

I think I found a regression/bug in Rails 7.1

Context

We, at Avo use one main engine but also have a few smaller ones alongside it to bring new features.

The issue consists in the fact that URLs that are generated and displayed in the initial, parent engine (avo), for the second engine do not contain the mount path of the first one.

What happens is:

• avo engine gets mounted on /avo
• pro engine gets mounted in the avo engine on /pro
• the expectation is that any route we call on the pro engine should be generated with the /avo prefix

*That is the way it worked on 7.0

Steps to reproduce

https://gist.github.com/adrianthedev/ab57557950a05804532377b69185d866

The script should be executed with the RAILS_VERSON env variable like so:

# run with rails 7.0.8.3 (last 7.0 version) and see the tests pass
RAILS_VERSION=7.0.8.3 ruby repro.rb

# run with rails 7.1 and see the tests fail
RAILS_VERSION=7.1.0 ruby repro.rb

Expected behavior

The generated URL should contain the /avo prefix in both 7.0 and 7.1.

Actual behavior

The generated URL does not contain the /avo prefix in Rails 7.1.

System configuration

Rails version: 7.0 and 7.1

Ruby version: 3.3.1

[rafaelfranca]

I didn't investigate yet but I'm almost sure this was by design. Now that you have a reproduction script, can you use it to bisect and find which commit changed the behavior?

[adrianthedev]

I'll try that. I did try it with the gem versions, so I definitely know that it happened between 7.0.8.3 and 7.1.0.beta1.

[adrianthedev]

I didn't investigate yet but I'm almost sure this was by design.

I'm not sure that is the case here. It worked fine (and it made sense to work like that) in pre 7.1.

It doesn't really make sense to slash that first path.
The routes should be appended as the second engine is mounted within the first one.

[adrianthedev]

Also, could you point me to a PR or piece of documentation where I can see how to apply the "bisect and find which commit" strategy?

[rafaelfranca]

Sure, here is what I do:

First I download your script:

curl https://gist.githubusercontent.com/adrianthedev/ab57557950a05804532377b69185d866/raw/c5c83c6383caabc0473c6178aa82817509dbd1fd/url_regression_in_7_1.rb > failure.rb

Change it to:

diff --git a/failure.rb b/failure.rb
index c2be736dd8..96b8794268 100644
--- a/failure.rb
+++ b/failure.rb
@@ -1,14 +1,6 @@
 # frozen_string_literal: true
 
-require "bundler/inline"
-
-RAILS_VERSION = ENV["RAILS_VERSION"] || "7.0.1"
-
-gemfile(true, quiet: true) do
-  source "https://rubygems.org"
-
-  gem "rails", RAILS_VERSION
-end
+require "bundler/setup"
 
 require "action_controller"
+require "rails"

Now I start a bisect:

git bisect start

Run the script to make sure it is failing:

ruby failure.rb

Mark that commit as bad:

git bisect bad

Go to a commit I know it is passing:

git checkout v7.0.8.3

Run the script again to know it is passing:

ruby failure.rb

It is passing so I mark the commit as good:

git bisect good

Now I just go running the script on each step to see if it is passing or not, and mark the commit as good it is passing, or bad if it is not passing.

All of this of course inside the rails repo

[adrianthedev]

I think I found the commit, it's a393e0de09

But I can't see how this broke the previous behavior.
I'd love some help or guidance on this.

[adrianthedev]

Correction. It's 95fa021465. I'm not sure why it didn't trigger the bug earlier when I was going through the commit list.

I noticed that by undoing this change, everything seems to be fixed for my use case.
I don't have too much knowledge about the routing system and what implications this might have.

[rafaelfranca]

@bensheldon any idea why that would break the behavior on this issue?

[bensheldon]

That's weird and unexpected that nested engines would do that! I'm also tagging @jonathanhefner who helped me with my change.

I want to ask, on the gist, what happens if you remove the include Rails.application.routes.url_helpers from the controller? Including route helpers in a controller or view should not be necessary. As a potential bug, we should isolate which set of route helpers (controller/views, or the ones hanging off Rails) is affected (could be both too, but they're different because the former has access to Rack Env and the latter does not).

[adrianthedev]

Thanks for chiming in @bensheldon. I feel a bit lost because I don't know anything so deep about routing in Rails.

When I remove the include Rails.application.routes.url_helpers from the controller with Rails 7.1.0.alpha I get this error ActionView::Template::Error (undefined local variable or method avo_pro' for an instance of #Class:0x000000017dc700a0)`

I left it there because it was ther ein the first place in the reproduction script. I don't know if removing that from the script behaves the same as in a regular app.

[zzak]

#51933 gets the repro script to pass, but it's not ready to be merged