Skip to content

refactor/file service - #9028

Merged
max-nextcloud merged 9 commits into
mainfrom
refactor/file-service
Aug 19, 2026
Merged

refactor/file service#9028
max-nextcloud merged 9 commits into
mainfrom
refactor/file-service

Conversation

@max-nextcloud

Copy link
Copy Markdown
Collaborator
  • refactor(backend): introduce FileService
  • chore(refactor): separate file getters for share and user
  • chore(refactor): get file in controller for apiService->create
  • chore(refactor): use constructor promotion for DocumentServce
  • chore(refactor): move file getters into FileService

@max-nextcloud
max-nextcloud force-pushed the refactor/file-service branch 10 times, most recently from f672b62 to ccbb342 Compare August 12, 2026 08:50
@max-nextcloud
max-nextcloud marked this pull request as ready for review August 12, 2026 09:00
@max-nextcloud max-nextcloud self-assigned this Aug 12, 2026
@max-nextcloud max-nextcloud moved this to 👀 In review in 📝 Productivity team Aug 12, 2026
@max-nextcloud
max-nextcloud requested a review from grnd-alt August 18, 2026 07:56
Comment thread lib/Controller/PublicSessionController.php Outdated
Comment thread lib/Service/FileService.php Outdated
@max-nextcloud
max-nextcloud force-pushed the refactor/file-service branch 2 times, most recently from 5faa125 to d3f9fe3 Compare August 18, 2026 15:13
@max-nextcloud
max-nextcloud requested a review from grnd-alt August 18, 2026 15:13
@max-nextcloud
max-nextcloud force-pushed the refactor/file-service branch from d3f9fe3 to c9d35f7 Compare August 18, 2026 18:22
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
max-nextcloud force-pushed the refactor/file-service branch 3 times, most recently from c4cca9f to 7232808 Compare August 19, 2026 04:49
* `checkSharePermissions` has not been throwing `NotPermittedException` for a while.
  See #3765 .
* `InvalidArgumentException` was not being handled.
* throw `NotFoundException` if file cannot be found and there is no share token.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
max-nextcloud force-pushed the refactor/file-service branch from 7232808 to 5907ff4 Compare August 19, 2026 05:35
}

public function unlock(File $file): void {
if (!$this->lockManager->isLockProviderAvailable()) {

@grnd-alt grnd-alt Aug 19, 2026

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.

nitpick: is there an advantage to calling this method here?

lockManager checks if there's a lockprovider as well, so if there is a lockProvider lockManager->getLockProvider is called twice?

same in the lock method

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Will address as part of #9062

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in a35187c


// Workaround to always open files with edit permissions if multiple occurrences of
// the same file id are in the user home, ideally we should also track the path of the file when opening
usort($files, static fn (Node $a, Node $b) => ($b->getPermissions() & Constants::PERMISSION_UPDATE) <=> ($a->getPermissions() & Constants::PERMISSION_UPDATE));

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.

I think this can be done more efficient. If I get this right this sorts the array by comparing if the PERMISSION_UPDATE is present, so essentially is a search for an element in the array which has the PERMISSION_UPDATE without control over which of those comes first?

as afterwards it's checked if there's also PERMISSION_READ I think this can be simplified into a for loop that searches for a file with PERMISSION_READ and PERMISSION_UPDATE.

something like this:

foreach ($files as $file) {
    if ($file->getPermissions() & (Constants::PERMISSION_UPDATE | Constants::PERMISSION_READ) === 3) {
        return $file;
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Will address as part of #9062

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in ab2acac

@max-nextcloud
max-nextcloud merged commit f2f6695 into main Aug 19, 2026
68 of 69 checks passed
@max-nextcloud
max-nextcloud deleted the refactor/file-service branch August 19, 2026 09:07
@github-project-automation github-project-automation Bot moved this from 👀 In review to ☑️ Done in 📝 Productivity team Aug 19, 2026
@max-nextcloud max-nextcloud mentioned this pull request Aug 19, 2026
21 tasks
max-nextcloud added a commit that referenced this pull request Aug 19, 2026
Addresses #9028 (comment) .

Signed-off-by: Max <max@nextcloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: ☑️ Done

Development

Successfully merging this pull request may close these issues.

2 participants