Fix REPORT response from webdav #4485

kobergj · 2022-08-31T10:58:34Z

There were multiple issue with REPORT search response from webdav. Also we want it to be consistent with PROPFIND endpoint.

the remote.php prefix was missing from the href (added even though not neccessary)
the ids were formated wrong, they should look different for shares and spaces.
the name of the resource was missing
the shareid was missing (for shares)
the prop shareroot (containing the name of the share root) was missing
the permissions prop was empty

owncloud/web#7557

ownclouders · 2022-08-31T11:24:03Z

💥 Acceptance test Core-API-Tests-ocis-storage-9 failed. Further test are cancelled...

kobergj · 2022-08-31T12:58:53Z

@ScharfViktor could you help me with the failing tests? We updated the REPORT endpoint to be consistent with the PROPFIND webdav endpoint:

we added the spaceid to the href (previously it was only storageid!opaqueid, now storageid$spaceid!opaqueid
we added a remote.php prefix to the url
we removed netencoding from the href (to be consistent with PROPFIND response)

It seems any of these three changes break the tests.
Do the test expect the href in a certain way? If so can we change that?
The last two bulletpoints are rather optional but the first one is a must have for proper functionality

ScharfViktor · 2022-08-31T13:16:16Z

@ScharfViktor could you help me with the failing tests? We updated the REPORT endpoint to be consistent with the PROPFIND webdav endpoint:

we added the spaceid to the href (previously it was only storageid!opaqueid, now storageid$spaceid!opaqueid

we added a remote.php prefix to the url

we removed netencoding from the href (to be consistent with PROPFIND response)

It seems any of these three changes break the tests. Do the test expect the href in a certain way? If so can we change that? The last two bulletpoints are rather optional but the first one is a must have for proper functionality

Yes sure. let me switch to your branch and debug it local

ScharfViktor · 2022-08-31T15:03:28Z

test failed because:

https://github.com/owncloud/core/blob/master/tests/acceptance/features/bootstrap/WebDav.php#L5319
here we get spaceId. spaceId looks like: 1284d238-aa92-42ce-bdc4-0b0000009157$5ae7fa50-cdf1-43d6-82a5-8f58c65c25f4

but search result give us: /remote.php/dav/spaces/1284d238-aa92-42ce-bdc4-0b0000009157$5ae7fa50-cdf1-43d6-82a5-8f58c65c25f4!5ae7fa50-cdf1-43d6-82a5-8f58c65c25f4/upload😀 😁/upload😀 😁.txt
here https://github.com/owncloud/core/blob/master/tests/acceptance/features/bootstrap/WebDav.php#L5326
we create $topWebDavPath witch we search in REPORT request. it's ease -> just replace to "/remote.php/dav/spaces/"

ScharfViktor · 2022-08-31T15:25:49Z

the ids were formated wrong, missing the spaceID

PROPFIND request give us: /remote.php/dav/spaces/1284d238-aa92-42ce-bdc4-0b0000009157$5ae7fa50-cdf1-43d6-82a5-8f58c65c25f4/upload%20folder/

should be results(REPORT and PROPFIND requests) be same?

kobergj · 2022-09-01T08:12:37Z

@ScharfViktor yes, the goal was to have them consistent. However there is one thing I don't understand. As you said spaceID looks like 1284d238-aa92-42ce-bdc4-0b0000009157$5ae7fa50-cdf1-43d6-82a5-8f58c65c25f4 why does search expect it to look like 1284d238-aa92-42ce-bdc4-0b0000009157!5ae7fa50-cdf1-43d6-82a5-8f58c65c25f4 ($ vs !). That is wrong in my opinion.

Second question, whats the usual way to fix such problems? Should I put them on expected failures, then fix the test and then remove them again? Or is there another procedure I should follow?

ScharfViktor · 2022-09-01T09:10:43Z

why does search expect it to look like 1284d238-aa92-42ce-bdc4-0b0000009157!5ae7fa50-cdf1-43d6-82a5-8f58c65c25f4 ($ vs !). That is wrong in my opinion.

No, not exactly. search result returns us 1284d238-aa92-42ce-bdc4-0b0000009157$5ae7fa50-cdf1-43d6-82a5-8f58c65c25f4!5ae7fa50-cdf1-43d6-82a5-8f58c65c25f4 (3 uuid: storageId . "$" . spaceId . "!" spaceId again?? or opaqueid)

ScharfViktor · 2022-09-01T09:12:57Z

Second question, whats the usual way to fix such problems? Should I put them on expected failures, then fix the test and then remove them again? Or is there another procedure I should follow?

Move please failed tests to expected failures. after merging your PR, I create 2 PRs (fix test in core and deleting tests from expected failures in ocis).

ScharfViktor · 2022-09-01T09:14:43Z

should be results(REPORT and PROPFIND requests) be same?

What about this one? It's just that if I change search tests to new implementation-> propfind tests will be failed

kobergj · 2022-09-01T09:50:21Z

No, not exactly. search result returns us 1284d238-aa92-42ce-bdc4-0b0000009157$5ae7fa50-cdf1-43d6-82a5-8f58c65c25f4!5ae7fa50-cdf1-43d6-82a5-8f58c65c25f4 (3 uuid: storageId . "$" . spaceId . "!" spaceId again?? or opaqueid)

Yes exactly. That was a bug on my side. When comparing to PROPFIND it should be storageID$SpaceID/PATH I'll change it that way.

Move please failed tests to expected failures. after merging your PR, I create 2 PRs (fix test in core and deleting tests from expected failures in ocis).

Thanks! will do so.

What about this one? It's just that if I change search tests to new implementation-> propfind tests will be failed

The fields that are in both PROPFIND and REPORT should be consistent. However PROPFIND might contain more fields than REPORT does. That means no changes should be done to PROPFIND test, we want REPORT to behave like PROPFIND not the other way around

kulmann

Commented on some typos, but otherwise looks good from what I can tell.

services/search/pkg/search/provider/searchprovider.go

changelog/unreleased/fix-search-report.md

Signed-off-by: jkoberg <[email protected]>

sonarqubecloud · 2022-09-02T11:42:58Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

31.0% Coverage
0.0% Duplication

butonic · 2022-10-07T08:47:36Z

services/webdav/pkg/service/v0/search.go

 	response := propfind.ResponseXML{
-		Href:     net.EncodePath(path.Join("/dav/spaces/", match.Entity.Ref.ResourceId.StorageId+"!"+match.Entity.Ref.ResourceId.OpaqueId, match.Entity.Ref.Path)),
+		Href:     net.EncodePath(path.Join("/remote.php/dav/spaces/", ref)),


why do we have to prefix with the legacy remote.php?

To be consistent with ocdav PROPFIND endpoint

micbar · 2023-09-18T15:09:42Z

@kobergj changelog and pull request id is not matching.

Can we fix that on master?

This was referenced Aug 31, 2022

[Technical] Search - collector ticket owncloud/web#7557

Closed

Incorrect file path in search results owncloud/web#7391

Closed

ScharfViktor mentioned this pull request Aug 31, 2022

[tests-only] Api test. fix search test #4463

Merged

AlexAndBear mentioned this pull request Aug 31, 2022

Open files in search context is broken owncloud/web#7456

Closed

AlexAndBear mentioned this pull request Aug 31, 2022

[full-ci] Add search support for shares owncloud/web#7560

Merged

10 tasks

kobergj force-pushed the FixSearchReport branch 3 times, most recently from 129d6b9 to 856c160 Compare September 2, 2022 08:21

kulmann approved these changes Sep 2, 2022

View reviewed changes

kulmann assigned kobergj Sep 2, 2022

kobergj added 3 commits September 2, 2022 13:36

adjust REPORT to PROPFIND endpoint

de6383f

Signed-off-by: jkoberg <[email protected]>

regenerate protogen

66f335b

Signed-off-by: jkoberg <[email protected]>

adjust expected failures

d00deb6

Signed-off-by: jkoberg <[email protected]>

kobergj force-pushed the FixSearchReport branch from d01663d to d00deb6 Compare September 2, 2022 11:37

kobergj merged commit ddd6a53 into owncloud:master Sep 2, 2022

kobergj deleted the FixSearchReport branch September 2, 2022 13:02

This was referenced Sep 2, 2022

E2e tests. implement tests for searching owncloud/web#7395

Closed

[test-only] Fix search tests owncloud/core#40330

Merged

[tests-only] removed the search test from the expected failures #4505

Merged

butonic reviewed Oct 7, 2022

View reviewed changes

kobergj mentioned this pull request Sep 19, 2023

[docs-only] Fix changelog reference #7306

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix REPORT response from webdav #4485

Fix REPORT response from webdav #4485

kobergj commented Aug 31, 2022 •

edited

Loading

ownclouders commented Aug 31, 2022 •

edited

Loading

kobergj commented Aug 31, 2022

ScharfViktor commented Aug 31, 2022

ScharfViktor commented Aug 31, 2022

ScharfViktor commented Aug 31, 2022

kobergj commented Sep 1, 2022

ScharfViktor commented Sep 1, 2022

ScharfViktor commented Sep 1, 2022

ScharfViktor commented Sep 1, 2022

kobergj commented Sep 1, 2022 •

edited

Loading

kulmann left a comment

sonarqubecloud bot commented Sep 2, 2022

butonic Oct 7, 2022

kobergj Oct 7, 2022

micbar commented Sep 18, 2023

Fix REPORT response from webdav #4485

Fix REPORT response from webdav #4485

Conversation

kobergj commented Aug 31, 2022 • edited Loading

ownclouders commented Aug 31, 2022 • edited Loading

kobergj commented Aug 31, 2022

ScharfViktor commented Aug 31, 2022

ScharfViktor commented Aug 31, 2022

ScharfViktor commented Aug 31, 2022

kobergj commented Sep 1, 2022

ScharfViktor commented Sep 1, 2022

ScharfViktor commented Sep 1, 2022

ScharfViktor commented Sep 1, 2022

kobergj commented Sep 1, 2022 • edited Loading

kulmann left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 2, 2022

butonic Oct 7, 2022

Choose a reason for hiding this comment

kobergj Oct 7, 2022

Choose a reason for hiding this comment

micbar commented Sep 18, 2023

kobergj commented Aug 31, 2022 •

edited

Loading

ownclouders commented Aug 31, 2022 •

edited

Loading

kobergj commented Sep 1, 2022 •

edited

Loading