ActiveStorage raises with missing method path_for when using MirrorService with DiskService as the primary service

[marcusmalmberg]

Upgrading to Rails 5.2.2 (from 5.2.1) breaks ActiveStorage using MirrorService with DiskService as the primary service.

Steps to reproduce

Use ActiveStorage and setup a mirrored service with Disk as primary, e.g.

local:
  service: Disk
  root: <%= Rails.root.join("storage") %>

amazon:
  ...

mirror:
  service: Mirror
  primary: local
  mirrors: [ amazon ]

When trying to fetch a stored resource it crashes.

Expected behavior

Expects to be able to use a mirror with a disk service as primary service.

Actual behavior

When fetching the resources while having the disk service as primary service it crashes.

System configuration

Rails version:
5.2.2

Ruby version:
ruby 2.5.0p0

---

It raises on missing method path_for on line: https://github.com/rails/rails/blob/5-2-stable/activestorage/app/controllers/active_storage/disk_controller.rb#L12

Changing the primary service to amazon works, as well as using rails 5.2.1 instead of 5.2.2.

As far as I can tell this problem was introduced with this change: 62bb663

Some random thoughts:

A potential solution could be to delegate :path_for to the primary service here:
https://github.com/rails/rails/blob/5-2-stable/activestorage/lib/active_storage/service/mirror_service.rb#L12
My problem with this is that the documentation for the MirrorService states that it is a meant to handle all uploads. This would both repurpose it to also handle fetching, but maybe more importantly, also change based on the need of the internal implementation of a specific service which isn't something that the other services are using.

Another option could be to change https://github.com/rails/rails/blob/5-2-stable/activestorage/app/controllers/active_storage/disk_controller.rb#L12 to check if the service is a mirrorservice and then use the primary service to call path_for, instead of on the mirror service. Using two different kind of services seems more error prone in the future and moves away from using an common interface for the services.

TL;DR:

The problem seems to be that the DiskController relies on a path_for method that is defined in the DiskService which isn't exposed/forwarded in the MirrorService.

A potential solution would be to extend the "interface" in the MirrorService to allow for the path_for method that is currently only used by the DiskService. That is, add path_for to delegate ..., to: :primary in the MirrorService

[georgeclaghorn]

+1 for delegating path_for to the primary service. Mind opening a PR?

[abhaynikam]

@marcusmalmberg: If you are not working on it I can raise a PR for the change.

cc/ @georgeclaghorn

[marcusmalmberg]

@abhaynikam I was planning on doing it tomorrow, so I haven't started on it yet. Feel free to make a PR :)

[marcusmalmberg]

What about backporting the fix in #35268 to rails-5-2 as well?

[georgeclaghorn]

I already backported it. 👍