-
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
Added searchByTags to view, storage and cache #12617
Changes from all commits
5101bc5
d6d1f14
c40de59
1df8dda
fa026a6
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 |
---|---|---|
|
@@ -344,6 +344,46 @@ public function searchByMime($mimetype) { | |
return $result; | ||
} | ||
|
||
/** | ||
* search for files by tag | ||
* | ||
* @param string|int $tag tag to search for | ||
* @param string $userId owner of the tags | ||
* @return array file data | ||
*/ | ||
public function searchByTag($tag, $userId = null) { | ||
// TODO: inject this | ||
$tagger = \OC::$server->getTagManager()->load('files', null, null, $userId); | ||
$result = array(); | ||
$exploreDirs = array(''); | ||
// FIXME: this is so wrong and unefficient, need to replace with actual DB queries | ||
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. @icewind1991 I didn't hear your 😱 ? 😉 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. The shared storage refactoring will take care of it 😄 |
||
while (count($exploreDirs) > 0) { | ||
$dir = array_pop($exploreDirs); | ||
$files = $this->getFolderContents($dir); | ||
// no results? | ||
if (!$files) { | ||
// maybe it's a single shared file | ||
$file = $this->get(''); | ||
$tags = $tagger->getTagsForObjects(array((int)$file['fileid'])); | ||
if (!empty($tags) && in_array($tag, current($tags))) { | ||
$result[] = $file; | ||
} | ||
continue; | ||
} | ||
foreach ($files as $file) { | ||
if ($file['mimetype'] === 'httpd/unix-directory') { | ||
$exploreDirs[] = ltrim($dir . '/' . $file['name'], '/'); | ||
} else { | ||
$tags = $tagger->getTagsForObjects(array((int)$file['fileid'])); | ||
if (!empty($tags) && in_array($tag, current($tags))) { | ||
$result[] = $file; | ||
} | ||
} | ||
} | ||
} | ||
return $result; | ||
} | ||
|
||
/** | ||
* get the size of a folder and set it in the cache | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -270,6 +270,63 @@ function testSearch() { | |
$this->assertEquals(2, count($this->cache->searchByMime('foo/file'))); | ||
} | ||
|
||
function testSearchByTag() { | ||
$userId = $this->getUniqueId('user'); | ||
\OC_User::createUser($userId, $userId); | ||
$this->loginAsUser($userId); | ||
$user = new \OC\User\User($userId, null); | ||
|
||
$file1 = 'folder'; | ||
$file2 = 'folder/foobar'; | ||
$file3 = 'folder/foo'; | ||
$file4 = 'folder/foo2'; | ||
$file5 = 'folder/foo3'; | ||
$data1 = array('size' => 100, 'mtime' => 50, 'mimetype' => 'foo/folder'); | ||
$fileData = array(); | ||
$fileData['foobar'] = array('size' => 1000, 'mtime' => 20, 'mimetype' => 'foo/file'); | ||
$fileData['foo'] = array('size' => 20, 'mtime' => 25, 'mimetype' => 'foo/file'); | ||
$fileData['foo2'] = array('size' => 25, 'mtime' => 28, 'mimetype' => 'foo/file'); | ||
$fileData['foo3'] = array('size' => 88, 'mtime' => 34, 'mimetype' => 'foo/file'); | ||
|
||
$id1 = $this->cache->put($file1, $data1); | ||
$id2 = $this->cache->put($file2, $fileData['foobar']); | ||
$id3 = $this->cache->put($file3, $fileData['foo']); | ||
$id4 = $this->cache->put($file4, $fileData['foo2']); | ||
$id5 = $this->cache->put($file5, $fileData['foo3']); | ||
|
||
$tagManager = \OC::$server->getTagManager()->load('files', null, null, $userId); | ||
$this->assertTrue($tagManager->tagAs($id1, 'tag1')); | ||
$this->assertTrue($tagManager->tagAs($id1, 'tag2')); | ||
$this->assertTrue($tagManager->tagAs($id2, 'tag2')); | ||
$this->assertTrue($tagManager->tagAs($id3, 'tag1')); | ||
$this->assertTrue($tagManager->tagAs($id4, 'tag2')); | ||
|
||
// use tag name | ||
$results = $this->cache->searchByTag('tag1', $userId); | ||
|
||
$this->assertEquals(2, count($results)); | ||
|
||
$this->assertEquals('folder', $results[0]['name']); | ||
$this->assertEquals('foo', $results[1]['name']); | ||
|
||
// use tag id | ||
$tags = $tagManager->getTagsForUser($userId); | ||
$this->assertNotEmpty($tags); | ||
$tags = array_filter($tags, function($tag) { return $tag->getName() === 'tag2'; }); | ||
$results = $this->cache->searchByTag(current($tags)->getId(), $userId); | ||
$this->assertEquals(3, count($results)); | ||
|
||
$this->assertEquals('folder', $results[0]['name']); | ||
$this->assertEquals('foobar', $results[1]['name']); | ||
$this->assertEquals('foo2', $results[2]['name']); | ||
|
||
$tagManager->delete('tag1'); | ||
$tagManager->delete('tag2'); | ||
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. PHPDoc indicates string[]|integer[], though code seems to change it to an array automatically. - Adjust PHPDoc? |
||
|
||
$this->logout(); | ||
\OC_User::deleteUser($userId); | ||
} | ||
|
||
function testMove() { | ||
$file1 = 'folder'; | ||
$file2 = 'folder/bar'; | ||
|
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.
$userId
is expected to be a\OCP\IUser
. - Also I don't get why we pass here an ID and below the user object? Maybe it makes more sense to always pass the IUser?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.
I also wonder... @icewind1991 asked me to pass
IUser
to the tag manager.One problem I could see for
searchByTag
is that you might not actually have an instance of another user but only the one from the session. We'd need to add yet another provider inOC::$server
to be able to get/create the user object of another non-logged in user ?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.
I'd personally prefer passing a string everywhere where only the string is needed...
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.
Note that manually creating the user requires you to do
new \OC\User\User($userId, null)
at the moment,null
being ugly and having to use the private namespace is ugly too.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.
Just asked @icewind1991 again he said it depends on the use case.
For
searchByTags()
it is less cumbersome for the API consumer to just pass a string.If the inconsistency still makes you unhappy I can change TagManager to use strings instead.
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.
I will change TagManager to take strings instead, especially that we only need the user id anyway.