Skip to content
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

Merged
merged 4 commits into from
Nov 3, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 171 additions & 0 deletions lib/private/Files/Storage/CacheableFlysystem.php
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) {
Copy link
Contributor

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 ?!

Copy link
Contributor Author

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

Foo/
├── bar
│   ├── foo.txt
│   └── some_another_file.txt
└── somefile.txt

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

$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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document $overRideCache

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@Hemant-Mann Hemant-Mann Oct 10, 2017

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@Hemant-Mann Hemant-Mann Oct 10, 2017

Choose a reason for hiding this comment

The 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

But then I'll have to modify the original Flysystem.php which does not have any concept of cache
OR
i'll have to duplicate the fopen function here and then add cache clear line after upload

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'];
}
}
36 changes: 36 additions & 0 deletions lib/public/Files/Storage/CacheableFlysystemAdapter.php
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;
}