3 Primary Keys with isCrossRef and ObjectCombinationCollection, ->diff() not working causing rows to be deleted and recreated

[SpikedCola]

Apologies in advance if this is hard to follow. This is with the latest version, dev-master 011aaa7.

My schema is using 3 primary keys on an isCrossRef table. In this case, 1 or more performers appearing at an event, with a flag to say who is a headliner. Schema is like this:

<table name="event_performers" idMethod="native" phpName="EventPerformer" isCrossRef="true">
    <column name="event_id" primaryKey="true" phpName="EventId" type="INTEGER" size="10" sqlType="int(10) unsigned" required="true"/>
    <column name="performer_id" primaryKey="true" phpName="PerformerId" type="INTEGER" size="10" sqlType="int(10) unsigned" required="true"/>
    <column name="is_headliner" primaryKey="true" phpName="IsHeadliner" type="BOOLEAN" size="1" sqlType="tinyint(1) unsigned" required="true"/>
    <column name="found_datetime" phpName="FoundDatetime" type="TIMESTAMP" required="true"/>

I am using an ObjectCombinationCollection to collect the perfomer & headliner flag, and calling the generated setPerformerHeadliner() function like this:

$performers = new \Propel\Runtime\Collection\ObjectCombinationCollection();
foreach ($headliners as $performerJson) {
	$performer = Performer::CreateFromJSON($performerJson);
	$performers->push($performer, true); // is headliner
}
foreach ($supportingActs as $performerJson) {
	$performer = Performer::CreateFromJSON($performerJson);
	$performers->push($performer, false); // is NOT headliner
}
$event->setPerformerIsHeadliners($performers);

The first pass works correctly, rows are inserted as expected.

On subsequent passes, however, the event_performers cross-reference rows are deleted and re-created on every pass.

Digging in to the generated setPerformerIsHeadliners() function, the existing records are fetched from the db, diff() is called, and then any records scheduled for deletion are removed:

public function setPerformerIsHeadliners(Collection $PerformerIsHeadliners, ?ConnectionInterface $con = null)
{
	$this->clearPerformerIsHeadliners();
	$currentPerformerIsHeadliners = $this->getPerformerIsHeadliners();

	$combinationCollPerformerIsHeadlinersScheduledForDeletion = $currentPerformerIsHeadliners->diff($PerformerIsHeadliners);

	foreach ($combinationCollPerformerIsHeadlinersScheduledForDeletion as $toDelete) {
		$this->removePerformerIsHeadliner(...$toDelete);
	}

	foreach ($PerformerIsHeadliners as $PerformerIsHeadliner) {
		if (!$currentPerformerIsHeadliners->contains(...$PerformerIsHeadliner)) {
			$this->doAddPerformerIsHeadliner(...$PerformerIsHeadliner);
		}
	}

If I print $combinationCollPerformerIsHeadlinersScheduledForDeletion, on every subsequent pass it lists all of my cross-references. So it appears diff() is not working as expected and all of my relationships are deleted.

Notice that if (!$currentPerformerIsHeadliners->contains(...$PerformerIsHeadliner)) { is using the spread operator.

$currentPerformerIsHeadliners returns an object of type ObjectCombinationCollection, but there is no overload for diff(). The version of diff() that is called is in the Collection class, which doesn't use the spread operator on the contains() call:

class Collection implements ArrayAccess, IteratorAggregate, Countable, Serializable
{
    public function diff(Collection $collection): self
    {
        $diff = clone $this;
        $diff->clear();

        foreach ($this as $object) {
            if (!$collection->contains($object)) {
                $diff[] = $object;
            }
        }

        return $diff;
    }

This results in the search() function of ObjectCombinationCollection being passed args that look like:

[ 0 => [ Performer_object, IsHeadliner_bool ] ]

You can see the foreach (func_get_args() as $pos => $obj) { loop is expecting to loop over the 2 args, checking if they are ActiveRecordInterface. But the double-array breaks that:

public function search($element)
{
	$hashes = [];
	$isActiveRecord = [];
	foreach (func_get_args() as $pos => $obj) {
		if ($obj instanceof ActiveRecordInterface) {
			$hashes[$pos] = $obj->hashCode();
			$isActiveRecord[$pos] = true;
		} else {
			$hashes[$pos] = $obj;
			$isActiveRecord[$pos] = false;
		}
	}

If I edit ObjectCombinationCollection and overload the diff() function, such that it uses the spread operator like in setPerformerIsHeadliners():

class ObjectCombinationCollection extends ObjectCollection
{
    public function diff(Collection $collection): self
    {
        $diff = clone $this;
        $diff->clear();

        foreach ($this as $object) {
            if (!$collection->contains(...$object)) {
                $diff[] = $object;
            }
        }

        return $diff;
    }

Then search() is passed args like [ Performer_object, IsHeadliner_bool ] which works correctly, and my generated setPerformerIsHeadliners() functions work properly. Rows aren't deleted erroneously on every pass.

I believe this is a bug.

[mringler]

That's a great report! I agree, it looks like a bug. Do you think you can write a test for your fix and create a pull request for that?

[SpikedCola]

Hi @mringler,

I have almost no experience writing tests, but I suppose this is a good opportunity to learn! Let me see what I can come up with.

[mringler]

That's the spirit! If you haven't found it already, there is documentation how to run the tests locally.

To get you started, it looks like there are a bunch of tests for ObjectCombinationCollection, but they are not at the expected place, but in the files:

tests/Propel/Tests/Generator/Builder/Om/GeneratedObjectM2MRelationThreePKsTest.php
tests/Propel/Tests/Generator/Builder/Om/GeneratedObjectM2MRelationThreePKs2Test.php

and they look pretty messy, too.

You can still put the tests into those files, but the right place would be in

tests/Propel/Tests/Runtime/Collection/ObjectCombinationCollection.php

There are tests for the parent's diff() at the end of

tests/Propel/Tests/Runtime/Collection/CollectionTest.php

it might help to have a look at those.

So you could create the ObjectCombinationCollection.php file using CollectionTest.php as a template (or find another way of course). Let me know if you run into issues. Godspeed!