Ocdav improvements

[C0rby]

Cleaned up the ocdav code to make it more readable and in one case a bit faster.

[C0rby]

@butonic, do you see any reason not to change the EncodePath to this: https://github.com/cs3org/reva/pull/2658/files?diff=unified&w=0#diff-14b3d240140c4f3987b05f5a2afcf2aff48b201f276ad52e419146bf4e2b7cbfR74?
Sure the output is a bit different than from the last implementation but this one is still compliant with the webdav rfc and quite a bit faster.

If there is no reason against it, we would need to update the acceptance tests

[dragotin]

Great one!

One question: Some functions are named with a "New"-Prefix, for example NewNotFoundNS. For me, the word New indicates that there happens an mem allocation but the functions are returning by value, right?
Is there any reason for the "New"-Prefix? For me, just not having it would be fully sufficient, in the example case that would just be props.NotFoundNS .

[C0rby]

One question: Some functions are named with a "New"-Prefix, for example NewNotFoundNS. For me, the word New indicates that there happens an mem allocation but the functions are returning by value, right? Is there any reason for the "New"-Prefix? For me, just not having it would be fully sufficient, in the example case that would just be props.NotFoundNS .

In Go the NewXXX() functions don't necessarily indicate memory allocations. It's just a way to say that you created a new instance of a struct and can use it. But I get where you're coming from since there is a function new which is used to allocate memory. I agree that we can remove the New prefix from those methods since it doesn't really add any context. 👍

[kiranparajuli589]

ocis-integration-tests failed at:

7505 | apiWebdavProperties2/getFileProperties.feature:47
7506 | apiWebdavProperties2/getFileProperties.feature:50
7507 | apiWebdavProperties2/getFileProperties.feature:51
7508 | apiWebdavProperties2/getFileProperties.feature:54
7509 | apiWebdavProperties2/getFileProperties.feature:59
7510 | apiWebdavProperties2/getFileProperties.feature:62
7511 | apiWebdavProperties2/getFileProperties.feature:79
7512 | apiWebdavProperties2/getFileProperties.feature:80
7513 | apiWebdavProperties2/getFileProperties.feature:83
7514 | apiWebdavProperties2/getFileProperties.feature:86
7515 | apiWebdavProperties2/getFileProperties.feature:87
7516 | apiWebdavProperties2/getFileProperties.feature:90
7517 | apiWebdavProperties2/getFileProperties.feature:97
7518 | apiWebdavProperties2/getFileProperties.feature:98
7519 | apiWebdavProperties2/getFileProperties.feature:101

failing about d:href

