Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sabre Update to 2.1 #14151

Merged
merged 4 commits into from Feb 25, 2015
Merged

Sabre Update to 2.1 #14151

merged 4 commits into from Feb 25, 2015

Conversation

@PVince81
Copy link
Contributor

@PVince81 PVince81 commented Feb 12, 2015

TODOs

  • fix connector classes
  • fix auth
  • ...

Regression testing

@@ -1,4 +1,6 @@
<?php
namespace OC\Connector\Sabre;

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Feb 12, 2015
Member

I'd be fine to kill the connector from the namespace and folder structure

This comment has been minimized.

@PVince81

PVince81 Feb 12, 2015
Author Contributor

Next time maybe, there's already enough to do to makes this work 😉

I'll namespacize the classes I touch though.

@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 12, 2015

  • use another approach for OC_xxx_Request class (getRawServerValue())
  • retest "Principal" stuff
  • double check "$propertyMap" in TagList
  • check anythng VObject related
@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 12, 2015

  • test: quota
@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 12, 2015

  • test: oc_properties
  • test: tagsplugin
@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 12, 2015

  • test: range requests

It looks like the methods "httpGet", etc have to be moved to a plugin now.

@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 12, 2015

@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 12, 2015

Oh, that is nice:

        if (!$this->server->enablePropfindDepthInfinity && $depth != 0) $depth = 1;
@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 12, 2015

  • double check all returned properties (because I removed our overridden methods and the Sabre Server code changed a lot)
@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 12, 2015

Looks like the "beforeGetProperties" approach isn't valid any more. Need to switch to another technique, maybe listenning to the "propFind" even on nodes.

  • move tags plugin to use "propFind" event instead
  • double check files plugin (need to return file id, etc) due to arch changes
@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 12, 2015

  • adapt custom props code to use "propFind" event as well
@evert
evert reviewed Feb 12, 2015
View changes
lib/private/connector/sabre/filesplugin.php Outdated
$this->server->subscribeEvent('beforeGetProperties', array($this, 'beforeGetProperties'));
$this->server->subscribeEvent('afterBind', array($this, 'sendFileIdHeader'));
$this->server->subscribeEvent('afterWriteContent', array($this, 'sendFileIdHeader'));
$this->server->on('beforeGetProperties', array($this, 'beforeGetProperties'));

This comment has been minimized.

@evert

evert Feb 12, 2015

Note that beforeGetProperties has been removed as of version 2.0. There's a new event called propFind that does what you need, and it's documented here:

http://sabre.io/dav/writing-plugins/

This comment has been minimized.

@PVince81

PVince81 Feb 12, 2015
Author Contributor

Thanks for the hint, already found it and implementing it now.

How about "beforeGetPropertiesForPath" ? That one was convenient for prefeteching data for multiple nodes in a go. Is that still possible ?

This comment has been minimized.

@evert

evert Feb 12, 2015

Yes, the idea here was that we simply still emit a propFind event. This event has an argument that's an instance of Sabre\DAV\PropFind. That object will have a depth property set to 1, which means that you'll need to prefetch the data. At least, that was the intention. I didn't test this myself!

@@ -14,7 +14,7 @@
use OCP\Files\StorageInvalidException;
use OCP\Files\StorageNotAvailableException;

