Fix quota with federated sharing #29220

PVince81 · 2017-10-12T11:05:22Z

Description

Fix quota with federated sharing and add tests for that.

quota check needs to ignore -3 (unlimited) quota value (returned by federated shares with no quota)
if unknown quota when checking a file on a federated share, check the parent
when not checking the parent, uploading a file to a federated share through DAV upload should still properly forward the insufficient storage exception and others

Related Issue

Fixes #29169

Motivation and Context

How Has This Been Tested?

Run federation_features/federated.feature:240 and quota.feature.

Screenshots (if appropriate):

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 · 2017-10-12T11:06:07Z

Note: I might want to move the DAV additions to a separate PR as I currently don't know how to properly automatically test this without commenting out the "check the parent instead" logic.

PVince81 · 2017-10-12T13:04:31Z

remove commented out code

At least this way we'll let the remote server decide in some cases whether there is enough quota when the quota is not retrievable. For that we need to properly forward the 507 error in the DAV code.

PVince81 · 2017-10-12T14:47:51Z

suspicious entries when running the tests:

[Thu Oct 12 16:45:52 2017] 127.0.0.1:51582 [201]: /public.php/webdav/testquota.txt-olddav-regular
[Thu Oct 12 16:45:53 2017] 127.0.0.1:51586 [207]: /public.php/webdav/testquota.txt-olddav-regular
[Thu Oct 12 16:45:53 2017] 127.0.0.1:51590 [207]: /public.php/webdav/
[Thu Oct 12 16:45:53 2017] Undefined index: size at /srv/www/htdocs/owncloud/lib/private/Files/View.php#1296

I didn't see this View warning on master, but also master doesn't have this one test.

PVince81 · 2017-10-12T14:59:44Z

Why can't I catch this with ??

diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php
index 3e4af7cf3d..9cb9bb482f 100644
--- a/lib/private/Files/View.php
+++ b/lib/private/Files/View.php
@@ -1294,6 +1294,10 @@ class View {
                try {
                        // if the file is not in the cache or needs to be updated, trigger the scanner and reload the data
                        if (!$data || $data['size'] === -1) {
+                               if (!!$data && !isset($data['size'])) {
+                                       $stack = json_encode(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 50));
+                                       \OCP\Util::writeLog('DEBUG', 'Size not set: Stack: ' . $stack, \OCP\Util::WARN);
+                               }
                                $this->lockFile($relativePath, ILockingProvider::LOCK_SHARED);

PVince81 · 2017-10-12T15:23:30Z

Stack trace:

[Thu Oct 12 17:26:49 2017] Undefined index: size at /srv/www/htdocs/owncloud/lib/private/Files/View.php#1296
[Thu Oct 12 17:26:49 2017] $data is {"checksum":"","mimetype":"application\/octet-stream","parent":"2147483987"}
[Thu Oct 12 17:26:49 2017] $internalPath: files_versions/PARENT (2)/testquota.txt-newdav-regular.v1507822008
[Thu Oct 12 17:26:49 2017] $relativePath: files_versions//PARENT (2)/testquota.txt-newdav-regular.v1507822008
[Thu Oct 12 17:26:49 2017] Size not set: Stack:
[
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/lib\\/private\\/Files\\/View.php",
    "line": 1353,
    "function": "getCacheEntry",
    "class": "OC\\Files\\View",
    "type": "->"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/apps\\/files_versions\\/lib\\/Storage.php",
    "line": 195,
    "function": "getFileInfo",
    "class": "OC\\Files\\View",
    "type": "->"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/apps\\/files_versions\\/lib\\/Hooks.php",
    "line": 60,
    "function": "store",
    "class": "OCA\\Files_Versions\\Storage",
    "type": "::"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/lib\\/private\\/legacy\\/hook.php",
    "line": 105,
    "function": "write_hook",
    "class": "OCA\\Files_Versions\\Hooks",
    "type": "::"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/apps\\/dav\\/lib\\/Connector\\/Sabre\\/File.php",
    "line": 283,
    "function": "emit",
    "class": "OC_Hook",
    "type": "::"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/apps\\/dav\\/lib\\/Connector\\/Sabre\\/File.php",
    "line": 189,
    "function": "emitPreHooks",
    "class": "OCA\\DAV\\Connector\\Sabre\\File",
    "type": "->"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/apps\\/dav\\/lib\\/Connector\\/Sabre\\/Directory.php",
    "line": 162,
    "function": "put",
    "class": "OCA\\DAV\\Connector\\Sabre\\File",
    "type": "->"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/lib\\/composer\\/sabre\\/dav\\/lib\\/DAV\\/Server.php",
    "line": 1095,
    "function": "createFile",
    "class": "OCA\\DAV\\Connector\\Sabre\\Directory",
    "type": "->"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/lib\\/composer\\/sabre\\/dav\\/lib\\/DAV\\/CorePlugin.php",
    "line": 525,
    "function": "createFile",
    "class": "Sabre\\DAV\\Server",
    "type": "->"
  },
  {
    "function": "httpPut",
    "class": "Sabre\\DAV\\CorePlugin",
    "type": "->"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/lib\\/composer\\/sabre\\/event\\/lib\\/EventEmitterTrait.php",
    "line": 105,
    "function": "call_user_func_array"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/lib\\/composer\\/sabre\\/dav\\/lib\\/DAV\\/Server.php",
    "line": 479,
    "function": "emit",
    "class": "Sabre\\Event\\EventEmitter",
    "type": "->"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/lib\\/composer\\/sabre\\/dav\\/lib\\/DAV\\/Server.php",
    "line": 254,
    "function": "invokeMethod",
    "class": "Sabre\\DAV\\Server",
    "type": "->"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/apps\\/dav\\/lib\\/Server.php",
    "line": 236,
    "function": "exec",
    "class": "Sabre\\DAV\\Server",
    "type": "->"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/apps\\/dav\\/appinfo\\/v2\\/remote.php",
    "line": 31,
    "function": "exec",
    "class": "OCA\\DAV\\Server",
    "type": "->"
  },
  {
    "file": "\\/srv\\/www\\/htdocs\\/owncloud\\/remote.php",
    "line": 175,
    "args": [
      "\\/srv\\/www\\/htdocs\\/owncloud\\/apps\\/dav\\/appinfo\\/v2\\/remote.php"
    ],
    "function": "require_once"
  }
]

Seems to be about versions...

PVince81 · 2017-10-13T13:19:31Z

If I remove the quota from the integration test, the warning disappears... Weird.

PVince81 · 2017-10-13T13:26:26Z

also if I set the quota high enough in the test, the warning disappears as well.

PVince81 · 2017-10-13T13:30:54Z

If I revert every code change and keep the test, the error appears... so this existed before.

PVince81 · 2017-10-13T13:50:09Z

I've raised #29245 to explain and analyze this bug. It seems unrelated to this PR. It's just that adding the new tests brought it into view.

PVince81 · 2017-10-13T14:00:42Z

Ok, now that we know the warning is unrelated, let's focus on finishing this.

jvillafanez

Code looks good, but tests are failing

PVince81 · 2017-10-16T08:53:10Z

Relevant failing tests:

19:19:53     /var/lib/jenkins/workspace/owncloud-core_core_PR-29220-7DOBZKU5OFHAVV4BVOWUOKNOOB2WEMRXAXPN3EVSUD4HDB6XAU2Q/tests/integration/features/external-storage.feature:63
19:19:53     /var/lib/jenkins/workspace/owncloud-core_core_PR-29220-7DOBZKU5OFHAVV4BVOWUOKNOOB2WEMRXAXPN3EVSUD4HDB6XAU2Q/tests/integration/features/sharing-v1.feature:597

PVince81 · 2017-10-16T12:24:11Z

Ha... failing tests are failing because I added an extra check to assert that setting the quota worked. And apparently here it didn't but the test still passed.

PVince81 · 2017-10-16T14:35:29Z

stable10: #29259

codecov · 2017-10-18T13:07:29Z

Codecov Report

Merging #29220 into master will increase coverage by 0.6%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master   #29220     +/-   ##
===========================================
+ Coverage     59.59%    60.2%   +0.6%     
- Complexity    17169    17182     +13     
===========================================
  Files          1029     1030      +1     
  Lines         57235    57270     +35     
===========================================
+ Hits          34111    34477    +366     
+ Misses        23124    22793    -331

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Server.php	`82.44% <100%> (+0.34%)`	`118 <2> (+3)`	⬆️
lib/private/Files/Storage/DAV.php	`88.81% <100%> (+77.75%)`	`163 <5> (+2)`	⬆️
lib/private/Http/Client/WebdavClientService.php	`100% <100%> (ø)`	`7 <7> (?)`
apps/dav/lib/Connector/Sabre/QuotaPlugin.php	`88.52% <100%> (ø)`	`26 <0> (+1)`	⬆️
lib/private/Security/CertificateManager.php	`94.5% <0%> (+1.09%)`	`36% <0%> (ø)`	⬇️

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 3103c29...4ba6cda. Read the comment docs.

PVince81 · 2017-10-19T14:34:11Z

I went through great lengths to make codecov happy: 5b5689c

make the DAV class testable by introducing a WebdavClientService and IWebdavClientService in core to be able to mock an instance of the Sabre Client
add tests for the exceptions

Now waiting to see the new coverage and I'll continue adding tests for that.

PVince81 · 2017-10-19T15:50:03Z

Added moar tests

PVince81 · 2017-10-20T11:29:52Z

Let's watch the test coverage for the DAV class increase now!

PVince81 · 2017-10-20T14:56:56Z

@jvillafanez @DeepDiver1975 please review again. To make codecov happy I had to go to great length and bootstrap unit tests for the Storage\DAV class. In my way of doing this I decided to cover all methods and discovered some small issues on the way which I fixed right away, some others which I will raise later.

PVince81 · 2017-10-20T14:58:35Z

tests/lib/Files/Storage/DavTest.php

+ * ownCloud
+ *
+ * @author Robin Appelman
+ * @copyright 2012 Robin Appelman icewind@owncloud.com


going to fix that right away...

Properly detect SPACE_UNLIMITED in quota check. Improve error handling when receiving errors like 507 when uploading files to a remote server.

PVince81 · 2017-10-20T15:06:19Z

Rebased and squashed the DAV test (+fix) commits together into one.

DAV.php coverage is now 88%

jvillafanez · 2017-10-23T08:58:53Z

lib/private/Http/Client/WebdavClientService.php

+	/**
+	 * Instantiate new Sabre client
+	 *
+	 * @param array $settings


Valid settings should be known both here and in the interface

Added tests for all FS methods of the Storage\DAV class and also fixed some related issues.

PVince81 · 2017-10-23T10:16:15Z

I've copied the Sabre client settings PHPDoc into the interface and WebdavClientService.

Since CI was all green and the PHPDoc will not affect it, I'm merging this directly.

DeepDiver1975 · 2017-10-23T10:18:50Z

I was still right in the middle of review ....

PVince81 · 2017-10-23T10:18:54Z

stable10: #29325

PVince81 · 2017-10-23T10:19:25Z

@DeepDiver1975 sorry, please continue then. I'll adjust in a subsequent PR.

DeepDiver1975 · 2017-10-23T10:10:38Z

lib/private/Http/Client/WebdavClientService.php

+ *
+ * @package OC\Http
+ */
+class WebdavClientService implements IWebdavClientService {


Should be named WebDav

DeepDiver1975 · 2017-10-23T10:13:03Z

tests/lib/Files/Storage/DavTest.php

+		$this->timeFactory = $this->createMock(ITimeFactory::class);
+		$this->overwriteService('TimeFactory', $this->timeFactory);
+
+		$this->httpClient = $this->createMock(IClient::class);


missing use of IClient

it's there: https://github.com/owncloud/core/pull/29220/files/4ba6cda1191234542d58f0b5be415392adca214b#diff-52c6f7ef7e46328fff29098af83d4b3cR26

DeepDiver1975 · 2017-10-23T10:21:43Z

tests/lib/Files/Storage/DavTest.php

+	}
+
+	/**
+	 * @expectedException OCP\Files\StorageNotAvailableException


namespace looks strange

DeepDiver1975 · 2017-10-23T10:23:15Z

tests/lib/Files/Storage/DavTest.php

+	private $webdavClientService;
+
+	/**
+	 * @var Client


please add | PHPUnit_Framework_MockObject_MockObject - this will fix issues with calling mock methods on these objects

DeepDiver1975 · 2017-10-23T10:24:07Z

@DeepDiver1975 sorry, please continue then. I'll adjust in a subsequent PR.

done

PVince81 · 2017-10-23T11:50:23Z

Follow up PR for the review items: #29327

PVince81 · 2017-11-08T09:49:31Z

regression: #29422

lock · 2019-08-02T00:02:35Z

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.

PVince81 added the 2 - Developing label Oct 12, 2017

PVince81 added this to the development milestone Oct 12, 2017

PVince81 self-assigned this Oct 12, 2017

PVince81 mentioned this pull request Oct 12, 2017

Federation: Upload reports insufficient storage but we are far from any limit #29169

Closed

PVince81 added 3 - To Review and removed 2 - Developing labels Oct 13, 2017

PVince81 force-pushed the fix-quota-fedsharing branch from 8d3dbf9 to b274f67 Compare October 13, 2017 14:41

PVince81 requested review from DeepDiver1975 and jvillafanez October 13, 2017 14:41

jvillafanez reviewed Oct 16, 2017

View reviewed changes

PVince81 added 4 - To release and removed 3 - To Review labels Oct 16, 2017

PVince81 mentioned this pull request Oct 16, 2017

[stable10] Fix quota with federated shares #29259

Closed

PVince81 force-pushed the fix-quota-fedsharing branch from ce0ea8f to 28fec80 Compare October 18, 2017 08:35

PVince81 added 3 - To Review and removed 4 - To release labels Oct 20, 2017

PVince81 commented Oct 20, 2017

View reviewed changes

Vincent Petry added 4 commits October 20, 2017 17:02

Catch errors when setting quota in integration tests

fc29c8a

Fix quota handling with federated shares

b8a765d

Properly detect SPACE_UNLIMITED in quota check. Improve error handling when receiving errors like 507 when uploading files to a remote server.

Fix integration tests to properly set quota in some cases

1592141

Update QuotaPluginTest for unlimited quota values

817ee41

PVince81 force-pushed the fix-quota-fedsharing branch from b15219c to 1a2903f Compare October 20, 2017 15:05

jvillafanez approved these changes Oct 23, 2017

View reviewed changes

Added WebdavClientService in core + unit tests for Storage\DAV class

4ba6cda

Added tests for all FS methods of the Storage\DAV class and also fixed some related issues.

PVince81 force-pushed the fix-quota-fedsharing branch from 1a2903f to 4ba6cda Compare October 23, 2017 10:15

PVince81 merged commit fe7802e into master Oct 23, 2017

PVince81 deleted the fix-quota-fedsharing branch October 23, 2017 10:16

PVince81 mentioned this pull request Oct 23, 2017

[stable10] Fix quota federated sharing (new) #29325

Merged

DeepDiver1975 reviewed Oct 23, 2017

View reviewed changes

PVince81 mentioned this pull request Oct 23, 2017

Small tweaks for WebDavClientService and tests #29327

Merged

PVince81 mentioned this pull request Nov 8, 2017

Fix Sabre\DAV\Client use statement for older PHP #29422

Merged

9 tasks

ownclouders added the status/STALE label Dec 20, 2017

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

Fix quota with federated sharing #29220

Fix quota with federated sharing #29220

Conversation

PVince81 commented Oct 12, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

PVince81 commented Oct 12, 2017

PVince81 commented Oct 12, 2017

PVince81 commented Oct 12, 2017

PVince81 commented Oct 12, 2017

PVince81 commented Oct 12, 2017 • edited Loading

PVince81 commented Oct 13, 2017

PVince81 commented Oct 13, 2017

PVince81 commented Oct 13, 2017

PVince81 commented Oct 13, 2017

PVince81 commented Oct 13, 2017

jvillafanez left a comment

Choose a reason for hiding this comment

PVince81 commented Oct 16, 2017

PVince81 commented Oct 16, 2017

PVince81 commented Oct 16, 2017

codecov bot commented Oct 18, 2017 • edited Loading

Codecov Report

PVince81 commented Oct 19, 2017

PVince81 commented Oct 19, 2017

PVince81 commented Oct 20, 2017

PVince81 commented Oct 20, 2017

PVince81 Oct 20, 2017

Choose a reason for hiding this comment

PVince81 commented Oct 20, 2017

jvillafanez Oct 23, 2017

Choose a reason for hiding this comment

PVince81 commented Oct 23, 2017

DeepDiver1975 commented Oct 23, 2017

PVince81 commented Oct 23, 2017

PVince81 commented Oct 23, 2017

DeepDiver1975 Oct 23, 2017

Choose a reason for hiding this comment

DeepDiver1975 Oct 23, 2017

Choose a reason for hiding this comment

PVince81 Oct 23, 2017

Choose a reason for hiding this comment

DeepDiver1975 Oct 23, 2017

Choose a reason for hiding this comment

DeepDiver1975 Oct 23, 2017

Choose a reason for hiding this comment

DeepDiver1975 commented Oct 23, 2017

PVince81 commented Oct 23, 2017

PVince81 commented Nov 8, 2017

lock bot commented Aug 2, 2019

PVince81 commented Oct 12, 2017 •

edited

Loading

codecov bot commented Oct 18, 2017 •

edited

Loading