ManyMany Relation fails when extra field name exists in the related objects fields

[dylangrech92]

Affected Version

4.6

Description

I have a fairly straight forward many-many relationship

class Collection extends DataObject {
    private static $many_many = [
        'Products' => Product::class
    ];

    private static $many_many_extraFields = [
        'Products' => [
            'Sort' => DBInt::class,
        ]
    ];
}

class Product extends DataObject {
    private static $belongs_many_many = array (
        'Collections' => Collection::class
    );
}

Now if I simply run this function on the Product class:

public function RelatedProducts(int $limit = 4): ArrayList
    {
        $products = new ArrayList();

        /** @var Collection $collection */
        foreach ($this->Collections() as $collection) {
           // Up to this point everything is fine. $collection is a Collection object with all of it's properties

            foreach ($collection->Products()->exclude('ID', $this->ID) as $product) {
               // This is where is all breaks apart. $collection->Products()->toArray() throws the error
                $products->add($product);
                if ($products->count() >= $limit) return $products;
            }
        }

        return $products;
    }

I get the error: Unknown column 'Collection.ClassName' in 'field list' because $this->Products() is not adding the join between the many-many table and the Collection table.

In fact once I add this function in the Collection class everything works perfectly;

public function Products()
{
    return $this
        ->getManyManyComponents('Products')
        ->innerJoin(self::$table_name, "Collection_Products.CollectionID = Collection.ID");
}

[dylangrech92]

UPDATE

It actually gets slightly more complicated. My "quick fix" breaks whenever Collection->Products() returns an UnsavedRelationList, as obviously, innerJoin cannot be called on UnsavedRelationList. So here's an update "quick fix";

/**
 * @return ManyManyList|\SilverStripe\ORM\UnsavedRelationList
 */
public function Products()
{
   $list = $this->getManyManyComponents('Products');
   return ($list instanceof UnsavedRelationList)
       ? $list
       : $list->innerJoin(self::$table_name, "Collection_Products.CollectionID = Collection.ID");
}

[sminnee]

I believe the issue is calling the A->B relation, and then on one of resulting items calling the B->A relation.

I agree that it's a bug, however, the fix will be a little tricky. In particular, what value of 'Sort' short be returned on the resulting items?

It will want a test and the test should clarify what values of many-many-extrafields are being returned. We should also add a test for a many-many-through relationship as well.

[dylangrech92]

@sminnee it might sound a bit naive but I think, B -> A should not even try to sort because the sort config is on A->B, in my opinion B->A should be assumed as un-ordered list. Unless of course there would be the option to set $belongs_many_many.sort in that case that is followed for reverse sort.

[dylangrech92]

Ok this is getting more weird... (and frustrating to be completely honest)

If I want to update the extra_fields I have to use $this->getManyManyComponents('Products'); directly. If I try to go through the magic method or through my little hack, nothing happens to the database. I tried debugging it a little bit but in all honestly, there is some magic going on which I simply don't understand.

If you look at ManyManyList::add specifically the last line DB::manipulate($manipulation); the $manipulation is exactly the same for all of the versions below;

$this->getManyManyComponents('Products');
$this->Products(); // my hack version and even the standard one

But only the add function called from $this->getManyManyComponents('Products'); actually writes to the DB.

[sminnee]

As a bit of a hack, try this:

foreach ($this->Collections() as $collection) {
+ $collection = DataObject::get_by_id(Collection::class, $collection->ID);

This will create unnecessary SQL queries but if it fixes your scenario it will at least validate my hypothesis that the bug is caused by metadata of the first many-many relationship traversal being left inside the Collection object and causing problems with the 2nd query.

[sminnee]

Him, I've attempted to replicate this bug with DataObjectTest but cannot. Could you provide a bit more detail about the SQL queries being created and the traces of the errors?

diff --git a/tests/php/ORM/DataObjectTest.php b/tests/php/ORM/DataObjectTest.php
index b3f1faeb4..bebe02e42 100644
--- a/tests/php/ORM/DataObjectTest.php
+++ b/tests/php/ORM/DataObjectTest.php
@@ -18,7 +18,9 @@ use SilverStripe\ORM\FieldType\DBPolymorphicForeignKey;
 use SilverStripe\ORM\FieldType\DBVarchar;
 use SilverStripe\ORM\ManyManyList;
 use SilverStripe\ORM\Tests\DataObjectTest\Company;
+use SilverStripe\ORM\Tests\DataObjectTest\EquipmentCompany;
 use SilverStripe\ORM\Tests\DataObjectTest\Player;
+use SilverStripe\ORM\Tests\DataObjectTest\Team;
 use SilverStripe\ORM\Tests\DataObjectTest\TreeNode;
 use SilverStripe\Security\Member;
 use SilverStripe\View\ViewableData;
@@ -2572,4 +2574,51 @@ class DataObjectTest extends SapphireTest
         $this->assertEquals(null, $staff->Salary);
         $this->assertEquals([], $staff->getChangedFields());
     }
