From 6a4ea2c15adb254291b095cfe21818aa28c26138 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 9 Sep 2016 12:19:29 +0200 Subject: Upload autorename on client side Removes the need for POST to collection which would hit against upload limits. The client tries to auto rename the file by adding a suffix "(2)". It tries to use the file list on the client side to guess a suitable name. In case a file still cannot be uploaded and creates a conflict, which can happen when the file was concurrently uploaded, the logic will continue increasing the suffix. --- apps/dav/lib/Connector/Sabre/FilesPlugin.php | 50 ------------- .../tests/unit/Connector/Sabre/FilesPluginTest.php | 81 --------------------- apps/files/js/file-upload.js | 85 ++++++++++++++-------- apps/files/js/templates/detailsview.handlebars.js | 28 +++++++ apps/files/tests/js/fileUploadSpec.js | 61 +++++++++++++++- 5 files changed, 139 insertions(+), 166 deletions(-) create mode 100644 apps/files/js/templates/detailsview.handlebars.js (limited to 'apps') diff --git a/apps/dav/lib/Connector/Sabre/FilesPlugin.php b/apps/dav/lib/Connector/Sabre/FilesPlugin.php index 39d15e0c6e9..539e22296f2 100644 --- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php @@ -172,8 +172,6 @@ class FilesPlugin extends ServerPlugin { $this->server = $server; $this->server->on('propFind', array($this, 'handleGetProperties')); $this->server->on('propPatch', array($this, 'handleUpdateProperties')); - // RFC5995 to add file to the collection with a suggested name - $this->server->on('method:POST', [$this, 'httpPost']); $this->server->on('afterBind', array($this, 'sendFileIdHeader')); $this->server->on('afterWriteContent', array($this, 'sendFileIdHeader')); $this->server->on('afterMethod:GET', [$this,'httpGet']); @@ -435,52 +433,4 @@ class FilesPlugin extends ServerPlugin { } } } - - /** - * POST operation on directories to create a new file - * with suggested name - * - * @param RequestInterface $request request object - * @param ResponseInterface $response response object - * @return null|false - */ - public function httpPost(RequestInterface $request, ResponseInterface $response) { - // TODO: move this to another plugin ? - if (!\OC::$CLI && !\OC::$server->getRequest()->passesCSRFCheck()) { - throw new BadRequest('Invalid CSRF token'); - } - - list($parentPath, $name) = \Sabre\HTTP\URLUtil::splitPath($request->getPath()); - - // Making sure the parent node exists and is a directory - $node = $this->tree->getNodeForPath($parentPath); - - if ($node instanceof Directory) { - // no Add-Member found - if (empty($name) || $name[0] !== '&') { - // suggested name required - throw new BadRequest('Missing suggested file name'); - } - - $name = substr($name, 1); - - if (empty($name)) { - // suggested name required - throw new BadRequest('Missing suggested file name'); - } - - // make sure the name is unique - $name = basename(\OC_Helper::buildNotExistingFileNameForView($parentPath, $name, $this->fileView)); - - $node->createFile($name, $request->getBodyAsStream()); - - list($parentUrl, ) = \Sabre\HTTP\URLUtil::splitPath($request->getUrl()); - - $response->setHeader('Content-Location', $parentUrl . '/' . rawurlencode($name)); - - // created - $response->setStatus(201); - return false; - } - } } diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php index 43ca119abff..d39f709493e 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php @@ -547,85 +547,4 @@ class FilesPluginTest extends TestCase { $this->assertEquals("false", $propFind->get(self::HAS_PREVIEW_PROPERTYNAME)); } - - public function postCreateFileProvider() { - $baseUrl = 'http://example.com/owncloud/remote.php/webdav/subdir/'; - return [ - ['test.txt', 'some file.txt', 'some file.txt', $baseUrl . 'some%20file.txt'], - ['some file.txt', 'some file.txt', 'some file (2).txt', $baseUrl . 'some%20file%20%282%29.txt'], - ]; - } - - /** - * @dataProvider postCreateFileProvider - */ - public function testPostWithAddMember($existingFile, $wantedName, $deduplicatedName, $expectedLocation) { - $request = $this->getMock('Sabre\HTTP\RequestInterface'); - $response = $this->getMock('Sabre\HTTP\ResponseInterface'); - - $request->expects($this->any()) - ->method('getUrl') - ->will($this->returnValue('http://example.com/owncloud/remote.php/webdav/subdir/&' . $wantedName)); - - $request->expects($this->any()) - ->method('getPath') - ->will($this->returnValue('/subdir/&' . $wantedName)); - - $request->expects($this->once()) - ->method('getBodyAsStream') - ->will($this->returnValue(fopen('data://text/plain,hello', 'r'))); - - $this->view->expects($this->any()) - ->method('file_exists') - ->will($this->returnCallback(function($path) use ($existingFile) { - return ($path === '/subdir/' . $existingFile); - })); - - $node = $this->createTestNode('\OCA\DAV\Connector\Sabre\Directory', '/subdir'); - - $node->expects($this->once()) - ->method('createFile') - ->with($deduplicatedName, $this->isType('resource')); - - $response->expects($this->once()) - ->method('setStatus') - ->with(201); - $response->expects($this->once()) - ->method('setHeader') - ->with('Content-Location', $expectedLocation); - - $this->assertFalse($this->plugin->httpPost($request, $response)); - } - - public function testPostOnNonDirectory() { - $request = $this->getMock('Sabre\HTTP\RequestInterface'); - $response = $this->getMock('Sabre\HTTP\ResponseInterface'); - - $request->expects($this->any()) - ->method('getPath') - ->will($this->returnValue('/subdir/test.txt/&abc')); - - $this->createTestNode('\OCA\DAV\Connector\Sabre\File', '/subdir/test.txt'); - - $this->assertNull($this->plugin->httpPost($request, $response)); - } - - /** - * @expectedException \Sabre\DAV\Exception\BadRequest - */ - public function testPostWithoutAddMember() { - $request = $this->getMock('Sabre\HTTP\RequestInterface'); - $response = $this->getMock('Sabre\HTTP\ResponseInterface'); - - $request->expects($this->any()) - ->method('getPath') - ->will($this->returnValue('/subdir/&')); - - $node = $this->createTestNode('\OCA\DAV\Connector\Sabre\Directory', '/subdir'); - - $node->expects($this->never()) - ->method('createFile'); - - $this->plugin->httpPost($request, $response); - } } diff --git a/apps/files/js/file-upload.js b/apps/files/js/file-upload.js index 18d0d973586..8bcaa2e24aa 100644 --- a/apps/files/js/file-upload.js +++ b/apps/files/js/file-upload.js @@ -100,26 +100,14 @@ OC.FileUpload.prototype = { /** * Return the final filename. - * Either this is the original file name or the file name - * after an autorename. * * @return {String} file name */ getFileName: function() { - // in case of autorename + // autorenamed name if (this._newName) { return this._newName; } - - if (this._conflictMode === OC.FileUpload.CONFLICT_MODE_AUTORENAME) { - - var locationUrl = this.getResponseHeader('Content-Location'); - if (locationUrl) { - this._newName = decodeURIComponent(OC.basename(locationUrl)); - return this._newName; - } - } - return this.getFile().name; }, @@ -141,9 +129,20 @@ OC.FileUpload.prototype = { return OC.joinPaths(this._targetFolder, this.getFile().relativePath || ''); }, + /** + * Returns conflict resolution mode. + * + * @return {int} conflict mode + */ + getConflictMode: function() { + return this._conflictMode || OC.FileUpload.CONFLICT_MODE_DETECT; + }, + /** * Set conflict resolution mode. * See CONFLICT_MODE_* constants. + * + * @param {int} mode conflict mode */ setConflictMode: function(mode) { this._conflictMode = mode; @@ -162,6 +161,30 @@ OC.FileUpload.prototype = { delete this.data.jqXHR; }, + /** + * Trigger autorename and append "(2)". + * Multiple calls will increment the appended number. + */ + autoRename: function() { + var name = this.getFile().name; + if (!this._renameAttempt) { + this._renameAttempt = 1; + } + + var dotPos = name.lastIndexOf('.'); + var extPart = ''; + if (dotPos > 0) { + this._newName = name.substr(0, dotPos); + extPart = name.substr(dotPos); + } else { + this._newName = name; + } + + // generate new name + this._renameAttempt++; + this._newName = this._newName + ' (' + this._renameAttempt + ')' + extPart; + }, + /** * Submit the upload */ @@ -179,7 +202,7 @@ OC.FileUpload.prototype = { } if (this.uploader.fileList) { - this.data.url = this.uploader.fileList.getUploadUrl(file.name, this.getFullPath()); + this.data.url = this.uploader.fileList.getUploadUrl(this.getFileName(), this.getFullPath()); } if (!this.data.headers) { @@ -191,12 +214,9 @@ OC.FileUpload.prototype = { this.data.type = 'PUT'; delete this.data.headers['If-None-Match']; - if (this._conflictMode === OC.FileUpload.CONFLICT_MODE_DETECT) { + if (this._conflictMode === OC.FileUpload.CONFLICT_MODE_DETECT + || this._conflictMode === OC.FileUpload.CONFLICT_MODE_AUTORENAME) { this.data.headers['If-None-Match'] = '*'; - } else if (this._conflictMode === OC.FileUpload.CONFLICT_MODE_AUTORENAME) { - // POST to parent folder, with slug - this.data.type = 'POST'; - this.data.url = this.uploader.fileList.getUploadUrl('&' + file.name, this.getFullPath()); } if (file.lastModified) { @@ -212,18 +232,6 @@ OC.FileUpload.prototype = { 'Basic ' + btoa(userName + ':' + (password || '')); } - if (!this.uploader.isXHRUpload()) { - data.formData = []; - - // pass headers as parameters - data.formData.push({name: 'headers', value: JSON.stringify(this.data.headers)}); - data.formData.push({name: 'requesttoken', value: OC.requestToken}); - if (data.type === 'POST') { - // still add the method to the URL - data.url += '?_method=POST'; - } - } - var chunkFolderPromise; if ($.support.blobSlice && this.uploader.fileUploadParam.maxChunkSize @@ -491,6 +499,14 @@ OC.Uploader.prototype = _.extend({ //show "file already exists" dialog var self = this; var file = fileUpload.getFile(); + // already attempted autorename but the server said the file exists ? (concurrently added) + if (fileUpload.getConflictMode() === OC.FileUpload.CONFLICT_MODE_AUTORENAME) { + // attempt another autorename, defer to let the current callback finish + _.defer(function() { + self.onAutorename(fileUpload); + }); + return; + } // retrieve more info about this file this.filesClient.getFileInfo(fileUpload.getFullPath()).then(function(status, fileInfo) { var original = fileInfo; @@ -603,6 +619,13 @@ OC.Uploader.prototype = _.extend({ onAutorename:function(upload) { this.log('autorename', null, upload); upload.setConflictMode(OC.FileUpload.CONFLICT_MODE_AUTORENAME); + + do { + upload.autoRename(); + // if file known to exist on the client side, retry + } while (this.fileList && this.fileList.inList(upload.getFileName())); + + // resubmit upload this.submitUploads([upload]); }, _trace:false, //TODO implement log handler for JS per class? diff --git a/apps/files/js/templates/detailsview.handlebars.js b/apps/files/js/templates/detailsview.handlebars.js new file mode 100644 index 00000000000..c109da77a63 --- /dev/null +++ b/apps/files/js/templates/detailsview.handlebars.js @@ -0,0 +1,28 @@ +(function() { + var template = Handlebars.template, templates = OCA.Files.Templates = OCA.Files.Templates || {}; +templates['detailsview'] = template({"1":function(container,depth0,helpers,partials,data) { + var stack1; + + return "\n"; +},"2":function(container,depth0,helpers,partials,data) { + var helper, alias1=depth0 != null ? depth0 : {}, alias2=helpers.helperMissing, alias3="function", alias4=container.escapeExpression; + + return "
  • \n " + + alias4(((helper = (helper = helpers.label || (depth0 != null ? depth0.label : depth0)) != null ? helper : alias2),(typeof helper === alias3 ? helper.call(alias1,{"name":"label","hash":{},"data":data}) : helper))) + + "\n
    • \n"; +},"compiler":[7,">= 4.0.0"],"main":function(container,depth0,helpers,partials,data) { + var stack1, helper, alias1=depth0 != null ? depth0 : {}; + + return "
    \n" + + ((stack1 = helpers["if"].call(alias1,(depth0 != null ? depth0.tabHeaders : depth0),{"name":"if","hash":{},"fn":container.program(1, data, 0),"inverse":container.noop,"data":data})) != null ? stack1 : "") + + "
    \n
    \n\n"; +},"useData":true}); +})(); diff --git a/apps/files/tests/js/fileUploadSpec.js b/apps/files/tests/js/fileUploadSpec.js index fa686dbf3e2..81a0a2df610 100644 --- a/apps/files/tests/js/fileUploadSpec.js +++ b/apps/files/tests/js/fileUploadSpec.js @@ -144,6 +144,10 @@ describe('OC.Upload tests', function() { uploader = new OC.Uploader($dummyUploader, { fileList: fileList }); + + var deferred = $.Deferred(); + conflictDialogStub.returns(deferred.promise()); + deferred.resolve(); }); afterEach(function() { conflictDialogStub.restore(); @@ -158,10 +162,6 @@ describe('OC.Upload tests', function() { expect(result[1].submit.calledOnce).toEqual(true); }); it('shows conflict dialog when no client side conflict', function() { - var deferred = $.Deferred(); - conflictDialogStub.returns(deferred.promise()); - deferred.resolve(); - var result = addFiles(uploader, [ {name: 'conflict.txt'}, {name: 'conflict2.txt'}, @@ -185,5 +185,58 @@ describe('OC.Upload tests', function() { expect(result[1].submit.calledOnce).toEqual(false); expect(result[2].submit.calledOnce).toEqual(true); }); + it('cancels upload when skipping file in conflict mode', function() { + var fileData = {name: 'conflict.txt'}; + var uploadData = addFiles(uploader, [ + fileData + ]); + + var upload = new OC.FileUpload(uploader, uploadData[0]); + var deleteStub = sinon.stub(upload, 'deleteUpload'); + + uploader.onSkip(upload); + expect(deleteStub.calledOnce).toEqual(true); + }); + it('overwrites file when choosing replace in conflict mode', function() { + var fileData = {name: 'conflict.txt'}; + var uploadData = addFiles(uploader, [ + fileData + ]); + + expect(uploadData[0].submit.notCalled).toEqual(true); + + var upload = new OC.FileUpload(uploader, uploadData[0]); + + uploader.onReplace(upload); + expect(upload.getConflictMode()).toEqual(OC.FileUpload.CONFLICT_MODE_OVERWRITE); + expect(uploadData[0].submit.calledOnce).toEqual(true); + }); + it('autorenames file when choosing replace in conflict mode', function() { + // needed for _.defer call + var clock = sinon.useFakeTimers(); + var fileData = {name: 'conflict.txt'}; + var uploadData = addFiles(uploader, [ + fileData + ]); + + expect(uploadData[0].submit.notCalled).toEqual(true); + + var upload = new OC.FileUpload(uploader, uploadData[0]); + var getResponseStatusStub = sinon.stub(upload, 'getResponseStatus'); + + uploader.onAutorename(upload); + expect(upload.getConflictMode()).toEqual(OC.FileUpload.CONFLICT_MODE_AUTORENAME); + expect(upload.getFileName()).toEqual('conflict (2).txt'); + expect(uploadData[0].submit.calledOnce).toEqual(true); + + // in case of server-side conflict, tries to rename again + getResponseStatusStub.returns(412); + uploader.fileUploadParam.fail.call($dummyUploader[0], {}, uploadData[0]); + clock.tick(500); + expect(upload.getFileName()).toEqual('conflict (3).txt'); + expect(uploadData[0].submit.calledTwice).toEqual(true); + + clock.restore(); + }); }); }); -- cgit v1.2.3