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.

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

Fix query preparation when in elemMatch #2299

Merged
merged 1 commit into from
Apr 13, 2021
Merged

Fix query preparation when in elemMatch #2299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
77 changes: 52 additions & 25 deletions lib/Doctrine/ODM/MongoDB/Query/Expr.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,7 @@ public function addAnd($expression, ...$expressions): self

$this->query['$and'] = array_merge(
$this->query['$and'],
array_map(
static function ($expression) {
return $expression instanceof Expr ? $expression->getQuery() : $expression;
},
func_get_args()
)
func_get_args()
);

return $this;
Expand All @@ -123,9 +118,7 @@ public function addNor($expression, ...$expressions): self

$this->query['$nor'] = array_merge(
$this->query['$nor'],
array_map(static function ($expression) {
return $expression instanceof Expr ? $expression->getQuery() : $expression;
}, func_get_args())
func_get_args()
);

return $this;
Expand All @@ -148,9 +141,7 @@ public function addOr($expression, ...$expressions): self

$this->query['$or'] = array_merge(
$this->query['$or'],
array_map(static function ($expression) {
return $expression instanceof Expr ? $expression->getQuery() : $expression;
}, func_get_args())
func_get_args()
);

return $this;
Expand All @@ -175,12 +166,8 @@ public function addOr($expression, ...$expressions): self
*/
public function addToSet($valueOrExpression): self
{
if ($valueOrExpression instanceof Expr) {
$valueOrExpression = $valueOrExpression->getQuery();
}

$this->requiresCurrentField();
$this->newObj['$addToSet'][$this->currentField] = $valueOrExpression;
$this->newObj['$addToSet'][$this->currentField] = static::convertExpression($valueOrExpression, $this->class);

return $this;
}
Expand Down Expand Up @@ -414,7 +401,7 @@ public function each(array $values): self
*/
public function elemMatch($expression): self
{
return $this->operator('$elemMatch', $expression instanceof Expr ? $expression->getQuery() : $expression);
return $this->operator('$elemMatch', $expression);
}

/**
Expand Down Expand Up @@ -601,7 +588,7 @@ public function getQuery(): array
{
return $this->dm->getUnitOfWork()
->getDocumentPersister($this->class->name)
->prepareQueryOrNewObj($this->query);
->prepareQueryOrNewObj($this->convertExpressions($this->query));
}

/**
Expand Down Expand Up @@ -878,7 +865,7 @@ public function nearSphere($x, $y = null): self
*/
public function not($expression): self
{
return $this->operator('$not', $expression instanceof Expr ? $expression->getQuery() : $expression);
return $this->operator('$not', $expression);
}

/**
Expand Down Expand Up @@ -978,12 +965,8 @@ public function position(int $position): self
*/
public function pull($valueOrExpression): self
{
if ($valueOrExpression instanceof Expr) {
$valueOrExpression = $valueOrExpression->getQuery();
}

$this->requiresCurrentField();
$this->newObj['$pull'][$this->currentField] = $valueOrExpression;
$this->newObj['$pull'][$this->currentField] = static::convertExpression($valueOrExpression, $this->class);

return $this;
}
Expand Down Expand Up @@ -1420,4 +1403,48 @@ private function wrapEqualityCriteria(): void

$query = ['$in' => [$query]];
}

private function convertExpressions(array $query, ?ClassMetadata $classMetadata = null): array
{
if ($classMetadata === null) {
$classMetadata = $this->class;
}

$convertedQuery = [];
foreach ($query as $key => $value) {
if (is_string($key) && $classMetadata->hasAssociation($key)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can a mapped field be named 9 (for whatever reasons) or will it end as a string in the array anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, that's something PHP doesn't like at all anyways: https://3v4l.org/GoKLK. Reason for this is to reduce the performance impact of the logic so we don't have to check for associations with numeric indexes. Since PHP converts numeric strings to numbers I'm not sure what would happen, but I'm willing to take my chances on this and wait for someone's stuff to break.

$targetDocument = $classMetadata->getAssociationTargetClass($key);

if ($targetDocument) {
$fieldMetadata = $this->dm->getClassMetadata($targetDocument);
}
}

if (is_array($value)) {
$convertedQuery[$key] = $this->convertExpressions($value, $fieldMetadata ?? $classMetadata);
continue;
}

$convertedQuery[$key] = static::convertExpression($value, $fieldMetadata ?? $classMetadata);
}

return $convertedQuery;
}

/**
* Converts expression objects to query arrays. Non-expression values are
* returned unmodified.
*
* @param Expr|mixed $expression
*/
private static function convertExpression($expression, ClassMetadata $classMetadata)
Copy link
Member

Choose a reason for hiding this comment

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

I might be tired but it's confusing we have a non-static convertExpressions and static convertExpression. Don't have any better idea or objections, just pointing this out :)

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 made it static to not be tempted to use $this->class for the conversion and instead relying on what the previous logic decided was the right class metadata instance.

{
if (! $expression instanceof Expr) {
return $expression;
}

$expression->setClassMetadata($classMetadata);

return $expression->getQuery();
}
}
57 changes: 57 additions & 0 deletions tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH1674Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php

declare(strict_types=1);

namespace Doctrine\ODM\MongoDB\Tests\Functional\Ticket;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;
use Doctrine\ODM\MongoDB\Tests\BaseTest;
use MongoDB\BSON\ObjectId;

class GH1674Test extends BaseTest
{
public function testElemMatchUsesCorrectMapping()
{
$builder = $this->dm->createQueryBuilder(GH1674Document::class);
$builder
->field('embedded')
->elemMatch(
$builder->expr()
->field('id')
->equals(1)
);

$this->assertSame(
[
'embedded' => [
'$elemMatch' => ['id' => '1'],
],
],
$builder->getQueryArray()
);
}
}

/** @ODM\Document */
class GH1674Document
{
/** @ODM\Id */
protected $id;

/** @ODM\EmbedMany(targetDocument=GH1674Embedded::class) */
protected $embedded;

public function __construct()
{
$this->id = new ObjectId();
$this->embedded = new ArrayCollection();
}
}

/** @ODM\EmbeddedDocument */
class GH1674Embedded
{
/** @ODM\Field */
public $id = [];
}