class ObjectTree extends \Sabre\DAV\ObjectTree {
class ObjectTree extends \Sabre\DAV\Tree {

This comment has been minimized.

@evert

evert Feb 12, 2015

A point I wanted to make here is that "you might not need it anymore".

You only really use this optimize moves and copies, but that's now better handled within specific node classes (your Directory and File classes) by implementing Sabre\DAV\IMoveTarget.

This should allow you to get rid of this class entirely. I think

This comment has been minimized.

@PVince81

PVince81 Feb 12, 2015
Author Contributor

I'll try and evaluate whether we want to remove this directly, but only after everything else works again 😄

@evert
evert reviewed Feb 12, 2015
View changes
lib/private/connector/sabre/server.php Outdated
@@ -82,223 +81,10 @@ public function getHTTPRange() {
protected function httpGet($uri) {

This comment has been minimized.

@evert

evert Feb 12, 2015

This method will be ignored, as the http* methods have moved to Sabre\DAV\CorePlugin. Perhaps this functionality is better implemented as a plugin/event though

This comment has been minimized.

@PVince81

PVince81 Feb 12, 2015
Author Contributor

Is there a way to catch the request headers in a plugin and then replace them ? Here the goal was mostly to prevent the range request to reach the server when encryption is enabled.

This comment has been minimized.

@evert

evert Feb 12, 2015

Yes, you can do a beforeMethod which now gets a Sabre\Http\Request object, which has a removeHeader() function.

This comment has been minimized.

@PVince81

PVince81 Feb 12, 2015
Author Contributor

Sweet 😄

@evert
evert reviewed Feb 12, 2015
View changes
lib/private/connector/sabre/tagsplugin.php Outdated
$this->server->subscribeEvent('updateProperties', array($this, 'updateProperties'));
$this->server->on('beforeGetProperties', array($this, 'beforeGetProperties'));
$this->server->on('beforeGetPropertiesForPath', array($this, 'beforeGetPropertiesForPath'));
$this->server->on('updateProperties', array($this, 'updateProperties'));

This comment has been minimized.

@evert

evert Feb 12, 2015

This became the 'propPatch' event.

@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 12, 2015

  • evaluate whether our "Tree" implementation is still needed: seems we still need it. To remove it, need deeper analysis of the logic + perf analysis, and I don't want to spend too much time on this now.
@evert
evert reviewed Feb 12, 2015
View changes
lib/private/vobject/compoundproperty.php Outdated
@@ -26,7 +26,7 @@
* This class overrides \Sabre\VObject\Property::serialize() to not
* double escape commas and semi-colons in compound properties.
*/
class CompoundProperty extends \Sabre\VObject\Property\Compound {
class CompoundProperty extends \Sabre\VObject\Property\Text {

This comment has been minimized.

@evert

evert Feb 12, 2015

This existed to work around issues with vobject 2.1. These issues should no longer exist, so should be removed.

@evert
evert reviewed Feb 12, 2015
View changes
lib/private/vobject/stringproperty.php Outdated
@@ -28,7 +28,7 @@
* This class overrides \Sabre\VObject\Property::serialize() properly
* escape commas and semi-colons in string properties.
*/
class StringProperty extends \Sabre\VObject\Property {
class StringProperty extends \Sabre\VObject\Property\Text {

This comment has been minimized.

@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 12, 2015

  • get rid of VObject custom types (CompoundObject and the other one)
  • find replacement for "beforeGetPropertiesForPath" to pre-cache tags (as we can read multiple at the same time from the DB)
@PVince81 PVince81 force-pushed the update-sabre2.1 branch to c6ca07c Feb 12, 2015
@evert
Copy link

@evert evert commented Feb 12, 2015

I'm pretty sure you can integrate scrutinizer properly with github these days, so the issue comments are no longer needed. They're a bit useless!

@evert
Copy link

@evert evert commented Feb 12, 2015

Same with jenkins! So noisy!

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Feb 12, 2015

The jenkins github integration does not allow multiple commit statuses. Maybe one day .....

@evert
Copy link

@evert evert commented Feb 12, 2015

Very awesome to see that this is a 'negative diff' so far ;) More lines removed than added.

@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 13, 2015

  • put etag through PROPPATCH (formerly done with ugly oc_properties logic)
@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 13, 2015

  • put mtime through PROPPATCH (formerly done with ugly oc_properties logic)
@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 13, 2015

@evert if I implement the custom properties logic in a plugin (a backend of PropertyStorage), should I remove "implement IProperties" from the Node then ?

@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 13, 2015

  • abuse allprops to also return "oc:id", "oc:permissions" and "oc:size" (worked on master) raise ticket for clients to always explicitly request these attrs (they'll need anyway once they start using oc:favorite, tags, etc)
@evert
Copy link

@evert evert commented Feb 13, 2015

@evert if I implement the custom properties logic in a plugin (a backend of PropertyStorage), should I remove "implement IProperties" from the Node then ?

Keeping it is optional. The new property system is designed so multiple separate plugins can 'opt in' to handling storing or retrieval of certain properties. If you no longer have a need for IProperties in nodes, you can remove it.

@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 13, 2015

@DeepDiver1975 I will not try and refactor the chunking into plugins right now to reduce the risk of regressions. I think we anyway need to rethink the way we do chunking, maybe send headers instead of using magic file names that need to be parsed. (I tried rewriting the URL in a beforeMethod but it messes up other things, so no easy fix here)

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Feb 23, 2015

Not reproducible while debugging - sounds like a timing issue - I hate it when that 💩 happens

05466030

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Feb 23, 2015

I'll continue tomorrow ....

@evert Many thanks for your support on this! Much appreciated!

tired

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Feb 25, 2015

well - the errors as of above and no longer reproducible:

deepdiver@alien:~/Development/ownCloud/core [update-sabre2.1]$ litmus -k https://localhost/oc/core/remote.php/webdav/ admin admin
-> running `copymove':
 0. init.................. pass
 1. begin................. pass
 2. copy_init............. pass
 3. copy_simple........... pass
 4. copy_overwrite........ pass
 5. copy_nodestcoll....... pass
 6. copy_cleanup.......... pass
 7. copy_coll............. pass
 8. copy_shallow.......... pass
 9. move.................. pass
10. move_coll............. pass
11. move_cleanup.......... pass
12. finish................ pass
<- summary for `copymove': of 13 tests run: 13 passed, 0 failed. 100.0%
deepdiver@alien:~/Development/ownCloud/core [update-sabre2.1]$ gedit debug.log 
deepdiver@alien:~/Development/ownCloud/core [update-sabre2.1]$ gedit debug.log 
deepdiver@alien:~/Development/ownCloud/core [update-sabre2.1]$ rm debug.log 
deepdiver@alien:~/Development/ownCloud/core [update-sabre2.1]$ litmus -k https://localhost/oc/core/remote.php/webdav/ admin admin
-> running `copymove':
 0. init.................. pass
 1. begin................. pass
 2. copy_init............. pass
 3. copy_simple........... pass
 4. copy_overwrite........ pass
 5. copy_nodestcoll....... pass
 6. copy_cleanup.......... pass
 7. copy_coll............. pass
 8. copy_shallow.......... pass
 9. move.................. pass
10. move_coll............. pass
11. move_cleanup.......... pass
12. finish................ pass
<- summary for `copymove': of 13 tests run: 13 passed, 0 failed. 100.0%
deepdiver@alien:~/Development/ownCloud/core [update-sabre2.1]$ rm debug.log 
deepdiver@alien:~/Development/ownCloud/core [update-sabre2.1]$ litmus -k https://localhost/oc/core/remote.php/webdav/ admin admin
-> running `copymove':
 0. init.................. pass
 1. begin................. pass
 2. copy_init............. pass
 3. copy_simple........... pass
 4. copy_overwrite........ pass
 5. copy_nodestcoll....... pass
 6. copy_cleanup.......... pass
 7. copy_coll............. pass
 8. copy_shallow.......... pass
 9. move.................. pass
10. move_coll............. pass
11. move_cleanup.......... pass
12. finish................ pass
<- summary for `copymove': of 13 tests run: 13 passed, 0 failed. 100.0%
deepdiver@alien:~/Development/ownCloud/core [update-sabre2.1]$ 
@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Feb 25, 2015

All unit tests are successfully executed now as well

deepdiver@alien:~/Development/ownCloud/core [update-sabre2.1]$ litmus -k https://localhost/oc/core/remote.php/webdav/ admin admin
-> running `basic':
 0. init.................. pass
 1. begin................. pass
 2. options............... WARNING: server does not claim Class 2 compliance
    ...................... pass (with 1 warning)
 3. put_get............... pass
 4. put_get_utf8_segment.. pass
 5. put_no_parent......... pass
 6. mkcol_over_plain...... pass
 7. delete................ pass
 8. delete_null........... pass
 9. delete_fragment....... pass
10. mkcol................. pass
11. mkcol_again........... pass
12. delete_coll........... pass
13. mkcol_no_parent....... pass
14. mkcol_with_body....... pass
15. finish................ pass
<- summary for `basic': of 16 tests run: 16 passed, 0 failed. 100.0%
-> 1 warning was issued.
-> running `copymove':
 0. init.................. pass
 1. begin................. pass
 2. copy_init............. pass
 3. copy_simple........... pass
 4. copy_overwrite........ pass
 5. copy_nodestcoll....... pass
 6. copy_cleanup.......... pass
 7. copy_coll............. pass
 8. copy_shallow.......... pass
 9. move.................. pass
10. move_coll............. pass
11. move_cleanup.......... pass
12. finish................ pass
<- summary for `copymove': of 13 tests run: 13 passed, 0 failed. 100.0%
-> running `props':
 0. init.................. pass
 1. begin................. pass
 2. propfind_invalid...... pass
 3. propfind_invalid2..... pass
 4. propfind_d0........... pass
 5. propinit.............. pass
 6. propset............... pass
 7. propget............... pass
 8. propextended.......... pass
 9. propmove.............. pass
10. propget............... pass
11. propdeletes........... pass
12. propget............... pass
13. propreplace........... pass
14. propget............... pass
15. propnullns............ pass
16. propget............... pass
17. prophighunicode....... pass
18. propget............... pass
19. propremoveset......... pass
20. propget............... pass
21. propsetremove......... pass
22. propget............... pass
23. propvalnspace......... pass
24. propwformed........... pass
25. propinit.............. pass
26. propmanyns............ pass
27. propget............... pass
28. propcleanup........... pass
29. finish................ pass
<- summary for `props': of 30 tests run: 30 passed, 0 failed. 100.0%
-> running `locks':
 0. init.................. pass
 1. begin................. pass
 2. options............... WARNING: server does not claim Class 2 compliance
    ...................... pass (with 1 warning)
 3. precond............... SKIPPED (locking tests skipped,
server does not claim Class 2 compliance)
-> 1 test was skipped.
<- summary for `locks': of 3 tests run: 3 passed, 0 failed. 100.0%
-> 1 warning was issued.
-> running `http':
 0. init.................. pass
 1. begin................. pass
 2. expect100............. SKIPPED (skipping for SSL server)
 3. finish................ pass
-> 1 test was skipped.
<- summary for `http': of 3 tests run: 3 passed, 0 failed. 100.0%
@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Feb 25, 2015

@PVince81 from my pov this is ready to go - anything missing?

Reasoning:
- a WebDAV server is not required to implement locking support
- WebDAV Locking is know to break the sync algorithm
- the current lock implementation is known to be broken (locks are not moved if a file is moved, locks on shared files don't work)
@DeepDiver1975 DeepDiver1975 force-pushed the update-sabre2.1 branch from 46b6fc6 to b3de86d Feb 25, 2015
@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 25, 2015

I hereby agree with your commit that removes the locks 👍

@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 25, 2015

Jenkins PR: #14483

@scrutinizer-notifier
Copy link

@scrutinizer-notifier scrutinizer-notifier commented Feb 25, 2015

The inspection completed: 25 new issues, 74 updated code elements

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Feb 25, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9932/
Test PASSed.

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Feb 25, 2015

👍

DeepDiver1975 added a commit that referenced this pull request Feb 25, 2015
@DeepDiver1975 DeepDiver1975 merged commit 799e144 into master Feb 25, 2015
1 check passed
1 check passed
default Merged build finished.
Details
@DeepDiver1975 DeepDiver1975 deleted the update-sabre2.1 branch Feb 25, 2015
@evert
Copy link

@evert evert commented Feb 25, 2015

Hi guys:

  1. I would not take the earlier litmus failure lightly. If you don't have a reasonable explanation why it happened in the first place, I suspect there may be some race condition and I wouldn't ignore it.
  2. Locking support is indeed not required, but a lot of clients do want it to be there. OS X finder for example will only allow read-only access to webdav if there's no locking support.
@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Feb 25, 2015

Hi guys:

I would not take the earlier litmus failure lightly. If you don't have a reasonable explanation why it happened in the first place, I suspect there may be some race condition and I wouldn't ignore it.

Sure thing - but we need to go ahead as a hell of other tasks depend on this change.
Furthermore - having the code in master will give it some more test coverage and thus more reliability.

Locking support is indeed not required, but a lot of clients do want it to be there. OS X finder for example will only allow read-only access to webdav if there's no locking support.

Sure - but I prefer to lock a file the right way and not that crappy way we do today where we - in some case - pretend to lock but we are actually not locking at all.

@PVince81
Copy link
Contributor Author

@PVince81 PVince81 commented Feb 25, 2015

Does litmus do concurrency tests ? If not I don't see how a race condition could happen.

Ideally the "new" locking (if ever) should be based on file id for better accuracy even with shared files, but even then it requires stable file ids first.

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Feb 25, 2015

Does litmus do concurrency tests ?

no the tests are executed sequentially

@evert
Copy link

@evert evert commented Feb 25, 2015

Sure - but I prefer to lock a file the right way and not that crappy way we do today where we - in some case - pretend to lock but we are actually not locking at all.

I actually think that webdav locks are not really useful, so I was considering writing an alternative locks plugin that pretends to support locking, but doesn't actually do anything and requires no storage.

Clients really ought to be using ETags and such instead of doing locking.

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Feb 25, 2015

From my personal pov it highly depends on the workflow - with locking changes can be prevented before they actually did take place.
Depending on the ETag will result in different version - so what's next? Displaying a diff of versions to the user is not always possible.

@evert
Copy link

@evert evert commented Feb 25, 2015

The problem with locks and how clients generally implement it, is that they only do the LOCK moments before they do the actual PUT (or other operation) and then do an UNLOCK right after. This behavior has no benefits over using etags and if headers.

For locks to be useful, they really need to be controlled by the user before work starts and unlocked after completion,

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Feb 25, 2015

For locks to be useful, they really need to be controlled by the user before work starts and unlocked after completion,

This is indeed the workflow I have in mind - 🙊 this is how M$ Office is doing it afaik

LukasReschke added a commit that referenced this pull request May 14, 2015
As discussed in #14151, we missed to add this to the `files_sharing` S2S public WebDAV backend though.
mmattel added a commit to mmattel/core that referenced this pull request May 22, 2015
As discussed in owncloud#14151, we missed to add this to the `files_sharing` S2S public WebDAV backend though.
@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.