Skip to content
Closed
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@
- Individual IT Services <info@individual-it.net>
- Informatyka Boguslawski sp. z o.o. sp.k., http://www.ib.pl/
- Iscle <albertiscle9@gmail.com>
- Israel Saba <israelsaba@gmail.com>
- J0WI <J0WI@users.noreply.github.com>
- Jaakko Salo <jaakkos@gmail.com>
- Jacob Neplokh <me@jacobneplokh.com>
Expand Down
6 changes: 5 additions & 1 deletion apps/dav/lib/Files/FileSearchBackend.php
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,11 @@ private function castValue(SearchPropertyDefinition $property, $value) {
case SearchPropertyDefinition::DATATYPE_DECIMAL:
case SearchPropertyDefinition::DATATYPE_INTEGER:
case SearchPropertyDefinition::DATATYPE_NONNEGATIVE_INTEGER:
return 0 + $value;
if (is_int($value) || is_float($value)) {
return 0 + $value;
}
$v = filter_var($value, FILTER_VALIDATE_FLOAT);
return $v === false ? 0 : $v;
Comment on lines +485 to +489
Copy link
Contributor

Choose a reason for hiding this comment

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

That means bad values are now silently cast to 0? I’m not sure if it’s a good idea. It would be better to start by fixing the dashboard widget that is causing the logging?

I would prefer an is_numeric test and at least logging (debug or even info) that there’s an issue, if possible with more info than the PHP warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree - if its an invalid value it should still fail and not be casted to a different valid value.
So one would not be able to properly detect wrong behaving code.

Meaning in that case the dashboard widget should be fixed as well

case SearchPropertyDefinition::DATATYPE_DATETIME:
if (is_numeric($value)) {
return max(0, 0 + $value);
Expand Down
96 changes: 95 additions & 1 deletion apps/dav/tests/unit/Files/FileSearchBackendTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use OCA\DAV\Connector\Sabre\FilesPlugin;
use OCA\DAV\Connector\Sabre\ObjectTree;
use OCA\DAV\Connector\Sabre\Server;
use OCA\DAV\Connector\Sabre\TagsPlugin;
use OCA\DAV\Files\FileSearchBackend;
use OCP\Files\FileInfo;
use OCP\Files\Folder;
Expand Down Expand Up @@ -171,6 +172,85 @@ public function testSearchSize(): void {
$this->assertEquals('/files/test/test/path', $result[0]->href);
}

public function testNumericLiteralIntPassThrough(): void {
$this->tree->expects($this->any())
->method('getNodeForPath')
->willReturn($this->davFolder);

$receivedQuery = null;
$this->searchFolder
->method('search')
->willReturnCallback(function (ISearchQuery $query) use (&$receivedQuery) {
$receivedQuery = $query;
return [
new \OC\Files\Node\File($this->rootFolder, $this->view, '/test/file'),
];
});

$query = $this->getBasicQuery(Operator::OPERATION_GREATER_THAN, FilesPlugin::SIZE_PROPERTYNAME, 10);
$this->search->search($query);

$this->assertNotNull($receivedQuery);
$comparison = $receivedQuery->getSearchOperation();
$this->assertInstanceOf(SearchComparison::class, $comparison);
$this->assertSame(ISearchComparison::COMPARE_GREATER_THAN, $comparison->getType());
$this->assertSame('size', $comparison->getField());
$this->assertSame(10, $comparison->getValue());
}

public function testNumericLiteralStringToFloat(): void {
$this->tree->expects($this->any())
->method('getNodeForPath')
->willReturn($this->davFolder);

$receivedQuery = null;
$this->searchFolder
->method('search')
->willReturnCallback(function (ISearchQuery $query) use (&$receivedQuery) {
$receivedQuery = $query;
return [
new \OC\Files\Node\File($this->rootFolder, $this->view, '/test/file'),
];
});

$query = $this->getBasicQuery(Operator::OPERATION_GREATER_THAN, FilesPlugin::SIZE_PROPERTYNAME, '10.5');
$this->search->search($query);

$this->assertNotNull($receivedQuery);
$comparison = $receivedQuery->getSearchOperation();
$this->assertInstanceOf(SearchComparison::class, $comparison);
$this->assertSame(ISearchComparison::COMPARE_GREATER_THAN, $comparison->getType());
$this->assertSame('size', $comparison->getField());
$this->assertSame(10, $comparison->getValue());
}

public function testNumericLiteralInvalidFallsBackToZero(): void {
$this->tree->expects($this->any())
->method('getNodeForPath')
->willReturn($this->davFolder);

/** @var ISearchQuery|null $receivedQuery */
$receivedQuery = null;
$this->searchFolder
->method('search')
->willReturnCallback(function (ISearchQuery $query) use (&$receivedQuery) {
$receivedQuery = $query;
return [
new \OC\Files\Node\File($this->rootFolder, $this->view, '/test/file'),
];
});

$query = $this->getBasicQuery(Operator::OPERATION_GREATER_THAN, FilesPlugin::SIZE_PROPERTYNAME, 'not-a-number');
$this->search->search($query);

$this->assertNotNull($receivedQuery);
$comparison = $receivedQuery->getSearchOperation();
$this->assertInstanceOf(SearchComparison::class, $comparison);
$this->assertSame(ISearchComparison::COMPARE_GREATER_THAN, $comparison->getType());
$this->assertSame('size', $comparison->getField());
$this->assertSame(0, $comparison->getValue());
}

public function testSearchMtime(): void {
$this->tree->expects($this->any())
->method('getNodeForPath')
Expand Down Expand Up @@ -258,14 +338,28 @@ private function getBasicQuery(string $type, string $property, int|string|null $
} else {
$where = new Operator(
$type,
[new SearchPropertyDefinition($property, true, true, true), new Literal($value)]
[new SearchPropertyDefinition($property, true, true, true, $this->getDataTypeForProperty($property)), new Literal($value)]
);
}
$limit = new Limit();

return new Query($select, $from, $where, $orderBy, $limit);
}

private function getDataTypeForProperty(string $property): string {
switch ($property) {
case FilesPlugin::SIZE_PROPERTYNAME:
case FilesPlugin::INTERNAL_FILEID_PROPERTYNAME:
return SearchPropertyDefinition::DATATYPE_NONNEGATIVE_INTEGER;
case '{DAV:}getlastmodified':
return SearchPropertyDefinition::DATATYPE_DATETIME;
case TagsPlugin::FAVORITE_PROPERTYNAME:
return SearchPropertyDefinition::DATATYPE_BOOLEAN;
default:
return SearchPropertyDefinition::DATATYPE_STRING;
}
}


public function testSearchNonFolder(): void {
$this->expectException(\InvalidArgumentException::class);
Expand Down