-
Notifications
You must be signed in to change notification settings - Fork 58
feat(mount-provider): implement IPartialMountProvider #2378
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1574,6 +1574,31 @@ public function leftJoinMountpoint(string $aliasMount, IFederatedUser $federated | |||||
| } | ||||||
|
|
||||||
|
|
||||||
| /** | ||||||
| * @param string $aliasMount | ||||||
| * @param string[] $paths | ||||||
| * @param bool $forChildren | ||||||
| */ | ||||||
| public function limitToMountpoints(string $aliasMount, array $paths, bool $forChildren = false): void { | ||||||
| if (count($paths) === 0) { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| $expr = $this->expr(); | ||||||
| $orX = $expr->orX(); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe move this into the $forChildren clause, since it's only used there. |
||||||
|
|
||||||
| if ($forChildren) { | ||||||
| foreach ($paths as $path) { | ||||||
| $orX->add($expr->like($aliasMount . '.mountpoint', $this->createNamedParameter($path . '/%'))); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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. |
||||||
| } | ||||||
| } else { | ||||||
| $orX->add($expr->in($aliasMount . '.mountpoint', $this->createNamedParameter($paths, IQueryBuilder::PARAM_STR_ARRAY))); | ||||||
| } | ||||||
|
|
||||||
| $this->andWhere($orX); | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| /** | ||||||
| * @param string $alias | ||||||
| * @param array $default | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -58,16 +58,21 @@ public function delete(string $token): void { | |||||||||
|
|
||||||||||
| /** | ||||||||||
| * @param IFederatedUser $federatedUser | ||||||||||
| * @param string[] $paths | ||||||||||
| * @param bool $forChildren | ||||||||||
|
Comment on lines
60
to
+62
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| * | ||||||||||
| * @return Mount[] | ||||||||||
| * @throws RequestBuilderException | ||||||||||
| */ | ||||||||||
| public function getForUser(IFederatedUser $federatedUser): array { | ||||||||||
| public function getForUser(IFederatedUser $federatedUser, array $paths = [], bool $forChildren = false): array { | ||||||||||
| $qb = $this->getMountSelectSql(); | ||||||||||
| $qb->setOptions([CoreQueryBuilder::MOUNT], ['getData' => true]); | ||||||||||
| $qb->leftJoinMember(CoreQueryBuilder::MOUNT); | ||||||||||
| $qb->leftJoinMountpoint(CoreQueryBuilder::MOUNT, $federatedUser); | ||||||||||
| $qb->limitToInitiator(CoreQueryBuilder::MOUNT, $federatedUser, 'circle_id'); | ||||||||||
| if (count($paths) !== 0) { | ||||||||||
| $qb->limitToMountpoints(CoreQueryBuilder::MOUNT, $paths, $forChildren); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return $this->getItemsFromRequest($qb); | ||||||||||
|
|
||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -29,16 +29,19 @@ | |||||||||||||||||||||||
| use OCP\DB\Exception; | ||||||||||||||||||||||||
| use OCP\Federation\ICloudIdManager; | ||||||||||||||||||||||||
| use OCP\Files\Config\IMountProvider; | ||||||||||||||||||||||||
| use OCP\Files\Config\IPartialMountProvider; | ||||||||||||||||||||||||
| use OCP\Files\Config\MountProviderArgs; | ||||||||||||||||||||||||
| use OCP\Files\Folder; | ||||||||||||||||||||||||
| use OCP\Files\IRootFolder; | ||||||||||||||||||||||||
| use OCP\Files\Mount\IMountPoint; | ||||||||||||||||||||||||
| use OCP\Files\NotFoundException; | ||||||||||||||||||||||||
| use OCP\Files\Storage\IStorageFactory; | ||||||||||||||||||||||||
| use OCP\Http\Client\IClientService; | ||||||||||||||||||||||||
| use OCP\IUser; | ||||||||||||||||||||||||
| use Override; | ||||||||||||||||||||||||
| use Psr\Log\LoggerInterface; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| class CircleMountProvider implements IMountProvider { | ||||||||||||||||||||||||
| class CircleMountProvider implements IMountProvider, IPartialMountProvider { | ||||||||||||||||||||||||
| use TArrayTools; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| public const EXTERNAL_STORAGE = ExternalStorage::class; | ||||||||||||||||||||||||
|
|
@@ -247,4 +250,55 @@ private function generateIncrementedMountpoint(Folder $fs, Mount $mount, IFedera | |||||||||||||||||||||||
| $n++; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #[Override] | ||||||||||||||||||||||||
| public function getMountsForPath(string $setupPathHint, bool $forChildren, array $mountProviderArgs, IStorageFactory $loader): array { | ||||||||||||||||||||||||
| /** @var array<string, array{federatedUser: IFederatedUser, paths: string[]}> $userMountRequests */ | ||||||||||||||||||||||||
| $userMountRequests = []; | ||||||||||||||||||||||||
| /** @var array<string, IMountPoint> $mounts */ | ||||||||||||||||||||||||
| $mounts = []; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** @var MountProviderArgs $mountProviderArg */ | ||||||||||||||||||||||||
| foreach ($mountProviderArgs as $mountProviderArg) { | ||||||||||||||||||||||||
| $user = $mountProviderArg->mountInfo->getUser(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| $parts = explode('/', $mountProviderArg->mountInfo->getMountPoint()); | ||||||||||||||||||||||||
| if ($parts[1] !== $user->getUID() || $parts[2] !== 'files') { | ||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (!isset($userMountRequests[$user->getUID()])) { | ||||||||||||||||||||||||
| $federatedUser = $this->federatedUserService->getLocalFederatedUser($user->getUID()); | ||||||||||||||||||||||||
| $userMountRequests[$user->getUID()] = [ | ||||||||||||||||||||||||
| 'federatedUser' => $federatedUser, | ||||||||||||||||||||||||
| 'paths' => [], | ||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+270
to
+276
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Might be easier to read 🤷♀️ |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| $userMountRequests[$user->getUID()]['paths'][] = '/' . implode('/', array_slice($parts, 3)); | ||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you check that the leading slash is correct here? |
||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| foreach ($userMountRequests as $uid => $userMountRequest) { | ||||||||||||||||||||||||
| $userItems = $this->mountRequest->getForUser( | ||||||||||||||||||||||||
| $userMountRequest['federatedUser'], | ||||||||||||||||||||||||
| $userMountRequest['paths'], | ||||||||||||||||||||||||
| $forChildren | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| foreach ($userItems as $item) { | ||||||||||||||||||||||||
| $mountPoint = '/' . $uid . '/files' . $item->getMountPoint(); | ||||||||||||||||||||||||
| if (isset($mounts[$mountPoint])) { | ||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||
| $this->fixDuplicateFile($uid, $item); | ||||||||||||||||||||||||
| $mounts[$mountPoint] = $this->generateCircleMount($item, $loader); | ||||||||||||||||||||||||
| } catch (\Exception $e) { | ||||||||||||||||||||||||
| $this->logger->warning('issue with teams\' partial mounts', ['exception' => $e]); | ||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this also rethrow or just continue? Maybe @salmart-dev you can answer that?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this throws it would break set up for folders with circle mounts or when setting up a folder with children if any child contains a circle mount, so my first thought would be "it shouldn't throw". Anyways, I'd log error here, since a missing mount is most probably going to be noticed. |
||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return $mounts; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less noise and more precise.