Commit b9995ca6 authored by Bernhard Schussek's avatar Bernhard Schussek
Browse files

Fixed search after/search from offset to work with NULL values

parent 63449646
.composer-cache
vendor
.php_cs.cache
stages:
- test
.php:
image: dockerhub.cwd.at/docker/php/cli:7.2
before_script:
- export COMPOSER_CACHE_DIR="$(pwd -P)/crm/.composer-cache"
- mkdir -p vendor/bin
- curl -sS https://getcomposer.org/installer | php -- --install-dir=vendor/bin --filename=composer.phar && chmod 755 vendor/bin/composer.phar
- vendor/bin/composer.phar install --ansi --prefer-dist --no-progress --no-suggest
cache:
key: 'composer'
paths:
- .composer-cache/files
policy: pull-push
test:
extends: .php
script:
- export TEST_FRAMEWORK_FLAGS="--stop-on-failure"
- php -d zend.enable_gc=0 vendor/bin/phpunit -c ./phpunit.xml.dist --coverage-text --colors=never $TEST_FRAMEWORK_FLAGS
stage: test
tags: [docker]
code_analysis:
extends: .php
script:
# call php-cs-fixer but always return successful, we check the state in the next step and provide a patch file
- vendor/bin/php-cs-fixer fix --config=.php_cs --ansi --verbose --using-cache=no || true
- if [[ $(git status --porcelain 2>/dev/null | grep '[^M]' | wc -l) > 0 ]]; then git diff > cs.patch; exit 1; fi
stage: test
tags: [docker]
artifacts:
name: cs.patch
when: on_failure
expire_in: 2 days
paths:
- cs.patch
\ No newline at end of file
<?php
/*
* This file is part of the CWD Data Doctrine ORM Bundle
*
* (c) cwd.at GmbH <office@cwd.at>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
declare(strict_types=1);
namespace Cwd\DataDoctrineORMBundle\Func\Postgresql\String;
use Doctrine\ORM\Query\AST\Functions\FunctionNode;
use Doctrine\ORM\Query\Lexer;
use Doctrine\ORM\Query\Parser;
use Doctrine\ORM\Query\SqlWalker;
/**
* Implements PosgreSQL's chr() function for DQL.
*/
class Chr extends FunctionNode
{
public $arithmeticPrimary;
/**
* {@inheritdoc}
*/
public function parse(Parser $parser)
{
$parser->match(Lexer::T_IDENTIFIER);
$parser->match(Lexer::T_OPEN_PARENTHESIS);
$this->arithmeticPrimary = $parser->ArithmeticPrimary();
$parser->match(Lexer::T_CLOSE_PARENTHESIS);
}
/**
* {@inheritdoc}
*/
public function getSql(SqlWalker $sqlWalker)
{
return sprintf(
'chr(%s)',
$this->arithmeticPrimary->dispatch($sqlWalker)
);
}
}
......@@ -25,7 +25,9 @@ class CompositeFieldsOffsetStrategy implements OffsetStrategy
* The separator character of the primary and the secondary field name in
* the offset.
*/
private const DEFAULT_SEPARATOR = '_';
private const SEPARATOR = "\x01";
private const SEPARATOR_DQL = 'CHR(1)';
/**
* @var string[]
......@@ -42,31 +44,15 @@ class CompositeFieldsOffsetStrategy implements OffsetStrategy
*/
private $joinedAliases;
/**
* @var string[]
*
* @deprecated We're always casting to text now. Setting this field is not
* required anymore.
*/
private $explicitCasts;
/**
* @var string
*/
private $separator;
public function __construct(array $fieldNames, array $defaultDirections, array $joinedAliases = [], array $explicitCasts = [], string $separator = self::DEFAULT_SEPARATOR)
public function __construct(array $fieldNames, array $defaultDirections, array $joinedAliases = [])
{
Assert::minCount($fieldNames, 1);
Assert::count($fieldNames, count($defaultDirections));
Assert::allOneOf($defaultDirections, OrderDirection::ALL);
Assert::allStringNotEmpty($explicitCasts);
$this->fieldNames = $fieldNames;
$this->defaultDirections = $defaultDirections;
$this->joinedAliases = $joinedAliases;
$this->explicitCasts = $explicitCasts;
$this->separator = $separator;
}
/**
......@@ -78,7 +64,7 @@ class CompositeFieldsOffsetStrategy implements OffsetStrategy
? sprintf(
'CONCAT(%s)',
implode(
sprintf(', \'%s\', ', $this->separator),
', CHR(1), ',
array_map(
function ($sortField) use ($alias) {
return $this->addAlias($sortField, $alias);
......@@ -97,13 +83,15 @@ class CompositeFieldsOffsetStrategy implements OffsetStrategy
*/
public function addSearchFromOffset(QueryBuilder $qb, string $alias, Offset $fromOffset, string $sortDirection): void
{
$invertDirections = $sortDirection !== $this->defaultDirections[0];
$fieldValues = $this->split($fromOffset);
$invertDirections = $sortDirection !== $this->defaultDirections[0];
$nextConditionPrefixes = [];
$lastIndex = count($fieldValues) - 1;
$clauses = [];
$lastIndex = count($this->defaultDirections) - 1;
foreach ($fieldValues as $i => $fieldValue) {
$fieldName = $this->fieldNames[$i];
$fieldNameWithAlias = $this->addAlias($fieldName, $alias);
$fieldDirection = $invertDirections
? OrderDirection::invert($this->defaultDirections[$i])
: $this->defaultDirections[$i];
......@@ -112,25 +100,77 @@ class CompositeFieldsOffsetStrategy implements OffsetStrategy
? implode(' AND ', $nextConditionPrefixes).' AND '
: '';
$qb->orWhere($conditionPrefix.sprintf(
'%s %s :fromOffset%s',
$this->addAlias($fieldName, $alias),
$i === $lastIndex
? (OrderDirection::ASCENDING === $fieldDirection ? '>=' : '<=')
: (OrderDirection::ASCENDING === $fieldDirection ? '>' : '<'),
$i
));
$qb->setParameter(
sprintf('fromOffset%s', $i),
$fieldValue
);
switch (true) {
case OrderDirection::ASCENDING === $fieldDirection && null !== $fieldValue:
$clauses[] = $conditionPrefix.sprintf(
'(%s %s :fromOffset%s OR %s IS NULL)',
$fieldNameWithAlias,
$i === $lastIndex ? '>=' : '>',
$i,
$fieldNameWithAlias
);
$nextConditionPrefixes[] = sprintf(
'%s = :fromOffset%s',
$fieldNameWithAlias,
$i
);
$qb->setParameter(
sprintf('fromOffset%s', $i),
$fieldValue
);
break;
case OrderDirection::ASCENDING === $fieldDirection && null === $fieldValue:
if ($i === $lastIndex) {
$clauses[] = $conditionPrefix.sprintf(
'%s IS NULL',
$fieldNameWithAlias
);
}
$nextConditionPrefixes[] = sprintf(
'%s IS NULL',
$fieldNameWithAlias
);
break;
case OrderDirection::DESCENDING === $fieldDirection && null !== $fieldValue:
$clauses[] = $conditionPrefix.sprintf(
'%s %s :fromOffset%s',
$fieldNameWithAlias,
$i === $lastIndex ? '<=' : '<',
$i
);
$nextConditionPrefixes[] = sprintf(
'%s = :fromOffset%s',
$fieldNameWithAlias,
$i
);
$qb->setParameter(
sprintf('fromOffset%s', $i),
$fieldValue
);
break;
case OrderDirection::DESCENDING === $fieldDirection && null === $fieldValue:
if ($i !== $lastIndex) {
$clauses[] = $conditionPrefix.sprintf(
'%s IS NOT NULL',
$fieldNameWithAlias
);
} elseif (0 !== $i) {
// Everything is <= NULL
$clauses[] = $conditionPrefix.'1 = 1';
}
$nextConditionPrefixes[] = sprintf(
'%s IS NULL',
$fieldNameWithAlias
);
break;
}
}
$nextConditionPrefixes[] = sprintf(
'%s = :fromOffset%s',
$this->addAlias($fieldName, $alias),
$i
);
if (count($clauses) > 0) {
$qb->andWhere($qb->expr()->orX(...$clauses));
}
}
......@@ -139,12 +179,15 @@ class CompositeFieldsOffsetStrategy implements OffsetStrategy
*/
public function addSearchAfterOffset(QueryBuilder $qb, string $alias, Offset $afterOffset, string $sortDirection): void
{
$invertDirections = $sortDirection !== $this->defaultDirections[0];
$fieldValues = $this->split($afterOffset);
$invertDirections = $sortDirection !== $this->defaultDirections[0];
$nextConditionPrefixes = [];
$clauses = [];
$lastIndex = count($this->defaultDirections) - 1;
foreach ($fieldValues as $i => $fieldValue) {
$fieldName = $this->fieldNames[$i];
$fieldNameWithAlias = $this->addAlias($fieldName, $alias);
$fieldDirection = $invertDirections
? OrderDirection::invert($this->defaultDirections[$i])
: $this->defaultDirections[$i];
......@@ -153,23 +196,79 @@ class CompositeFieldsOffsetStrategy implements OffsetStrategy
? implode(' AND ', $nextConditionPrefixes).' AND '
: '';
$qb->orWhere($conditionPrefix.sprintf(
'%s %s :afterOffset%s',
$this->addAlias($fieldName, $alias),
OrderDirection::ASCENDING === $fieldDirection ? '>' : '<',
$i
));
$qb->setParameter(
sprintf('afterOffset%s', $i),
$fieldValue
);
switch (true) {
case OrderDirection::ASCENDING === $fieldDirection && null !== $fieldValue:
// when ASC, NULL is sorted after non NULL, however
// NULL > '...' evaluates to FALSE
$clauses[] = $conditionPrefix.sprintf(
'(%s > :afterOffset%s OR %s IS NULL)',
$fieldNameWithAlias,
$i,
$fieldNameWithAlias
);
$nextConditionPrefixes[] = sprintf(
'%s = :afterOffset%s',
$fieldNameWithAlias,
$i
);
$qb->setParameter(
sprintf('afterOffset%s', $i),
$fieldValue
);
break;
case OrderDirection::ASCENDING === $fieldDirection && null === $fieldValue:
// NULL is sorted after non NULL
// hence nothing is strictly greater than NULL
if (0 === $i && $lastIndex === $i) {
// Never evaluate to true for the first clause if it is
// also the last clause
// If we have multiple clauses, we can omit the
// last clause for logical simplicity
$clauses[] = $conditionPrefix.'1 = 0';
}
$nextConditionPrefixes[] = sprintf(
'%s IS NULL',
$fieldNameWithAlias
);
break;
case OrderDirection::DESCENDING === $fieldDirection && null !== $fieldValue:
$clauses[] = $conditionPrefix.sprintf(
'%s < :afterOffset%s',
$fieldNameWithAlias,
$i
);
$nextConditionPrefixes[] = sprintf(
'%s = :afterOffset%s',
$fieldNameWithAlias,
$i
);
$qb->setParameter(
sprintf('afterOffset%s', $i),
$fieldValue
);
break;
case OrderDirection::DESCENDING === $fieldDirection && null === $fieldValue:
// NULL is sorted before non NULL
// hence everything is strictly less than NULL
$clauses[] = $conditionPrefix.sprintf(
'%s IS NOT NULL',
$fieldNameWithAlias,
OrderDirection::ASCENDING === $fieldDirection ? '>' : '<',
$i
);
$nextConditionPrefixes[] = sprintf(
'%s IS NULL',
$fieldNameWithAlias
);
break;
}
}
$nextConditionPrefixes[] = sprintf(
'%s = :afterOffset%s',
$this->addAlias($fieldName, $alias),
$i
);
if (count($clauses) > 0) {
$qb->andWhere($qb->expr()->orX(...$clauses));
}
}
......@@ -222,7 +321,13 @@ class CompositeFieldsOffsetStrategy implements OffsetStrategy
{
$string = $offset->toString();
$numberOfFields = count($this->fieldNames);
$pieces = explode($this->separator, $string, $numberOfFields);
$pieces = explode(self::SEPARATOR, $string, $numberOfFields);
foreach ($pieces as $key => $value) {
if ('' === $value) {
$pieces[$key] = null;
}
}
return array_pad($pieces, $numberOfFields, null);
}
......
......@@ -100,7 +100,7 @@ abstract class OwnedValue
*
* This method copies the data of the value object into the owned value.
*
* @param null|static $ownedValue The owned value
* @param static|null $ownedValue The owned value
* @param object $value The value object
* @param object $owner The owner
*/
......@@ -147,9 +147,9 @@ abstract class OwnedValue
/**
* Extracts the value object of an owned value.
*
* @param null|OwnedValue $ownedValue The owned value. May be null
* @param OwnedValue|null $ownedValue The owned value. May be null
*
* @return null|object The value object or null
* @return object|null The value object or null
*/
public static function unwrap(?self $ownedValue)
{
......
......@@ -54,7 +54,7 @@ class CompositeFieldOffsetStrategyTest extends TestCase
public function testIsAnOffsetStrategy()
{
$this->assertTrue(is_a(CompositeFieldOffsetStrategy::class, OffsetStrategy::class));
$this->assertTrue(is_a(CompositeFieldOffsetStrategy::class, OffsetStrategy::class, true));
}
/**
......
<?php
/*
* This file is part of the CWD Data Doctrine ORM Bundle
*
* (c) cwd.at GmbH <office@cwd.at>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
declare(strict_types=1);
namespace Cwd\DataDoctrineORMBundle\Tests\OffsetStrategy;
use Cwd\DataBundle\Specification\Offset;
use Cwd\DataBundle\Specification\OrderDirection;
use Cwd\DataDoctrineORMBundle\OffsetStrategy\CompositeFieldsOffsetStrategy;
use Cwd\DataDoctrineORMBundle\OffsetStrategy\OffsetStrategy;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Query\Expr;
use Doctrine\ORM\Query\Parameter;
use Doctrine\ORM\QueryBuilder;
use PHPUnit\Framework\TestCase;
use function preg_replace;
use Prophecy\Prophecy\ObjectProphecy;
/**
* @covers \Cwd\DataDoctrineORMBundle\OffsetStrategy\CompositeFieldsOffsetStrategy
*/
class CompositeFieldsOffsetStrategyTest extends TestCase
{
/**
* @var EntityManagerInterface|ObjectProphecy
*/
private $emProphecy;
/**
* @var QueryBuilder
*/
private $qb;
/**
* @var CompositeFieldsOffsetStrategy
*/
private $offsetStrategy;
protected function setUp()
{
$this->emProphecy = $this->prophesize(EntityManagerInterface::class);
$this->emProphecy->getExpressionBuilder()->willReturn(new Expr());
$this->qb = new QueryBuilder($this->emProphecy->reveal());
$this->qb->from('Entity', 'e');
$this->qb->select('e.*');
$this->qb->where('condition = 1');
$this->offsetStrategy = new CompositeFieldsOffsetStrategy(['a', 'b'], [OrderDirection::ASCENDING, OrderDirection::ASCENDING]);
}
public function testIsAnOffsetStrategy()
{
$this->assertTrue(is_a(CompositeFieldsOffsetStrategy::class, OffsetStrategy::class, true));
}
/**
* @dataProvider provideSearchAfterOffsetWithOneParameterTests
*/
public function testCanSearchAfterOffsetWithOneParameter(string $description, string $offset, string $direction, array $expectedParams, string $expectedDql)
{
$this->offsetStrategy = new CompositeFieldsOffsetStrategy(['a'], [OrderDirection::ASCENDING]);
$this->offsetStrategy->addSearchAfterOffset(
$this->qb,
'e',
Offset::fromString($offset),
$direction
);
$params = $this->qb->getParameters();
$this->assertSame($this->sanitizeDql($expectedDql), $this->qb->getDQL());
$this->assertCount(count($expectedParams), $params);
$i = 0;
foreach ($expectedParams as $paramName => $paramValue) {
$this->assertEquals(new Parameter($paramName, $paramValue), $params[$i]);
++$i;
}
}
public function provideSearchAfterOffsetWithOneParameterTests()
{
yield [
'Parameter not NULL, ASC',
'foo',
OrderDirection::ASCENDING,
[
'afterOffset0' => 'foo',
],
<<<'DQL'
SELECT e.*
FROM Entity e
WHERE condition = 1
AND ((e.a > :afterOffset0 OR e.a IS NULL))
DQL
];
yield [
'Parameter NULL, ASC',
'',
OrderDirection::ASCENDING,
[],
<<<'DQL'
SELECT e.*
FROM Entity e
WHERE condition = 1
AND 1 = 0
DQL
];
yield [
'Parameter not NULL, DESC',
'foo',
OrderDirection::DESCENDING,
[
'afterOffset0' => 'foo',
],
<<<'DQL'
SELECT e.*
FROM Entity e
WHERE condition = 1
AND e.a < :afterOffset0
DQL
];
yield [
'Parameter NULL, DESC',
'',
OrderDirection::DESCENDING,
[],
<<<'DQL'
SELECT e.*
FROM Entity e
WHERE condition = 1
AND e.a IS NOT NULL
DQL
];
}
/**
* @dataProvider provideSearchAfterOffsetWithTwoParametersTests
*/
public function testCanSearchAfterOffsetWithTwoParameters(string $description, string $offset, string $direction, array $expectedParams, string $expectedDql)
{
$this->offsetStrategy->addSearchAfterOffset(
$this->qb,
'e',
Offset::fromString($offset),
$direction
);
$params = $this->qb->getParameters();
$this->assertSame($this->sanitizeDql($expectedDql), $this->qb->getDQL());
$this->assertCount(count($expectedParams), $params);
$i = 0;
foreach ($expectedParams as $paramName => $paramValue) {
$this->assertEquals(new Parameter($paramName, $paramValue), $params[$i]);
++$i;
}
}
public function provideSearchAfterOffsetWithTwoParametersTests()
{
yield [
'Parameters not NULL, ASC',
"foo\x01bar",
OrderDirection::ASCENDING,
[
'afterOffset0' => 'foo',
'afterOffset1' => 'bar',
],
<<<'DQL'
SELECT e.*
FROM Entity e
WHERE condition = 1
AND (
((e.a > :afterOffset0 OR e.a IS NULL))
OR (e.a = :afterOffset0 AND (e.b > :afterOffset1 OR e.b IS NULL))
)
DQL