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

The Great Fix of object store acceptance tests .... #31285

Merged
merged 12 commits into from
May 17, 2018
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
3 changes: 3 additions & 0 deletions apps/dav/lib/DAV/CopyPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use OCA\DAV\Connector\Sabre\File;
use OCA\DAV\Files\ICopySource;
use OCP\Files\ForbiddenException;
use OCP\Lock\ILockingProvider;
use Sabre\DAV\IFile;
use Sabre\DAV\Server;
use Sabre\DAV\ServerPlugin;
Expand Down Expand Up @@ -83,7 +84,9 @@ public function httpCopy(RequestInterface $request, ResponseInterface $response)
$copySuccess = $sourceNode->copy($destinationNode->getFileInfo()->getPath());
}
if (!$copySuccess) {
$destinationNode->acquireLock(ILockingProvider::LOCK_SHARED);
$destinationNode->put($sourceNode->get());
Copy link
Contributor

Choose a reason for hiding this comment

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

$destinationNode->put itself is not setting locks ? Need to go deeper, it is a logic of put itself, isnt it?

Copy link
Member Author

Choose a reason for hiding this comment

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

search for other places there put is used - the node is locked before

$destinationNode->releaseLock(ILockingProvider::LOCK_SHARED);
}

$this->server->emit('afterBind', [$copyInfo['destination']]);
Expand Down
4 changes: 2 additions & 2 deletions lib/private/Files/Cache/Propagator.php
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,9 @@ public function propagateChange($internalPath, $time, $sizeDifference = 0) {
->where($builder->expr()->eq('storage', $builder->createNamedParameter($storageId, IQueryBuilder::PARAM_INT)))
->andWhere($builder->expr()->in('path_hash', $hashParams))
->andWhere($builder->expr()->gt('size', $builder->expr()->literal(-1, IQueryBuilder::PARAM_INT)));
}

$builder->execute();
$builder->execute();
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
Member Author

Choose a reason for hiding this comment

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

🔀

}
}

