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

return share-types in ocs propfind responses #1329

Merged
merged 2 commits into from
Dec 9, 2020

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Nov 23, 2020

Implements owncloud/ocis#929

@C0rby C0rby requested a review from labkode as a code owner November 23, 2020 10:24
@C0rby C0rby requested review from butonic and kulmann November 23, 2020 10:24
Copy link
Member

@labkode labkode left a comment

Choose a reason for hiding this comment

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

This change is very heavy on the server side.
Please add a config option to the dav service: include-shares-in-propfind and set it as false by default.
These behaviour will be enabled only when this option is true.

@butonic
Copy link
Contributor

butonic commented Nov 23, 2020

@corby the ListShares rpc can take a path argument. Use it to get a list of all grants in a folder instead of every file individually. You can send that request in parralel to the ListContainer.

When the ListContainer request has returned you can prepare a map[fileid]metadata instead of the current list.
When both coroutines have returned you can iterate over the grants and apply them by looking up the metadata using the fileid.

@labkode that should not be heavy.

@labkode
Copy link
Member

labkode commented Nov 23, 2020

@butonic that is a good improvement, however that config option is needed to enable or disable it on demand. We don't use it for example.

@C0rby C0rby force-pushed the ocis-ocs-share-indicators branch from 51d19ec to a8e945f Compare November 25, 2020 11:59
@C0rby
Copy link
Contributor Author

C0rby commented Nov 25, 2020

So after talking with @butonic, I rewrote it and we read the shares directly during the ListFolder call from the extended attributes.
This should be more performant but only works for user shares right now. Public link shares will be tackled some other time.

@C0rby C0rby force-pushed the ocis-ocs-share-indicators branch 2 times, most recently from c8c6092 to ebb0852 Compare November 25, 2020 12:53
@C0rby C0rby requested a review from labkode November 25, 2020 15:00
kulmann
kulmann previously approved these changes Dec 2, 2020
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Code looks good

@@ -387,6 +392,12 @@ func (n *Node) AsResourceInfo(ctx context.Context) (ri *provider.ResourceInfo, e
log.Error().Err(err).Interface("node", n).Msg("could not list attributes")
}

if common.FindString(mdKeys, _ShareTypesKey) != -1 {
if n.hasUserShares(ctx) {
ri.ArbitraryMetadata.Metadata[_ShareTypesKey] = "0"
Copy link
Member

Choose a reason for hiding this comment

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

I was confused by the "0" and needed to ask. For the record, it would be really cool to have constants for the share types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. 👍

@C0rby C0rby force-pushed the ocis-ocs-share-indicators branch from d509b4e to 5cefb2b Compare December 2, 2020 15:05
pkg/storage/fs/ocis/node.go Outdated Show resolved Hide resolved
@labkode labkode merged commit 80ba33a into cs3org:master Dec 9, 2020
@C0rby C0rby deleted the ocis-ocs-share-indicators branch December 9, 2020 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants