Cause Fog::File.read to return its contents after upload #1517
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This solves the case where uploading a file to Fog and then reading its body immediately afterward results in a deleted, closed tempfile rather than the actual contents of the upload. carrierwaveuploader#1338
stormsilver
changed the title
|
Looks good for me 👍 |
bensie
added a commit
that referenced
this pull request
Dec 9, 2014
Cause Fog::File.read to return its contents after upload
|
Thanks for this! 👍 |
DarkArc
added a commit
to topechelon/carrierwave
that referenced
this pull request
Apr 12, 2018
This fixes an issue with file upload streaming. Previously passing the file was added in PR carrierwaveuploader#468, and was subsequently broken by PR carrierwaveuploader#1517. This corrects the approach implemented in PR carrierwaveuploader#468, without introducing the problem in PR carrierwaveuploader#1517. New behavior: In the case of a fog based retrieval request, the file will be fetched as it was previously. The file read method will then see that file.body is set to a non-nil value, and return the file content from the file object. In the case of a fog based storage request, the file will be uploaded either as full text, or in batches as it was in PR carrierwaveuploader#468. However, to address the issue observed in PR carrierwaveuploader#1517 we set the body to nil, and then store a reference to the source file (the CarrierWave::SanitizedFile/read compatible object passed). This in turn results in the read method seeing no body on the file object, and consulting the source file for the file contents. The approach to storage requests is taken over two other approaches: Approach carrierwaveuploader#1: We could read the file into memory as carrierwave currently does, however, this results in large files often exhausting the heap space of the process, thus making carrierwave a poor choice for large file uploads. Approach carrierwaveuploader#2: We could not store a reference to the original source file, however, this would result in an additional retrieval requests carrierwave does not currently make, potentially costing in bandwidth for current users.
DarkArc
added a commit
to topechelon/carrierwave
that referenced
this pull request
Apr 12, 2018
This fixes an issue with file upload streaming. Previously passing the file was added in PR carrierwaveuploader#468, and was subsequently broken by PR carrierwaveuploader#1517. This corrects the approach implemented in PR carrierwaveuploader#468, without introducing the problem in PR carrierwaveuploader#1517. New behavior: In the case of a fog based retrieval request, the file will be fetched as it was previously. The file read method will then see that file.body is set to a non-nil value, and return the file content from the file object. In the case of a fog based storage request, the file will be uploaded either as full text, or in batches as it was in PR carrierwaveuploader#468. However, to address the issue observed in PR carrierwaveuploader#1517 we set the body to nil, and then store a reference to the source file (the CarrierWave::SanitizedFile/read compatible object passed). This in turn results in the read method seeing no body on the file object, and consulting the source file for the file contents. The approach to storage requests is taken over two other approaches: Approach carrierwaveuploader#1: We could read the file into memory as carrierwave currently does, however, this results in large files often exhausting the heap space of the process, thus making carrierwave a poor choice for large file uploads. Approach carrierwaveuploader#2: We could not store a reference to the original source file, however, this would result in an additional retrieval requests carrierwave does not currently make, potentially costing in bandwidth for current users.
DarkArc
added a commit
to topechelon/carrierwave
that referenced
this pull request
Apr 12, 2018
This fixes an issue with file upload streaming. Previously passing the file was added in PR carrierwaveuploader#468, and was subsequently broken by PR carrierwaveuploader#1517. This corrects the approach implemented in PR carrierwaveuploader#468, without introducing the problem in PR carrierwaveuploader#1517. New behavior: In the case of a fog based retrieval request, the file will be fetched as it was previously. The file read method will then see that file.body is set to a non-nil value, and return the file content from the file object. In the case of a fog based storage request, the file will be uploaded either as full text, or in batches as it was in PR carrierwaveuploader#468. However, to address the issue observed in PR carrierwaveuploader#1517 we set the body to nil, and then store a reference to the source file (the CarrierWave::SanitizedFile/read compatible object passed). This in turn results in the read method seeing no body on the file object, and consulting the source file for the file contents. The approach to storage requests is taken over two other approaches: Approach carrierwaveuploader#1: We could read the file into memory as carrierwave currently does, however, this results in large files often exhausting the heap space of the process, thus making carrierwave a poor choice for large file uploads. Approach carrierwaveuploader#2: We could not store a reference to the original source file, however, this would result in an additional retrieval requests carrierwave does not currently make, potentially costing in bandwidth for current users.
DarkArc
added a commit
to topechelon/carrierwave
that referenced
this pull request
Apr 12, 2018
This fixes an issue with file upload streaming. Previously passing the file was added in PR carrierwaveuploader#468, and was subsequently broken by PR carrierwaveuploader#1517. This corrects the approach implemented in PR carrierwaveuploader#468, without introducing the problem in PR carrierwaveuploader#1517. New behavior: In the case of a fog based retrieval request, the file will be fetched as it was previously. The file read method will then see that file.body is set to a non-nil value, and return the file content from the file object. In the case of a fog based storage request, the file will be uploaded either as full text, or in batches as it was in PR carrierwaveuploader#468. However, to address the issue observed in PR carrierwaveuploader#1517 we set the body to nil, and then store a reference to the source file (the CarrierWave::SanitizedFile/read compatible object passed). This in turn results in the read method seeing no body on the file object, and consulting the source file for the file contents. The approach to storage requests is taken over two other approaches: Approach carrierwaveuploader#1: We could read the file into memory as carrierwave currently does, however, this results in large files often exhausting the heap space of the process, thus making carrierwave a poor choice for large file uploads. Approach carrierwaveuploader#2: We could not store a reference to the original source file, however, this would result in an additional retrieval requests carrierwave does not currently make, potentially costing in bandwidth for current users.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This solves the case where uploading a file to Fog and then reading its body
immediately afterward results in a deleted, closed tempfile rather
than the actual contents of the upload. #1338