rails: Active Storage Disk Service NoMethodError: undefined method `rails_disk_service_url'

@stefannibrasil Sorry I went back to re-read this thread and gain some more context.

After reading your patch I think the disk_service_enabled? path might be more complicated that we need.

What if we just did something like ActiveStorage.draw_routes :disk_service and that would only draw the disk_service routes you required.

So the options would be [all, disk only, or none]. /cc @pixeltrix

@lfalcao @stefannibrasil the way to do it would be to detect if the current storage adapter is using the disk service and always draw the specific routes for it. If anyone wants to have a go at fixing it then you’ll need to change the Active Storage routes to something like the following logic:

if ActiveStorage.draw_routes
  Rails.application.routes.draw do
    # draw full routes
  end
elsif ActiveStorage.disk_service_enabled?
  Rails.application.routes.draw do
    scope ActiveStorage.routes_prefix do
      get  "/disk/:encoded_key/*filename" => "active_storage/disk#show", as: :rails_disk_service
      put  "/disk/:encoded_token" => "active_storage/disk#update", as: :update_rails_disk_service
    end
  end
end

What any aspiring contributor needs to do is work out how to implement disk_service_enabled?. They also need to write the railties tests which ensure that this feature works and doesn’t get broken in the future.

One tip - break out the full routes into a separate file and use the draw method to load it as demonstrated in this tweet: https://twitter.com/excid3/status/1366432061695922181

hello @stefannibrasil

I don’t like the idea of conditional based on environment, as someone can use it with other environments or even production and have this problem. So, I would try to when using disk service load these required routes anyway even with draw_routes = false…

@stefannibrasil your gist worked here fine with ruby 3.0.0p0.

@lauramosher thanks for your helpful reproduction script - always makes my life easier with one 👍🏻

The disk service is unique amongst the included services in that it reflects back on the application and not to some third-party service such as Amazon’s S3 service. So when you disable routes with draw_routes = false then the url helpers and routes for the disk service are not defined, leading to the error you saw. It’s possible to make your example pass by adding the following:

Rails.application.routes.draw do
  scope ActiveStorage.routes_prefix do
    get  "/disk/:encoded_key/*filename" => "active_storage/disk#show", as: :rails_disk_service
    put  "/disk/:encoded_token" => "active_storage/disk#update", as: :update_rails_disk_service
  end
end

after the Rails.application.initialize! line. To make it work in your application you just need to include the scope block in your config/routes.rb file.

Since the disk service is commonly used for development and testing it probably makes sense for us to draw the disk related routes irrespective of the draw_routes setting if the disk service is currently active. We’d probably restrict that behaviour to the development and test environments since it’s not really designed for production use and most apps run off ephemeral storage on services like Heroku.

Thanks for the report - hope this information solves your problem.

@stefannibrasil Could you elaborate on how this is a breaking change? Do you mean from the current behavior or the patch you’ve wrote?

Since draw_routes is already either true or false, introducing a third option :disk_only wouldn’t be breaking any compatibility afaict.

I’d like to wait and see what @pixeltrix thinks of this path before making a decision and suggesting more work for you though. 🙏

Thank you very much @lfalcao @pixeltrix for your thoughts and detailed context, I appreciate it. I will work on it this week! 💪

Thank you @lfalcao and @pixeltrix I forgot to include the image file in the gist so just changing the Ruby version was enough.

I’m trying to get started with contributing more to rails. What do you think is the best way to move forward with this issue?

• Document in the API-only module this extra step needed for this edge case;
• Add a conditional clause to restrict this behavior to the development and test environments;

Thank you!

hey @pixeltrix, we were able to reproduce the error. We also tried your suggestion, but we still get the same error. Can you double check? Maybe we’re missing something.

The only error I get when running your gist is the following:

Error:
BugTest#test_active_storage_url_does_not_raise:
RuntimeError: blank.jpg file does not exist
    ~/.rvm/gems/ruby-2.7.2/gems/rack-test-1.1.0/lib/rack/test/uploaded_file.rb:71:in `initialize_from_file_path'
    ~/.rvm/gems/ruby-2.7.2/gems/rack-test-1.1.0/lib/rack/test/uploaded_file.rb:34:in `initialize'
    rails-42043b.rb:74:in `new'
    rails-42043b.rb:74:in `test_active_storage_url_does_not_raise'

Creating a blank.jpg file makes the test pass.