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

ActiveStorage configuration resolve_model_to_route is not picked up by rails_blob_* URL generator #41172

Closed
aliismayilov opened this issue Jan 19, 2021 · 5 comments · Fixed by #41186

Comments

@aliismayilov
Copy link
Contributor

Steps to reproduce

  1. Create a new rails app.
  2. Set up activestorage with Amazon (S3) as backend service.
  3. Set config.active_storage.resolve_model_to_route = :rails_storage_proxy.
  4. Create a model with attachment. E.g. class Picture < ApplicationRecord; has_one_attached :file; end.
  5. Create a new record with attachment.
  6. Render rails_blob_path.

Expected behavior

The link should start with /rails/active_storage/blobs/proxy/.

Actual behavior

The link starts with /rails/active_storage/blobs/redirect/.

System configuration

Rails version: 6.1.1

Ruby version: 2.7.2p137

@aliismayilov aliismayilov changed the title Configuration resolve_model_to_route=rails_storage_proxy does not work ActiveStorage configuration resolve_model_to_route is not picked up by rails_blob_* URL generator Jan 19, 2021
@weilandia
Copy link
Contributor

weilandia commented Jan 20, 2021

Related:

I'm seeing

Rails.application.config.active_storage.resolve_model_to_route = :rails_storage_proxy

obj = ActiveStorage::Attachment.first

Rails.application.routes.url_helpers.url_for(obj) == "http://localhost:3000/rails/active_storage/blobs/proxy/..."

# BUT

Rails.application.routes.url_helpers.rails_blob_url(obj) == "http://localhost:3000/rails/active_storage/blobs/redirect/..."

Is this by design?

@aliismayilov
Copy link
Contributor Author

Related:

I'm seeing

Rails.application.config.active_storage.resolve_model_to_route = :rails_storage_proxy

obj = ActiveStorage::Attachment.first

Rails.application.routes.url_helpers.url_for(obj) == "http://localhost:3000/rails/active_storage/blobs/proxy/..."

# BUT

Rails.application.routes.url_helpers.rails_blob_url(obj) == "http://localhost:3000/rails/active_storage/blobs/redirect/..."

Is this by design?

I wouldn't think this is intentional. From my understanding of the docs and release notes is that rails_storage_proxy should generate proxy URLs by default. In case you need to use the redirect strategy while the default is proxy, you'll need to use rails_storage_redirect_url.

@composerinteralia
Copy link
Member

composerinteralia commented Jan 23, 2021

It looks like when we added proxying in #34477 the rails_blob and rails_representation url helpers didn't come along for the ride. Unsure whether that was intentional, but one way to fix would be to change:

direct :rails_representation do |representation, options|
signed_blob_id = representation.blob.signed_id
variation_key = representation.variation.key
filename = representation.blob.filename
route_for(:rails_blob_representation, signed_blob_id, variation_key, filename, options)
end

 direct :rails_representation do |representation, options| 
-   signed_blob_id = representation.blob.signed_id 
-   variation_key  = representation.variation.key 
-   filename       = representation.blob.filename 
-  
-   route_for(:rails_blob_representation, signed_blob_id, variation_key, filename, options)
+   route_for(ActiveStorage.resolve_model_to_route, representation, options) 
 end 

and

direct :rails_blob do |blob, options|
route_for(:rails_service_blob, blob.signed_id, blob.filename, options)
end

 direct :rails_blob do |blob, options| 
-   route_for(:rails_service_blob, blob.signed_id, blob.filename, options) 
+   route_for(ActiveStorage.resolve_model_to_route, blob, options) 
 end 

@aliismayilov
Copy link
Contributor Author

@composerinteralia please see #41186

@composerinteralia
Copy link
Member

Ah, I totally missed your PR. Left one comment, but what you did makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants