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.

Sign up for GitHub

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

Jump to bottom

Store storage availability in database #13641

Merged
merged 2 commits into from
Aug 7, 2015
Merged

Store storage availability in database #13641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df19cab
Store storage availability in database
Jan 23, 2015
75a5e6e
Fix mounting wrapped storages resulting in many-layered wrapping
Mar 7, 2015
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
12 changes: 10 additions & 2 deletions apps/files_external/lib/config.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,8 +494,16 @@ public static function getBackendStatus($class, $options, $isPersonal) {
if (class_exists($class)) {
try {
$storage = new $class($options);
if ($storage->test($isPersonal)) {
return self::STATUS_SUCCESS;

try {
$result = $storage->test($isPersonal);
$storage->setAvailability($result);
if ($result) {
return self::STATUS_SUCCESS;
}
} catch (\Exception $e) {
$storage->setAvailability(false);
throw $e;
}
} catch (Exception $exception) {
\OCP\Util::logException('files_external', $exception);
Expand Down
12 changes: 12 additions & 0 deletions db_structure.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,18 @@
<length>4</length>
</field>

<field>
<name>available</name>
<type>boolean</type>
<default>true</default>
<notnull>true</notnull>
</field>

<field>
<name>last_checked</name>
<type>integer</type>
</field>

<index>
<name>storages_id_index</name>
<unique>true</unique>
Expand Down
48 changes: 38 additions & 10 deletions lib/private/files/cache/storage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,27 +43,25 @@ class Storage {

/**
* @param \OC\Files\Storage\Storage|string $storage
* @param bool $isAvailable
* @throws \RuntimeException
*/
public function __construct($storage) {
public function __construct($storage, $isAvailable = true) {
if ($storage instanceof \OC\Files\Storage\Storage) {
$this->storageId = $storage->getId();
} else {
$this->storageId = $storage;
}
$this->storageId = self::adjustStorageId($this->storageId);

$sql = 'SELECT `numeric_id` FROM `*PREFIX*storages` WHERE `id` = ?';
$result = \OC_DB::executeAudited($sql, array($this->storageId));
if ($row = $result->fetchRow()) {
if ($row = self::getStorageById($this->storageId)) {
$this->numericId = $row['numeric_id'];
} else {
$connection = \OC_DB::getConnection();
if ($connection->insertIfNotExist('*PREFIX*storages', ['id' => $this->storageId])) {
if ($connection->insertIfNotExist('*PREFIX*storages', ['id' => $this->storageId, 'available' => $isAvailable])) {
$this->numericId = \OC_DB::insertid('*PREFIX*storages');
} else {
$result = \OC_DB::executeAudited($sql, array($this->storageId));
if ($row = $result->fetchRow()) {
if ($row = self::getStorageById($this->storageId)) {
$this->numericId = $row['numeric_id'];
} else {
throw new \RuntimeException('Storage could neither be inserted nor be selected from the database');
Expand All @@ -72,6 +70,16 @@ public function __construct($storage) {
}
}

/**
* @param string $storageId
* @return array|null
*/
public static function getStorageById($storageId) {
$sql = 'SELECT * FROM `*PREFIX*storages` WHERE `id` = ?';
$result = \OC_DB::executeAudited($sql, array($storageId));
return $result->fetchRow();
}

/**
* Adjusts the storage id to use md5 if too long
* @param string $storageId storage id
Expand Down Expand Up @@ -120,15 +128,35 @@ public static function getStorageId($numericId) {
public static function getNumericStorageId($storageId) {
$storageId = self::adjustStorageId($storageId);

$sql = 'SELECT `numeric_id` FROM `*PREFIX*storages` WHERE `id` = ?';
$result = \OC_DB::executeAudited($sql, array($storageId));
if ($row = $result->fetchRow()) {
if ($row = self::getStorageById($storageId)) {
return $row['numeric_id'];
} else {
return null;
}
}

/**
* @return array|null [ available, last_checked ]
*/
public function getAvailability() {
Copy link
Member

Choose a reason for hiding this comment

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

I'd merge this into one function where all information is returned. With the current design two queries will be fired to get availability and last checked

if ($row = self::getStorageById($this->storageId)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's not possible to retrieve + cache this together when reading the full storage entry ? (to avoid extra SQL calls)

Copy link
Member Author

Choose a reason for hiding this comment

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

The caching would be done in getStorageById(), since a couple of functions use that. But, prior to this PR, we never cached it (see getNumericStorageId() above, and the constructor). Do we want that in the scope of this PR?

return [
'available' => $row['available'],
'last_checked' => $row['last_checked']
];
} else {
return null;
}
}

/**
* @param bool $isAvailable
*/
public function setAvailability($isAvailable) {
$sql = 'UPDATE `*PREFIX*storages` SET `available` = ?, `last_checked` = ? WHERE `id` = ?';
\OC_DB::executeAudited($sql, array($isAvailable, time(), $this->storageId));
}

/**
* Check if a string storage id is known
*
Expand Down
7 changes: 6 additions & 1 deletion lib/private/files/mount/mountpoint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
use \OC\Files\Filesystem;
use OC\Files\Storage\StorageFactory;
use OC\Files\Storage\Storage;
use OC\Files\Storage\Wrapper\Wrapper;
use OCP\Files\Mount\IMountPoint;

class MountPoint implements IMountPoint {
Expand Down Expand Up @@ -92,7 +93,11 @@ public function __construct($storage, $mountpoint, $arguments = null, $loader =
$this->mountPoint = $mountpoint;
if ($storage instanceof Storage) {
$this->class = get_class($storage);
$this->storage = $this->loader->wrap($this, $storage);
$this->storage = $storage;
// only wrap if not already wrapped
if (!($this->storage instanceof Wrapper)) {
$this->storage = $this->loader->wrap($this, $this->storage);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required by this PR or is an additional fix ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this fix, unit tests will fail, since our unit tests attempt to re-wrap storages sometimes causing function nesting limit failures. In the actual codebase it makes no difference, since we never pass a wrapped storage to be re-wrapped.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, this is a poor fix. There may be usecases where instead of a Storage being passed in, a Wrapper is (aka pre-wrapped storage) but still needs to be wrapped with the other storage wrappers too.

}
} else {
// Update old classes to new namespace
if (strpos($storage, 'OC_Filestorage_') !== false) {
Expand Down
19 changes: 19 additions & 0 deletions lib/private/files/storage/common.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,11 @@ public function cleanPath($path) {
return implode('/', $output);
}

/**
* Test a storage for availability
*
* @return bool
*/
public function test() {
if ($this->stat('')) {
return true;
Expand Down Expand Up @@ -650,4 +655,18 @@ public function releaseLock($path, $type, ILockingProvider $provider) {
public function changeLock($path, $type, ILockingProvider $provider) {
$provider->changeLock('files/' . md5($this->getId() . '::' . trim($path, '/')), $type);
}

/**
* @return array [ available, last_checked ]
*/
public function getAvailability() {
return $this->getStorageCache()->getAvailability();
}

/**
* @param bool $isAvailable
*/
public function setAvailability($isAvailable) {
$this->getStorageCache()->setAvailability($isAvailable);
}
}
Loading