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 #2086

Merged
merged 2 commits into from
Oct 5, 2021

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Sep 20, 2021

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

This is the rework of #2071
See #2071 (comment).

@C0rby C0rby marked this pull request as ready for review September 27, 2021 13:58
@C0rby C0rby requested a review from labkode as a code owner September 27, 2021 13:58
@C0rby C0rby requested review from butonic, refs and ishank011 September 27, 2021 13:58
Copy link
Member

@refs refs left a comment

Choose a reason for hiding this comment

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

I think having persistence here and not only a memory implementation will take this PR further 👯

changelog/unreleased/list-favorites.md Show resolved Hide resolved

// InMemoryManager implements the Manager interface to manage favorites using an in-memory storage.
type InMemoryManager struct {
favorites map[string]map[string]*provider.ResourceId
Copy link
Member

Choose a reason for hiding this comment

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

also mind that a map is not thread safe and will panic if concurrent writes. A mutex should guard against this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, it won't work without a lock

butonic
butonic previously approved these changes Sep 30, 2021
Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

This is the first step. @labkode @ishank011 please review.

Copy link
Contributor

@ishank011 ishank011 left a comment

Choose a reason for hiding this comment

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

@butonic @C0rby looks okay for a short-term solution, but we need to discuss how to have a better implementation

@C0rby
Copy link
Contributor Author

C0rby commented Oct 4, 2021

@ishank011, it was agreed that this is just the short-term solution and a more future proof solution will follow. See #2036

@ishank011
Copy link
Contributor

@C0rby thanks! Can you add the mutex protection? We can merge it then

ishank011
ishank011 previously approved these changes Oct 5, 2021
refs
refs previously approved these changes Oct 5, 2021
@ishank011
Copy link
Contributor

@C0rby can you take a look at the failing tests, please?

@C0rby C0rby dismissed stale reviews from refs and ishank011 via 19aafb9 October 5, 2021 10:03
@C0rby
Copy link
Contributor Author

C0rby commented Oct 5, 2021

Yeah, I'm on it.

David Christofas added 2 commits October 5, 2021 12:43
I added filter-files to the dav REPORT API. This enables the listing of
favorites.
@ishank011 ishank011 merged commit 4676db3 into cs3org:master Oct 5, 2021
wkloucek added a commit to owncloud/ocis that referenced this pull request Oct 6, 2021
wkloucek added a commit to owncloud/ocis that referenced this pull request Oct 6, 2021
wkloucek added a commit to owncloud/ocis that referenced this pull request Oct 6, 2021
ownclouders pushed a commit to owncloud/ocis that referenced this pull request Oct 6, 2021
Merge: c0a20b2 e2a6b5d
Author: Willy Kloucek <[email protected]>
Date:   Wed Oct 6 09:15:44 2021 +0200

    Merge pull request #2581 from owncloud/report_unexpected_passes

    [full-ci] fix unexpected passes for cs3org/reva#2086
@C0rby C0rby deleted the list-favorites branch June 13, 2022 10:11
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.

4 participants