Merge 2e15671 into cecd948

api-platform · Mar 3, 2021 · f3a610b · f3a610b
2 parents cecd948 + 2e15671
commit f3a610b
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 59 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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,
                 ],