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.

Sign up for GitHub

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

Jump to bottom

feat: add remote item id to search report responses #9095

Merged
merged 3 commits into from
May 15, 2024
Merged

feat: add remote item id to search report responses #9095

merged 3 commits into from
May 15, 2024

Conversation

JammingBen
Copy link
Contributor

@JammingBen JammingBen commented May 7, 2024 •
edited

Description

Adds the remote item id to search REPORT responses for shared resources.

This is needed for the Web client to correctly resolve into a share from a search result. The Web client used the shareId in the past, however, with owncloud/web#9784 we need to use the actual resource id (= remote item id) for this.

@dschmidt and I also thought about making the property more general by naming it spaceId and exposing it not only for shares but for all search results. But since we only need this for shares in this is the least invasive approach, we decided to go this way. Opinions welcome though.

Related Issue

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)
  • Technical debt
  • Tests only (no source changes)

@JammingBen JammingBen self-assigned this May 7, 2024
@JammingBen JammingBen marked this pull request as ready for review May 7, 2024 13:58
@JammingBen JammingBen requested a review from micbar May 8, 2024 06:06
@JammingBen JammingBen mentioned this pull request May 8, 2024
Merged
7 tasks
@JammingBen
Copy link
Contributor Author

JammingBen commented May 8, 2024 •
edited

@micbar The Go vulnerability check fails now that I added f594db6. Could this be related? Other places in this webdav search service also don't use the protobuf getters 🤷 Okay this seems to be related to something else.

rhafer
rhafer reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow I wonder what the semantics of the remote item id in the dav response are (and I also wonder about the naming 🙊 ).

AFAIU the goal is to be able to reference where a search result is coming from, correct?

So if a search result is a file inside for a shared folder, you need to know the resource ID of the shared folder. Right?

If it's a file inside of a space that the user is able to access you need the resource id of the space root. Also right?

If that is true, should it really be called remoteItemId? I guess this is coming from the graph services remoteItem property in the sharedWithMe/sharedByMe responses?

I'd appreciate some more details in the commit message about why this is needed and what the new property actually represents. (I fear I know the answer already, but do we document all the extended webdav properties and their meaning somewhere?)

I can't really give a good review for this. As I don't know the search code enough. So this is neither a 👍 or a 👎 just some thoughts.

@JammingBen
Copy link
Contributor Author

JammingBen commented May 8, 2024 •
edited

Thanks for your input!

So if a search result is a file inside for a shared folder, you need to know the resource ID of the shared folder. Right?

Yes.

If it's a file inside of a space that the user is able to access you need the resource id of the space root. Also right?

Yes. The Web client extracts the space id from the file id. I know that's not ideal, but that's how it currently works.

If that is true, should it really be called remoteItemId? I guess this is coming from the graph services remoteItem property in the sharedWithMe/sharedByMe responses?

Yes, the naming comes from the Graph terminology.

There is already a dav property called shareRoot, so we could also stay with that terminology and name the new property shareRootId, as this is the file id matching the share root path.

(I fear I know the answer already, but do we document all the extended webdav properties and their meaning somewhere?)

There is https://owncloud.dev/apis/http/webdav/#supported-webdav-properties, but it doesn't seem to be maintained because it's missing quite a few properties.

@micbar
Copy link
Contributor

micbar commented May 10, 2024

There is https://owncloud.dev/apis/http/webdav/#supported-webdav-properties, but it doesn't seem to be maintained because it's missing quite a few properties.

I added the ones where i know that we use them. Which ones are missing?

@JammingBen
Copy link
Contributor Author

JammingBen commented May 14, 2024 •
edited

I added the ones where i know that we use them. Which ones are missing?

@micbar These are missing at a first glance:

  • oc:shareid
  • oc:shareroot
  • oc:activelock (this is already part of <d:lockdiscovery />).
  • oc:audio
  • oc:location
  • oc:public-link-item-type
  • oc:public-link-permission
  • oc:public-link-expiration
  • oc:public-link-share-datetime
  • oc:public-link-share-owner
  • oc:trashbin-original-filename
  • oc:trashbin-original-location
  • oc:trashbin-delete-datetime

How do we want to continue with this PR? I can make the commit message more descriptive and explain why and how to use this new property. Also, as stated above, I could rename it to shareRootId, to match with the naming convention of shareRoot. Though that might be confusing because it's different to the Graph naming terminology.

@JammingBen
feat: add remote item id to search report responses
d40a643 
Adds the remote item id to search `REPORT` responses for shared resources and resources that are part of such. This id represents the id of the original resource that has been shared (= the remote item) and is needed for clients to correctly resolve their locations.
@JammingBen
fix: use getters for protobuf types
164f865
@JammingBen
docs: add missing supported webdav props
783692e
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 15, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
58.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@JammingBen
Copy link
Contributor Author

@micbar As discussed, I added docs for the missing supported webdav props via 783692e and added more detail to the original commit message.

micbar
micbar approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@JammingBen JammingBen merged commit 6ebe33e into master May 15, 2024
4 checks passed
@delete-merged-branch delete-merged-branch bot deleted the feat/search-remote-item-id branch May 15, 2024 14:42
ownclouders pushed a commit that referenced this pull request May 15, 2024
@JammingBen
Merge pull request #9095 from owncloud/feat/search-remote-item-id
691347f 
feat: add remote item id to search report responses
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhafer rhafer rhafer left review comments

@dschmidt dschmidt dschmidt approved these changes

@micbar micbar micbar approved these changes

@kobergj kobergj Awaiting requested review from kobergj

@fschade fschade Awaiting requested review from fschade

Assignees

@JammingBen JammingBen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Sharing NG] Add remote item id to search response
4 participants
@JammingBen @micbar @rhafer @dschmidt