Skip to content

Improve qb validation #553

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

Merged
merged 6 commits into from
Sep 7, 2014
Merged

Improve qb validation #553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2416752
fixed tests
lsmith77 Aug 29, 2014
a56b830
typo fix
lsmith77 Aug 29, 2014
5fa4e2c
pushed field/orderby validation into the QB
lsmith77 Aug 29, 2014
e0ad9f0
Fixed validation
dantleech Aug 30, 2014
8fc1db1
Merge pull request #554 from dantleech/improve_qb_validation
lsmith77 Aug 31, 2014
9bab892
specifically handle order by error cases in the QB
lsmith77 Sep 1, 2014
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
27 changes: 1 addition & 26 deletions lib/Doctrine/ODM/PHPCR/DocumentRepository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,23 +146,7 @@ public function findBy(array $criteria, array $orderBy = null, $limit = null, $o
$orderByNode = $qb->orderBy();

if ($orderBy) {
$classMeta = $this->getClassMetadata();
foreach ($orderBy as $field => $order) {
if ($field === $classMeta->nodename) {
throw new InvalidArgumentException(sprintf(
'It is not possible to order by a nodename property "%s->%s"',
$classMeta->name,
$field
));
}
if ($classMeta->hasAssociation($field)) {
throw new InvalidArgumentException(sprintf(
'It is not possible to order by association field "%s->%s"',
$classMeta->name,
$field
));
}

$order = strtolower($order);
if (!in_array($order, array('asc', 'desc'))) {
throw new InvalidArgumentException(sprintf(
Expand Down Expand Up @@ -202,16 +186,7 @@ public function findBy(array $criteria, array $orderBy = null, $limit = null, $o
*/
protected function constraintField(ConstraintFactory $where, $field, $value, $alias)
{
$classMeta = $this->getClassMetadata();
if ($classMeta->hasAssociation($field)) {
throw new InvalidArgumentException(sprintf(
'It is not possible to filter on association fields %s->%s',
$classMeta->name,
$field
));
}

if ($field === $this->getClassMetadata()->nodename) {
if ($field === $this->class->nodename) {
$where->eq()->name($alias)->literal($value);
} else {
$where->eq()->field($alias.'.'.$field)->literal($value);
Expand Down
49 changes: 46 additions & 3 deletions lib/Doctrine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ protected function getPhpcrProperty($originalAlias, $odmField)
$this->validateAlias($originalAlias);
$meta = $this->aliasMetadata[$originalAlias];;


if ($meta->hasField($odmField)) {
$fieldMeta = $meta->getField($odmField);
} elseif ($meta->hasAssociation($odmField)) {
Expand Down Expand Up @@ -637,9 +636,30 @@ protected function walkConstraintNot(ConstraintNot $node)

protected function walkOperandDynamicField(OperandDynamicField $node)
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe the method name is not ideal, but is this method just for handling order by?

Copy link
Contributor

Choose a reason for hiding this comment

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

No iirc, its for handling all dynamic operands, e.g. ->field('a.foo') (i.e. PropertyValueInterface in PHPCR) or ->length('a.foo') (PHPCR LengthInterface).

Copy link
Member Author

Choose a reason for hiding this comment

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

but why did you assume in the error that the method is being called for an ordering?

Copy link
Member Author

Choose a reason for hiding this comment

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

in other words. i am still unsure if your changes make sense or just please the unit tests

Copy link
Member Author

Choose a reason for hiding this comment

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

in my original commit we only checked for nodetype and associations inside the foreach for orderings:
5fa4e2c#diff-0f74c7373c7c111b598f851ef2957d0eR760

I am still not sure where the issue was with that version of the PR

Copy link
Contributor

Choose a reason for hiding this comment

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

In your version you call ->getField in the ordering walker. But ->getField only applies to the OperandDynamicField node, not to any of the other DynamicOperands (length, lowercase, localname) etc.

The error message should be about not being able to use the nodename (or an association) field as a dynamic operand.

In fact we should probably just simply check to see if the field is mapped to a something which corresponds to a PHPCR property?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah .. for ordering it needs to be a "simple name", ie. a property.
for filtering it should additionally not allow node names and associations.
for reference associations i am actually not quite sure if we should not allow equality conditions.

Copy link
Contributor

Choose a reason for hiding this comment

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

So by having this here we also prevent:

$qb->where()->eq()->field('a.nodenamefield')->literal('foo');

because the user should be doing

$qb->where()->eq()->localname('a')->literal('foo');

I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have pushed an improvement for this.

{
$alias = $node->getAlias();
$field = $node->getField();

$classMeta = $this->aliasMetadata[$alias];

if ($field === $classMeta->nodename) {
throw new InvalidArgumentException(sprintf(
'Cannot use nodename property "%s->%s" in a field operand; use "localname()" instead.',
$classMeta->name,
$field
));
}

if ($classMeta->hasAssociation($field)) {
throw new InvalidArgumentException(sprintf(
'Cannot use association property "%s->%s" in a field operand.',
$classMeta->name,
$field
));
}

list($alias, $phpcrProperty) = $this->getPhpcrProperty(
$node->getAlias(),
$node->getField()
$alias,
$field
);

$op = $this->qomf->propertyValue(
Expand Down Expand Up @@ -751,6 +771,29 @@ protected function walkOrderBy(OrderBy $node)
QBConstants::NT_OPERAND_DYNAMIC
);

if ($dynOp instanceof OperandDynamicField) {
$alias = $dynOp->getAlias();
$field = $dynOp->getField();

$classMeta = $this->aliasMetadata[$alias];

if ($field === $classMeta->nodename) {
throw new InvalidArgumentException(sprintf(
'It is not possible to order by a nodename property "%s->%s"',
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this repeat the logic in walkDynamicFieldOperand?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah .. but the key bit is giving a different error message to make it easier to figure out what one did wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that we repeat this code - and it also should be repeated for walking constriants, and by extension we should do the same for all other places where there are invalid things. In short, we should handle this error once.

I would rather the exception were more expressive at the source:

You cannot use the nodename property in a dynamic operand
"where()->eq()->field('a.foobar')->literal('bar')->end()->orderBy()->asc()->field('a.nodename')`

For example.

But this would require us to boost the query builders self-awareness a little so that it is capable of producing such a trail. But I think that should be another issue. What do you think?

Otherwise I would be happy to remove the validation from the orderBy and only have it in the other method. The user should be able to relate the field name to their query and solve the problem.

$classMeta->name,
$field
));
}

if ($classMeta->hasAssociation($field)) {
throw new InvalidArgumentException(sprintf(
'It is not possible to order by an association field "%s->%s"',
$classMeta->name,
$field
));
}
}

$phpcrDynOp = $this->dispatch($dynOp);

if ($ordering->getOrder() == QOMConstants::JCR_ORDER_ASCENDING) {
Expand Down
14 changes: 11 additions & 3 deletions tests/Doctrine/Tests/ODM/PHPCR/Functional/DocumentRepositoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,17 +153,25 @@ public function testFindByOnNodename()
$this->dm->persist($user);
$this->dm->flush();

$users = $this->dm->getRepository('Doctrine\Tests\Models\CMS\CmsTeamUser')->findBy(array('nodename' =>'beberlei'));
$users = $this->dm->getRepository('Doctrine\Tests\Models\CMS\CmsTeamUser')->findBy(array('nodename' => 'beberlei'));
$this->assertCount(1, $users);
$this->assertEquals($user->username, $users['/functional/lsmith/beberlei']->username);
}

/**
* @expectedException \Doctrine\ODM\PHPCR\Exception\InvalidArgumentException
*/
public function testFindByOrderNonExistentDirectionString()
{
$this->dm->getRepository('Doctrine\Tests\Models\CMS\CmsTeamUser')->findBy(array('nodename' =>'beberlei'), array('username' =>'nowhere'));
}

/**
* @expectedException \Doctrine\ODM\PHPCR\Exception\InvalidArgumentException
*/
public function testFindByOrderNodename()
{
$this->dm->getRepository('Doctrine\Tests\Models\CMS\CmsTeamUser')->findBy(array('nodename' =>'beberlei'), array('nodename'));
$this->dm->getRepository('Doctrine\Tests\Models\CMS\CmsTeamUser')->findBy(array('nodename' =>'beberlei'), array('nodename' =>'asc'));
}

/**
Expand All @@ -179,7 +187,7 @@ public function testFindByOnAssociation()
*/
public function testFindByOrderAssociation()
{
$this->dm->getRepository('Doctrine\Tests\Models\CMS\CmsTeamUser')->findBy(array('nodename' =>'beberlei'), array('parent'));
$this->dm->getRepository('Doctrine\Tests\Models\CMS\CmsTeamUser')->findBy(array('username' =>'beberlei'), array('parent' => 'asc'));
}

public function testFindOneBy()
Expand Down
13 changes: 13 additions & 0 deletions tests/Doctrine/Tests/ODM/PHPCR/Functional/QueryBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,19 @@ public function testOrderBy()
$this->assertEquals('js', $res->first()->username);
}

/**
* @depends testFrom
* @expectedException \Doctrine\ODM\PHPCR\Exception\InvalidArgumentException
*/
public function testOrderByNonSimpleField()
{
$qb = $this->createQb();
$qb->from('a')->document('Doctrine\Tests\Models\Blog\User', 'a');
$qb->orderBy()->asc()->localname('a.username')->end();

$qb->getQuery()->execute();
}

/**
* Removes jcr:primaryType from row values,
* Jackrabbit does not return this, but doctrinedbal does.
Expand Down
47 changes: 45 additions & 2 deletions tests/Doctrine/Tests/ODM/PHPCR/Query/Builder/BuilderConverterPhpcrTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,19 @@ public function setUp()
->method('getName')
->will($me->returnValue($documentFqn));

$meta->expects($me->any())
->method('hasAssociation')
->will($me->returnCallback(function ($field) {
if ($field === 'associationfield') {
return true;
}

return false;
}));

$meta->nodename = 'nodenameProperty';
$meta->name = 'MyClassName';

return $meta;
}));

Expand Down Expand Up @@ -480,7 +493,6 @@ public function provideTestDispatchOperands()
'phpcr_class' => 'LiteralInterface',
)),
);

}

/**
Expand Down Expand Up @@ -547,6 +559,37 @@ public function testOrderBy()
$this->assertCount(2, $res);
}

public function provideOrderByDynamicField()
{
return array(
array('alias_1.ok_field', null),
array('alias_1.nodenameProperty', 'It is not possible to order by a nodename property "MyClassName->nodenameProperty"'),
array('alias_1.associationfield', 'It is not possible to order by an association field "MyClassName->associationfield"'),
);
}

/**
* @dataProvider provideOrderByDynamicField
*/
public function testOrderByDynamicField($field, $exception)
{
$this->primeBuilder();
$order1 = $this->createNode('Ordering', array(QOMConstants::JCR_ORDER_ASCENDING));

$orderBy = $this->createNode('OrderBy', array());
$orderBy->addChild($order1);

$op = $this->createNode('OperandDynamicField', array($field));
$order1->addChild($op);

if (null !== $exception) {
$this->setExpectedException('Doctrine\ODM\PHPCR\Exception\InvalidArgumentException', $exception);
}

$res = $this->converter->dispatch($orderBy);
$this->assertCount(1, $res);
}

public function testGetQuery()
{
$me = $this;
Expand Down Expand Up @@ -622,7 +665,7 @@ public function testGetQuery()
}

/**
* @expectedException Doctrine\ODM\PHPCR\Exception\InvalidArgumentException
* @expectedException \Doctrine\ODM\PHPCR\Exception\InvalidArgumentException
* @expectedExceptionMessage You must specify a primary alias
*/
public function testGetQueryMoreThanOneSourceNoPrimaryAlias()
Expand Down