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
Extract ActiveStorage::Streaming so your own controllers can use it #41440
Conversation
activestorage/app/controllers/active_storage/base_controller.rb
Outdated
Show resolved
Hide resolved
activestorage/app/controllers/concerns/active_storage/streaming.rb
Outdated
Show resolved
Hide resolved
activestorage/app/controllers/concerns/active_storage/streaming.rb
Outdated
Show resolved
Hide resolved
activestorage/app/controllers/concerns/active_storage/streaming.rb
Outdated
Show resolved
Hide resolved
…g.rb Co-authored-by: Kasper Timm Hansen <kaspth@gmail.com>
Like the pre-extraction code.
|
||
def show | ||
http_cache_forever public: true do | ||
set_content_headers_from representation.image | ||
stream representation | ||
stream_from_storage representation.image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this code supposed to stream the representation instead of the image? I think that is the reason why the tests are broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were broken differently before. Think we need the image for the content type headers, but the representation for the streaming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did found this weird. That you can't just stream the representation directly. Maybe @georgeclaghorn knows why that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All representations are supposed to produce an image, so I think this is correct?
module ActiveStorage::Streaming | ||
DEFAULT_BLOB_STREAMING_DISPOSITION = "inline" | ||
|
||
include ActionController::Live |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this module included in api_only
mode? Asking because otherwise they won't be able to use ActiveStorage
anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api mode does not support streaming? That sounds like something we should fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was a legit question, I don't really know 😇 maybe everything is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do verify! It's a good question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow rails/rails#41440. This PR disables `Layout/FirstArgumentIndentation` cop.
Follow rails/rails#41440. This PR disables `Layout/FirstArgumentIndentation` cop.
Really appreciate this change. I am wondering if this changes behavior I noticed with the previous implementation of |
blob.download do |chunk|
stream.write chunk
end We're streaming the download and streaming the writing. This is happening in chunks. So it doesn't download the whole thing first. |
If you want to build your own controller that streams from active storage, it's helpful to have a method that does the streaming correctly. One such already existed unextracted in ActiveStorage::BaseController.