Keep fileId for deleted Items in a separate cache

[VicDeo]

Fixes #30762

Description

Store items as path->id before deletion and resolve path into fileId using this cache in FileCustomPropertiesBackend.

Motivation and Context

node is already deleted and the tree cache is purged when the property storage backend is processing the delete callback so it is not possible to resolve the path into id using getNodeForPath
As soon as file custom properties are referenced by fileId it leads to orphaned properties and log entries like this when files/directories are deleted

{"reqId":"yN2bBgP0f4ZlsxbW6o2u","level":2,"time":"2018-03-13T10:14:32+00:00","remoteAddr":"127.0.0.1","user":"admin","app":"dav","method":"DELETE","url":"\/owncloud-core\/remote.php\/dav\/files\/admin\/folder1","message":"Could not get node for path: \"files\/admin\/folder1\" : File with name folder1 could not be located"}

How Has This Been Tested?

1. Create a directory folder1
2. Delete a directory folder1

expected

No log entries

actual

{"reqId":"yN2bBgP0f4ZlsxbW6o2u","level":2,"time":"2018-03-13T10:14:32+00:00","remoteAddr":"127.0.0.1","user":"admin","app":"dav","method":"DELETE","url":"\/owncloud-core\/remote.php\/dav\/files\/admin\/folder1","message":"Could not get node for path: \"files\/admin\/folder1\" : File with name folder1 could not be located"}

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.

[codecov]

Codecov Report

Merging #31093 into master will increase coverage by <.01%.
The diff coverage is 87.5%.

@@             Coverage Diff              @@
##             master   #31093      +/-   ##
============================================
+ Coverage      62.6%    62.6%   +<.01%     
- Complexity    18253    18263      +10     
============================================
  Files          1145     1146       +1     
  Lines         68566    68581      +15     
  Branches       1234     1234              
============================================
+ Hits          42925    42938      +13     
- Misses        25280    25282       +2     
  Partials        361      361

Flag	Coverage Δ	Complexity Δ
#javascript	52.05% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	63.8% <87.5%> (ø)	18263 <9> (+10)	⬆️

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/lib/Server.php	51.07% <0%> (ø)	24 <0> (ø)	⬇️
apps/dav/lib/Connector/Sabre/ServerFactory.php	94.11% <100%> (ø)	11 <0> (ø)	⬇️
apps/dav/lib/DAV/FileCustomPropertiesPlugin.php	87.5% <87.5%> (ø)	4 <4> (?)
apps/dav/lib/DAV/FileCustomPropertiesBackend.php	88.88% <92.85%> (-0.25%)	31 <5> (+6)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b36c56...e950ec9. Read the comment docs.

[PVince81]

use a CappedCache to avoid overflows ? in case of overflow we'd then still see the warnings that this PR is protecting against.

[VicDeo]

it holds just path=>id info that is not stored into DB. For 32bit PHP any string could be up to 2Gb in length,while 64 bit has no limit on the string length.

[PVince81]

I meant CappedCache to avoid having too many items inside this array.

Good that you also checked about string overflow

[PVince81]

This approach reminds me of what we do with trashbin where we take note of information before a delete and use it after, so it matches an existing pattern. I think it's a good idea.

@VicDeo would be good to know if we have any Webdav route that would cause this cache to contain lots of entries. As far as I understand we'd usually only have one entry as DELETE only applies to one item. But maybe if you delete a parent node this would contain all children ?

[VicDeo]

@PVince81

if you delete a parent node this would contain all children

can't say that for sure, need to check.

Regarding coverage: my local tests include the part of the trait that is claimed to be uncovered by codecov. Do I need to refactor the trait into a trivial C&P to convince codecov that real coverage is more than 45%?

[VicDeo]

@PVince81 According to xdebug, DELETE invoked on parent doesn't propagate to children.
The cache is not persistent, it persists within a request only.

[PVince81]

my local tests include the part of the trait that is claimed to be uncovered by codecov

I'd expect codecov to pick it up if the trait's code is running during your unit tests.

DELETE invoked on parent doesn't propagate to children.

Good to know, so it is possible that your new array in the trait will only ever contain a single item. I'd still prefer if you'd use the CappedMemoryCache there in case other people start using this trait so we have no memory overflow surprises. (might need a good doc, maybe rename the attribute to something about "cache" so it's clear that there is expiration involved)

[VicDeo]

@PVince81

• refactored trait into abstract base class, let's see would it be 45% or more.
• replaced array with CappedMemoryCache as you like it more :)

[VicDeo]

@ownclouders rebase

[ownclouders]

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

[ownclouders]

Automated rebase with GitMate.io was successful! 🎉

[VicDeo]

@PVince81 @DeepDiver1975 another review please

[PVince81]

👍 looks great now

[PVince81]

please backport

[VicDeo]

Stable10: #31479

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