ActionController::RoutingError with Rails 6 Engines

[tleish]

Bug Report

Describe the bug

When using StimulusReflex with Rails 6.0.3.4 from a Rails engine, I get the error on a StimulusReflex::Channel#receive:

StimulusReflex::Channel Failed to re-render http://localhost:3000/orders/pages 
No route matches "http://localhost:3000/orders/pages" 
/action_dispatch/routing/route_set.rb:878:in `recognize_path_with_request'

To Reproduce

1. Create a rails application with StimulusReflex
2. Create a Rails engine
3. Add the code described in StimulusReflex Quickstart to the engine
4. Click on the link "Increment: 0" link and you will get the error

Expected behavior

I expect demo to work with an engine and no error.

Source of the Error

In digging into the code, the problem appears to come from stimulus_reflex/reflex.rb

StimulusReflex::Reflex#url_params calls StimulusReflex::Reflex#request to get the request object, and then passes the request object into Rails.application.routes.recognize_path_with_request. The problem is the way Rails mutates request.env.

For example, the following code will fail with an engine in StimulusReflex::Reflex#request

uri = URI.parse(url)
path = ActionDispatch::Journey::Router::Utils.normalize_path(uri.path)
      query_hash = Rack::Utils.parse_nested_query(uri.query)
      req = ActionDispatch::Request.new(
        connection.env.merge(
          Rack::MockRequest.env_for(uri.to_s).merge(
            "rack.request.query_hash" => query_hash,
            "rack.request.query_string" => uri.query,
            "ORIGINAL_SCRIPT_NAME" => "",
            "ORIGINAL_FULLPATH" => path,
            Rack::SCRIPT_NAME => "",
            Rack::PATH_INFO => path,
            Rack::REQUEST_PATH => path,
            Rack::QUERY_STRING => uri.query
          )
        )
      )
Rails.application.routes.recognize_path_with_request(req, url, req.env[:extras] || {})
Rails.application.routes.recognize_path_with_request(req, url, req.env[:extras] || {})

The first Rails.application.routes.recognize_path_with_request works, but the second call throws the ActionController::RoutingError. The #recognize_path_with_request mutates req.env.

Before:

req.env = {
  #...  
  "SCRIPT_NAME" => "",
  "PATH_INFO" => "/orders/pages",
  "action_dispatch.request.path_parameters" => {},
}

After:

req.env = {
  #...  
  "SCRIPT_NAME" => "/orders",
  "PATH_INFO" => "/pages",
  "action_dispatch.request.path_parameters" => {
    controller: "my_engine/orders/pages",
        action: "index"
   },
}

If I update the above scratch code to req.clone to avoid mutation of the original req object, it works (but causes other issues in StimulusReflex)

#...
Rails.application.routes.recognize_path_with_request(req.clone, url, req.env[:extras] || {})
Rails.application.routes.recognize_path_with_request(req.clone, url, req.env[:extras] || {})

So, the mutation of the req.env object causes something in Rails.application.routes.recognize_path_with_request to throw a `ActionController::RoutingError.

Possible Solution

With the existing code in #request

def request
    @request ||= begin
      ...
      path_params = Rails.application.routes.recognize_path_with_request(req, url, req.env[:extras] || {})
      req.env.merge(ActionDispatch::Http::Parameters::PARAMETERS_KEY => path_params)
      ...
    end
  end