@issue-ocis-reva-214
--
4713 | Scenario Outline: Do a PROPFIND of various file names                                                              # /drone/src/tmp/testrunner/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature:38
4714 | Given using <dav_version> DAV path                                                                               # FeatureContext::usingOldOrNewDavPath()
4715 | And user "Alice" has uploaded file with content "uploaded content" to "<file_name>"                              # FeatureContext::userHasUploadedAFileWithContentTo()
4716 | When user "Alice" gets the properties of file "<file_name>" using the WebDAV API                                 # WebDavPropertiesContext::userGetsThePropertiesOfFolder()
4717 | Then the HTTP status code should be "201"                                                                        # FeatureContext::thenTheHTTPStatusCodeShouldBe()
4718 | And the properties response should contain an etag                                                               # WebDavPropertiesContext::thePropertiesResponseShouldContainAnEtag()
4719 | And the value of the item "//d:response/d:href" in the response to user "Alice" should match "/<expected_href>/" # WebDavPropertiesContext::assertValueOfItemInResponseToUserRegExp()
4720 |  
4721 | Examples:
4722 | \| dav_version \| file_name     \| expected_href                                                  \|
4723 | \| old         \| /C++ file.cpp \| remote\.php\/webdav\/C%2[bB]%2[bB]%20file\.cpp                 \|
4724 | item "//d:response/d:href" found with value "/remote.php/webdav/C++%20file.cpp", expected to match regex pattern: "/remote\.php\/webdav\/C%2[bB]%2[bB]%20file\.cpp/"
4725 | Failed asserting that '/remote.php/webdav/C++%20file.cpp' matches PCRE pattern "/remote\.php\/webdav\/C%2[bB]%2[bB]%20file\.cpp/".
4726 | \| old         \| /file #2.txt  \| remote\.php\/webdav\/file%20%232\.txt                          \|
4727 | \| old         \| /file ?2.txt  \| remote\.php\/webdav\/file%20%3[fF]2\.txt                       \|
4728 | \| old         \| /file &2.txt  \| remote\.php\/webdav\/file%20%262\.txt                          \|
4729 | item "//d:response/d:href" found with value "/remote.php/webdav/file%20&2.txt", expected to match regex pattern: "/remote\.php\/webdav\/file%20%262\.txt/"
4730 | Failed asserting that '/remote.php/webdav/file%20&2.txt' matches PCRE pattern "/remote\.php\/webdav\/file%20%262\.txt/".
4731 | \| new         \| /C++ file.cpp \| remote\.php\/dav\/files\/%username%\/C%2[bB]%2[bB]%20file\.cpp \|
4732 | item "//d:response/d:href" found with value "/remote.php/dav/files/Alice/C++%20file.cpp", expected to match regex pattern: "/remote\.php\/dav\/files\/Alice\/C%2[bB]%2[bB]%20file\.cpp/"
4733 | Failed asserting that '/remote.php/dav/files/Alice/C++%20file.cpp' matches PCRE pattern "/remote\.php\/dav\/files\/Alice\/C%2[bB]%2[bB]%20file\.cpp/".
4734 | \| new         \| /file #2.txt  \| remote\.php\/dav\/files\/%username%\/file%20%232\.txt          \|
4735 | \| new         \| /file ?2.txt  \| remote\.php\/dav\/files\/%username%\/file%20%3[fF]2\.txt       \|
4736 | \| new         \| /file &2.txt  \| remote\.php\/dav\/files\/%username%\/file%20%262\.txt          \|
4737 | item "//d:response/d:href" found with value "/remote.php/dav/files/Alice/file%20&2.txt", expected to match regex pattern: "/remote\.php\/dav\/files\/Alice\/file%20%262\.txt/"
4738 | Failed asserting that '/remote.php/dav/files/Alice/file%20&2.txt' matches PCRE pattern "/remote\.php\/dav\/files\/Alice\/file%20%262\.txt/".
4739 | \| spaces      \| /C++ file.cpp \| dav\/spaces\/%spaceid%\/C%2[bB]%2[bB]%20file\.cpp              \|
4740 | item "//d:response/d:href" found with value "/dav/spaces/5cfaabcc-3fa5-103c-96cf-773576e2cf3c/C++%20file.cpp", expected to match regex pattern: "/dav\/spaces\/5cfaabcc\-3fa5\-103c\-96cf\-773576e2cf3c\/C%2[bB]%2[bB]%20file\.cpp/"
4741 | Failed asserting that '/dav/spaces/5cfaabcc-3fa5-103c-96cf-773576e2cf3c/C++%20file.cpp' matches PCRE pattern "/dav\/spaces\/5cfaabcc\-3fa5\-103c\-96cf\-773576e2cf3c\/C%2[bB]%2[bB]%20file\.cpp/".
4742 | \| spaces      \| /file #2.txt  \| dav\/spaces\/%spaceid%\/file%20%232\.txt                       \|
4743 | \| spaces      \| /file ?2.txt  \| dav\/spaces\/%spaceid%\/file%20%3[fF]2\.txt                    \|
4744 | \| spaces      \| /file &2.txt  \| dav\/spaces\/%spaceid%\/file%20%262\.txt                       \|
4745 | item "//d:response/d:href" found with value "/dav/spaces/5e7d54f4-3fa5-103c-96d8-773576e2cf3c/file%20&2.txt", expected to match regex pattern: "/dav\/spaces\/5e7d54f4\-3fa5\-103c\-96d8\-773576e2cf3c\/file%20%262\.txt/"
4746 | Failed asserting that '/dav/spaces/5e7d54f4-3fa5-103c-96d8-773576e2cf3c/file%20&2.txt' matches PCRE pattern "/dav\/spaces\/5e7d54f4\-3fa5\-103c\-96d8\-773576e2cf3c\/file%20%262\.txt/".

[kiranparajuli589]

I will look and find more about the test failures.

[C0rby]

