Scanner repairs invalid path

[PVince81]

⚠️ EXPERIMENTAL ⚠️

Description

Whenever the path doesn't match the parent/child relationship, the path
value is corrected in the oc_filecache table.

Related Issue

Fixes the fallout created by the bug #28018

Motivation and Context

Because we hate filecache inconsistencies

How Has This Been Tested?

Run the steps from #28018.
Run the query from #28018 (comment) to find the broken entry.
Run occ files:scan --all
Check the broken entry again: the path is corrected.

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

TODOs:

• add error handling in case we can't fix the path, for example if there is already an entry with that path
• check what happens if people continue using the broken folder, whether it's still repairable
• unit test

[jvillafanez]

Does the path variable contains the whole file path or just the directory?

[PVince81]

Both contain a value relative to the user's home, like "files/folderA/one"

[jvillafanez]

👍

[jvillafanez]

Code looks fine. 👍

[PVince81]

Thanks. I will not merge it yet because need to think of more twisted scenarios where the target entry to rename to already exists in oc_filecache.

[PVince81]

To check:

• is it possible to manually create the target entry through user action and through the original bug and block the repair ?
  • have the target created through user action
  • have the target created by admin who might have tried to fiddle with data folder + occ files:scan
  • other twisted ways ?

To decide:

• what to do ?
  • fail and log a warning ? (the failure will appear every time the scan happens, so might need to provide a clue for an admin to make a decision which entry to keep)
  • assume target is bogus and delete it (potential metadata loss)
  • assume source is bogus and delete it (potential metadata loss)
  • ?

[PVince81]

side note:

• also try reproducing the breakage by moving through two received shares but not from the same user, in case the result is slightly different

[PVince81]

• what happens if someone deletes the bogus entries, do they land broken in trash ? might need to repair that too, or at least discard

[PVince81]

Turns out that if you write anything inside the bogus "folderB/one/two" folder through Webdav, the code accesses it by a mechanism different than "path". And since the code finds out that the parent entry isn't in the oc_filecache, it automatically creates it.

So it is indeed possible to block the target repair. Running the current code from this PR doesn't repair anything and the old entries stay there.

If we decide to overwrite the new target (folderB/one/two) with the old source (folderA/one/two) then we lose the potentially new metadata that the user might have added. I think that the probability that the relevant metadata to keep is higher for the source "folderA/one/two", so we could use that...

[PVince81]

Oh... the reason why this PR doesn't fix anythting is because it does not find the problem while scanning, since there is no hole in the tree. So fixing the case I just mentioned above would require a completely different approach and location in the code. Maybe a repair step would be fine as it would run during an update and be done with it (assuming there is no other bug that recreates this broken situation)

[PVince81]

When we delete the target entry, if that one happens to have metadata already, we could try and move it to the source entry. Unfortunately this is complex and requires a lot of testing: this would require finding all shares, tags, etc assigned to the target's fileid and then changing that fileid to point to the source. And we can't cover third party apps.

[PVince81]

I don't want to waste too much time with this so let's go with this approach:

• rewrite the PR to be a RepairStep instead
• the repair step uses some of the queries used in the original ticket to find broken entries
• fix each entry. If a target entry already exists for that entry, just delete it (yes, we need to make a cut somewhere) and also write a log about it

Having it in RepairStep form means it only runs once after an update, which might be ok for now. One can always rerun repair steps manually if any other kind of undiscovered bug recreates the broken situation.

[PVince81]

Uh oh... what if the storage value is not the same between parent and child ?

• check if the breakage is doable by doing between two different storages (ex: external storages)

[PVince81]

• test with more sub-entries in the source storage as it would result in multiple broken entries for a single MOVE

[PVince81]

Obsoleted by #28253

[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.