Fix dropbox storage to not store the whole file in memory #23272
Conversation
|
By analyzing the blame information on this pull request, we identified @Xenopathic, @icewind1991 and @realriot to be potential reviewers |
|
Will this be backported? |
|
|
||
| $client = \OC::$server->getHTTPClientService()->newClient(); | ||
| try { | ||
| $tmpFile = \OC::$server->getTempManager()->getTemporaryFile($ext); |
There was a problem hiding this comment.
I don't see $ext defined in fopen at this point
There was a problem hiding this comment.
So maybe just getTemporaryFile?
There was a problem hiding this comment.
Ah, copy-paste fail...
|
Possibly 9.0.1 |
PVince81
force-pushed
the
dropbox-downloadnotthefullfileinmemory
branch
from
2877dbe
to
f708310
Compare
|
Looks like that breaks unit tests. |
|
Failing tests related to encoding, let's find out how much more we need to encode. |
Since the library can only store the full response in memory on download, we use an alternate client lib and set the correct headers to be able to stream the content to a temp file.
PVince81
force-pushed
the
dropbox-downloadnotthefullfileinmemory
branch
from
f708310
to
ab50ba7
Compare
|
I removed the |
|
Ok, now it's giving the same old failures we already had in #19868 |
| @@ -196,7 +196,7 @@ private function getOAuthBaseParams() { | |||
| * @return array Array for request's headers section like | |||
| * array('Authorization' => 'OAuth ...'); | |||
| */ | |||
| private function getOAuthHeader($uri, $params, $method = 'GET', $oAuthParams = null) { | |||
| public function getOAuthHeader($uri, $params, $method = 'GET', $oAuthParams = null) { | |||
There was a problem hiding this comment.
can we list changes made to 3rdparty things somewhere?
There was a problem hiding this comment.
in general I'd agree, but @LukasReschke told me we already super-patched this lib before due to security issues.
Maybe I can try and track down his changes and add them to "patch.txt" too
There was a problem hiding this comment.
in general I'd agree, but @LukasReschke told me we already super-patched this lib before due to security issues.
… the library was patched before that already as well … it's a nightmare 🙈 … also unmaintained upstream so we can't copy new releases over.
I'd just do 🙈 here and hope that at some point we switch to the proper Dropbox SDK.
|
So can I get your thumbs up @icewind1991 @LukasReschke ? @karlitschek backport to 9.0.1 ? (another memory issue, for dropbox download) |
|
great. please backport |
|
👍 |
1 similar comment
|
👍 |
|
stable9: #23372 |
…einmemory Fix dropbox storage to not store the whole file in memory
|
Thanks! |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Since the library can only store the full response in memory on
download, we use an alternate client lib and set the correct headers to
be able to stream the content to a temp file.
Note: I had to patch the library and also note that the library is unmaintained and that we should really find some time to update to a better library... #13841
Similar approach like for GDrive.
Fixes #4040
Please review @LukasReschke @icewind1991 @Xenopathic