Skip to content

perf(external-share): Port to Entity and use snowflake Ids #55369

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.

Sign up for GitHub

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

Jump to bottom
Open
CarlSchwan wants to merge 9 commits into
base: master
Choose a base branch
from
Open

perf(external-share): Port to Entity and use snowflake Ids #55369

CarlSchwan wants to merge 9 commits into from

Conversation

@CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Sep 28, 2025 •
edited
Loading

  • Port away from Files::buildNotExistingFileName
  • Use IUser and IGroup instead of plain string
  • Resolves: #

Summary

TODO

  • ...

Checklist

@CarlSchwan CarlSchwan added this to the Nextcloud 33 milestone Sep 28, 2025
@CarlSchwan CarlSchwan self-assigned this Sep 28, 2025
@CarlSchwan CarlSchwan requested a review from a team as a code owner September 28, 2025 09:27
@CarlSchwan CarlSchwan requested review from Altahrim, icewind1991 and salmart-dev and removed request for a team September 28, 2025 09:27
@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label Sep 28, 2025
@CarlSchwan CarlSchwan requested a review from come-nc September 28, 2025 09:28
@CarlSchwan CarlSchwan force-pushed the carl/cleanup-external-manager branch 2 times, most recently from c3a408f to 0c9834a Compare September 28, 2025 10:01
@CarlSchwan CarlSchwan added technical debt ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) feature: sharing labels Sep 28, 2025
come-nc
come-nc reviewed Sep 28, 2025
View reviewed changes
@CarlSchwan CarlSchwan force-pushed the carl/cleanup-external-manager branch from 0c9834a to 4b5ed3f Compare September 28, 2025 10:20
@CarlSchwan CarlSchwan force-pushed the carl/cleanup-external-manager branch from 4b5ed3f to 853ea64 Compare September 28, 2025 10:54
Altahrim
Altahrim approved these changes Sep 28, 2025
View reviewed changes
@come-nc
Copy link
Contributor

come-nc commented Sep 28, 2025

According to integation tests you broke federated shares @CarlSchwan

