-
Notifications
You must be signed in to change notification settings - Fork 176
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
feat: add remote item id to search report responses #9095
Conversation
94d404d
to
f594db6
Compare
f594db6
to
5f6fecd
Compare
There was a problem hiding this 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.
Thanks for your input!
Yes.
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.
Yes, the naming comes from the Graph terminology. There is already a dav property called
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? |
@micbar These are missing at a first glance:
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 |
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.
5f6fecd
to
783692e
Compare
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
feat: add remote item id to search report responses
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