feat(mount-provider): implement IPartialMountProvider#2378
feat(mount-provider): implement IPartialMountProvider#2378cristianscheid wants to merge 2 commits intomasterfrom
Conversation
Signed-off-by: Cristian Scheid <cristianscheid@gmail.com>
…single query Signed-off-by: Cristian Scheid <cristianscheid@gmail.com>
|
@provokateurin I've addressed your review comments and updated the PR description with new testing instructions. Thanks! |
|
|
||
| if ($forChildren) { | ||
| foreach ($paths as $path) { | ||
| $orX->add($expr->like($aliasMount . '.mountpoint', $this->createNamedParameter($path . '/%'))); |
There was a problem hiding this comment.
| $orX->add($expr->like($aliasMount . '.mountpoint', $this->createNamedParameter($path . '/%'))); | |
| $orX->add($expr->like($aliasMount . '.mountpoint', $this->createNamedParameter($path . '/_%'))); |
This is what we settled on with @salmart-dev and @artonge, because a mountpoint could have a trailing slash, but not children, and would match without this.
| } | ||
|
|
||
| $expr = $this->expr(); | ||
| $orX = $expr->orX(); |
There was a problem hiding this comment.
Maybe move this into the $forChildren clause, since it's only used there.
| * @param string $aliasMount | ||
| * @param string[] $paths | ||
| * @param bool $forChildren |
There was a problem hiding this comment.
| * @param string $aliasMount | |
| * @param string[] $paths | |
| * @param bool $forChildren | |
| * @param list<string> $paths |
Less noise and more precise.
| * @param IFederatedUser $federatedUser | ||
| * @param string[] $paths | ||
| * @param bool $forChildren |
There was a problem hiding this comment.
| * @param IFederatedUser $federatedUser | |
| * @param string[] $paths | |
| * @param bool $forChildren | |
| * @param list<string> $paths |
| if (!isset($userMountRequests[$user->getUID()])) { | ||
| $federatedUser = $this->federatedUserService->getLocalFederatedUser($user->getUID()); | ||
| $userMountRequests[$user->getUID()] = [ | ||
| 'federatedUser' => $federatedUser, | ||
| 'paths' => [], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
| if (!isset($userMountRequests[$user->getUID()])) { | |
| $federatedUser = $this->federatedUserService->getLocalFederatedUser($user->getUID()); | |
| $userMountRequests[$user->getUID()] = [ | |
| 'federatedUser' => $federatedUser, | |
| 'paths' => [], | |
| ]; | |
| } | |
| $userMountRequests[$user->getUID()] ??= [ | |
| 'federatedUser' => $this->federatedUserService->getLocalFederatedUser($user->getUID()), | |
| 'paths' => [], | |
| ]; |
Might be easier to read 🤷♀️
| ]; | ||
| } | ||
|
|
||
| $userMountRequests[$user->getUID()]['paths'][] = '/' . implode('/', array_slice($parts, 3)); |
There was a problem hiding this comment.
Did you check that the leading slash is correct here?
| $this->fixDuplicateFile($uid, $item); | ||
| $mounts[$mountPoint] = $this->generateCircleMount($item, $loader); | ||
| } catch (\Exception $e) { | ||
| $this->logger->warning('issue with teams\' partial mounts', ['exception' => $e]); |
There was a problem hiding this comment.
| $this->logger->warning('issue with teams\' partial mounts', ['exception' => $e]); | |
| $this->logger->error('Failed to create Teams mount', ['exception' => $e]); |
There was a problem hiding this comment.
Should this also rethrow or just continue? Maybe @salmart-dev you can answer that?
Summary
Implemented
IPartialMountProviderinCircleMountProviderby addinggetMountsForPath()and addedlimitToMountpoint()toCoreQueryBuilderto support filtering.Testing
To test this implementation, I did the following:
if (!$this->configService->isLocalInstance($event->getOrigin()))condition inlib/FederatedItems/Files/FileShare::manage()to allow mount entries to be created locallymock_team_1,mock_team_2andmock_team_3mock_folder_1,mock_folder_2andmock_folder_3and shared with their respective folders (mock_team_1,mock_team_2andmock_team_3)returninlib/AppInfo/Application::registerMountProvider()to force registration ofCircleMountProviderregardless of GlobalScale availabilitygetMountsForUser(), accessednextcloud.local/index.php/apps/dashboard/, triggering thegetMountsForUser()methodgetMountsForPath()directly:Results
forChildrenwas set to false. With multiple folders on my setup (mock_team_1,mock_team_2andmock_team_3), in the test above only the requested mounts that exist in the DB were returned for the given paths when callinggetForUser()insidegetMountsForPath(), confirming thatlimitToMountpoints()is filtering correctly at the database level.forChildrendid not work as expected when set to true. During testing, circles always stored mountpoints relatively. For example, sharing a folder created insidemock_foldergenerated acircles_mountentry with mountpoint/mock_subfolderinstead of/mock_folder/mock_subfolder. As a result, theLIKE '/mock_folder/%'filter used whenforChildrenis true will not match these entries.generateCircleMount()threw an exception in both the originalgetMountsForUser()and on new methodgetMountsForPath(). I believe this happened because the mount entries were artificially created by bypassing the remote instance check inFileShare::manage(), resulting in incomplete data compared to what would be present in a real GlobalScale environment.Checklist
3. to review, feature component)stable32)AI (if applicable)