diff --git a/CHANGELOG.md b/CHANGELOG.md index 4beea5d57b3..cbdb5a0e951 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ * Doctrine: Support for foreign identifiers (#4042) * Doctrine: Support for binary UUID in search filter (#3774) * Doctrine: Do not add join or lookup for search filter with empty value (#3703) +* Doctrine: Reduce code duplication in search filter (#3541) * JSON Schema: Allow generating documentation when property and method start from "is" (property `isActive` and method `isActive`) (#4064) * OpenAPI: Fix missing 422 responses in the documentation (#4086) * OpenAPI: Fix error when schema is empty (#4051) diff --git a/src/Bridge/Doctrine/MongoDbOdm/Filter/SearchFilter.php b/src/Bridge/Doctrine/MongoDbOdm/Filter/SearchFilter.php index 153dae64fa4..e14f74888ff 100644 --- a/src/Bridge/Doctrine/MongoDbOdm/Filter/SearchFilter.php +++ b/src/Bridge/Doctrine/MongoDbOdm/Filter/SearchFilter.php @@ -87,12 +87,17 @@ protected function filterProperty(string $property, $value, Builder $aggregation [$matchField, $field, $associations] = $this->addLookupsForNestedProperty($property, $aggregationBuilder, $resourceClass); } - /** - * @var MongoDBClassMetadata - */ - $metadata = $this->getNestedMetadata($resourceClass, $associations); - $caseSensitive = true; + $strategy = $this->properties[$property] ?? self::STRATEGY_EXACT; + + // prefixing the strategy with i makes it case insensitive + if (0 === strpos($strategy, 'i')) { + $strategy = substr($strategy, 1); + $caseSensitive = false; + } + + /** @var MongoDBClassMetadata */ + $metadata = $this->getNestedMetadata($resourceClass, $associations); if ($metadata->hasField($field) && !$metadata->hasAssociation($field)) { if ('id' === $field) { @@ -107,23 +112,9 @@ protected function filterProperty(string $property, $value, Builder $aggregation return; } - $strategy = $this->properties[$property] ?? self::STRATEGY_EXACT; + $this->addEqualityMatchStrategy($strategy, $aggregationBuilder, $field, $matchField, $values, $caseSensitive, $metadata); - // prefixing the strategy with i makes it case insensitive - if (0 === strpos($strategy, 'i')) { - $strategy = substr($strategy, 1); - $caseSensitive = false; - } - - $inValues = []; - foreach ($values as $inValue) { - $inValues[] = $this->addEqualityMatchStrategy($strategy, $field, $inValue, $caseSensitive, $metadata); - } - - $aggregationBuilder - ->match() - ->field($matchField) - ->in($inValues); + return; } // metadata doesn't have the field, nor an association on the field @@ -132,7 +123,6 @@ protected function filterProperty(string $property, $value, Builder $aggregation } $values = array_map([$this, 'getIdFromValue'], $values); - $associationFieldIdentifier = 'id'; $doctrineTypeField = $this->getDoctrineFieldType($property, $resourceClass); if (null !== $this->identifiersExtractor) { @@ -149,23 +139,39 @@ protected function filterProperty(string $property, $value, Builder $aggregation return; } + $this->addEqualityMatchStrategy($strategy, $aggregationBuilder, $field, $matchField, $values, $caseSensitive, $metadata); + } + + /** + * Add equality match stage according to the strategy. + */ + private function addEqualityMatchStrategy(string $strategy, Builder $aggregationBuilder, string $field, string $matchField, $values, bool $caseSensitive, ClassMetadata $metadata): void + { + $inValues = []; + foreach ($values as $inValue) { + $inValues[] = $this->getEqualityMatchStrategyValue($strategy, $field, $inValue, $caseSensitive, $metadata); + } + $aggregationBuilder ->match() ->field($matchField) - ->in($values); + ->in($inValues); } /** - * Add equality match stage according to the strategy. + * Get equality match value according to the strategy. * * @throws InvalidArgumentException If strategy does not exist * * @return Regex|string */ - private function addEqualityMatchStrategy(string $strategy, string $field, $value, bool $caseSensitive, ClassMetadata $metadata) + private function getEqualityMatchStrategyValue(string $strategy, string $field, $value, bool $caseSensitive, ClassMetadata $metadata) { $type = $metadata->getTypeOfField($field); + if (!MongoDbType::hasType($type)) { + return $value; + } if (MongoDbType::STRING !== $type) { return MongoDbType::getType($type)->convertToDatabaseValue($value); } diff --git a/src/Bridge/Doctrine/Orm/Filter/SearchFilter.php b/src/Bridge/Doctrine/Orm/Filter/SearchFilter.php index 0fda7e6470d..858406d421e 100644 --- a/src/Bridge/Doctrine/Orm/Filter/SearchFilter.php +++ b/src/Bridge/Doctrine/Orm/Filter/SearchFilter.php @@ -91,6 +91,15 @@ protected function filterProperty(string $property, $value, QueryBuilder $queryB [$alias, $field, $associations] = $this->addJoinsForNestedProperty($property, $alias, $queryBuilder, $queryNameGenerator, $resourceClass); } + $caseSensitive = true; + $strategy = $this->properties[$property] ?? self::STRATEGY_EXACT; + + // prefixing the strategy with i makes it case insensitive + if (0 === strpos($strategy, 'i')) { + $strategy = substr($strategy, 1); + $caseSensitive = false; + } + $metadata = $this->getNestedMetadata($resourceClass, $associations); if ($metadata->hasField($field)) { @@ -106,15 +115,6 @@ protected function filterProperty(string $property, $value, QueryBuilder $queryB return; } - $caseSensitive = true; - $strategy = $this->properties[$property] ?? self::STRATEGY_EXACT; - - // prefixing the strategy with i makes it case insensitive - if (0 === strpos($strategy, 'i')) { - $strategy = substr($strategy, 1); - $caseSensitive = false; - } - $this->addWhereByStrategy($strategy, $queryBuilder, $queryNameGenerator, $alias, $field, $values, $caseSensitive, $metadata); return; @@ -145,34 +145,12 @@ protected function filterProperty(string $property, $value, QueryBuilder $queryB $associationAlias = $alias; $associationField = $field; - $valueParameter = $queryNameGenerator->generateParameterName($associationField); if ($metadata->isCollectionValuedAssociation($associationField)) { $associationAlias = QueryBuilderHelper::addJoinOnce($queryBuilder, $queryNameGenerator, $alias, $associationField); $associationField = $associationFieldIdentifier; } - $type = $metadata->getTypeOfField($associationField); - - if (1 === \count($values)) { - $queryBuilder - ->andWhere($queryBuilder->expr()->eq($associationAlias.'.'.$associationField, ':'.$valueParameter)) - ->setParameter($valueParameter, $values[0], $type); - - return; - } - - $parameters = $queryBuilder->getParameters(); - $inQuery = []; - - foreach ($values as $val) { - $inQuery[] = ':'.$valueParameter; - $parameters->add(new Parameter($valueParameter, $val, $type)); - $valueParameter = $queryNameGenerator->generateParameterName($associationField); - } - - $queryBuilder - ->andWhere($associationAlias.'.'.$associationField.' IN ('.implode(', ', $inQuery).')') - ->setParameters($parameters); + $this->addWhereByStrategy($strategy, $queryBuilder, $queryNameGenerator, $associationAlias, $associationField, $values, $caseSensitive, $metadata); } /** diff --git a/tests/Bridge/Doctrine/Orm/Filter/SearchFilterTest.php b/tests/Bridge/Doctrine/Orm/Filter/SearchFilterTest.php index 8add7097424..3f74e66625b 100644 --- a/tests/Bridge/Doctrine/Orm/Filter/SearchFilterTest.php +++ b/tests/Bridge/Doctrine/Orm/Filter/SearchFilterTest.php @@ -543,11 +543,11 @@ public function provideApplyTestData(): array $filterFactory, ], 'mixed IRI and entity ID values for relations' => [ - sprintf('SELECT %s FROM %s %1$s INNER JOIN %1$s.relatedDummies relatedDummies_a1 WHERE %1$s.relatedDummy IN (:relatedDummy_p1, :relatedDummy_p2) AND relatedDummies_a1.id = :relatedDummies_p4', $this->alias, Dummy::class), + sprintf('SELECT %s FROM %s %1$s INNER JOIN %1$s.relatedDummies relatedDummies_a1 WHERE %1$s.relatedDummy IN (:relatedDummy_p1, :relatedDummy_p2) AND relatedDummies_a1.id = :id_p4', $this->alias, Dummy::class), [ 'relatedDummy_p1' => 1, 'relatedDummy_p2' => 2, - 'relatedDummies_p4' => 1, + 'id_p4' => 1, ], $filterFactory, ],