-
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
New Cacheable Flysystem Adapter #29077
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,171 @@ | ||
<?php | ||
/** | ||
* @author Hemant Mann <hemant.mann121@gmail.com> | ||
* | ||
* @copyright Copyright (c) 2017, ownCloud GmbH | ||
* @license AGPL-3.0 | ||
* | ||
* This code is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU Affero General Public License, version 3, | ||
* as published by the Free Software Foundation. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU Affero General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU Affero General Public License, version 3, | ||
* along with this program. If not, see <http://www.gnu.org/licenses/> | ||
* | ||
*/ | ||
|
||
namespace OC\Files\Storage; | ||
|
||
use League\Flysystem\FileNotFoundException; | ||
use Icewind\Streams\IteratorDirectory; | ||
|
||
/** | ||
* Generic Cacheable adapter between flysystem adapters and owncloud's storage system | ||
* | ||
* To use: subclass and call $this->buildFlysystem with the flysystem adapter of choice | ||
*/ | ||
abstract class CacheableFlysystem extends \OCP\Files\Storage\FlysystemStorageAdapter { | ||
/** | ||
* Stores the results in cache for the current request to prevent multiple requests to the API | ||
* | ||
* @var array | ||
*/ | ||
protected $cacheContents = []; | ||
|
||
/** | ||
* Get the location which will be used as a key in cache | ||
* If Storage is not case sensitive then convert the key to lowercase | ||
* | ||
* @param string $path Path to file/folder | ||
* @return string | ||
*/ | ||
public function getCacheLocation($path) { | ||
$location = $this->buildPath($path); | ||
if ($location === '') { | ||
$location = '/'; | ||
} | ||
return $location; | ||
} | ||
|
||
public function buildPath($path) { | ||
$location = parent::buildPath($path); | ||
if ($this->isCaseInsensitiveStorage) { | ||
$location = strtolower($location); | ||
} | ||
return $location; | ||
} | ||
|
||
/** | ||
* Check if Cache Contains the data for given path, if not then get the data | ||
* from flysystem and store it in cache for this request lifetime | ||
* | ||
* @param string $path Path to file/folder | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. document There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done see below this line |
||
* @param boolean $overRideCache Whether to fetch the contents from Cache or directly from the API | ||
* @return array|boolean | ||
*/ | ||
public function getFlysystemMetadata($path, $overRideCache = false) { | ||
$location = $this->getCacheLocation($path); | ||
if (!isset($this->cacheContents[$location]) || $overRideCache) { | ||
try { | ||
$this->cacheContents[$location] = $this->flysystem->getMetadata($location); | ||
} catch (FileNotFoundException $e) { | ||
// do not store this info in cache as it might interfere with Upload process | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should look into storing this info at some point. I know it works because we do this for other storages like DAV. Maybe the upload interference is a missing stat clear in another place in the code. I suggest to look into this separately. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes i faced some problem with upload process if i stored the info that file is not present in storage. i don't remember exactly what it was |
||
return false; | ||
} | ||
} | ||
return $this->cacheContents[$location]; | ||
} | ||
|
||
/** | ||
* Store the list of files/folders in the cache so that subsequent requests in the | ||
* same request life cycle does not call the flysystem API | ||
* | ||
* @param array $contents Return value of $this->flysystem->listContents | ||
*/ | ||
public function updateCache($contents) { | ||
foreach ($contents as $object) { | ||
$path = $object['path']; | ||
$this->cacheContents[$path] = $object; | ||
} | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function opendir($path) { | ||
try { | ||
$location = $this->buildPath($path); | ||
$content = $this->flysystem->listContents($location); | ||
$this->updateCache($content); | ||
} catch (FileNotFoundException $e) { | ||
return false; | ||
} | ||
$names = array_map(function ($object) { | ||
return $object['basename']; | ||
}, $content); | ||
return IteratorDirectory::wrap($names); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function fopen($path, $mode) { | ||
switch ($mode) { | ||
case 'r': | ||
case 'rb': | ||
return parent::fopen($path, $mode); | ||
|
||
default: | ||
unset($this->cacheContents[$this->getCacheLocation($path)]); | ||
return parent::fopen($path, $mode); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it should be possible to invalidate the cache after an upload, see how it's done here: https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/DAV.php#L405 and https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/DAV.php#L503 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
But then I'll have to modify the original Flysystem.php which does not have any concept of cache I added it before to prevent copy/paste of duplicate code and even if file upload failed the cache is only set for a single request so I did not think it would be an issue |
||
} | ||
return false; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function filesize($path) { | ||
if ($this->is_dir($path)) { | ||
return 0; | ||
} else { | ||
$info = $this->getFlysystemMetadata($path); | ||
return $info['size']; | ||
} | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function filemtime($path) { | ||
$info = $this->getFlysystemMetadata($path); | ||
return $info['timestamp']; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function stat($path) { | ||
$info = $this->getFlysystemMetadata($path); | ||
return [ | ||
'mtime' => $info['timestamp'], | ||
'size' => $info['size'] | ||
]; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function filetype($path) { | ||
if ($path === '' or $path === '/' or $path === '.') { | ||
return 'dir'; | ||
} | ||
$info = $this->getFlysystemMetadata($path); | ||
return $info['type']; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
<?php | ||
/** | ||
* @author Hemant Mann <hemant.mann121@gmail.com> | ||
* | ||
* @copyright Copyright (c) 2017, ownCloud GmbH | ||
* @license AGPL-3.0 | ||
* | ||
* This code is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU Affero General Public License, version 3, | ||
* as published by the Free Software Foundation. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU Affero General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU Affero General Public License, version 3, | ||
* along with this program. If not, see <http://www.gnu.org/licenses/> | ||
* | ||
*/ | ||
|
||
namespace OCP\Files\Storage; | ||
|
||
/** | ||
* Public Cacheable storage adapter to be extended to connect to | ||
* flysystem adapters | ||
* | ||
* @since 10.0.4 | ||
*/ | ||
abstract class CacheableFlysystemAdapter extends \OC\Files\Storage\CacheableFlysystem { | ||
/** | ||
* Check that if storage is case insensitive or not | ||
* @var boolean | ||
*/ | ||
protected $isCaseInsensitiveStorage = false; | ||
} |
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.
ewww, is that a thing in Dropbox ?!
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.
Yes unfortunately it is
Suppose you have the following directory structure
In one case dropbox api was returning the folder name Foo as both Foo and foo due to which the directory list of the folder Foo was coming as empty but in reality it was not