add uploads_path param

[tanganellilore]

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

Summary

Alternative solution instead usage of symlink proposed here: #38506.

In this solution I'm not use the symlink but directly new virtual mount to configuration passed.

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)

[tanganellilore]

Recommit with correct approach suggested in last comment of #38506 from @icewind1991.

Path now it's splitted from cache_path to cache_path and uploads_path . In that way we can choice different paths.

[kesselb]

Recommit with correct approach suggested in last comment of #38506 from @icewind1991.

I don't think we suggested introducing another configuration option. The feature you are looking for is already there.

Would you mind explaining why you need to separate uploads and cache? Did you run into problems?

[tanganellilore]

My only concern is to have a clear segmentation of folder/mountpoint/disk associated.
For example for uploads we need to be sure that we have space on it for chunks uploads (ad we can stimate it), for cache folder, for my knowledge, if not exists or no space left shouldn't be a problem.

Moreover we can decide to use a different mount point in different path, associated to different disk (and size).

But this is an idea/features, and I notice only after my PR and reading code that cache_path is used for cache path and uploads path (and this is not document ed well).

[kesselb]

Does it work?

If cache_path === uploads_path the expected /uploads/ subfolder is created in the cache_path block. But if you define a different uploads_path (not inside the cache_path) who is going to create the uploads subfolder for the user?

[kesselb]

	/**
	 * Get the cache mount for a user
	 *
	 * @param IUser $user
	 * @param IStorageFactory $loader
	 * @return \OCP\Files\Mount\IMountPoint[]
	 */
	public function getMountsForUser(IUser $user, IStorageFactory $loader) {
		$cacheBaseDir = $this->config->getSystemValueString('cache_path', '');
		$uploadsBaseDir = $this->config->getSystemValueString('uploads_path', '');

		if ($cacheBaseDir === '' && $uploadsBaseDir === '') {
			return [];
		}

		$cacheDir = rtrim($cacheBaseDir, '/') . '/' . $user->getUID();
		if (!file_exists($cacheDir)) {
			mkdir($cacheDir, 0770, true);
		}

		if ($uploadsBaseDir === '') {
			$uploadsDir = $cacheDir . '/uploads';
		} else {
			$uploadsDir = rtrim($uploadsBaseDir, '/') . '/' . $user->getUID();
		}

		if (!file_exists($uploadsDir)) {
			mkdir($uploadsDir, 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' => $uploadsDir], $loader, null, null, self::class)
		];
	}

I guess this version should cover the old case and new case.
Yet, we should first merge #38633 and then add a test for uploads_path in this pull requests.

[tanganellilore]

Thanks @kesselb for highlight it. You are correct, I missing the removal of creation uploads user path in the cache if, and creation of it on upload if.

Everything working in my previous commit, because uplaod function check and create uploads folder if not exists.

Now should be fixed, if uploads_path is defined we use it (and create relative user path), if not we use the value of cache_path (and create user uploads folder), if also cache_path is empty, we skip it.

[tanganellilore]

@kesselb Any news on this PR?

[kesselb]

Hi @tanganellilore,

Sorry, I apparently missed your ping last year.

#38633 was merged, and therefore we have unit tests now for the component.

• We need to decide whether we want this feature or not. I see how this configuration flag could be useful and still, it's another piece of code to maintain and something that's not enabled by default and therefore easier to break. If Robin, as expert, is okay with it, I'm not going to disagree.
• The linter, for the php style, is not happy: https://github.com/nextcloud/server/actions/runs/9565047825/job/26367133223?pr=38587
• The existing tests need an update and at best some new: https://github.com/nextcloud/server/actions/runs/9565047804/job/26367283313?pr=38587

Please ping me again if the linter and nodb tests are happy, I will trigger another review then. Note some tests (e.g., cypress) are failing for forks, don't worry.

[skjnldsv]

@susnux @juliushaertl can I get your opinion on this?
That seems interesting, but there might be some underlying issues. Do you have any insights?

[juliusknorr]

Suggested change

	* Defaults to the value of '' if unset.
	* Defaults to `cache_path` if set, otherwise ''

[juliusknorr]

I think this is reasonable, though I currently lack insight on what we actually use the cache folder for. Tend to agree with @kesselb that two separate config values seem to much.

Needs a rebase and resolving the conflict.

[tanganellilore]

Cache folder is used for chunk upload if i remeber well.
It's possible that in some configuration we wanto have cache folder with ssd storage (to be very fast on chunk assembling).

See issue linkedin to this MR.

I think that i can do rebase, but before tell me if this will be merged

[kesselb]

[skjnldsv]

I think that i can do rebase, but before tell me if this will be merged

Would be nice yeah! We're about to start the 31 release schedule. Maybe have this merged soon to be safe?

I would also like some tests for this
If possible 😬

[tanganellilore]

Why closed?

[susnux]

Why closed?

Because it was marked as stale. If you still want to get this feature in and actively develop it then please just reopen the PR :)