Error if references are not stored correctly for lookups (#2465)

alanpoulain · web-flow · commit bcfc1c442283 · 2019-02-02T18:37:31.000+01:00
* Error if references are not stored correctly for lookups

* Fix tests

* Throw repositoryMethodLookupNotAllowed exception and only allow id ref
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -195,7 +195,7 @@ jobs:
           name: Run Behat tests
           command: |-
             mkdir -p build/logs/tmp build/cov
-            for f in $(find features -name '*.feature' -not -path 'features/main/exposed_state.feature' -not -path 'features/elasticsearch/*' | circleci tests split --split-by=timings); do
+            for f in $(find features -name '*.feature' -not -path 'features/main/exposed_state.feature' -not -path 'features/elasticsearch/*' -not -path 'features/mongodb/*' | circleci tests split --split-by=timings); do
               _f=${f//\//_}
               FEATURE="${_f}" phpdbg -qrr vendor/bin/behat --profile=coverage --suite=default --format=progress --out=std --format=junit --out=build/logs/tmp/"${_f}" "$f"
             done
diff --git a/behat.yml.dist b/behat.yml.dist
@@ -18,7 +18,7 @@ default:
         - 'Behat\MinkExtension\Context\MinkContext'
         - 'Behatch\Context\RestContext'
       filters:
-        tags: '~@postgres&&~@elasticsearch'
+        tags: '~@postgres&&~@mongodb&&~@elasticsearch'
     postgres:
       contexts:
         - 'DoctrineContext':
@@ -37,7 +37,7 @@ default:
         - 'Behat\MinkExtension\Context\MinkContext'
         - 'Behatch\Context\RestContext'
       filters:
-        tags: '~@sqlite&&~@elasticsearch'
+        tags: '~@sqlite&&~@mongodb&&~@elasticsearch'
     mongodb:
       contexts:
         - 'DoctrineContext':
@@ -106,4 +106,4 @@ coverage:
         - 'Behat\MinkExtension\Context\MinkContext'
         - 'Behatch\Context\RestContext'
       filters:
-        tags: '~@postgres&&~@elasticsearch'
+        tags: '~@postgres&&~@mongodb&&~@elasticsearch'
diff --git a/features/doctrine/date_filter.feature b/features/doctrine/date_filter.feature
@@ -404,7 +404,7 @@ Feature: Date filter on collections
       },
       "hydra:search": {
         "@type": "hydra:IriTemplate",
-        "hydra:template": "/dummies{?dummyBoolean,relatedDummy.embeddedDummy.dummyBoolean,dummyDate[before],dummyDate[strictly_before],dummyDate[after],dummyDate[strictly_after],relatedDummy.dummyDate[before],relatedDummy.dummyDate[strictly_before],relatedDummy.dummyDate[after],relatedDummy.dummyDate[strictly_after],description[exists],relatedDummy.name[exists],dummyBoolean[exists],relatedDummy[exists],dummyFloat,dummyFloat[],dummyPrice,dummyPrice[],order[id],order[name],order[description],order[relatedDummy.name],order[relatedDummy.symfony],order[dummyDate],dummyFloat[between],dummyFloat[gt],dummyFloat[gte],dummyFloat[lt],dummyFloat[lte],dummyPrice[between],dummyPrice[gt],dummyPrice[gte],dummyPrice[lt],dummyPrice[lte],id,id[],name,alias,description,relatedDummy.name,relatedDummy.name[],relatedDummies,relatedDummies[],dummy,relatedDummies.name,relatedDummy.thirdLevel.level,relatedDummy.thirdLevel.level[],relatedDummy.thirdLevel.fourthLevel.level,relatedDummy.thirdLevel.fourthLevel.level[],properties[]}",
+        "hydra:template": "/dummies{?dummyBoolean,relatedDummy.embeddedDummy.dummyBoolean,dummyDate[before],dummyDate[strictly_before],dummyDate[after],dummyDate[strictly_after],relatedDummy.dummyDate[before],relatedDummy.dummyDate[strictly_before],relatedDummy.dummyDate[after],relatedDummy.dummyDate[strictly_after],description[exists],relatedDummy.name[exists],dummyBoolean[exists],relatedDummy[exists],dummyFloat,dummyFloat[],dummyPrice,dummyPrice[],order[id],order[name],order[description],order[relatedDummy.name],order[relatedDummy.symfony],order[dummyDate],dummyFloat[between],dummyFloat[gt],dummyFloat[gte],dummyFloat[lt],dummyFloat[lte],dummyPrice[between],dummyPrice[gt],dummyPrice[gte],dummyPrice[lt],dummyPrice[lte],id,id[],name,alias,description,relatedDummy.name,relatedDummy.name[],relatedDummies,relatedDummies[],dummy,relatedDummies.name,relatedDummy.thirdLevel.level,relatedDummy.thirdLevel.level[],relatedDummy.thirdLevel.fourthLevel.level,relatedDummy.thirdLevel.fourthLevel.level[],relatedDummy.thirdLevel.badFourthLevel.level,relatedDummy.thirdLevel.badFourthLevel.level[],relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level,relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level[],properties[]}",
         "hydra:variableRepresentation": "BasicRepresentation",
         "hydra:mapping": [
           {
@@ -701,6 +701,30 @@ Feature: Date filter on collections
             "property": "relatedDummy.thirdLevel.fourthLevel.level",
             "required": false
           },
+          {
+            "@type": "IriTemplateMapping",
+            "variable": "relatedDummy.thirdLevel.badFourthLevel.level",
+            "property": "relatedDummy.thirdLevel.badFourthLevel.level",
+            "required": false
+          },
+          {
+            "@type": "IriTemplateMapping",
+            "variable": "relatedDummy.thirdLevel.badFourthLevel.level[]",
+            "property": "relatedDummy.thirdLevel.badFourthLevel.level",
+            "required": false
+          },
+          {
+            "@type": "IriTemplateMapping",
+            "variable": "relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level",
+            "property": "relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level",
+            "required": false
+          },
+          {
+            "@type": "IriTemplateMapping",
+            "variable": "relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level[]",
+            "property": "relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level",
+            "required": false
+          },
           {
               "@type": "IriTemplateMapping",
               "variable": "properties[]",
diff --git a/features/main/crud.feature b/features/main/crud.feature
@@ -4,7 +4,6 @@ Feature: Create-Retrieve-Update-Delete
   I need to be able to retrieve, create, update and delete JSON-LD encoded resources.
 
   @createSchema
-  @mongodb
   Scenario: Create a resource
     When I add "Content-Type" header equal to "application/ld+json"
     And I send a "POST" request to "/dummies" with body:
@@ -56,7 +55,6 @@ Feature: Create-Retrieve-Update-Delete
     }
     """
 
-  @mongodb
   Scenario: Get a resource
     When I send a "GET" request to "/dummies/1"
     Then the response status code should be 200
@@ -93,7 +91,6 @@ Feature: Create-Retrieve-Update-Delete
     }
     """
 
-  @mongodb
   Scenario: Get a not found exception
     When I send a "GET" request to "/dummies/42"
     Then the response status code should be 404
@@ -140,7 +137,7 @@ Feature: Create-Retrieve-Update-Delete
       "hydra:totalItems": 1,
       "hydra:search": {
         "@type": "hydra:IriTemplate",
-        "hydra:template": "/dummies{?dummyBoolean,relatedDummy.embeddedDummy.dummyBoolean,dummyDate[before],dummyDate[strictly_before],dummyDate[after],dummyDate[strictly_after],relatedDummy.dummyDate[before],relatedDummy.dummyDate[strictly_before],relatedDummy.dummyDate[after],relatedDummy.dummyDate[strictly_after],description[exists],relatedDummy.name[exists],dummyBoolean[exists],relatedDummy[exists],dummyFloat,dummyFloat[],dummyPrice,dummyPrice[],order[id],order[name],order[description],order[relatedDummy.name],order[relatedDummy.symfony],order[dummyDate],dummyFloat[between],dummyFloat[gt],dummyFloat[gte],dummyFloat[lt],dummyFloat[lte],dummyPrice[between],dummyPrice[gt],dummyPrice[gte],dummyPrice[lt],dummyPrice[lte],id,id[],name,alias,description,relatedDummy.name,relatedDummy.name[],relatedDummies,relatedDummies[],dummy,relatedDummies.name,relatedDummy.thirdLevel.level,relatedDummy.thirdLevel.level[],relatedDummy.thirdLevel.fourthLevel.level,relatedDummy.thirdLevel.fourthLevel.level[],properties[]}",
+        "hydra:template": "/dummies{?dummyBoolean,relatedDummy.embeddedDummy.dummyBoolean,dummyDate[before],dummyDate[strictly_before],dummyDate[after],dummyDate[strictly_after],relatedDummy.dummyDate[before],relatedDummy.dummyDate[strictly_before],relatedDummy.dummyDate[after],relatedDummy.dummyDate[strictly_after],description[exists],relatedDummy.name[exists],dummyBoolean[exists],relatedDummy[exists],dummyFloat,dummyFloat[],dummyPrice,dummyPrice[],order[id],order[name],order[description],order[relatedDummy.name],order[relatedDummy.symfony],order[dummyDate],dummyFloat[between],dummyFloat[gt],dummyFloat[gte],dummyFloat[lt],dummyFloat[lte],dummyPrice[between],dummyPrice[gt],dummyPrice[gte],dummyPrice[lt],dummyPrice[lte],id,id[],name,alias,description,relatedDummy.name,relatedDummy.name[],relatedDummies,relatedDummies[],dummy,relatedDummies.name,relatedDummy.thirdLevel.level,relatedDummy.thirdLevel.level[],relatedDummy.thirdLevel.fourthLevel.level,relatedDummy.thirdLevel.fourthLevel.level[],relatedDummy.thirdLevel.badFourthLevel.level,relatedDummy.thirdLevel.badFourthLevel.level[],relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level,relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level[],properties[]}",
         "hydra:variableRepresentation": "BasicRepresentation",
         "hydra:mapping": [
           {
@@ -437,6 +434,30 @@ Feature: Create-Retrieve-Update-Delete
             "property": "relatedDummy.thirdLevel.fourthLevel.level",
             "required": false
           },
+          {
+            "@type": "IriTemplateMapping",
+            "variable": "relatedDummy.thirdLevel.badFourthLevel.level",
+            "property": "relatedDummy.thirdLevel.badFourthLevel.level",
+            "required": false
+          },
+          {
+            "@type": "IriTemplateMapping",
+            "variable": "relatedDummy.thirdLevel.badFourthLevel.level[]",
+            "property": "relatedDummy.thirdLevel.badFourthLevel.level",
+            "required": false
+          },
+          {
+            "@type": "IriTemplateMapping",
+            "variable": "relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level",
+            "property": "relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level",
+            "required": false
+          },
+          {
+            "@type": "IriTemplateMapping",
+            "variable": "relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level[]",
+            "property": "relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level",
+            "required": false
+          },
           {
               "@type": "IriTemplateMapping",
               "variable": "properties[]",
@@ -448,7 +469,6 @@ Feature: Create-Retrieve-Update-Delete
     }
     """
 
-  @mongodb
   Scenario: Update a resource
     When I add "Content-Type" header equal to "application/ld+json"
     And I send a "PUT" request to "/dummies/1" with body:
@@ -502,7 +522,6 @@ Feature: Create-Retrieve-Update-Delete
     }
     """
 
-  @mongodb
   Scenario: Update a resource with empty body
     When I add "Content-Type" header equal to "application/ld+json"
     And I send a "PUT" request to "/dummies/1"
@@ -543,7 +562,6 @@ Feature: Create-Retrieve-Update-Delete
     }
     """
 
-  @mongodb
   Scenario: Delete a resource
     When I send a "DELETE" request to "/dummies/1"
     Then the response status code should be 204
diff --git a/features/main/relation.feature b/features/main/relation.feature
@@ -20,6 +20,7 @@ Feature: Relations support
       "@id": "/third_levels/1",
       "@type": "ThirdLevel",
       "fourthLevel": null,
+      "badFourthLevel": null,
       "id": 1,
       "level": 3,
       "test": true
diff --git a/features/main/subresource.feature b/features/main/subresource.feature
@@ -245,6 +245,7 @@ Feature: Subresource support
       "@id": "/third_levels/1",
       "@type": "ThirdLevel",
       "fourthLevel": "/fourth_levels/1",
+      "badFourthLevel": null,
       "id": 1,
       "level": 3,
       "test": true
@@ -262,6 +263,7 @@ Feature: Subresource support
       "@context": "/contexts/FourthLevel",
       "@id": "/fourth_levels/1",
       "@type": "FourthLevel",
+      "badThirdLevel": [],
       "id": 1,
       "level": 4
     }
diff --git a/features/mongodb/filters.feature b/features/mongodb/filters.feature
@@ -0,0 +1,29 @@
+@mongodb
+Feature: Filters on collections
+  In order to retrieve large collections of resources
+  As a client software developer
+  I need to retrieve collections with filters
+
+  @createSchema
+  Scenario: Error when getting collection with nested properties if references are not correctly stored (owning side)
+    Given there is a dummy object with a fourth level relation
+    When I send a "GET" request to "/dummies?relatedDummy.thirdLevel.badFourthLevel.level=4"
+    Then the response status code should be 500
+    And the response should be in JSON
+    And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"
+    And the JSON node "@context" should be equal to "/contexts/Error"
+    And the JSON node "@type" should be equal to "hydra:Error"
+    And the JSON node "hydra:title" should be equal to "An error occurred"
+    And the JSON node "hydra:description" should be equal to "Cannot use reference 'badFourthLevel' in class 'ThirdLevel' for lookup or graphLookup: dbRef references are not supported."
+    And the JSON node "trace" should exist
+
+  Scenario: Error when getting collection with nested properties if references are not correctly stored (not owning side)
+    When I send a "GET" request to "/dummies?relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level=3"
+    Then the response status code should be 500
+    And the response should be in JSON
+    And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"
+    And the JSON node "@context" should be equal to "/contexts/Error"
+    And the JSON node "@type" should be equal to "hydra:Error"
+    And the JSON node "hydra:title" should be equal to "An error occurred"
+    And the JSON node "hydra:description" should be equal to "Cannot use reference 'badThirdLevel' in class 'FourthLevel' for lookup or graphLookup: dbRef references are not supported."
+    And the JSON node "trace" should exist
diff --git a/src/Bridge/Doctrine/MongoDbOdm/PropertyHelperTrait.php b/src/Bridge/Doctrine/MongoDbOdm/PropertyHelperTrait.php
@@ -17,6 +17,7 @@
 use Doctrine\Common\Persistence\Mapping\ClassMetadata;
 use Doctrine\ODM\MongoDB\Aggregation\Builder;
 use Doctrine\ODM\MongoDB\Mapping\ClassMetadata as MongoDbOdmClassMetadata;
+use Doctrine\ODM\MongoDB\Mapping\MappingException;
 
 /**
  * Helper trait regarding a property in a MongoDB document using the resource metadata.
@@ -41,6 +42,7 @@ abstract protected function getClassMetadata(string $resourceClass): ClassMetada
      * Adds the necessary lookups for a nested property.
      *
      * @throws InvalidArgumentException If property is not nested
+     * @throws MappingException
      *
      * @return array An array where the first element is the $alias of the lookup,
      *               the second element is the $field name
@@ -66,9 +68,23 @@ protected function addLookupsForNestedProperty(string $property, Builder $aggreg
                 $alias .= $propertyAlias;
                 $referenceMapping = $classMetadata->getFieldMapping($association);
 
+                if (($isOwningSide = $referenceMapping['isOwningSide']) && MongoDbOdmClassMetadata::REFERENCE_STORE_AS_ID !== $referenceMapping['storeAs']) {
+                    throw MappingException::cannotLookupDbRefReference($classMetadata->getReflectionClass()->getShortName(), $association);
+                }
+                if (!$isOwningSide) {
+                    if (isset($referenceMapping['repositoryMethod']) || !isset($referenceMapping['mappedBy'])) {
+                        throw MappingException::repositoryMethodLookupNotAllowed($classMetadata->getReflectionClass()->getShortName(), $association);
+                    }
+
+                    $targetClassMetadata = $this->getClassMetadata($referenceMapping['targetDocument']);
+                    if ($targetClassMetadata instanceof MongoDbOdmClassMetadata && MongoDbOdmClassMetadata::REFERENCE_STORE_AS_ID !== $targetClassMetadata->getFieldMapping($referenceMapping['mappedBy'])['storeAs']) {
+                        throw MappingException::cannotLookupDbRefReference($classMetadata->getReflectionClass()->getShortName(), $association);
+                    }
+                }
+
                 $aggregationBuilder->lookup($classMetadata->getAssociationTargetClass($association))
-                    ->localField($referenceMapping['isOwningSide'] ? $localField : '_id')
-                    ->foreignField($referenceMapping['isOwningSide'] ? '_id' : $referenceMapping['mappedBy'])
+                    ->localField($isOwningSide ? $localField : '_id')
+                    ->foreignField($isOwningSide ? '_id' : $referenceMapping['mappedBy'])
                     ->alias($alias);
                 $aggregationBuilder->unwind("\$$alias");
 
diff --git a/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php b/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php
@@ -135,7 +135,7 @@ public function testApplyToCollectionWithOrderOverriddenWithAssociation()
         $classMetadataProphecy->hasAssociation('name')->shouldBeCalled()->willReturn(false);
         $classMetadataProphecy->getAssociationTargetClass('author')->shouldBeCalled()->willReturn(Dummy::class);
         $classMetadataProphecy->hasReference('author')->shouldBeCalled()->willReturn(true);
-        $classMetadataProphecy->getFieldMapping('author')->shouldBeCalled()->willReturn(['isOwningSide' => true]);
+        $classMetadataProphecy->getFieldMapping('author')->shouldBeCalled()->willReturn(['isOwningSide' => true, 'storeAs' => ClassMetadata::REFERENCE_STORE_AS_ID]);
 
         $resourceMetadataFactoryProphecy->create(Dummy::class)->shouldBeCalled()->willReturn(new ResourceMetadata(null, null, null, null, null, ['order' => ['author.name']]));
 
diff --git a/tests/Fixtures/TestBundle/Document/FourthLevel.php b/tests/Fixtures/TestBundle/Document/FourthLevel.php
@@ -42,6 +42,11 @@ class FourthLevel
      */
     private $level = 4;
 
+    /**
+     * @ODM\ReferenceMany(targetDocument=ThirdLevel::class, cascade={"persist"}, mappedBy="badFourthLevel", storeAs="id")
+     */
+    public $badThirdLevel;
+
     /**
      * @return int
      */
diff --git a/tests/Fixtures/TestBundle/Document/ThirdLevel.php b/tests/Fixtures/TestBundle/Document/ThirdLevel.php
@@ -58,6 +58,11 @@ class ThirdLevel
      */
     public $fourthLevel;
 
+    /**
+     * @ODM\ReferenceOne(targetDocument=FourthLevel::class, cascade={"persist"})
+     */
+    public $badFourthLevel;
+
     /**
      * @return int
      */
diff --git a/tests/Fixtures/TestBundle/Entity/FourthLevel.php b/tests/Fixtures/TestBundle/Entity/FourthLevel.php
@@ -44,6 +44,11 @@ class FourthLevel
      */
     private $level = 4;
 
+    /**
+     * @ORM\OneToMany(targetEntity=ThirdLevel::class, cascade={"persist"}, mappedBy="badFourthLevel")
+     */
+    public $badThirdLevel;
+
     /**
      * @return int
      */
diff --git a/tests/Fixtures/TestBundle/Entity/ThirdLevel.php b/tests/Fixtures/TestBundle/Entity/ThirdLevel.php
@@ -59,6 +59,11 @@ class ThirdLevel
      */
     public $fourthLevel;
 
+    /**
+     * @ORM\ManyToOne(targetEntity=FourthLevel::class, cascade={"persist"})
+     */
+    public $badFourthLevel;
+
     /**
      * @return int
      */
diff --git a/tests/Fixtures/app/config/config_mongodb.yml b/tests/Fixtures/app/config/config_mongodb.yml
@@ -36,7 +36,7 @@ services:
         tags:      [ { name: 'api_platform.filter', id: 'my_dummy.mongodb.range' } ]
     app.my_dummy_resource.mongodb.search_filter:
         parent:    'api_platform.doctrine_mongodb.odm.search_filter'
-        arguments: [ { 'id': 'exact', 'name': 'partial', 'alias': 'start', 'description': 'word_start', 'relatedDummy.name': 'exact', 'relatedDummies': 'exact', 'dummy': 'ipartial', 'relatedDummies.name': 'start', 'embeddedDummy.dummyName': 'partial', 'relatedDummy.thirdLevel.level': 'exact', 'relatedDummy.thirdLevel.fourthLevel.level': 'exact' } ]
+        arguments: [ { 'id': 'exact', 'name': 'partial', 'alias': 'start', 'description': 'word_start', 'relatedDummy.name': 'exact', 'relatedDummies': 'exact', 'dummy': 'ipartial', 'relatedDummies.name': 'start', 'embeddedDummy.dummyName': 'partial', 'relatedDummy.thirdLevel.level': 'exact', 'relatedDummy.thirdLevel.fourthLevel.level': 'exact', 'relatedDummy.thirdLevel.badFourthLevel.level': 'exact', 'relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level': 'exact' } ]
         tags:      [ { name: 'api_platform.filter', id: 'my_dummy.mongodb.search' } ]
     app.related_dummy_resource.mongodb.search_filter:
         parent:    'api_platform.doctrine_mongodb.odm.search_filter'
diff --git a/tests/Fixtures/app/config/config_orm.yml b/tests/Fixtures/app/config/config_orm.yml
@@ -4,7 +4,7 @@ imports:
 services:
     app.my_dummy_resource.search_filter:
         parent:    'api_platform.doctrine.orm.search_filter'
-        arguments: [ { 'id': 'exact', 'name': 'partial', 'alias': 'start', 'description': 'word_start', 'relatedDummy.name': 'exact', 'relatedDummies': 'exact', 'dummy': 'ipartial', 'relatedDummies.name': 'start', 'embeddedDummy.dummyName': 'partial', 'relatedDummy.thirdLevel.level': 'exact', 'relatedDummy.thirdLevel.fourthLevel.level': 'exact' } ]
+        arguments: [ { 'id': 'exact', 'name': 'partial', 'alias': 'start', 'description': 'word_start', 'relatedDummy.name': 'exact', 'relatedDummies': 'exact', 'dummy': 'ipartial', 'relatedDummies.name': 'start', 'embeddedDummy.dummyName': 'partial', 'relatedDummy.thirdLevel.level': 'exact', 'relatedDummy.thirdLevel.fourthLevel.level': 'exact', 'relatedDummy.thirdLevel.badFourthLevel.level': 'exact', 'relatedDummy.thirdLevel.fourthLevel.badThirdLevel.level': 'exact' } ]
         tags:      [ { name: 'api_platform.filter', id: 'my_dummy.search' } ]
 
     # Tests if the id default to the service name, do not add id attributes here
