Work directly on the storage when uploading over webdav #12006
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,12 +83,12 @@ class File extends Node implements IFile { | |
| */ | ||
| public function put($data) { | ||
| try { | ||
| $exists = $this->fileView->file_exists($this->path); | ||
| if ($this->info && $exists && !$this->info->isUpdateable()) { | ||
| throw new Forbidden(); | ||
| } | ||
| } catch (StorageNotAvailableException $e) { | ||
| throw new ServiceUnavailable("File is not updatable: " . $e->getMessage()); | ||
| } | ||
|
|
||
| // verify path of the target | ||
|
|
@@ -110,14 +110,31 @@ public function put($data) { | |
| $partFilePath = $this->path; | ||
| } | ||
|
|
||
| /** @var \OC\Files\Storage\Storage $storage */ | ||
| list($storage, $internalPartPath) = $this->fileView->resolvePath($partFilePath); | ||
| list(, $internalPath) = $this->fileView->resolvePath($this->path); | ||
| try { | ||
| $target = $storage->fopen($internalPartPath, 'wb'); | ||
| if ($target === false) { | ||
| \OC_Log::write('webdav', '\OC\Files\Filesystem::fopen() failed', \OC_Log::ERROR); | ||
| $this->fileView->unlink($partFilePath); | ||
| // because we have no clue about the cause we can only throw back a 500/Internal Server Error | ||
| throw new Exception('Could not write file contents'); | ||
| } | ||
| list($count, ) = \OC_Helper::streamCopy($data, $target); | ||
|
There was a problem hiding this comment. I was thinking whether we should check |
||
| fclose($target); | ||
|
|
||
| // if content length is sent by client: | ||
| // double check if the file was fully received | ||
| // compare expected and actual size | ||
| if (isset($_SERVER['CONTENT_LENGTH']) && $_SERVER['REQUEST_METHOD'] !== 'LOCK') { | ||
| $expected = $_SERVER['CONTENT_LENGTH']; | ||
| if ($count != $expected) { | ||
| $storage->unlink($internalPartPath); | ||
| throw new BadRequest('expected filesize ' . $expected . ' got ' . $count); | ||
| } | ||
| } | ||
|
|
||
| } catch (NotPermittedException $e) { | ||
| // a more general case - due to whatever reason the content could not be written | ||
| throw new Forbidden($e->getMessage()); | ||
|
|
@@ -138,56 +155,64 @@ public function put($data) { | |
| // returning 503 will allow retry of the operation at a later point in time | ||
| throw new ServiceUnavailable("Encryption not ready: " . $e->getMessage()); | ||
| } catch (StorageNotAvailableException $e) { | ||
| throw new ServiceUnavailable("Failed to write file contents: " . $e->getMessage()); | ||
| } | ||
|
|
||
| try { | ||
| if ($needsPartFile) { | ||
| // rename to correct path | ||
| try { | ||
| $renameOkay = $storage->rename($internalPartPath, $internalPath); | ||
| $fileExists = $storage->file_exists($internalPath); | ||
| if ($renameOkay === false || $fileExists === false) { | ||
| \OC_Log::write('webdav', '\OC\Files\Filesystem::rename() failed', \OC_Log::ERROR); | ||
| $this->fileView->unlink($partFilePath); | ||
| throw new Exception('Could not rename part file to final file'); | ||
| } | ||
| } catch (\OCP\Files\LockNotAcquiredException $e) { | ||
| // the file is currently being written to by another process | ||
| throw new FileLocked($e->getMessage(), $e->getCode(), $e); | ||
| } | ||
| } | ||
|
|
||
| // since we skipped the view we need to scan and emit the hooks ourselves | ||
| $storage->getScanner()->scanFile($internalPath); | ||
|
|
||
| $view = \OC\Files\Filesystem::getView(); | ||
| if ($view) { | ||
| $hookPath = $view->getRelativePath($this->fileView->getAbsolutePath($this->path)); | ||
| if (!$exists) { | ||
| \OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_create, array( | ||
| \OC\Files\Filesystem::signal_param_path => $hookPath | ||
| )); | ||
| } else { | ||
| \OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_update, array( | ||
| \OC\Files\Filesystem::signal_param_path => $hookPath | ||
| )); | ||
| } | ||
| \OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_write, array( | ||
| \OC\Files\Filesystem::signal_param_path => $hookPath | ||
| )); | ||
| } | ||
|
|
||
| // allow sync clients to send the mtime along in a header | ||
| $request = \OC::$server->getRequest(); | ||
| if (isset($request->server['HTTP_X_OC_MTIME'])) { | ||
| if ($this->fileView->touch($this->path, $request->server['HTTP_X_OC_MTIME'])) { | ||
| header('X-OC-MTime: accepted'); | ||
| } | ||
| } | ||
| $this->refreshInfo(); | ||
| } catch (StorageNotAvailableException $e) { | ||
| throw new ServiceUnavailable("Failed to check file size: " . $e->getMessage()); | ||
| } | ||
|
|
||
| return '"' . $this->info->getEtag() . '"'; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the data | ||
| * | ||
| * @return string|resource | ||
| * @throws Forbidden | ||
| * @throws ServiceUnavailable | ||
|
|
@@ -201,12 +226,13 @@ public function get() { | |
| // returning 503 will allow retry of the operation at a later point in time | ||
| throw new ServiceUnavailable("Encryption not ready: " . $e->getMessage()); | ||
| } catch (StorageNotAvailableException $e) { | ||
| throw new ServiceUnavailable("Failed to open file: " . $e->getMessage()); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Delete the current file | ||
| * | ||
| * @throws Forbidden | ||
| * @throws ServiceUnavailable | ||
| */ | ||
|
|
@@ -221,7 +247,7 @@ public function delete() { | |
| throw new Forbidden(); | ||
| } | ||
| } catch (StorageNotAvailableException $e) { | ||
| throw new ServiceUnavailable("Failed to unlink: " . $e->getMessage()); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -236,7 +262,7 @@ public function getContentType() { | |
| $mimeType = $this->info->getMimetype(); | ||
|
|
||
| // PROPFIND needs to return the correct mime type, for consistency with the web UI | ||
| if (isset($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PROPFIND') { | ||
| return $mimeType; | ||
| } | ||
| return \OC_Helper::getSecureMimeType($mimeType); | ||
|
|
@@ -266,8 +292,7 @@ public function getDirectDownload() { | |
| * @throws NotImplemented | ||
| * @throws ServiceUnavailable | ||
| */ | ||
| private function createFileChunked($data) { | ||
|
There was a problem hiding this comment. @icewind1991 looks like this part also needs to be changed to work directly on the storage, at least when writing the part file. Or is it intentional/not possible ? There was a problem hiding this comment. It should be changed yes but the logic is different enough that I dont want to do it in this PR |
||
| list($path, $name) = \Sabre\HTTP\URLUtil::splitPath($this->path); | ||
|
|
||
| $info = \OC_FileChunking::decodeName($name); | ||
|
|
@@ -278,7 +303,7 @@ private function createFileChunked($data) | |
| $bytesWritten = $chunk_handler->store($info['index'], $data); | ||
|
|
||
| //detect aborted upload | ||
| if (isset ($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PUT') { | ||
| if (isset($_SERVER['CONTENT_LENGTH'])) { | ||
| $expected = $_SERVER['CONTENT_LENGTH']; | ||
| if ($bytesWritten != $expected) { | ||
|
|
@@ -290,7 +315,7 @@ private function createFileChunked($data) | |
| } | ||
|
|
||
| if ($chunk_handler->isComplete()) { | ||
| list($storage,) = $this->fileView->resolvePath($path); | ||
| $needsPartFile = $this->needsPartFile($storage); | ||
|
|
||
| try { | ||
|
|
@@ -319,15 +344,15 @@ private function createFileChunked($data) | |
| // allow sync clients to send the mtime along in a header | ||
| $request = \OC::$server->getRequest(); | ||
| if (isset($request->server['HTTP_X_OC_MTIME'])) { | ||
| if ($this->fileView->touch($targetPath, $request->server['HTTP_X_OC_MTIME'])) { | ||
| header('X-OC-MTime: accepted'); | ||
| } | ||
| } | ||
|
|
||
| $info = $this->fileView->getFileInfo($targetPath); | ||
| return $info->getEtag(); | ||
| } catch (StorageNotAvailableException $e) { | ||
| throw new ServiceUnavailable("Failed to put file: " . $e->getMessage()); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -338,13 +363,14 @@ private function createFileChunked($data) | |
| * Returns whether a part file is needed for the given storage | ||
| * or whether the file can be assembled/uploaded directly on the | ||
| * target storage. | ||
| * | ||
| * @param \OCP\Files\Storage $storage | ||
| * @return bool true if the storage needs part file handling | ||
| */ | ||
| private function needsPartFile($storage) { | ||
| // TODO: in the future use ChunkHandler provided by storage | ||
| // and/or add method on Storage called "needsPartFile()" | ||
| return !$storage->instanceOfStorage('OCA\Files_Sharing\External\Storage') && | ||
| !$storage->instanceOfStorage('OC\Files\Storage\OwnCloud'); | ||
| } | ||
| } | ||
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
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.
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.
I don't see any code change that would pass a string for $data.
In what new situation can this happen ?
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.
unit tests
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.
can you make it so that the unit tests do not require adding this here ? AFAIK it should be possible to create a dummy stream or a stream based on a local temp file.