Skip to content

Conversation

@salmart-dev
Copy link
Contributor

Summary

Adds a $forChildren parameter to IPartialMountProvider so that providers have clear information about when they should load a single mount or all mounts in a path.

TODO

  • ...

Checklist

Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com>
@salmart-dev salmart-dev added this to the Nextcloud 33 milestone Dec 29, 2025
@salmart-dev salmart-dev self-assigned this Dec 29, 2025
@salmart-dev salmart-dev added the 3. to review Waiting for reviews label Dec 29, 2025
@salmart-dev salmart-dev requested a review from a team as a code owner December 29, 2025 17:09
@salmart-dev salmart-dev requested review from Altahrim, CarlSchwan, leftybournes, sorbaugh and yemkareems and removed request for a team December 29, 2025 17:09
@icewind1991
Copy link
Member

Would it make sense to have $forChildren return both the mounts for $path and for the children, instead of only the children.

Otherwise, if a folder and it's child-mounts have the same mount provider, we would be calling the provider twice.

Though it might be more work for the provider to deal with the "both" case instead of only for the children.

@salmart-dev
Copy link
Contributor Author

Would it make sense to have $forChildren return both the mounts for $path and for the children, instead of only the children.

Otherwise, if a folder and it's child-mounts have the same mount provider, we would be calling the provider twice.

Though it might be more work for the provider to deal with the "both" case instead of only for the children.

The current flow of setupForPath is to get the mount for the given path and then for the children if $includeChildren is true. This was not a problem before because we were tracking which providers were already set up and skip them when necessary. Now, if we'd have the call for the children also return the mount for $path we'd have to manually remove it which would be cumbersome + cause ugly outcomes in those providers that have side-effects when building mounts (e.g. SharedMount).

Regarding the issue of calling the provider multiple times, I am trying to take extra care so that if it happens, we don't end up setting up the same mounts more than once.

If the setup is called for a folder with $forChildren set to true, any setup call for sub-paths will be a no-op.

@icewind1991
Copy link
Member

Would it make sense to have $forChildren return both the mounts for $path and for the children, instead of only the children.
Otherwise, if a folder and it's child-mounts have the same mount provider, we would be calling the provider twice.
Though it might be more work for the provider to deal with the "both" case instead of only for the children.

The current flow of setupForPath is to get the mount for the given path and then for the children if $includeChildren is true. This was not a problem before because we were tracking which providers were already set up and skip them when necessary. Now, if we'd have the call for the children also return the mount for $path we'd have to manually remove it which would be cumbersome + cause ugly outcomes in those providers that have side-effects when building mounts (e.g. SharedMount).

Regarding the issue of calling the provider multiple times, I am trying to take extra care so that if it happens, we don't end up setting up the same mounts more than once.

If the setup is called for a folder with $forChildren set to true, any setup call for sub-paths will be a no-op.

Fair enough, we can always tweak things in the future.

@salmart-dev salmart-dev merged commit ed6d0e5 into master Dec 30, 2025
206 of 208 checks passed
@salmart-dev salmart-dev deleted the fix/54562/add-forchildren-to-setupforpath branch December 30, 2025 15:37
@salmart-dev salmart-dev added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 30, 2025
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants