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

Implement listing favorites via the dav report API #2071

Merged
merged 1 commit into from
Sep 17, 2021

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Sep 14, 2021

I added filter-files to the dav REPORT API. This enables the listing of
favorites.

This PR also includes a new in-memory storage.FS implementation which I use for the unit tests of the favorite package but it can also be extended and used for other unit tests depending on a storage.

@update-docs
Copy link

update-docs bot commented Sep 14, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@C0rby C0rby force-pushed the list-favorites branch 5 times, most recently from 6dda83f to ed91069 Compare September 15, 2021 11:10
@C0rby
Copy link
Contributor Author

C0rby commented Sep 15, 2021

@butonic, could you have a look at this approach?

I think we still need some system to notify the several services about certain events like file created, file deleted, etc.
Here for example if a file is marked as favorite and then deleted, the favorite manager doesn't know that the files was deleted and will still return it.

Apart from that is that the expected approach? Do we still need to list all file from the storage provider and filter them for the favorite flag?

@C0rby C0rby force-pushed the list-favorites branch 5 times, most recently from 41821f1 to 2ede95f Compare September 16, 2021 14:48
I added filter-files to the dav REPORT API. This enables the listing of
favorites.
@C0rby C0rby marked this pull request as ready for review September 16, 2021 16:02
@C0rby C0rby requested a review from labkode as a code owner September 16, 2021 16:02
@C0rby C0rby requested review from butonic and refs September 16, 2021 16:02
@C0rby C0rby self-assigned this Sep 16, 2021
@labkode
Copy link
Member

labkode commented Sep 17, 2021

@C0rby @butonic
For every single operation we have to contact the favourite manager, which in a prod scenario it won't be in memory but a SQL or another storage that adds latency. I think this is a good starting point and I'm merging it as is to unblock the UI development.

However, this needs to be revisited. Either adding a cache or store this info in the web client and the web client will add the fav icon where needed by matching fileids from the webdav response.

@labkode labkode merged commit df33e69 into cs3org:master Sep 17, 2021
labkode added a commit that referenced this pull request Sep 17, 2021
@labkode
Copy link
Member

labkode commented Sep 17, 2021

@butonic @C0rby I've reverted this PR. I didn't realize of the hack of using the direct fs in the ocdav layer that bypass all the grpc logic, plus it breaks any deployment just by running it with another driver.

Please remove the dependency on the storage.FS, there is no reason to have it there and FS interactions can happen via GRPC.

@C0rby
Copy link
Contributor Author

C0rby commented Sep 20, 2021

Right, I'll change the implementation to make the calls via GRPC and then we'll solve the potential performance problem later on.
Sorry for the inconvenience.

glpatcern pushed a commit to glpatcern/reva that referenced this pull request Sep 23, 2021
glpatcern pushed a commit to glpatcern/reva that referenced this pull request Sep 23, 2021
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.

3 participants