@CarlSchwan CarlSchwan force-pushed the carl/cleanup-external-manager branch 2 times, most recently from 04fd1d0 to 05935e0 Compare September 29, 2025 10:00
come-nc
come-nc reviewed Sep 29, 2025
View reviewed changes
@CarlSchwan CarlSchwan force-pushed the carl/cleanup-external-manager branch 2 times, most recently from c1e8ffc to 46c8632 Compare September 30, 2025 08:24
icewind1991
icewind1991 reviewed Sep 30, 2025
View reviewed changes
icewind1991
icewind1991 reviewed Sep 30, 2025
View reviewed changes
icewind1991
icewind1991 reviewed Sep 30, 2025
View reviewed changes
icewind1991
icewind1991 reviewed Sep 30, 2025
View reviewed changes
apps/files_sharing/lib/External/Manager.php Outdated
return $this->cloudFederationProviderManager->sendNotification($remoteDomain, $notification);
try {
$response = $this->cloudFederationProviderManager->sendCloudNotification($remoteDomain, $notification);
} catch (OCMProviderException) {
Copy link
Member

@icewind1991 icewind1991 Sep 30, 2025

Choose a reason for hiding this comment

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

maybe add this catch block around the switch and have it return false directly instead of relying on the $response staying null bahavior.

Copy link
Member Author

@CarlSchwan CarlSchwan Oct 1, 2025

Choose a reason for hiding this comment

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

reverted the whole part, since I suspect this is breaking the integration test

Copy link
Contributor

@come-nc come-nc Oct 1, 2025

Choose a reason for hiding this comment

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

still failing 😭

@CarlSchwan CarlSchwan force-pushed the carl/cleanup-external-manager branch 2 times, most recently from e9a5835 to 86518ae Compare October 29, 2025 16:19
provokateurin
provokateurin requested changes Oct 29, 2025
View reviewed changes
apps/files_sharing/openapi.json
Copy link
Member

@provokateurin provokateurin Oct 29, 2025

Choose a reason for hiding this comment

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

These changes will break the clients

Copy link
Member Author

@CarlSchwan CarlSchwan Oct 30, 2025

Choose a reason for hiding this comment

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

I don't really see a way around that. Using an int will fails as JSON doesn't support such big numbers, particularly on 32bits machine

Copy link
Collaborator

@Altahrim Altahrim Oct 31, 2025

Choose a reason for hiding this comment

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

It can already fail on 32 bit systems as we are using BIGINT as unique keys. PHP is using signed int so on 32 bit the maximum is 2^31.

The difference is now even "small" instances will have large IDs. On 32 bits servers, we know PHP fail to keep the right value with float(lose precision) and int (truncate).

I don't know if OpenAPI offers a way to represent large numbers, even for 32 bit systems.

Copy link
Member

@provokateurin provokateurin Nov 3, 2025

Choose a reason for hiding this comment

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

Either way, this change is not allowed 🤷‍♀️

@CarlSchwan CarlSchwan force-pushed the carl/cleanup-external-manager branch 4 times, most recently from ac27b7e to 71bf292 Compare October 30, 2025 16:54
@CarlSchwan CarlSchwan force-pushed the carl/cleanup-external-manager branch 4 times, most recently from 1024558 to 0494e0b Compare November 26, 2025 17:19
@CarlSchwan CarlSchwan requested review from a team and nickvergessen as code owners November 28, 2025 11:04
@CarlSchwan CarlSchwan force-pushed the carl/cleanup-external-manager branch 3 times, most recently from 689a67c to 62e4f97 Compare December 1, 2025 10:26
nickvergessen
nickvergessen reviewed Dec 1, 2025
View reviewed changes
@CarlSchwan CarlSchwan force-pushed the carl/cleanup-external-manager branch 2 times, most recently from 3514ae7 to 80fe6ea Compare December 3, 2025 10:40
@CarlSchwan
refactor(external-share): Cleanup OCA\FIles_Sharing\External\Manager
28c1bf9 
- Port away from Files::buildNotExistingFileName
- Use IUser and IGroup instead of plain string

Signed-off-by: Carl Schwan <carl.schwan@nextcloud.com>
@CarlSchwan
refactor(OC_Helper): remove buildNotExistingFileNameForView
f8fe599 
Move the functionality in the last place it is used OC\Files\Node\Folder

Signed-off-by: Carl Schwan <carl.schwan@nextcloud.com>
@CarlSchwan
fix(sharing): Adapt Hooks to new API
20fe3bf 
Signed-off-by: Carl Schwan <carl.schwan@nextcloud.com>
@CarlSchwan
fix: Revert change to the cloud integration
907c359 
This might fixes the test.

Signed-off-by: Carl Schwan <carl.schwan@nextcloud.com>
@CarlSchwan
refactor: use DI-injected logger instead of Server::get
32d38bf 
Signed-off-by: Carl Schwan <carl.schwan@nextcloud.com>
@CarlSchwan
perf(external-sharing): Port to Entity and SnowflakeId
a086192 
This removes all the read after write and we don't need to queries all
the time the same share in the same request anymore.

Signed-off-by: Carl Schwan <carl.schwan@nextcloud.com>

Signed-off-by: Carl Schwan <carl.schwan@nextcloud.com>
@CarlSchwan
fix: Remove auto increment from share_external
7598332 
Signed-off-by: Carl Schwan <carl.schwan@nextcloud.com>
@CarlSchwan
refactor(external-share): Port more code to string as type for the id
96e907a 
Signed-off-by: Carl Schwan <carl.schwan@nextcloud.com>
@CarlSchwan CarlSchwan force-pushed the carl/cleanup-external-manager branch from 80fe6ea to 1919d60 Compare December 3, 2025 15:57
@CarlSchwan
refactor(integration): Refactor and fix federation integration tests
f2d3b2e 
Signed-off-by: Carl Schwan <carl.schwan@nextcloud.com>
@CarlSchwan CarlSchwan force-pushed the carl/cleanup-external-manager branch from 1919d60 to f2d3b2e Compare December 3, 2025 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@icewind1991 icewind1991 icewind1991 left review comments

@provokateurin provokateurin provokateurin requested changes

@salmart-dev salmart-dev Awaiting requested review from salmart-dev salmart-dev is a code owner automatically assigned from nextcloud/server-backend

@come-nc come-nc Awaiting requested review from come-nc

@Altahrim Altahrim Awaiting requested review from Altahrim

Requested changes must be addressed to merge this pull request.

Assignees

@CarlSchwan CarlSchwan

Labels

3. to review Waiting for reviews feature: sharing ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) technical debt

Projects

None yet

Milestone

Nextcloud 33

Development

Successfully merging this pull request may close these issues.

7 participants

@CarlSchwan @come-nc @nickvergessen @icewind1991 @Altahrim @provokateurin