Skip to content

Commit

Permalink
Issue #677: Set back reference when calling add
Browse files Browse the repository at this point in the history 
Provided tests for testing if all references are set for crossreferences before saving
  • Loading branch information
Daniel Tschinder committed May 3, 2013
1 parent 4f33db4 commit 3ea2f2d
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 2 deletions.
11 changes: 9 additions & 2 deletions generator/lib/builder/om/PHP5ObjectBuilder.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4579,8 +4579,7 @@ public function add{$relatedObjectClassName}($crossObjectClassName $crossObjectN
}
if (!\$this->" . $collName . "->contains(" . $crossObjectName . ")) { // only add it if the **same** object is not already associated
\$this->doAdd{$relatedObjectClassName}($crossObjectName);
\$this->" . $collName . "[]= " . $crossObjectName . ";
\$this->" . $collName . "[] = " . $crossObjectName . ";
}
return \$this;
Expand All @@ -4597,6 +4596,8 @@ protected function addCrossFKDoAdd(&$script, ForeignKey $refFK, ForeignKey $cros
{
$relatedObjectClassName = $this->getFKPhpNameAffix($crossFK, $plural = false);

$selfRelationNamePlural = $this->getFKPhpNameAffix($refFK, $plural = true);

$lowerRelatedObjectClassName = lcfirst($relatedObjectClassName);

$joinedTableObjectBuilder = $this->getNewObjectBuilder($refFK->getTable());
Expand All @@ -4615,6 +4616,12 @@ protected function doAdd{$relatedObjectClassName}(\${$lowerRelatedObjectClassNam
{$foreignObjectName} = new {$className}();
{$foreignObjectName}->set{$relatedObjectClassName}(\${$lowerRelatedObjectClassName});
\$this->add{$refKObjectClassName}({$foreignObjectName});
// set the back reference to this object directly as using provided method either results
// in endless loop or in multiple relations
if (!\${$lowerRelatedObjectClassName}->get{$selfRelationNamePlural}()->contains(\$this)) {

This comment has been minimized.

Sign in to view

Copy link
@Gamesh

Gamesh Jan 29, 2015

Contributor

This line of code uses up a lot of memory, by loading all related objects.
if you for example have a Book and Author, when you set and author on a book, all of authors books are loaded and checked if they are related. In my Case one author has about 1000 books, and if i do a batch add several authors to one book, then on each add for each author 1000 objects are loaded, by the second iteration you end up with 2000 objects and it quickly reaches memory limit

This comment has been minimized.

Sign in to view

Copy link
@marcj

marcj Jan 29, 2015

Member

You can use clearAllReferences on those foreign objects to reset those values. Perfect for migration/long-running scripts.

This comment has been minimized.

Sign in to view

Copy link
@Gamesh

Gamesh Jan 29, 2015

Contributor

i know about clearAllReferences, but this happend inside doAdd... propel's method, which i don't modify, it is automatically generated, so i cannot use clearAllReferences inside and i cannot call it afterwards on the object as it dies with out of memory Fatal error, because it eats all memory up checking if related object exists and loading all those related objects which are never feed. it costs about 20MB of ram for each relation

This comment has been minimized.

Sign in to view

Copy link
@Gamesh

Gamesh Jan 29, 2015

Contributor

Wouldn't it be more simple and less memory intensive, to check in the relation's object instead of loading related objects. What i mean is if you have a Book and Authors and they are related by many-to-many then you have a cross-reference BookAuthor, which is a relation that contains BookId and AuthorID. And then when adding back reference load all Author->getBookAuthors() and check if Book is in that collection, instead of loading all Author->getBooks() like it is now on every add. You can image loading a 1000 Books on call to doAddAuthor

This comment has been minimized.

Sign in to view

Copy link
@marcj

marcj Jan 29, 2015

Member

Yepp, it would.
\$foreignCollection = \${$lowerRelatedObjectClassName}->get{$selfRelationNamePlural}();
\$foreignCollection[] = \$this;

This comment has been minimized.

Sign in to view

Copy link
@Gamesh

Gamesh Jan 29, 2015

Contributor

And these $foreignCollection vars seem to be unused

This comment has been minimized.

Sign in to view

Copy link
@marcj

marcj Jan 29, 2015

Member

you're commenting a very old commit. May 3, 2013

This comment has been minimized.

Sign in to view

Copy link
@Gamesh

Gamesh Jan 29, 2015

Contributor

i'm also aware of that, but it exists in most recent version of the code also, so it's still unused dead code, which also loads all those related objects and never uses them

This comment has been minimized.

Sign in to view

Copy link
@marcj

marcj Jan 29, 2015

Member

well we need those loading of relations because we need to generate later a collection-diff to be able to detect which relation needs to be unlinked during the save method.

This comment has been minimized.

Sign in to view

Copy link
@Gamesh

Gamesh Jan 29, 2015

Contributor

$foreingCollection is a local variable, and in this method's scope it is not used, unless you do a global $foreignCollection anywhere, but i doubt that :)

   /**
     * @param   Tvf $tvf The tvf object to add.
     */
    protected function doAddTvf(Tvf $tvf)
    {
        // set the back reference to this object directly as using provided method either results
        // in endless loop or in multiple relations
        if (!$tvf->getOrders()->contains($this)) { $orderTvf = new OrderTvf();
            $orderTvf->setTvf($tvf);
            $this->addOrderTvf($orderTvf);

            $foreignCollection = $tvf->getOrders();
            $foreignCollection[] = $this;
        }
    }

this is how it looks when generated, as you can see $foreignCollection is initialized then $this is assigned to it and it never is used again

This comment has been minimized.

Sign in to view

Copy link
@nibsirahsieu

nibsirahsieu Jan 29, 2015

Contributor

related issue #901
}
}
";
}
Expand Down
27 changes: 27 additions & 0 deletions test/testsuite/generator/builder/om/GeneratedObjectRelTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,33 @@ public function testOneToManyGetter()
$this->assertNotNull($books->getCurrent(), 'getRelCol() initialize the internal iterator at the beginning');
}

/**
* @group issue677
*/
public function testManyToManySetterIsNotLoosingAnyReference()
{
$list1 = new BookClubList();
$list2 = new BookClubList();
$book = new Book();

$book->addBookClubList($list1);
$book->addBookClubList($list2);

$lists = $book->getBookClubLists();
$this->assertCount(2, $lists, 'setRelCol is losing references to referenced object');

$rels = $book->getBookListRels();
$this->assertCount(2, $rels, 'setRelCol is losing references to relation object');

foreach ($rels as $rel) {
$this->assertNotNull($rel->getBook(), 'setRelCol is losing backreference on set relation to local object');
$this->assertNotNull($rel->getBookClubList(), 'setRelCol is losing backreference on set relation to referenced object');
}

foreach ($lists as $list) {
$this->assertCount(1, $list->getBooks(), 'setRelCol is losing backreference on set objects');
}
}
public function testManyToManyCounterExists()
{
$this->assertTrue(method_exists('BookClubList', 'countBooks'), 'Object generator correcly adds counter for the crossRefFk');
Expand Down

0 comments on commit 3ea2f2d

Please sign in to comment.