Mounting several external storages over the same mount point causes the "old" storages to get overwritten

[jvillafanez]

Steps to reproduce

1. Mount a external storage in folder "F1"
2. Mount another external storage in the same folder "F1"
3. The first external storage won't be accessible because the "F1" folder will access to the last external storage

Note: the same happens if a local folder "F1" is already created and an external storage is mounted there

Expected behaviour

Either core throws an error telling that it's not possible to mount the external storage in that location (so the app can handle this error by its own) or core rename the mount automatically

Actual behaviour

The last external storage overwrites the other storages making them inaccessible

@MTRichards

[PVince81]

CC @Xenopathic if you're interested

The easiest way would be to show an error.

[MTRichards]

Can we do this in core, so that only 1 mount point can be mounted with a given name?
@craigpg @karlitschek

[PVince81]

@schiesbn @icewind1991 can the logic from the server to server sharing be reused someone to avoid duplicate mount points ?

[icewind1991]

server<->server sharing re-uses the logic from file upload to generate unique names

[MTRichards]

@jvillafanez
Can we reuse that then, and not fail to mount, but increment the name ((1), (2), etc) so duplicates are not created?

[jvillafanez]

As long as we can detect the name collision, there is no problem.

However, I'm not sure who (or what component) should be responsible to detect that name collision (the core or each app using external storage)
I think the core should be responsible to detect this, either by renaming the mount (with whatever logic) or by refusing to mount the storage (throwing an exception at the time to mount the storage, and let each app handle it).

If each app is responsible to detect the collision, first of all, every app (using external storage) MUST implement code to detect the collision, otherwise we won't solve anything because a "failing" app can still override the mount point.
And the second point, is that I'm not sure whether the app should have any knowledge about the whole ownCloud FS. It should just be "I want to mount this storage here". If ownCloud reject it because the target folder exists then the app can try to mount the storage in other place, or take other kind of action

[PVince81]

The code for collision detection could be called from the OC_Mount_Config class (in files_external/lib/config.php), maybe at the time when the mount point is saved. It would automatically save it with an alternative name and somehow needs to notify the called / the UI about it.
It might be easier to detect the collision and just throw an error...

[rperezb]

Could it be possible to detect the collision from the core and throw an error?
The key thing is that if any app using the external storage does not add the detection, it may overwritte other mountpoint.

[RobinMcCorkell]

I think this is a duplicate of #8174 - in which case I'm working on a (possible) solution. It's just more difficult than I anticipated, and I might have to convert the whole client-side representation of the mount points from the current DOM model to a full JS object model

[PVince81]

@Xenopathic why do you need to convert the JS part ? Is it to be able to find out whether the server changed the mount point ? Or are you trying to detect collisions on the JS side ?

Trouble is that collisions can also happen when mounting onto existing folders, I'm not sure whether we want to allow this.
And another case is when a personal mount is mounted on top of a system mount.

If automatically renaming is too complicated then let's go with the "throw an exception" route.

Can you submit a WIP PR where we could assist you somehow ?

[PVince81]

Also note that a mount point name in the UI can be set to a subdirectory like "/mounts/myawesomemount" which might complicate it even more Just in case you guys weren't aware of such cases 😄

[RobinMcCorkell]

@PVince81 I was thinking it through today, I might be able to get away with shunting the data into the DOM model. There needs to be a balance between compactness (DOM model) and code readability (JS object), but hopefully I'll get it right. Collisions between mount points should be detected on the client side.

[PVince81]

@Xenopathic any update on your second part of the fix, the one with validation ?
No need to over-engineer it for now as it's needed soon. 😄

[PVince81]

Note: if your fix for mount point duplication detection is based on #10428 we'll need to backport that as well...

[PVince81]

@Xenopathic if you don't have time to fix it, could you at least push your remaining commits on a branch so someone else could finalize it ?

[RobinMcCorkell]

@PVince81 Wow, I was meaning to reply to this when I got the notification 4 days ago but completely forgot... I do have the commits on a local branch, but there isn't anything substantial there and they would need to be rewritten anyway. I think the core devs need to get together and discuss a proper exception heirarchy and framework before commits of that kind can be merged proper.

[PVince81]

Ok, understood.
We'd still need a quicker fix for the mountpoint conflict validation for 7.0.3, so there is not much time for longer discussion about exception handling.

Let us know whether you'd like to try implementing it or whether someone else should do it 😄

[ghost]

@PVince81 @MTRichards moving this to 9.1 as we look into files_external then?

[MTRichards]

Yes. Seems to solve a problem, and may be wise to work it into files external since we are consolidating there for consistent handling of issues.

[PVince81]

Yeah. I hope we can make some more changes/additions under the hood to make such check possible (oc_mounts table)

[PVince81]

Moving to backlog.

The potential solution is overly complex due to not fitting architecture and this issue doesn't seem to be a major blocker for any one so far.

Ideally we should rework the FS mount system as described in #26190 which would make it possible to catch conflicts in a central place.

[ownclouders]

Hey, this issue has been closed because the label status/STALE is set and there were no updates for 7 days. Feel free to reopen this issue if you deem it appropriate.

(This is an automated comment from GitMate.io.)

[PVince81]

Leaving closed. Requires proper mount manager. See #26190

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.