Skip to content

implement recommended recipient api for unified sharing - #63927

Merged
provokateurin merged 3 commits into
masterfrom
unified-sharing-recommended
Sep 16, 2026
Merged

provokateurin merged 3 commits into
masterfrom
unified-sharing-recommended

Conversation

@icewind1991

@icewind1991 icewind1991 commented Sep 1, 2026 •

Copy link
Copy Markdown
Member

Summary

Add api for suggesting recipients for a share

TODO

  • Actually testing the implementation
  • Adding tests

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@icewind1991 icewind1991 added this to the Nextcloud 36 milestone Sep 1, 2026
@icewind1991 icewind1991 added the 2. developing Work in progress label Sep 1, 2026

@provokateurin provokateurin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My idea was to incorporate this into the existing search endpoint and have it trigger when the query is empty. I think this would simplify it a bit on the frontend/client side.

While the implementation with greater than instead of offset is much better than the Collaboration API provides, I don't think it is necessary, because in the current use case only a limited set of results is desired anyway (I think 3) and there is no pagination.

So I think making it part of the existing search endpoint would work and the query could just be limited to 3 all the time (without any sort of pagination).

If we want to have it work in a different way at some point, we can just do that later.

Comment thread lib/private/Sharing/SharingBackend.php Outdated
@icewind1991
icewind1991 force-pushed the unified-sharing-recommended branch from 683c657 to 9b24e39 Compare September 2, 2026 21:15
Comment on lines +4796 to +4799
// querying for the owner, with no further filters
['shares' => [
['recipients' => [$recipient1, $recipient2], 'owner' => 'owner']
], 'user' => $owner, 'expected' => ['recipient1', 'recipient2']],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can set these descriptions as keys for the array. PHPUnit will also log them on failure, which makes it easier to figure out which case is failing.

@icewind1991
icewind1991 force-pushed the unified-sharing-recommended branch from 9b24e39 to a98a1ac Compare September 3, 2026 12:03
@icewind1991

Copy link
Copy Markdown
Member Author

My idea was to incorporate this into the existing search endpoint and have it trigger when the query is empty. I think this would simplify it a bit on the frontend/client side.

Sounds good, will adjust it

While the implementation with greater than instead of offset is much better than the Collaboration API provides, I don't think it is necessary, because in the current use case only a limited set of results is desired anyway (I think 3) and there is no pagination.

Couldn't get the after logic working anyway, so had to switch to the basic `offset

@icewind1991
icewind1991 force-pushed the unified-sharing-recommended branch from a98a1ac to 6f8f308 Compare September 3, 2026 14:19
@icewind1991 icewind1991 changed the title [WIP] feat: implement recipient api for unified sharing [WIP] feat: implement recommended recipient api for unified sharing Sep 3, 2026
@icewind1991
icewind1991 force-pushed the unified-sharing-recommended branch 7 times, most recently from 9e3f8b0 to f52f21c Compare September 3, 2026 22:50
@icewind1991
icewind1991 marked this pull request as ready for review September 4, 2026 13:08
@icewind1991
icewind1991 requested a review from a team as a code owner September 4, 2026 13:08
@icewind1991
icewind1991 requested review from Altahrim, CarlSchwan, come-nc and sorbaugh and removed request for a team September 4, 2026 13:08
@icewind1991 icewind1991 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 4, 2026
@icewind1991
icewind1991 force-pushed the unified-sharing-recommended branch from f52f21c to c0c712f Compare September 4, 2026 13:13
@icewind1991 icewind1991 changed the title [WIP] feat: implement recommended recipient api for unified sharing implement recommended recipient api for unified sharing Sep 4, 2026
@icewind1991

Copy link
Copy Markdown
Member Author

/backport to stable35

Comment thread lib/unstable/Sharing/ISharingManager.php Outdated
Comment thread apps/sharing/lib/Controller/ApiV1Controller.php Outdated
Comment thread lib/private/Sharing/SharingBackend.php Outdated
Comment thread lib/private/Sharing/SharingBackend.php Outdated
Comment thread lib/private/Sharing/SharingBackend.php Outdated
Comment thread lib/private/Sharing/SharingBackend.php
Comment thread lib/private/Sharing/SharingBackend.php
@provokateurin

Copy link
Copy Markdown
Member

@icewind1991 did you forget to push?


$query->selectColumns('recipient_class_id', 'recipient_value', 'recipient_instance', 'initiator_user_id', 'initiator_instance')
->selectAlias($query->func()->count('*'), 'count')
->from('sharing_share_recipients', 'r')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sharing_share_recipients only has a PK on id, no index on share_id or initiator_user_id. So this joins and groups over the whole table, and the NOT IN scans it again. The picker hits this on every open, which means two full scans each time on bigger instances. Can we add the indexes here?

Comment thread lib/private/Sharing/SharingBackend.php Outdated
Comment thread lib/private/Sharing/SharingManager.php Outdated
Comment thread lib/private/Sharing/SharingBackend.php Outdated
@skjnldsv

skjnldsv commented Sep 11, 2026 •

Copy link
Copy Markdown
Member

Couple of small things:

  • coalesce uses #[Override] while the rest of that file uses #[\Override]
  • the new RuntimeException when there is no session user gives a 500, a 401 would say more. The route is NoAdminRequired so it should not happen anyway

@icewind1991
icewind1991 force-pushed the unified-sharing-recommended branch 2 times, most recently from 6b7b2a8 to 755a063 Compare September 16, 2026 13:44
Comment thread lib/public/DB/QueryBuilder/IFunctionBuilder.php Outdated
Comment thread tests/lib/Sharing/AbstractSharingManagerTests.php Outdated
Comment thread lib/private/Sharing/SharingBackend.php Outdated
@icewind1991
icewind1991 force-pushed the unified-sharing-recommended branch from 755a063 to 605a957 Compare September 16, 2026 15:48
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
icewind1991 force-pushed the unified-sharing-recommended branch from 605a957 to 328efec Compare September 16, 2026 16:38
@provokateurin
provokateurin merged commit aa4ce67 into master Sep 16, 2026
153 checks passed
@provokateurin
provokateurin deleted the unified-sharing-recommended branch September 16, 2026 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants