Fix segfault on Model::save() with circular relations

[stecman]

In cases where related models are set before save on both sides of a hasMany/belongsTo relationship, Model::save() goes into an infinite loop as its pre and post-save checks for relations to save never check if a related model had already been saved:

Save A
 \_ A hasMany B (post-save related records)
  \_ Save B
   \_ B belongsTo A (pre-save related records)
    \_ Save A
     \_ ...

For me this was causing a segfault, though with xdebug installed it would error before crashing at the max_nesting_level.

I ran into this while writing a model where relationships are traversed in both directions as part of some pre-save processing (with a little plumbing to use relations before saving).

[virgofx]

@stecman The change impacts such a core area of the Phalcon ORM which will have drastic implications if not thoroughly tested.

Can you provide exact model setup to reproduce the issue in your case (where it crashed) and then before this can get integrated there will definitely need to be updates into the model tests that incorporate this.

[Jurigag]

Wouldn't this break saving when we change something on relations of saved model, like it wouldn't save them again?

[stecman]

@virgofx Here's a minimal setup to replicate this problem:

Database and model:

CREATE TABLE `robot` (
    id INT NOT NULL AUTO_INCREMENT,
    name VARCHAR(255), -- stop ORM throwing when no fields to save
    PRIMARY KEY (id)
);

CREATE TABLE `part` (
    id INT NOT NULL AUTO_INCREMENT,
    robot_id INT NOT NULL,
    PRIMARY KEY (id)
);

class Robot extends \Phalcon\Mvc\Model
{
    public function initialize()
    {
        $this->hasMany('id', 'Part', 'robot_id', [
            'alias' => 'Parts'
        ]);
    }
}

class Part extends \Phalcon\Mvc\Model
{
    public function initialize()
    {
        $this->belongsTo('robot_id', 'Robot', 'id');
    }
}

Repro:

$robot = new Robot();
$part = new Part();

$robot->parts = [$part];
$part->robot = $robot;

$robot->save(); // <-- segfaults

[Jurigag]

I guess we need to test it.

[CameronHall]

I tried reproducing this myself a while ago on 3.4.1 and couldn't. Can anyone else reproduce?

[stecman]

@CameronHall - curious, I can still reproduce this with v3.4.1 and v3.4.3. Can you try this all-in-one script?

If you don't have the SQLite driver installed, there's a MySQL variant here.

<?php

$di = new \Phalcon\Di\FactoryDefault();

$di->setShared('db', function () {
    return new \Phalcon\Db\Adapter\Pdo\Sqlite([
        'dbname' => 'issue13354.sqlite'
    ]);
});

$di['db']->execute(<<<SQL

DROP TABLE IF EXISTS `robot`;
DROP TABLE IF EXISTS `part`;

CREATE TABLE `robot` (
    id INT,
    name VARCHAR(255), -- stop ORM throwing when no fields to save
    PRIMARY KEY (id)
);

CREATE TABLE `part` (
    id INT,
    robot_id INT,
    PRIMARY KEY (id)
);
SQL);

class Robot extends \Phalcon\Mvc\Model
{
    public function initialize()
    {
        $this->hasMany('id', 'Part', 'robot_id', [
            'alias' => 'Parts'
        ]);
    }
}

class Part extends \Phalcon\Mvc\Model
{
    public function initialize()
    {
        $this->belongsTo('robot_id', 'Robot', 'id');
    }
}

// Repro
$robot = new Robot();
$part = new Part();

$robot->parts = [$part];
$part->robot = $robot;

echo "Going to save..\n";

$robot->save(); // <-- segfaults

echo "Finished executing\n";

Running under v3.4.3 I see:

$ php issue13354.php 
Going to save..
Segmentation fault

Running with this patch I see:

$ php issue13354.php 
Going to save..
Finished executing

[CameronHall]

@stecman beautiful! Just what I needed - the SQLite driver was a great idea. I managed to reproduce the issue.

I'll give the patch a whirl later tonight and we can go from there.

[niden]

@stecman can you rebase this for the 4.0.x branch?

As for the tests you can add them in integration/Mvc/Model and if you need to add fixtures you can add them to tests/_data/fixtures

[niden]

And we need the changelog updated please :)

[stecman]

Rebased against 4.0.x and changelog updated 👍 I'll get the test case added in the next couple of days, but I've confirmed the issue still exists on 4.0.x and this patch fixes it.

The original patch targeting 3.4.x can be found here if anyone needs it.

[niden]

@stecman Thank you for that. Ping me when this is fully ready.

[niden]

@stecman nudge for the tests on this and if you don't mind rebasing the code.

I would like to get this in the resolved column

[stecman]

@niden Rebased again and test case added!

[niden]

@stecman can you have a look at the tests please? also if you don't mind rebase this

[stecman]

Aha, thanks - missed that the leading underscore had been dropped from _dirtyState. Should be good to go now

[codecov]

Codecov Report

Merging #13354 into 4.0.x will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##            4.0.x   #13354      +/-   ##
==========================================
+ Coverage   72.23%   72.26%   +0.03%     
==========================================
  Files         464      464              
  Lines       96056    96061       +5     
==========================================
+ Hits        69386    69422      +36     
+ Misses      26670    26639      -31

Impacted Files	Coverage Δ
ext/phalcon/mvc/model.zep.c	71.99% <0%> (+0.7%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86075c0...62136f3. Read the comment docs.

[niden]

Thank you @stecman