Yeah the href fails because the encoding changed a bit. But both the old and the new encoding are valid URL encodings.
To make the tests more stable I propose that we don't check the returned, encoded string but instead first decode it and then compare it.

Take /C++ file.cpp for example. The server returns /C++%20file.cpp which when decoded results in: /C++ file.cpp.

[kiranparajuli589]

Sure, I can make a PR for that. 👍

[kiranparajuli589]

@C0rby
here is owncloud/core pr to modify tests for href assertions
to check the ci you can use

CORE_COMMITID=ad47e05061880681186894ba6c01bd97988ce09a
CORE_BRANCH=assert-entry-href-after-decode

[kiranparajuli589]

hmm..dav paths do not have remote.php, so failing again https://drone.cernbox.cern.ch/cs3org/reva/6431/15/6

@issue-ocis-reva-214
--
4720 | Scenario Outline: Do a PROPFIND of various file names                                                 # /drone/src/tmp/testrunner/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature:38
4721 | Given using <dav_version> DAV path                                                                  # FeatureContext::usingOldOrNewDavPath()
4722 | And user "Alice" has uploaded file with content "uploaded content" to "<file_name>"                 # FeatureContext::userHasUploadedAFileWithContentTo()
4723 | When user "Alice" gets the properties of file "<file_name>" using the WebDAV API                    # WebDavPropertiesContext::userGetsThePropertiesOfFolder()
4724 | Then the HTTP status code should be "201"                                                           # FeatureContext::thenTheHTTPStatusCodeShouldBe()
4725 | And the properties response should contain an etag                                                  # WebDavPropertiesContext::thePropertiesResponseShouldContainAnEtag()
4726 | And there should be an entry with href containing "<expected_href>" in the response to user "Alice" # WebDavPropertiesContext::assertEntryWithHrefMatchingRegExpInResponseToUser()
4727 |  
4728 | Examples:
4729 | \| dav_version \| file_name     \| expected_href                                \|
4730 | \| old         \| /C++ file.cpp \| remote.php/webdav/C++ file.cpp               \|
4731 | \| old         \| /file #2.txt  \| remote.php/webdav/file #2.txt                \|
4732 | \| old         \| /file ?2.txt  \| remote.php/webdav/file ?2.txt                \|
4733 | \| old         \| /file &2.txt  \| remote.php/webdav/file &2.txt                \|
4734 | \| new         \| /C++ file.cpp \| remote.php/dav/files/%username%/C++ file.cpp \|
4735 | \| new         \| /file #2.txt  \| remote.php/dav/files/%username%/file #2.txt  \|
4736 | \| new         \| /file ?2.txt  \| remote.php/dav/files/%username%/file ?2.txt  \|
4737 | \| new         \| /file &2.txt  \| remote.php/dav/files/%username%/file &2.txt  \|
4738 | \| spaces      \| /C++ file.cpp \| dav/spaces/%spaceid%/C++ file.cpp            \|
4739 | Cannot find any entry having href with value dav/spaces/7b510952\-487b\-103c\-9605\-ed21cff07fe3/C++ file.cpp in response to Alice
4740 | Failed asserting that false is true.
4741 | \| spaces      \| /file #2.txt  \| dav/spaces/%spaceid%/file #2.txt             \|
4742 | Cannot find any entry having href with value dav/spaces/7bcd16be\-487b\-103c\-9608\-ed21cff07fe3/file #2.txt in response to Alice
4743 | Failed asserting that false is true.
4744 | \| spaces      \| /file ?2.txt  \| dav/spaces/%spaceid%/file ?2.txt             \|
4745 | Cannot find any entry having href with value dav/spaces/7c4e2d08\-487b\-103c\-960b\-ed21cff07fe3/file ?2.txt in response to Alice
4746 | Failed asserting that false is true.
4747 | \| spaces      \| /file &2.txt  \| dav/spaces/%spaceid%/file &2.txt             \|
4748 | Cannot find any entry having href with value dav/spaces/7cd6972e\-487b\-103c\-960e\-ed21cff07fe3/file &2.txt in response to Alice
4749 | Failed asserting that false is true.

I'll update the core PR 🚀

[kiranparajuli589]

I've updated the related core PR. Latest commit 589c426d801d9f176a8c7c395714afb06f116445 can be used to check the ci

[individual-it]

#39947 is merged, please change back to master with the latest commit