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

Fix regression introduced by fixing mounting the same engine in multiple locations #29898

Conversation

deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Jul 23, 2017

Summary

I introduced this regression in #29662. The previous version would prefer the current script_name if one is available in the request for resolving the mount context in engine route helpers. But that would make them incorrect in some cases.

For example (just describing the test I added), if an engine is mounted at

resources :fruits do
  mount Bukkits::Engine => "/bukkits"
end

and the current request path is /fruits/1/bukkits/posts, calling fruit_bukkits.posts_path(fruit_id: 2) would incorrectly return /fruits/1/bukkits/posts. This is because the script_name present in the request (in this case /fruits/1/bukkits) was preferred (by using reverse_merge! instead of merge! here).

This patch takes care of fixing that. My initial attempt was changing reverse_merge! with merge! but although that fixed the specific issue I was having, it broke some other tests. The problem is that the previous script name might contain relevant information that is not known to the lambda that resolves the script name from the parameters and the mount point location. For example, any context provided to the specific request via the environment's SCRIPT_NAME variable is not known to this method.

So instead of preferring the script_namer resolution, I merged that with the current script name. So if the previous script name was /foo/fruits/1/bukkits, and the resulting one from calling the lambda is /fruits/2/bukkits, the resulting script name used for the route helper will be /foo/fruits/2/bukkits.

Other Information

Sorry for the probably not very well explained PR description. I'm not very familliar with rails internals, so it's hard for me to reason about them.

Also, excuse the irrelevant commits. I added them while investigating the problem, but then I found out about the imminent release, so I wanted to open this ASAP to avoid introducing the regression.

EDIT: I tried to improve a bit the description of the problem & the solution.

@rails-bot
Copy link

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

@deivid-rodriguez deivid-rodriguez force-pushed the follow_up_to_multiple_location_engine_mounting branch from c3c6b4a to 6177a7c Compare July 23, 2017 21:07
@deivid-rodriguez
Copy link
Contributor Author

I removed the irrelevant commits.

@deivid-rodriguez deivid-rodriguez force-pushed the follow_up_to_multiple_location_engine_mounting branch from 6177a7c to 2bd60c8 Compare July 24, 2017 12:28
@rafaelfranca rafaelfranca merged commit bd0d1e2 into rails:master Jul 24, 2017
rafaelfranca added a commit that referenced this pull request Jul 24, 2017
…_location_engine_mounting

Fix regression introduced by fixing mounting the same engine in multiple locations
rafaelfranca added a commit that referenced this pull request Jul 24, 2017
…_location_engine_mounting

Fix regression introduced by fixing mounting the same engine in multiple locations
@deivid-rodriguez deivid-rodriguez deleted the follow_up_to_multiple_location_engine_mounting branch July 24, 2017 21:41
@moiristo
Copy link

moiristo commented Oct 27, 2020

@deivid-rodriguez I'm currently experiencing an issue with https://github.com/rails/rails/pull/29898/files#diff-8c7c1c75ea2a8b62a683d163898cd0b75c000165a1e96523d5197b91fbed88a1R57 when running tests under rails 6.0.3.4.

For some reason I don't know yet, previous_script_name is an empty string in my case when I fetch an engine route name twice (second time fails). When this is passed into this method, an error will be raised:

NoMethodError: undefined method `join' for nil:NilClass
actionpack (6.0.3.4) lib/action_dispatch/routing/routes_proxy.rb:65:in `merge_script_names'`

Obviously, changing the line to return new_script_name if previous_script_name.blank? fixes the issue. It also works again when I pass script_name: nil in the url_options. But I don't know if previous_script_name being an empty string is a possibility that should occur at all. It is passed via url_options somehow.

Below are the details of how I've setup my app, maybe it may help. As you can see, I have an optional scope (/:identifier/:owner_type/:owner_identifier). When this is not specified, the error does not occur.

# routes.rb

scope '/app_store' do
  mount Cat::Engine => "/cat"
end
# engine routes

Cat::Engine.routes.draw do
  scope module: 'cat' do
    scope '/:direct_access_token/:locale' do
      scope '(/:identifier/:owner_type/:owner_identifier)' do
        resources :cards, as: 'cat_cards'
      end
    end
  end
end
# test
class Cat::CardsControllerTest < ActionDispatch::IntegrationTest
  let(:path_params) { { direct_access_token: direct_access_token.to_param, locale: 'en', identifier: '1', owner_type: 'reservation', owner_identifier: '2' } }
  
  it 'outputs engine paths' do
    cat_engine.cat_cards_path(path_params)
    cat_engine.cat_cards_path(path_params) # error
  end
end

@deivid-rodriguez
Copy link
Contributor Author

Hi @moiristo, it's been a while since this patch and I don't recall the details but it sounds like I might've missed an edge case related to optional scopes? I guess you can try run the railties tests with your fix and see if something breaks.

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.

None yet

5 participants