protected function getParents($path) {
Expand Down
2 changes: 1 addition & 1 deletion lib/private/Files/ObjectStore/NoopScanner.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public function scanFile($file, $reuseExisting = 0, $parentId = -1, $cacheData =
public function scan($path, $recursive = self::SCAN_RECURSIVE, $reuse = -1, $lock = true) {
// we only update the checksums - still returning no data
$meta = $this->storage->getMetaData($path);
if (isset($meta['checksum'])) {
if (!empty($meta['checksum'])) {
$this->storage->getCache()->put(
$path,
['checksum' => $meta['checksum']]
Expand Down
3 changes: 2 additions & 1 deletion lib/private/Share20/DefaultShareProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,8 @@ private function isAccessibleResult($data) {
// exclude shares leading to trashbin on home storages
$pathSections = \explode('/', $data['path'], 2);
// FIXME: would not detect rare md5'd home storage case properly
if ($pathSections[0] !== 'files' && \explode(':', $data['storage_string_id'], 2)[0] === 'home') {
$storagePrefix = \explode(':', $data['storage_string_id'], 2)[0];
if ($pathSections[0] !== 'files' && \in_array($storagePrefix, ['home', 'object'], true)) {
Copy link
Contributor

@mrow4a mrow4a May 14, 2018

Choose a reason for hiding this comment

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

Why object? Could you add some doc explaining what is that for?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are basically two different types of storage prefixes: 'home' for regular file system and 'object' for objectstore - the later part was missing in some places - fixed in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, makes sense!

return false;
}
return true;
Expand Down
1 change: 1 addition & 0 deletions lib/private/User/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ public function delete() {

// Delete the users entry in the storage table
Storage::remove('home::' . $this->getUID());
Storage::remove('object::user:' . $this->getUID());
Copy link
Contributor

Choose a reason for hiding this comment

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

so there are now two homes for user?

Copy link
Member Author

Choose a reason for hiding this comment

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

see above

Copy link
Contributor

Choose a reason for hiding this comment

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

can be something else then X=user in object::X: ?


\OC::$server->getCommentsManager()->deleteReferencesOfActor('users', $this->getUID());
\OC::$server->getCommentsManager()->deleteReadMarksFromUser($this);
Expand Down
8 changes: 8 additions & 0 deletions lib/private/legacy/util.php
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,14 @@ public static function copyr($source, \OCP\Files\Folder $target) {
}
$child = $target->newFile($file);
\stream_copy_to_stream($sourceFileHandle, $child->fopen('w'));
\fclose($child);
\fclose($sourceFileHandle);

// update cache sizes
$cache = $target->getStorage()->getCache();
if ($cache instanceof \OC\Files\Cache\Cache) {
$cache->correctFolderSize($child->getInternalPath());
}
}
}
}
Expand Down
2 changes: 0 additions & 2 deletions tests/acceptance/features/apiFederation/federated.feature
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ Feature: federated
| storage | A_NUMBER |
| mail_send | 0 |
| uid_owner | user0 |
| storage_id | home::user0 |
| file_parent | A_NUMBER |
| displayname_owner | user0 |
| share_with | user1@REMOTE |
Expand All @@ -53,7 +52,6 @@ Feature: federated
| storage | A_NUMBER |
| mail_send | 0 |
| uid_owner | user1 |
| storage_id | home::user1 |
| file_parent | A_NUMBER |
| displayname_owner | user1 |
| share_with | user0@LOCAL |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Feature: sharing

Scenario: orphaned shares
Given user "user0" has been created
And a new browser session for "user0" has been started
And user "user1" has been created
And user "user0" has created a folder "/common"
And user "user0" has created a folder "/common/sub"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ Feature: sharing
| storage | A_NUMBER |
| mail_send | 0 |
| uid_owner | user0 |
| storage_id | home::user0 |
| file_parent | A_NUMBER |
| share_with_displayname | user1 |
| displayname_owner | user0 |
Expand Down
2 changes: 1 addition & 1 deletion tests/acceptance/features/apiSharing-v1/mergeShare.feature
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ Feature: sharing
And user "user0" shares folder "/merge-test-inside-twogroups-perms" with group "group2" using the API
And user "user0" gets the following properties of folder "/merge-test-inside-twogroups-perms" using the API
|{http://owncloud.org/ns}permissions|
Then the single response should contain a property "{http://owncloud.org/ns}permissions" with value "RDNVCK"
Then the single response should contain a property "{http://owncloud.org/ns}permissions" with value "RDNVCK" or with value "RMDNVCK"
And as "user0" the folder "/merge-test-inside-twogroups-perms (2)" should not exist
And as "user0" the folder "/merge-test-inside-twogroups-perms (3)" should not exist

Expand Down
6 changes: 0 additions & 6 deletions tests/acceptance/features/apiSharing-v1/updateShare.feature
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ Feature: sharing
| storage | A_NUMBER |
| mail_send | 0 |
| uid_owner | user0 |
| storage_id | home::user0 |
| file_parent | A_NUMBER |
| displayname_owner | user0 |
| url | AN_URL |
Expand Down Expand Up @@ -84,7 +83,6 @@ Feature: sharing
| storage | A_NUMBER |
| mail_send | 0 |
| uid_owner | user0 |
| storage_id | home::user0 |
| file_parent | A_NUMBER |
| displayname_owner | user0 |
| url | AN_URL |
Expand Down Expand Up @@ -114,7 +112,6 @@ Feature: sharing
| storage | A_NUMBER |
| mail_send | 0 |
| uid_owner | user0 |
| storage_id | home::user0 |
| file_parent | A_NUMBER |
| displayname_owner | user0 |
| url | AN_URL |
Expand Down Expand Up @@ -144,7 +141,6 @@ Feature: sharing
| storage | A_NUMBER |
| mail_send | 0 |
| uid_owner | user0 |
| storage_id | home::user0 |
| file_parent | A_NUMBER |
| displayname_owner | user0 |
| url | AN_URL |
Expand Down Expand Up @@ -174,7 +170,6 @@ Feature: sharing
| storage | A_NUMBER |
| mail_send | 0 |
| uid_owner | user0 |
| storage_id | home::user0 |
| file_parent | A_NUMBER |
| displayname_owner | user0 |
| url | AN_URL |
Expand Down Expand Up @@ -205,7 +200,6 @@ Feature: sharing
| storage | A_NUMBER |
| mail_send | 0 |
| uid_owner | user0 |
| storage_id | home::user0 |
| file_parent | A_NUMBER |
| displayname_owner | user0 |
| mimetype | text/plain |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ Feature: trashbin-new-endpoint

Scenario: Trashbin can be emptied
Given user "user0" has been created
And a new browser session for "user0" has been started
And user "user0" has deleted file "/textfile0.txt"
And user "user0" has deleted file "/textfile1.txt"
And as "user0" the file "/textfile0.txt" should exist in trash
Expand Down Expand Up @@ -181,6 +182,7 @@ Feature: trashbin-new-endpoint

@local_storage
@no_default_encryption
@skip_on_objectstore
Scenario: Deleting a folder in external storage moves it to the trashbin
Given the administrator has invoked occ command "files:scan --all"
And user "user0" has been created
Expand All @@ -191,6 +193,7 @@ Feature: trashbin-new-endpoint

@local_storage
@no_default_encryption
@skip_on_objectstore
Scenario: Deleting a file in external storage moves it to the trashbin and can be restored
Given the administrator has invoked occ command "files:scan --all"
And user "user0" has been created
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ Feature: trashbin-new-endpoint

Scenario: Trashbin can be emptied
Given user "user0" has been created
And a new browser session for "user0" has been started
And user "user0" has deleted file "/textfile0.txt"
And user "user0" has deleted file "/textfile1.txt"
And as "user0" the file "/textfile0.txt" should exist in trash
Expand Down Expand Up @@ -181,6 +182,7 @@ Feature: trashbin-new-endpoint

@local_storage
@no_default_encryption
@skip_on_objectstore
Scenario: Deleting a folder into external storage moves it to the trashbin
Given the administrator has invoked occ command "files:scan --all"
And user "user0" has been created
Expand All @@ -191,6 +193,7 @@ Feature: trashbin-new-endpoint

@local_storage
@no_default_encryption
@skip_on_objectstore
Scenario: Deleting a file into external storage moves it to the trashbin and can be restored
Given the administrator has invoked occ command "files:scan --all"
And user "user0" has been created
Expand All @@ -208,6 +211,7 @@ Feature: trashbin-new-endpoint

@local_storage
@no_default_encryption
@skip_on_objectstore
Scenario: Deleting an updated file into external storage moves it to the trashbin and can be restored
Given the administrator has invoked occ command "files:scan --all"
And user "user0" has been created
Expand Down
8 changes: 8 additions & 0 deletions tests/acceptance/features/bootstrap/Trashbin.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ public function emptyTrashbin($user) {
$user, 'POST', "/index.php/apps/files_trashbin/ajax/delete.php", $body
);
$this->theHTTPStatusCodeShouldBe('200');
$decodedResponse = \json_decode($this->response->getBody(), true);
if (isset($decodedResponse['status'])) {
PHPUnit_Framework_Assert::assertNotEquals('error', $decodedResponse['status']);
}
}

/**
Expand Down Expand Up @@ -150,6 +154,10 @@ private function sendUndeleteRequest($user, $elementTrashID) {
$user, 'POST', "/index.php/apps/files_trashbin/ajax/undelete.php", $body
);
$this->theHTTPStatusCodeShouldBe('200');
$decodedResponse = \json_decode($this->response->getBody(), true);
if (isset($decodedResponse['status'])) {
PHPUnit_Framework_Assert::assertNotEquals('error', $decodedResponse['status']);
}
}

/**
Expand Down
18 changes: 17 additions & 1 deletion tests/acceptance/features/bootstrap/WebDav.php
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,22 @@ public function asTheFileOrFolderShouldExist($user, $entry, $path) {
*/
public function theSingleResponseShouldContainAPropertyWithValue(
$key, $expectedValue
) {
$this->theSingleResponseShouldContainAPropertyWithValueAndAlternative($key, $expectedValue, $expectedValue);
}

/**
* @Then the single response should contain a property :key with value :value or with value :altValue
*
* @param string $key
* @param string $expectedValue
* @param string $altExpectedValue
*
* @return int
* @throws \Exception
*/
public function theSingleResponseShouldContainAPropertyWithValueAndAlternative(
$key, $expectedValue, $altExpectedValue
) {
$keys = $this->response;
if (!\array_key_exists($key, $keys)) {
Expand Down Expand Up @@ -675,7 +691,7 @@ public function theSingleResponseShouldContainAPropertyWithValue(
}
}

if ($value != $expectedValue) {
if ($value != $expectedValue && $value != $altExpectedValue) {
throw new \Exception(
"Property \"$key\" found with value \"$value\", expected \"$expectedValue\""
);
Expand Down
5 changes: 5 additions & 0 deletions tests/acceptance/run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,11 @@ else
BEHAT_FILTER_TAGS="~@skip&&~@masterkey_encryption"
fi

if test "$OC_TEST_ON_OBJECTSTORE" = "1"; then
BEHAT_FILTER_TAGS="$BEHAT_FILTER_TAGS&&~@skip_on_objectstore"
fi


BEHAT_FILTER_TAGS="$BEHAT_FILTER_TAGS&&@api"

if test "$BEHAT_FILTER_TAGS"; then
Expand Down