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

Unify/cleanup filesystem setup #31431

Merged
merged 9 commits into from
Mar 8, 2022
Merged

Unify/cleanup filesystem setup #31431

merged 9 commits into from
Mar 8, 2022

Conversation

icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Mar 3, 2022

  • Moves filesystem setup logic from static OC_Util/OC\Files\Filesystem functions to it's own class
  • New setup logic is called from all the relevant old places for compatibility
  • Includes a fair number of changes to unit tests since they tend to do a lot of weird things with the filesystem

Requires

Part of #31265

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Mar 3, 2022
@icewind1991 icewind1991 mentioned this pull request Mar 3, 2022
8 tasks
@icewind1991 icewind1991 added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 3, 2022
@icewind1991 icewind1991 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 4, 2022
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 overall it looks fine, see minor comments

lib/private/Files/SetupManager.php Outdated Show resolved Hide resolved
$this->setupForUser($user);
}

public function tearDown() {
Copy link
Member

Choose a reason for hiding this comment

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

does it make sense to also have a tear down for a single user or is it too complicated ?

use case could be when running bkg jobs that iterate over lots of users and needs the FS

or as a middle group, one that only tears down the users, not the root ? (but root setup is likely cheap, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Root setup is generally cheap yes so I'm not sure if there is much to gain there.

Additionally, background jobs can probably be adjusted to not teardown the filesystem but instead setup for an additional user

tests/lib/Avatar/GuestAvatarTest.php Outdated Show resolved Hide resolved
@PVince81 PVince81 requested review from CarlSchwan, Pytal and come-nc March 4, 2022 14:35
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
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 added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 4, 2022
apps/files_sharing/lib/Updater.php Show resolved Hide resolved
@icewind1991 icewind1991 merged commit e8872f0 into master Mar 8, 2022
@icewind1991 icewind1991 deleted the fs-setup-manager branch March 8, 2022 14:50
@danxuliu
Copy link
Member

Since 22875bf the integration tests in Talk fail.

When a conversation is associated to a file (a conversation opened in the Chat tab in the Files app) and a participant gets the possible mentions the list includes the current participants of the conversation as well as other users with access to that file but that are not in the conversation yet.

To do that the node for the file id is got using $this->rootFolder->getById($fileId). The path of the root node is empty (''), so with the previous code this call ended setting up the full filesystem for the user and finding the node. However, with the new code (due to the substr_count($path, '/') < 2 check) only the root filesystem is set up, the node is not found and the list of other users with access to the file ends being empty.

If $this->rootFolder->getById($fileId) is replaced by if ($this->userId) { $userFolder = $this->rootFolder->getUserFolder($this->userId); $nodes = $userFolder->getById($fileId); } in Talk then everything works again.

@icewind1991 Should that be changed in Talk because the code is wrong and it just happened to work until now? Or is the code in Talk fine and it is a regression that should be fixed in the server?

Thanks!

@icewind1991
Copy link
Member Author

@danxuliu can you check if #31598 helps

@danxuliu
Copy link
Member

@danxuliu can you check if #31598 helps

Yes, the failing tests pass again with #31598. Thanks!

@jotoeri
Copy link
Member

jotoeri commented Mar 21, 2022

Similar thing on forms - Unittests fail on tearDown with server masterbranch. Seems to me to be connected to this PR? And still consists after the mentioned PR...

Do you have any idea on this, @icewind1991?

Exception in OCA\Forms\Tests\Unit\Activity\ActivityManagerTest::tearDownAfterClass
Argument 1 passed to OC\Files\Config\UserMountCache::__construct() must implement interface OCP\IDBConnection, null given, called in /home/runner/work/forms/forms/lib/private/Server.php on line 935

https://github.com/nextcloud/forms/runs/5618838041

}

$current = dirname($current);
if ($current === '.' || $current === '/') {
$current = '';
}
}

throw new NotFoundException("No mount for path " . $path . " existing mounts: " . implode(",", array_keys($this->mounts)));
Copy link
Contributor

Choose a reason for hiding this comment

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

@icewind1991 The throw should be highlighted in #37039 as it change how you would use the API.
Already got bitten in activity: nextcloud/activity#1165

Copy link
Contributor

Choose a reason for hiding this comment

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

@artonge artonge added the pending documentation This pull request needs an associated documentation update label May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants