Use dav versions in files #29257

DeepDiver1975 · 2017-10-16T11:28:15Z

Description

This PR is based on #29207 and integrated files_versions ui via webdav

Related Issue

#29087

Motivation and Context

We shall use the webdav api for versioning

How Has This Been Tested?

manual testing in files app
cadaver

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

PVince81

Looks good so far.

I didn't remember seeing tests for the new Sabre nodes, please add some. (I expect codecov will complain about this).

Can you clarify the part about Webdav COPY vs $storage->restoreVersion. If they are connected, does the error message propagate properly when restoring to another file than the original one ? (no HTTP 500).

PVince81 · 2017-10-23T12:13:49Z

apps/dav/lib/DAV/CorePlugin.php

+use Sabre\HTTP\RequestInterface;
+use Sabre\HTTP\ResponseInterface;
+
+class CorePlugin extends ServerPlugin {


please rename to something that says "plugin to avoid deletion on copy" or something. CorePlugin is the wrong name for this. Also please add PHPDoc to describe this.

PVince81 · 2017-10-23T12:14:55Z

apps/dav/lib/DAV/CorePlugin.php

+		if ($sourceNode instanceof ICopySource) {
+			$copySuccess = $sourceNode->copy($destinationNode->getFileInfo()->getPath());
+		}
+		if (!$copySuccess) {


wouldn't doing a simple return here let the actual CorePlugin do the fallback ? this would remove the need for duplicating the response related code below

No because the fall back will still perform the delete of the target.

PVince81 · 2017-10-23T12:15:07Z

apps/dav/lib/Files/ICopySource.php

+namespace OCA\DAV\Files;
+
+
+interface ICopySource {


PVince81 · 2017-10-23T12:15:25Z

apps/dav/lib/Files/ICopySource.php

+	 * @param string $destinationPath
+	 * @return boolean
+	 */
+	public function copy($destinationPath);


Write a short sentence for the PHPDoc, at least on the interface.

PVince81 · 2017-10-23T12:15:40Z

apps/dav/lib/Meta/MetaFile.php

+use OCA\DAV\Files\ICopySource;
+use Sabre\DAV\File;
+
+class MetaFile extends File implements ICopySource {


what's a MetaFile ? => PHPDoc

PVince81 · 2017-10-23T12:22:58Z

core/js/files/client.js

+		 * @param {Object} [headers=null] additional headers
+		 *
+		 * @return {Promise} promise
+		 */


@since 10.0.4 ?

PVince81 · 2017-10-23T12:23:43Z

lib/private/Files/Meta/MetaFileVersionNode.php

+
+	public function copy($targetPath) {
+		//TODO: inject
+		$target = \OC::$server->getRootFolder()->get($targetPath);


can we inject the root folder ? (lazy root folder if possible)

PVince81 · 2017-10-23T12:25:04Z

apps/dav/lib/DAV/CorePlugin.php

+
+		$copySuccess = false;
+		if ($sourceNode instanceof ICopySource) {
+			$copySuccess = $sourceNode->copy($destinationNode->getFileInfo()->getPath());


Do I understand it correctly that doing a Webdav COPY is not the same as calling $storage->restoreVersion() ?
If it was I'd expect to have some validation to make sure the COPY is operating on the same storage + target file and fail if it doesn't.

Do I understand it correctly that doing a Webdav COPY is not the same as calling $storage->restoreVersion() ?

within the implementation of copy() restoreVersion() is called if applicable

PVince81 · 2017-10-23T12:25:49Z

lib/private/Files/Meta/MetaVersionCollection.php

+		/** @var IVersionedStorage | Storage $storage */
+		$versions = $storage->getVersions($internalPath);
+		return array_map(function($version) use ($storage, $internalPath) {
+			return new MetaFileVersionNode($this, $version['version'],$storage, $internalPath);


missing space ],$storage

PVince81 · 2017-10-23T12:28:09Z

lib/public/Files/Storage/IVersionedStorage.php

+ * Interface IVersionedStorage - storage layer to access version of a file
+ *
+ * @package OCP\Files\Storage
+ * @since 10.1.0


so no backports then?

we need to make a decision on this - since no database change is involved we can add this to 10.0.x as well

PVince81 · 2017-11-07T10:01:54Z

some known troubles with multiple mounts: #29490

codecov · 2017-11-08T17:23:51Z

Codecov Report

Merging #29257 into master will increase coverage by 0.04%.
The diff coverage is 67.7%.

@@             Coverage Diff              @@
##             master   #29257      +/-   ##
============================================
+ Coverage     61.71%   61.76%   +0.04%     
- Complexity    17482    17501      +19     
============================================
  Files          1047     1045       -2     
  Lines         57714    57712       -2     
============================================
+ Hits          35619    35644      +25     
+ Misses        22095    22068      -27

Impacted Files	Coverage Δ	Complexity Δ
apps/files_versions/appinfo/routes.php	`75% <ø> (-15%)`	`0 <0> (ø)`
apps/dav/lib/Connector/Sabre/Node.php	`75% <ø> (ø)`	`49 <0> (ø)`	⬇️
apps/dav/lib/Meta/MetaFile.php	`0% <0%> (ø)`	`13 <6> (+6)`	⬆️
apps/dav/lib/Meta/MetaFolder.php	`0% <0%> (ø)`	`7 <3> (ø)`	⬇️
apps/dav/lib/Meta/RootCollection.php	`30.76% <0%> (-5.6%)`	`6 <0> (+1)`
lib/private/Preview.php	`80.29% <0%> (-0.18%)`	`165 <0> (+1)`
lib/private/Files/Meta/MetaVersionCollection.php	`71.42% <100%> (-1.55%)`	`12 <1> (-2)`
apps/dav/lib/Connector/Sabre/FilesPlugin.php	`87.01% <100%> (+1.2%)`	`48 <0> (+3)`	⬆️
apps/dav/lib/Connector/Sabre/Server.php	`100% <100%> (ø)`	`1 <0> (ø)`	⬇️
lib/private/Files/Meta/MetaFileVersionNode.php	`88.88% <75%> (+18.88%)`	`18 <2> (+3)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ff6e28...630b0a0. Read the comment docs.

DeepDiver1975 · 2017-11-09T16:57:07Z

@PVince81 please review and test - THX

DeepDiver1975 · 2017-11-15T11:15:01Z

apps/files_versions/tests/VersioningTest.php

@@ -118,57 +118,6 @@ protected function tearDown() {
 		parent::tearDown();
 	}

-
-	public function testMoveFileIntoSharedFolderAsRecipient() {


removed - access to source is checked on the meta node level

DeepDiver1975 · 2017-11-15T11:15:13Z

apps/files_versions/tests/VersioningTest.php

@@ -430,106 +379,6 @@ public function testMoveFolder() {
 	}


-	public function testMoveFolderIntoSharedFolderAsRecipient() {


removed - access to source is checked on the meta node level

DeepDiver1975 · 2017-11-15T11:15:20Z

apps/files_versions/tests/VersioningTest.php

-		\OC::$server->getShareManager()->deleteShare($share);
-	}
-
-	public function testRenameSharedFile() {


removed - access to source is checked on the meta node level

DeepDiver1975 · 2017-11-15T11:15:45Z

lib/private/Files/Meta/MetaFileVersionNode.php

@@ -102,6 +103,9 @@ public function getContent() {
 	public function copy($targetPath) {
 		$target = $this->root->get($targetPath);
 		if ($target instanceof File && $target->getId() === $this->parent->getId()) {
+			if (!$target->isUpdateable()) {


this is where the write access is verified

DeepDiver1975 · 2017-11-15T11:16:26Z

lib/private/Preview.php

@@ -1334,8 +1334,9 @@ public static function post_delete_versions($args) {
 	 */
 	public static function post_delete($args, $prefix = '') {
 		$path = Files\Filesystem::normalizePath($args['path']);
+		$user = isset($args['user']) ? $args['user'] : \OC_User::getUser();


the user is now part of the hook arguments to make sure the correct user is used

Wow, our code can be quite obscure sometimes. I was confused when seeing that you extended the rollback event to include user, but this method is called post_delete.

Turns out that hook registration in base.php is wiring the "rollback" event to the "post_delete_versions" method here.

PVince81 · 2017-11-17T09:18:42Z

apps/files_versions/lib/Storage.php

-
-			$versionCreated = false;
+	public static function restoreVersion($uid, $filename, $fileToRestore, $revision) {
+		if(\OCP\Config::getSystemValue('files_versions', Storage::DEFAULTENABLED) !== true) {


old check was against string "true" and you replaced it with boolean true. I hope this won't break existing configs.

Of course, I'd expect the original code to use booleans...

Storage::DEFAULTENABLED is true and not 'true' and the old code used == and not ===/!==

PVince81 · 2017-11-17T09:39:21Z

BUG: encryption + share from a system-wide ext storage, available for admin only (not recipient): as share recipient, revert file to older version: preview does not update on-disk even after page refresh.

It does work for local files.

DeepDiver1975 · 2017-11-17T09:42:53Z

for previews there is a follow up pr - #29319

PVince81 · 2017-11-17T09:44:47Z

@DeepDiver1975 does the follow up solve the problem ? I had the impression that it's a hook related bug when using ext storage + share, since regular scenarios work already.

If you intend to fix it in that PR, please add it as checklist item there.

DeepDiver1975 · 2017-11-17T09:49:09Z

@DeepDiver1975 does the follow up solve the problem ? I had the impression that it's a hook related bug when using ext storage + share, since regular scenarios work already.

If you intend to fix it in that PR, please add it as checklist item there.

The whole preview generation will be changed - fixing anything in here is useless work.
As example: I'm currently integrating objectstore versioning based on this branch - no previews are displayed at all.

PVince81 · 2017-11-17T09:49:58Z

Thanks for clarification. I wasn't aware that the DAV previews would also touch the lower levels.

Sounds good then.

PVince81

👍

DeepDiver1975 · 2017-11-17T09:50:32Z

THX

PVince81 · 2017-11-17T09:51:30Z

@DeepDiver1975 can you backport ICopySource to stable10 so we can fix #29561 ?

cortho · 2017-11-29T09:50:51Z

@DeepDiver1975 regarding the change of this method:
https://github.com/owncloud/core/pull/29257/files#diff-efc44835a2c59b3707c965b80a3d669e
will files_versions then have a new version in info.xml (currently 1.3.0)?

DeepDiver1975 · 2017-11-29T11:27:35Z

will files_versions then have a new version in info.xml (currently 1.3.0)?

I see no reason to bump the version. Are you asking to find out about the API change?

cortho · 2017-11-29T11:30:45Z

ok.

Are you asking to find out about the API change?

yes, this was the idea behind

Edit: formatting

lock · 2019-08-01T20:02:41Z

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.

DeepDiver1975 added the 2 - Developing label Oct 16, 2017

DeepDiver1975 added this to the development milestone Oct 16, 2017

DeepDiver1975 self-assigned this Oct 16, 2017

DeepDiver1975 requested a review from PVince81 October 16, 2017 11:28

DeepDiver1975 force-pushed the use-dav-versions-in-files branch 4 times, most recently from 878c833 to 52cdaa2 Compare October 19, 2017 20:11

DeepDiver1975 changed the title ~~[WIP] Use dav versions in files~~ Use dav versions in files Oct 20, 2017

DeepDiver1975 added 3 - To Review and removed 2 - Developing labels Oct 20, 2017

This was referenced Oct 23, 2017

Ability to Manage Version Branches #23002

Closed

Public API to retrieve/manage versions #10571

Closed

PVince81 reviewed Oct 23, 2017

View reviewed changes

DeepDiver1975 force-pushed the use-dav-versions-in-files branch 3 times, most recently from e40074b to 105d4d4 Compare October 30, 2017 15:45

DeepDiver1975 changed the base branch from master to feature/29087 November 2, 2017 12:51

DeepDiver1975 force-pushed the use-dav-versions-in-files branch from 2b8e059 to 1ecfc92 Compare November 2, 2017 13:21

DeepDiver1975 changed the base branch from feature/29087 to master November 2, 2017 13:30

DeepDiver1975 force-pushed the use-dav-versions-in-files branch 2 times, most recently from ec59013 to 8654b6c Compare November 2, 2017 14:41

PVince81 modified the milestones: development, planned Nov 3, 2017

DeepDiver1975 force-pushed the use-dav-versions-in-files branch from 8654b6c to 26ec180 Compare November 8, 2017 12:01

owncloud deleted a comment from codecov bot Nov 8, 2017

DeepDiver1975 force-pushed the use-dav-versions-in-files branch from 26ec180 to 18dd0ef Compare November 9, 2017 13:44

DeepDiver1975 added 4 commits November 15, 2017 12:13

Migrate files versions frontend to use dav for versions

503e675

Enhance tests

5296aa2

Add Content-Disposition header to GET responses of versions

3028bbb

Fix version restore of shared and encrypted external storage

630b0a0

DeepDiver1975 force-pushed the use-dav-versions-in-files branch from f959060 to 630b0a0 Compare November 15, 2017 11:13

DeepDiver1975 commented Nov 15, 2017

View reviewed changes

PVince81 reviewed Nov 17, 2017

View reviewed changes

PVince81 approved these changes Nov 17, 2017

View reviewed changes

PVince81 merged commit 7c1889f into master Nov 17, 2017

PVince81 deleted the use-dav-versions-in-files branch November 17, 2017 09:50

ownclouders added the status/STALE label Dec 20, 2017

SamuAlfageme mentioned this pull request Mar 14, 2018

File Versions on the Client: Use the Server API owncloud/client#4556

Open

felix-schwarz mentioned this pull request Mar 14, 2018

Versioning support owncloud/ios-sdk#10

Open

DeepDiver1975 mentioned this pull request Apr 9, 2018

[stable10] Backport all objectstore related changes #31050

Merged

8 tasks

lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019

		@@ -430,106 +379,6 @@ public function testMoveFolder() {
		}


		public function testMoveFolderIntoSharedFolderAsRecipient() {

		namespace OCA\DAV\Files;


		interface ICopySource {

Use dav versions in files #29257

Use dav versions in files #29257

Conversation

DeepDiver1975 commented Oct 16, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

PVince81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Nov 7, 2017

codecov bot commented Nov 8, 2017 • edited Loading

Codecov Report

DeepDiver1975 commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Nov 17, 2017 • edited Loading

DeepDiver1975 commented Nov 17, 2017

PVince81 commented Nov 17, 2017

DeepDiver1975 commented Nov 17, 2017

PVince81 commented Nov 17, 2017

PVince81 left a comment

Choose a reason for hiding this comment

DeepDiver1975 commented Nov 17, 2017

PVince81 commented Nov 17, 2017

cortho commented Nov 29, 2017

DeepDiver1975 commented Nov 29, 2017

cortho commented Nov 29, 2017 • edited Loading

lock bot commented Aug 1, 2019

DeepDiver1975 commented Oct 16, 2017 •

edited

Loading

codecov bot commented Nov 8, 2017 •

edited

Loading

PVince81 commented Nov 17, 2017 •

edited

Loading

cortho commented Nov 29, 2017 •

edited

Loading