Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pruning all collections in table for inherited elements. #11014

Closed

Conversation

DnwK98
Copy link

@DnwK98 DnwK98 commented Oct 16, 2023

When we have two collections with inherited types in one entity, in this example it's PetStore containing Cat and Dog

Cat extends Pet
Dog extends Pet
// Cat and dog are stored in same table using entity_discriminator

PetStore
   Cat[] cats
   Dog[] dogs

And we clear one of them using clear(), second gets cleared too, due to DELETE statement without proper filter.

I think it's bug.

There is fix and test for it.

Bypass before this is merged:
Instead of using

collection.clear()
// or
collection = new ArrayCollection(newElements)

load collection, and clear elements manually in your code

foreach(collection as element){
    collection.removeElement(element)
}
foreach(newElements as newElement){
    collection.add(newElement)
}

@DnwK98
Copy link
Author

DnwK98 commented Dec 21, 2023

Can I do something to help in review?

@DnwK98 DnwK98 changed the base branch from 2.16.x to 2.20.x April 13, 2024 10:17
@@ -183,6 +183,12 @@ private function deleteEntityCollection(PersistentCollection $collection): int
$types[] = PersisterHelper::getTypeOfColumn($joinColumn['referencedColumnName'], $sourceClass, $this->em);
}

if ($targetClass->discriminatorValue && $targetClass->discriminatorColumn) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this will work for joined table inheritance as well, can you use joined inheritance instead of single table in the test please? Single table is a special case of joined table

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't affect joined inheritance, because its collections are removed using deleteJoinedEntityCollection() instead of deleteEntityCollection()

For joined inheritance this bug doesn't appear, because separate collections are filtered using JOIN to exact tables.

...
        $sql = $query->getSQL();
        //   SELECT c0_.id AS id_0 FROM Dog d1_
        //        INNER JOIN collection_with_inheritance_pet c0_ ON d1_.id = c0_.id WHERE d1_.petStore_id = ?

        assert(is_string($sql));
        $statement = 'INSERT INTO ' . $tempTable . ' (' . $idColumnList . ') ' . $sql;
        //   INSERT INTO collection_with_inheritance_pet_id_tmp (id)
        //        SELECT c0_.id AS id_0 FROM Dog d1_
        //            INNER JOIN collection_with_inheritance_pet c0_ ON d1_.id = c0_.id
        //        WHERE d1_.petStore_id = ?

        $parameters = array_values($sourceClass->getIdentifierValues($collection->getOwner()));
        $numDeleted = $this->conn->executeStatement($statement, $parameters);

        // 3) Delete records on each table in the hierarchy
        $classNames = array_merge($targetClass->parentClasses, [$targetClass->name], $targetClass->subClasses);

        foreach (array_reverse($classNames) as $className) {
            $tableName = $this->quoteStrategy->getTableName($this->em->getClassMetadata($className), $this->platform);
            $statement = 'DELETE FROM ' . $tableName . ' WHERE (' . $idColumnList . ')'
                . ' IN (SELECT ' . $idColumnList . ' FROM ' . $tempTable . ')';
            //   DELETE FROM collection_with_inheritance_pet
            //   WHERE (id) IN (SELECT id FROM collection_with_inheritance_pet_id_tmp)

            $this->conn->executeStatement($statement);
        }
...

@DnwK98
Copy link
Author

DnwK98 commented Sep 11, 2024

As it fixes bug maybe it should target 2.19.x

@DnwK98
Copy link
Author

DnwK98 commented Sep 11, 2024

Fixed in the meantime in #11501

@DnwK98 DnwK98 closed this Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants