Fix SharedCache to forward numeric id of wrapped cache #27172

Merged
merged 1 commit into from Feb 21, 2017

Projects

None yet

2 participants

@PVince81
Collaborator

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

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

TODO:

  • investigate all code paths using this method to make sure that returning a proper value doesn't cause other side effects

@jvillafanez crazy stuff...

@PVince81 PVince81 added this to the 10.0 milestone Feb 16, 2017
@PVince81
Collaborator

Before this fix I ran all tests and added this:

diff --git a/lib/private/Files/Cache/Cache.php b/lib/private/Files/Cache/Cache.php
index fb7f9596fe..7f2aa5d1e3 100644
--- a/lib/private/Files/Cache/Cache.php
+++ b/lib/private/Files/Cache/Cache.php
@@ -505,6 +505,13 @@ class Cache implements ICache {
                        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);
+                       }
+                       if (is_null($targetStorageId) || $targetStorageId === false) {
+                               throw new \Exception('Invalid target storage id: ' . $targetStorageId);
+                       }
+
                        // sql for final update
                        $moveSql = 'UPDATE `*PREFIX*filecache` SET `storage` =  ?, `path` = ?, `path_hash` = ?, `name` = ?, `parent` =? WHERE `fileid` = ?';

It seem no test failed, maybe no test covers this code path apart from my new test in #27042

@PVince81
Collaborator

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.

@PVince81
Collaborator

AHA, see a real life manifestation of the bug through the OCS Share API (before the fix): storage is false
storage-false

@PVince81
Collaborator

I grepped the code for getNumericStorageId and all usage occurrences I found were from methods that get overridden by CacheWrapper (from SharedCache) which would redirect the call to the underlying source cache.

The only occurrence is the one I fixed in this PR.
Cache::getMoveInfo() is a protected method and is not part of the ICache interface, so is not overridden by CacheWrapper and so the internal call to getNumericStorageId would reach the top level method from SharedCache when called.

@PVince81
Collaborator

I have now removed SharedCache::getNumericStorageId() because it's parent class CacheWrapper::getNumericStorageId() already does what's necessary.

@PVince81
Collaborator

@jvillafanez please review

@PVince81
Collaborator

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);
@jvillafanez
jvillafanez Feb 17, 2017 Contributor

We might need to throw a better exception.

@PVince81
PVince81 Feb 17, 2017 Collaborator

Any suggestions ? This should actually never happen... I added it here in case we missed some code paths.

@jvillafanez
jvillafanez Feb 20, 2017 Contributor

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.

@jvillafanez
Contributor

I guess this needs to be tested by QA. I can't foresee all the possibilities and error cases of this change

@PVince81
Collaborator

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.

@PVince81
Collaborator

@jvillafanez I just added integration tests for cross-storage move.

  • move into share
  • move out of share: not added, will be part of #27042 where the behavior is changed as well
  • move into ext storage
  • move out of ext storage

These did not trigger the exception in question from above, so it seems to work fine.

@PVince81
Collaborator

@jvillafanez I added the exception in the PHPDoc.

Funnily enough that method isn't on any public interface ๐Ÿ™ˆ

@jvillafanez
Contributor

I added the exception in the PHPDoc.

That's cheating!! ๐Ÿคฃ

I guess it's fine since this is a bug fix. ๐Ÿ‘

@PVince81
Collaborator

@DeepDiver1975 Jenkins needs the boot

14:56:24 Fire up the mysql docker
14:56:24 docker: Error response from daemon: grpc: the connection is unavailable.

(third observed occurrence on three different PRs)

@PVince81 PVince81 Remove SharedCache::getNumericStorageId to let CacheWrapper do it
The CacheWrapper will properly forward the call to the wrapped cache.
b257565
@PVince81
Collaborator

Rebased for CI

@PVince81 PVince81 merged commit af8696c into master Feb 21, 2017

4 checks passed

Scrutinizer 2 new issues, 1 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the sharing-sharedcachenumericid branch Feb 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment