Explicit loading of ActiveStorage routes

[justinko]

Fixes #31228

Motivation / Background

Users are creating a "catchall" route in routes.rb that will always precede the ActiveStorage routes. Railtie routes that are loaded after the app's routes are getting caught in it.

Detail

Moving the ActiveStorage routes from Rails.applications.routes to ActiveStorage::Engine.routes ensures they precede any route defined in Rails.application.routes.

If you a user wants explicit loading of the routes, they can do the following:

config.active_storage.draw_routes = false

# in routes.rb
mount ActiveStorage::Engine => "/my_storage"

Additional information

Should we do this for other engines as well?

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[zzak]

I've been thinking about this, and I'd rather extract it to a method and call it, than use the file path. Since that also makes it easier to test 🙏

[justinko]

@zzak I pushed a concept.

Were you thinking more along the lines of:

module ActiveStorage
  module Routes
    delf self.apply(router)
      router.instance_eval do
        # add the routes here
      end
    end
  end
end

# from routes.rb
ActiveStorage::Routes.apply(self) # or could have ActiveStorage.routes call this

[zzak]

You shouldn't need instance_eval, since you can do router.draw { scope ActiveStorage.routes_prefix { ... } }.

I've only tested this in very limited context though, so some diligence is required 🙏

[justinko]

Unfortunately that calls https://api.rubyonrails.org/classes/ActionDispatch/Routing/Mapper/Resources.html#method-i-draw

[justinko]

Another option is router.scope ActiveStorage.routes_prefix do

[zzak]

Can you also update the source_location for these tests?
https://github.com/rails/rails/blob/main/railties/test/commands/routes_test.rb#L341-L391

That should make the build green.

[zzak]

If it doesn't make a difference maybe router.scope <prefix> {} does look nicer

[zzak]

Can you squash your commits please?

[zzak]

Can you update the PR description to reflect the current state too, please?

[justinko]

Done. Thanks for your patience! What's left? Change log?

[zzak]

Yeah, I'm on the fence whether it should be documented at all.

Let's see what others think if we can get a review 🙏

[zzak]

Saw this suggested in the other ticket, but I wonder if we should instead follow these conventions:
https://edgeguides.rubyonrails.org/engines.html#generating-an-engine

Mainly:

ActiveStorage::Engine.routes.draw do
  # routes
end

[justinko]

Won't work because the routes use the direct method. You'll get this error:

The direct method can't be used inside a routes scope block

You could get around that error by not isolating the engine ... but that'll open a whole other can of worms.

[justinko]

Looks like I got it to work ... stay tuned.

[justinko]

What do you think of this approach? https://github.com/rails/rails/compare/main...justinko:mount-activestorage?expand=1

The routes then look like this:

Seems cleaner ... and I think direct route helpers such as rails_blob_url could be deprecated in favor of active_storage.blob_url ... but that's a different subject.

[zzak]

After a cursory review, but I think that might be a breaking change. Though possibly having them isolated like that is semantically better.

Just based on having to add mounted_helpers to the tests raised a flag but that might be unavoidable.

I also noticed routes_prefix and draw_routes are missing from your implementation, unless I missed something.

Last thing, for me, I don't want to put any logic inside config/routes.rb except maybe the if draw_routes. My reasoning is that I want to be able to call ActiveStorage.draw_routes (or whatever) anywhere I want and get access to the helpers without having to initialize an app and call Rails.application.routes.url_helpers. 🙏

[justinko]

Got some tests to fix.

[justinko]

This approach isn't going to work because there's a hard dependency on rails_direct_uploads_url: https://github.com/rails/rails/blob/main/actiontext/app/helpers/action_text/tag_helper.rb#L38

I'm going to go with a different approach. What do you think of something like:

# in routes.rb
Rails.application.routes.draw do
  scope "/rails/active_storage" do
   import ActiveStorage::Engine.routes, as: "rails"
  end
end

This would just be another way to integrate an engine's routes without mounting.

If that's not a good idea then I'm going to move on from this issue.

[zzak]

I assume you mean scope ActiveStorage.routes_prefix { }, but I'm not sure what import .. as: provides over your initial instance_eval implementation.

If we're trying to get the routes to show up isolated in the routes command, or apply other Engine-y conventions, I think we should be cautious about what it is we're looking for and what problem it's solving.

I appreciate you taking the time to investigate this, but if all we solve is the open ticket and move the routes into a proper method, then I am happy and will help get a review as I'm very interested in this.

Thank you!

[justinko]

Sorry, let me be more clear.

The core problem is route load order, which is caused by an engine using Rails.application.routes.draw. Engines do this because they want their routes to exist in the main_app (e.g. rails_blob_url). If the routes are drawn in the engine, then users are forced to use engine_name.foo_url.

It would be great for an engine to draw its routes within the engine, and let the user decide whether to import those routes into main app or mount them.

So a user that wants to have control of when Active Storage routes are loaded, would do this:

Rails.application.config.active_storage.draw_routes = false

Rails.application.routes.draw do
  import ActiveStorage::Engine.routes
end

I haven't looked into feasibility, but in theory it's just merging RouteSet's.

[justinko]

I spiked import and it's doable but not worth the complexity.

I just thought of another potential solution. Another spike commencing.

[justinko]

Okay, I really like this solution: justinko@180a8cf

No breaking changes and it allows mount ActiveStorage::Engine => "/foo"

All it does is create 2 separate sets of routes, one for Rails.application.routes.draw and the other under ActiveStorage::Engine.routes.draw.

If they choose to mount, then they'll need to add this to their controller:

def rails_direct_uploads_url
  active_storage.direct_uploads_url
end

If you prefer it, then I'll clean it up and get a pull ready.

[justinko]

If you prefer ActiveStorage::Routes.apply(self) then we can go back to that, but we won't be able to use ActiveStorage::Engine.routes.draw.

[justinko]

Starting over with a fresh ActiveStorage::Routes.apply(self)