-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix SharedCache to forward numeric id of wrapped cache #27172
Conversation
Before this fix I ran all tests and added this:
It seem no test failed, maybe no test covers this code path apart from my new test in #27042 |
Oh well, let's add it here as well. At least we can have this in 10.0 but I wouldn't backport unless we find a real bug associated with this. |
416c1be
to
32ff7ce
Compare
I grepped the code for The only occurrence is the one I fixed in this PR. |
32ff7ce
to
2ff43ce
Compare
I have now removed |
@jvillafanez please review |
Tests passed on Jenkins but unpublished |
@@ -505,6 +505,13 @@ public function moveFromCache(ICache $sourceCache, $sourcePath, $targetPath) { | |||
list($sourceStorageId, $sourcePath) = $sourceCache->getMoveInfo($sourcePath); | |||
list($targetStorageId, $targetPath) = $this->getMoveInfo($targetPath); | |||
|
|||
if (is_null($sourceStorageId) || $sourceStorageId === false) { | |||
throw new \Exception('Invalid source storage id: ' . $sourceStorageId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to throw a better exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any suggestions ? This should actually never happen... I added it here in case we missed some code paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc says it throws a \OC\DatabaseException
. I think it could fit if the expected handling for that exception matches what we want to do. We'll need to change the docs if it doesn't fit.
I guess this needs to be tested by QA. I can't foresee all the possibilities and error cases of this change |
I already looked at all code paths leading to it here #27172 (comment). Maybe the additional things to test, just in case, is related to cross-storage moving. I'll check if there are integration tests for this. |
@jvillafanez I just added integration tests for cross-storage move.
These did not trigger the exception in question from above, so it seems to work fine. |
9533adf
to
07b9902
Compare
@jvillafanez I added the exception in the PHPDoc. Funnily enough that method isn't on any public interface 🙈 |
That's cheating!! 🤣 I guess it's fine since this is a bug fix. 👍 |
@DeepDiver1975 Jenkins needs the boot
(third observed occurrence on three different PRs) |
The CacheWrapper will properly forward the call to the wrapped cache.
07b9902
to
b257565
Compare
Rebased for CI |
add backport of this onto #27172 which requires it |
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. |
Description
The SharedCache is a CacheJail which is a CacheWrapper.
Its method
getNumericStorageId()
was returning a non-existing attribute value$this->numericId
which was always false.This fix makes sure it returns the numeric id of the underlying
$this->sourceCache
instead.Related Issue
None
Motivation and Context
Because it fixes the unit test for #27042 where a cross-storage move requires the numeric id of the source storage (shared cache).
How Has This Been Tested?
Unit test only and ran all tests locally on #27042 with mysql.
Screenshots (if appropriate):
Types of changes
Checklist:
TODO:
@jvillafanez crazy stuff...