+
+    public function testTraverseManyManyAndBackAgain()
+    {
+        $company1 = new EquipmentCompany(['Name' => 'Co 1']);
+        $company2 = new EquipmentCompany(['Name' => 'Co 2']);
+        $company3 = new EquipmentCompany(['Name' => 'Co 3']);
+        $company1->write();
+        $company2->write();
+        $company3->write();
+
+        $team1 = new Team(['Title' => 'Team 1']);
+        $team2 = new Team(['Title' => 'Team 2']);
+        $team3 = new Team(['Title' => 'Team 3']);
+        $team1->write();
+        $team2->write();
+        $team3->write();
+
+        $company1->SponsoredTeams()->add($team1, ['SponsorFee' => 5]);
+        $company1->SponsoredTeams()->add($team2, ['SponsorFee' => 15]);
+
+        $company2->SponsoredTeams()->add($team2, ['SponsorFee' => 25]);
+        $company2->SponsoredTeams()->add($team3, ['SponsorFee' => 35]);
+
+        $company3->SponsoredTeams()->add($team1, ['SponsorFee' => 45]);
+        $company3->SponsoredTeams()->add($team3, ['SponsorFee' => 55]);
+
+
+        $teams = $company1->SponsoredTeams()->sort('Title');
+        foreach ($teams as $team) {
+            $otherCompanies = $team->Sponsors()->exclude('ID', $company1->ID)->sort('Name');
+            foreach ($otherCompanies as $otherCompany) {
+                $list[$team->Title . ' // ' . $team->SponsorFee][$otherCompany->Name] = $otherCompany->SponsorFee;
+            }
+        }
+
+        $this->assertEquals(
+            [
+                'Team 1 // 5' => [
+                    'Co 3' => 45,
+                ],
+                'Team 2 // 15' => [
+                    'Co 2' => 25,
+                ],
+            ],
+            $list
+        );
+    }
 }

[dylangrech92]

Ok, after a lot of trial and error I've actually managed to find the actual reason why this is happening. I put some sample code here for testing: https://github.com/dylangrech92/SS4_ManyManyRelationsExperiment

If you run; ObjectsControllerAB you notice that everything works fine for all scenarios But if you run: ObjectsControllerAC you'll immediately get the error: [Emergency] Uncaught SilverStripe\ORM\Connect\DatabaseException: Couldn't run query: SELECT DISTINCT CASE WHEN "ObjectA"."ClassName" IN ('ObjectA') THEN "ObjectA_ObjectCs"."Sort" WHEN "ObjectC"."ClassName" IN ('ObjectC') THEN "ObjectC"."Sort" ELSE NULL END AS "Sort", "ObjectC"."ClassName", "ObjectC"."LastEdited", "ObjectC"."Created", "ObjectC"."Title", "ObjectC"."ID", CASE WHEN "ObjectC"."ClassName" IS NOT NULL THEN "ObjectC"."ClassName" ELSE 'ObjectC' END AS "RecordClassName" FROM "ObjectC" INNER JOIN "ObjectA_ObjectCs" ON "ObjectA_ObjectCs"."ObjectCID" = "ObjectC"."ID" WHERE ("ObjectA_ObjectCs"."ObjectAID" = ?) 42S22-1054: Unknown column 'ObjectA.ClassName' in 'field list'

The reason why this seems to happen is due to a collision in column names. If you look closely ObjectC has it's own Sort column independent from the one in the relationship. For some reason that column is interfering with the creation of proper joins.

If you wanted to write a test for it, I guess you could try to use the SiteTree object since it has it's own Sort column (I haven't tested it myself)

Notes For my project I simply renamed the ManyMany sort column to _sort as to avoid the collision, but I think that if is "intended" behavior, a much more clear error should be thrown like; "Logic Error: Cannot determine column to use as "Sort" found; ObjectC.Sort collides with ObjectA.ObjectCs.Sort"

[sminnee]

Thanks for digging into this!

Yeah I think that because there's no object to bind the extra fields to other than the object being iterated on, this kind of situation should throw an error at the dev/build point.

Arguably this is an API breakage, as people might have set their projects up with a conflict of this nature and just never iterated on the objects in such a way that this issue is triggered. If we block this behaviour at the dev/build level then such people will have to refactor their database in order to upgrade.

The alternative approach would be to throw only a warning for such a database at dev/build, and when iterating on $objectA->ObjectCs() simply fail to include the many-many-extraField in the query.

[sminnee]

Would be good to get @silverstripe/core-team input on which approach to take

[sminnee]

Downgrading impact to medium as this only affects inconsistently designed databases, but it's a confusing error if you hit it so not going so far as low

[ScopeyNZ]

Can we put some kind of detection when we actually build the schema and throw the warning there? Then we're preventing people from creating the footgun, but leaving people who already have the footgun alone.

[tractorcow]

I just ran into this issue, and I suggest the same fix as @ScopeyNZ. It's much quicker (and less complexity) to error rather than try to support the collision.

[sminnee]

Yeah agreed - deal with it in dev/build. Anyone want to PR?