From 313b881d2b38f4a0ed43afc68dafacd65524b0b7 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 23 Feb 2016 16:19:23 +0100 Subject: [PATCH 1/2] Do not check all chunks of a chunked upload if we do not need to Fixes #22601 Before we did a full test on all chunks to verify if a chunked upload was completed. This is unneeded since if we are missing one chunk we can already fail. Also we look from back to front since it is much more likely that we find a missing chunk thus can error out early. --- lib/private/filechunking.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/private/filechunking.php b/lib/private/filechunking.php index ece215e7344..32cbb7559f0 100644 --- a/lib/private/filechunking.php +++ b/lib/private/filechunking.php @@ -74,14 +74,16 @@ class OC_FileChunking { public function isComplete() { $prefix = $this->getPrefix(); - $parts = 0; $cache = $this->getCache(); - for($i=0; $i < $this->info['chunkcount']; $i++) { - if ($cache->hasKey($prefix.$i)) { - $parts ++; + $chunkcount = (int)$this->info['chunkcount']; + + for($i=($chunkcount-1); $i >= 0; $i--) { + if (!$cache->hasKey($prefix.$i)) { + return false; } } - return $parts == $this->info['chunkcount']; + + return true; } /** From 91c7d293ca0c41addd25d79fa060be6506d898b1 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 7 Mar 2016 20:55:49 +0100 Subject: [PATCH 2/2] Added tests * Unit tests for OC_Filechunking to verify the isComplete function * Intergration tests to show that shuffling chunks is all fine --- .../integration/features/bootstrap/WebDav.php | 11 +++ .../features/webdav-related.feature | 27 +++++++ tests/lib/filechunking.php | 70 +++++++++++++++++++ 3 files changed, 108 insertions(+) create mode 100644 tests/lib/filechunking.php diff --git a/build/integration/features/bootstrap/WebDav.php b/build/integration/features/bootstrap/WebDav.php index fa6761d9f71..0430768022c 100644 --- a/build/integration/features/bootstrap/WebDav.php +++ b/build/integration/features/bootstrap/WebDav.php @@ -262,5 +262,16 @@ trait WebDav { } } + /** + * @Given user :user uploads chunk file :num of :total with :data to :destination + */ + public function userUploadsChunkFileOfWithToWithChecksum($user, $num, $total, $data, $destination) + { + $num -= 1; + $data = \GuzzleHttp\Stream\Stream::factory($data); + $file = $destination . '-chunking-42-'.$total.'-'.$num; + $this->makeDavRequest($user, 'PUT', $file, ['OC-Chunked' => '1'], $data); + } + } diff --git a/build/integration/features/webdav-related.feature b/build/integration/features/webdav-related.feature index 63f205d1e4c..019df3436f8 100644 --- a/build/integration/features/webdav-related.feature +++ b/build/integration/features/webdav-related.feature @@ -141,3 +141,30 @@ Feature: webdav-related Given Logging in using web as "admin" When Sending a "PROPFIND" to "/remote.php/webdav/welcome.txt" with requesttoken Then the HTTP status code should be "207" + + Scenario: Upload chunked file asc + Given user "user0" exists + And user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChunkedFile.txt" + And user "user0" uploads chunk file "2" of "3" with "BBBBB" to "/myChunkedFile.txt" + And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChunkedFile.txt" + When As an "user0" + And Downloading file "/myChunkedFile.txt" + Then Downloaded content should be "AAAAABBBBBCCCCC" + + Scenario: Upload chunked file desc + Given user "user0" exists + And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChunkedFile.txt" + And user "user0" uploads chunk file "2" of "3" with "BBBBB" to "/myChunkedFile.txt" + And user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChunkedFile.txt" + When As an "user0" + And Downloading file "/myChunkedFile.txt" + Then Downloaded content should be "AAAAABBBBBCCCCC" + + Scenario: Upload chunked file random + Given user "user0" exists + And user "user0" uploads chunk file "2" of "3" with "BBBBB" to "/myChunkedFile.txt" + And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChunkedFile.txt" + And user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChunkedFile.txt" + When As an "user0" + And Downloading file "/myChunkedFile.txt" + Then Downloaded content should be "AAAAABBBBBCCCCC" diff --git a/tests/lib/filechunking.php b/tests/lib/filechunking.php new file mode 100644 index 00000000000..2f3f411275d --- /dev/null +++ b/tests/lib/filechunking.php @@ -0,0 +1,70 @@ + + * + * @copyright Copyright (c) 2016, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ +namespace Test; + +class FileChunking extends \Test\TestCase { + + public function dataIsComplete() { + return [ + [1, [], false], + [1, [0], true], + [2, [], false], + [2, [0], false], + [2, [1], false], + [2, [0,1], true], + [10, [], false], + [10, [0,1,2,3,4,5,6,7,8], false], + [10, [1,2,3,4,5,6,7,8,9], false], + [10, [0,1,2,3,5,6,7,8,9], false], + [10, [0,1,2,3,4,5,6,7,8,9], true], + ]; + } + + /** + * @dataProvider dataIsComplete + * @param $total + * @param array $present + * @param $expected + */ + public function testIsComplete($total, array $present, $expected) { + $fileChunking = $this->getMockBuilder('\OC_FileChunking') + ->setMethods(['getCache']) + ->setConstructorArgs([[ + 'name' => 'file', + 'transferid' => '42', + 'chunkcount' => $total, + ]]) + ->getMock(); + + $cache = $this->getMock('\OCP\ICache'); + + $cache->expects($this->atLeastOnce()) + ->method('hasKey') + ->will($this->returnCallback(function ($key) use ($present) { + $data = explode('-', $key); + return in_array($data[3], $present); + })); + + $fileChunking->method('getCache')->willReturn($cache); + + $this->assertEquals($expected, $fileChunking->isComplete()); + } +}