Instead of calling Rails.application.routes.recognize_path_with_request twice (first in #request and then in #url_params), #url_params could re-use the output of Rails.application.routes.recognize_path_with_request captured in the #request method:

  def url_params
-    @url_params ||= Rails.application.routes.recognize_path_with_request(request, request.url, request.env[:extras] || {})
+    @url_params ||= request.env[ActionDispatch::Http::Parameters::PARAMETERS_KEY]
  end

This fixes my Rails engine test.

What gotchas am I not thinking about?

Versions

StimulusReflex

• Gem: 3.3.0
• Node package: Not Applicable

External tools

• Ruby: 2.7.1
• Rails: 6.0.3.4
• Node: Not Applicable

Browser

• Browser: any
• Version: Not Applicable

[leastbad]

This is a really fascinating issue, @tleish - thanks for bringing it to our attention.

We have definitely had lots of issues with request.env; I'm pretty sure that you're the first person to realize that calling recognize_path_with_request is mutating it. That explains a lot, TBH.

Still, the big ? floating over this issue is why it's failing for you and your app and this URL, while so many people are using page morphs without issue. In my view, this doesn't invalidate your experience so much as make me wonder what's different. Do you have to have an MVCE or Github repo we could poke at?

Either way, I'm really glad that you're on board to help us patch this up. I'll definitely be looking into this right away.

[leastbad]

Hey again... I've spent a fair bit of time looking into this and am at least re-familiarized with the parts of the codebase that this touches. That said, I personally have very little direct experience working with Rails engines, so that MVCE will be really important.

What is much clearer to me now than when I left my initial reply is that what you're showing would likely work if it wasn't happening in the context of an engine... leading to the now semi-obvious statement that our current approach doesn't play well with engines; this isn't something we've explicitly designed for (see above) but with your help, I'm confident that we'll come up with a viable solution.

When I run a page morph for my / route, with the Home controller and index action... my req.env looks like:

req.env = {
  #...  
  "SCRIPT_NAME" => "",
  "PATH_INFO" => "/",
  "action_dispatch.request.path_parameters" => {
    :controller=>"home",
    :action=>"index"
  }
}

... which is as expected. So my second question to you is... following the same format, what would you want req.env to look like? I'm trying to figure out if there's value in figuring out that we're working in an engine and handling the request slightly differently. Specifically, I'm wondering about the SCRIPT_NAME and the formatting of the controller name, whether you're nesting or in an engine.

As for whether your proposed patch has issues or not, I'm still looking into that. Presumably we did it that way for a reason, and I'm chasing down what that reason was.

[tleish]

Hi @leastbad,

Try this small app.

see: https://github.com/tleish/stimulus-reflex-within-rails-engine

To reproduce the error, install and run the app. The navigate to http://localhost:3000/orders/pages and click the "Increment" link. You should see the error in the log.

If you can uncomment the the monkeypatch in config/initializers/stimulus_reflex.rb then StimulusReflex works.

[tleish]

Looking PR https://github.com/hopsoft/stimulus_reflex/pull/172, @db0sch says the following:

The main thing I don't like in my code is calling Rails.application.routes.recognize_path_with_request once again, in addition to StimulusReflex::Reflex#url_params. But at this stage, I'm not sure how to avoid this call.

The PR was created in response to a devise issue reported in #173, which also dealt with routes. Perhaps it's because devise is also an engine and the way in which Rails.application.routes.recognize_path_with_request mutates the env?

[leastbad]

I'm just installing everything and noticing that you have the package.json set to use v3.2.3 and Gemfile is using v3.3.0.

Can you please verify that you have both libraries set to v3.3.0?

[tleish]

I updated package.json to match Gemfile and pushed to github, still same issue.

[leastbad]

Alright, I think I have a solution.

The only place in the app that Reflex#url_params is used is in page_broadcaster.rb - it's just used as a way to figure out the name of the current action. I thought that perhaps url_params was something that needed to exist for Rails or Devise, but that simply doesn't appear to be the case.

If we swap out line 6 of page_broadcaster.rb to be: reflex.controller.process reflex.params[:action] ... it seems as though we can drop the url_params method entirely.

Am I over-simplifying this? @hopsoft @julianrubisch @db0sch @marcoroth

[julianrubisch]

Incidentally, I was looking at this method lately in search of a solution for https://github.com/julianrubisch/futurism/pull/69

We have a similar problem - figuring out the current action - there too. @rickychilcott

I must say wrestling around with request.env is a nightmare.

TLDR: I don’t see a problem here.

[RolandStuder]

I had a look at it as well, indeed I see that recognize_path_with_request alters the req.env and actually that makes the line that manually puts the path_params unnecessary https://github.com/hopsoft/stimulus_reflex/blob/master/lib/stimulus_reflex/reflex.rb#L86

What a weird behavior, wouldn't happen if ruby only had pure functions ;-)

The fix looks fine to me.

[leastbad]

@tleish thanks for bringing this issue to our attention. The fix is now in the master branch, which will have to hold you over until v3.4.0.pre0 comes out soon.

[tleish]

@leastbad - upgrading to v3.4.0.pre0 requires cable_ready js package. Is cable_ready now a required dependency?

[leastbad]

Yes, sir! It is, in fact, the backbone of the entire operation.