Skip to content
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

Ignore shared Collective mounts when scanning files #42497

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

arnowelzel
Copy link
Contributor

@arnowelzel arnowelzel commented Dec 27, 2023

Summary

When scanning files using occ files:scan, shared Collective mounts trigger "Path not found" error messages. This change will make sure that that Collective mounts will be ignored.

Checklist

@arnowelzel
Copy link
Contributor Author

/backport to stable28

Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change, looks good to me :)

@emoral435
Copy link
Contributor

Fast review ~!

Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but let's wait for @max-nextcloud or @mejo- input :)

@max-nextcloud
Copy link
Contributor

@mejo- is the better person when it comes to backend changes.

I have some uninformed questions though...

  • What will happen if collectives is not installed and the used class is not available?
    Will php be happy with that?
  • You are talking about a shared collective - are you referring to a collective that has been explicitly shared by link or a normal collective with multiple members?

In general it seems not ideal to me to have collectives specific code in server. I wonder if we could instead make CollectiveStorage a SharedStorage as that could happen on the collectives side of things.

@max-nextcloud
Copy link
Contributor

I remember that collectives storage was designed closely to what groupfolders does. Does this issue not occur with groupfolders ? How is the groupfolders implementation different from collectives?

@arnowelzel
Copy link
Contributor Author

@mejo- is the better person when it comes to backend changes.

I have some uninformed questions though...

  • What will happen if collectives is not installed and the used class is not available?
    Will php be happy with that?

I tested this case and had no problem without Collectives installed, since instanceOfStorage() will only check the class name and it should not matter, if the class actually exists.

But I agree it would be better not to rely on multiple classes here but instead have a common method which can be used. Maybe adding a method isShared() to the Storage interface may help. This sould return true by SharedStorage or CollectiveStorage and otherwise false so the scanner knows when to ignore the storage:

			// don't scan shared storages, these can be scanned when scanning the owner's storage
			if ($storage->isShared()) {
				continue;
			}

However this would involve substantial refactoring of the backend code.

  • You are talking about a shared collective - are you referring to a collective that has been explicitly shared by link or a normal collective with multiple members?

I refer to a normal collective which is shared with multiple members of a circle.

@skjnldsv
Copy link
Member

@max-nextcloud @mejo- any update?

Signed-off-by: Arno Welzel <github@arnowelzel.de>
Signed-off-by: Arno Welzel <github@arnowelzel.de>
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 24, 2024
lib/private/Files/Utils/Scanner.php Fixed Show fixed Hide fixed
lib/private/Files/Utils/Scanner.php Fixed Show resolved Hide resolved
@skjnldsv skjnldsv added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Feb 24, 2024
@skjnldsv
Copy link
Member

@arnowelzel psalm said no

@arnowelzel
Copy link
Contributor Author

arnowelzel commented Feb 25, 2024

Since using the class name of the Collective storage is not possible in any way without triggering a psalm error (even when the class exists it is not allowed as parameter for \OC\Files\Storage\Wrapper\Wrapper::instanceOfStorage()) I changed the test as following:

			// don't scan received local shares or collectives, these can be scanned when scanning the owner's storage
			if (substr($storage->getId(), 0, 6) !== 'home::') {
				continue;
			}

The idea here is, that only home storages should get scanned. Recieved shared storage start with shared:: and Collectives, which trigger the error, report IDs as local:: followed by the physical path. However I don't know if any other storages which should indeed be scanned, also report an ID starting with something different from home::.

I also noticed the following code in \OCA\Collectives\Mount\CollectiveStorage - maybe in the past this problem was mitigated by using NoopScanner which does not exist any longer?

	public function getScanner($path = '', $storage = null): Scanner {
		if (!$storage) {
			$storage = $this;
		}
		if ($storage->instanceOfStorage(ObjectStoreStorage::class)) {
			// NoopScanner is private API and kept here for compatibility with older releases
			$storage->scanner = class_exists(NoopScanner::class) ? new NoopScanner($storage) : new ObjectStoreScanner($storage);
		} elseif (!isset($storage->scanner)) {
			$storage->scanner = new Scanner($storage);
		}
		return $storage->scanner;
	}

@mejo-
Copy link
Member

mejo- commented Mar 7, 2024

Probably @icewind1991 is the best to decide whether it's a good idea to limit scanning to home storages.

// don't scan received local shares, these can be scanned when scanning the owner's storage
if ($storage->instanceOfStorage(SharedStorage::class)) {
// don't scan received local shares or collectives, these can be scanned when scanning the owner's storage
if (substr($storage->getId(), 0, 6) !== 'home::') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break scanning of external storages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What ID do external storages have?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they all have different prefixes, in general attempts to filter by storage id is imprecise at best.

@icewind1991
Copy link
Member

What is stopping the collectives from being scanned correctly in the first place?

@icewind1991 icewind1991 closed this Mar 8, 2024
@icewind1991 icewind1991 reopened this Mar 8, 2024
@arnowelzel
Copy link
Contributor Author

What is stopping the collectives from being scanned correctly in the first place?

They are shared storages like SharedStorage and thus don't provide a physical path which can be scanned.

@icewind1991
Copy link
Member

If they can't be scanned at all then having them use NoopScanner is probably the best (shared storage should also move to that really)

@arnowelzel
Copy link
Contributor Author

If they can't be scanned at all then having them use NoopScanner is probably the best (shared storage should also move to that really)

Well - NoopScanner does not exist any longer.

@icewind1991
Copy link
Member

Right, it got moved to ObjectStoreScanner when it got slightly less noop, should be good to still use that one or just create your own

@arnowelzel
Copy link
Contributor Author

Right, it got moved to ObjectStoreScanner when it got slightly less noop, should be good to still use that one or just create your own

It seems, this is what Collectives is using right now - but it does not work. See https://github.com/nextcloud/collectives/blob/main/lib/Mount/CollectiveStorage.php#L62-L74:

public function getScanner($path = '', $storage = null): Scanner {
		if (!$storage) {
			$storage = $this;
		}
		if ($storage->instanceOfStorage(ObjectStoreStorage::class)) {
			// NoopScanner is private API and kept here for compatibility with older releases
			$storage->scanner = class_exists(NoopScanner::class) ? new NoopScanner($storage) : new ObjectStoreScanner($storage);
		} elseif (!isset($storage->scanner)) {
			$storage->scanner = new Scanner($storage);
		}
		return $storage->scanner;
	}

@susnux susnux added this to the Nextcloud 30 milestone Apr 18, 2024
This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Shared Collectives cause "Path not found" when scannig files with "occ files:scan --all"
8 participants