Add uploadsdirectory param

[tanganellilore]

• Resolves: Add support to custom user uploads dir #38505

Summary

This PR will address the feature above, adding new config parameter and manage it.

The "smart" solution adopted is to use the conjunction of param that already exists localstorage.allowsymlinks and new config param uploadsdirectory.
The idea is to use a symlink to avoid the rework in all files/functions where <user_id>/uploads it's used.

The only piece of code that require changes is apps/dav/lib/Upload/UploadHome.php that check if localstorage.allowsymlinks is true and uploadsdirectory is setted, if yes we create symlink (if not already exists), if not, we continue to use the current implementation.

In case of user changes localstorage.allowsymlinks or uploadsdirectory, we will delete current link (or directory) and recreate it (link or dir based on settings).

TODO

• ...

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[come-nc]

Comments on code

[come-nc]

Small comments on codestyle.
Codewise this looks good to me. I’d just like other opinion on the chosen solution to rely on symlink for this, does anyone see this causing trouble in the future?

[tanganellilore]

Thnaks @come-nc ,

the other alternative, is to rework where chunk uploads are store, without usage of symlink, but I don't have a lot fo experience on nextcloud codebase to identfy where uploads folder is used (i suppose on ChunkPlugin, but i don't know if somewhere else)

[artonge]

Not 100% sure about the current logic, I left a few comments to make is more readable.

Also, did you consider changing the code to directly write files to the PHP tmp folder? Probably a bigger change, but it would remove the need of creating and maintaining symlinks.
Or we could also append each successive chunks to the same final file, which would also reduce the read/write to 1. But chunks are probably coming unordered.

[artonge]

More explicit check?

Suggested change

	if ($allowSymlinks && $uploadsPath !== '' && $absoluteUserPath) {
	if ($allowSymlinks && $uploadsPath !== '' && $absoluteUserPath !== null) {

[artonge]

We do we check for !$rootView->file_exists($userPath)?

[artonge]

I would invert the if/else statement to have a more readable condition.

Suggested change

	if (!$rootView->file_exists($userPath) || !is_link($absoluteUserPath) || ($uploadUserPath != realpath($absoluteUserPath)) ) {
	if ($rootView->file_exists($userPath) && is_link($absoluteUserPath) && ($uploadUserPath === realpath($absoluteUserPath)) ) {

[artonge]

More explicit check?

Suggested change

	if ($absoluteUserPath && is_link($absoluteUserPath)) {
	if ($absoluteUserPath !== null && is_link($absoluteUserPath)) {

[artonge]

Could you also check if adding tests is doable?

[tanganellilore]

As already commented, we can think to use directly a path like tmp for php, but i don't know which function need to be rewrite, in each piece/application.

If everything is under this repo, i can check it, but i don't know all dependecies

[tanganellilore]

@come-nc and @artonge I proposed new PR that should be address removal of symlink.

Let me know if I need to fix this one or we move to the "new" approach.

Thanks

[icewind1991]

The use case of having the uploads go somewhere else is already supported by setting the cache_path config value:

When set, uploads will go into $cache_path/$userId/uploads

server/lib/private/Files/Mount/CacheMountProvider.php

Lines 54 to 70 in 426c034

	public function getMountsForUser(IUser $user, IStorageFactory $loader) {
	$cacheBaseDir = $this->config->getSystemValueString('cache_path', '');
	if ($cacheBaseDir !== '') {
	$cacheDir = rtrim($cacheBaseDir, '/') . '/' . $user->getUID();
	if (!file_exists($cacheDir)) {
	mkdir($cacheDir, 0770, true);
	mkdir($cacheDir . '/uploads', 0770, true);
	}
	return [
	new MountPoint('\OC\Files\Storage\Local', '/' . $user->getUID() . '/cache', ['datadir' => $cacheDir], $loader, null, null, self::class),
	new MountPoint('\OC\Files\Storage\Local', '/' . $user->getUID() . '/uploads', ['datadir' => $cacheDir . '/uploads'], $loader, null, null, self::class)
	];
	} else {
	return [];
	}
	}

[tanganellilore]

Thanks @icewind1991, Yes i notice it when i work in the new PR, but this means that one config cover two different path.
I think to maintain 2 path separated with 2 different config.

What so you think? If yes we can consider only my last PR and close this one.

From my point of view, other PR #38587 it's much correct (and will be documented)

[tanganellilore]

Close in favour of #38587