-
Notifications
You must be signed in to change notification settings - Fork 63
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
Adds fix for content-lenght value missing for files uploaded via ActionD... #622
Conversation
0f479d6
to
7c47e59
Compare
def initialize | ||
@content = StringIO.new("hello world") | ||
end | ||
def bytesize |
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 add some whitespace between the methods.
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 method ever used?
@@ -254,6 +254,9 @@ def save(*) | |||
payload = behaves_like_io?(content) ? content.read : content | |||
headers = { 'Content-Type' => mime_type } | |||
headers['Content-Disposition'] = "attachment; filename=\"#{@original_name}\"" if @original_name | |||
# Setting the content-lenght is required until we figure out why Faraday | |||
# is not doing this automatically for files uploaded via ActionDispatch. | |||
headers['Content-Length'] = payload.size.to_s if payload.is_a?(ActionDispatch::Http::UploadedFile) |
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.
@jcoyne Although I like the payload.is_a?
approach better than using payload.class.name == "ActionDispatch::Http::UploadedFile"
that problem with payload.is_a?
is that it will force a dependency between ActiveFedora and ActionDispatch::Http::UploadedFile and the code won't run unless the class ActionDispatch::Http::UploadedFile is defined.
I am thinking of reverting back to class.name == "ActionDispatch::Http::UploadedFile"
. Thoughts?
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.
@jcoyne I could also do defined?(ActionDispatch::Http::UploadedFile) and payload.is_a?(ActionDispatch::Http::UploadedFile)
That seems like a good compromise.
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.
Yeah, that's a good idea.
7c47e59
to
30275f9
Compare
@@ -292,6 +295,10 @@ def stream(range = nil, &block) | |||
|
|||
private | |||
|
|||
def is_actiondispatch_uploadedfile?(payload) |
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.
How about something shorter like def uploaded_file?(payload)
or is that misleading?
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.
@mjgiarlo I like the shorter name better too. I can go either way.
…onDispatch:Http::UploadedFile
30275f9
to
979b068
Compare
Adds fix for content-lenght value missing for files uploaded via ActionD...
...ispatch:Http::UploadedFile