phalcon / cphalcon

High performance, full-stack PHP framework delivered as a C extension.
https://phalcon.io
BSD 3-Clause "New" or "Revised" License
10.79k stars 1.97k forks source link

[BUG]: getSnapshotData returns null #15837

Open Nialld opened 2 years ago

Nialld commented 2 years ago

model getSnapshotData under some situations returns null , should be array otherwise calls getChangedFIelds throws "The 'keepSnapshots' option must be enabled to track changes"

the way serialize and unserialize work in Model.zep seem to be the problem

The first time i serialize a model , the attributes and snapshot seem to be identical, leaving the snapshot variable serialised in a null state

Follow up with an unserialize, then snapshot property is null, breaking calls to getChangedFIelds / hasChanged etc

i believe the default state of snapshot should not be null in serialize function as below.

public function serialize() -> string
    {
        /**
         * Use the standard serialize function to serialize the array data
         */
        var attributes, manager, dirtyState, snapshot = null;

        let attributes = this->toArray(),
            dirtyState = this->dirtyState,
            manager = <ManagerInterface> this->getModelsManager(),
            snapshot = [];

        if manager->isKeepingSnapshots(this) && this->snapshot != null && attributes != this->snapshot {
            let snapshot = this->snapshot;
        }

        return serialize(
            [
                "attributes":  attributes,
                "snapshot":    snapshot,
                "dirtyState":  dirtyState
            ]
        );
    }

Details

niden commented 2 years ago

@Nialld I checked this and you are correct, the exception is thrown

Using your example (the below is a test):

Since the model has not changed, there is no snapshot data so the code that checks for that will throw the exception.

However, looking at it, it should not check the snapshot property but check the models manager if the snapshots have been enabled. At that point you will not get an exception but you will end up with an empty array.

There was an error in the code which I fixed in my branch. The snapshot property is always an array and never null. It initializes as an empty array.

Can you have a look at this test:

https://github.com/niden/cphalcon/blob/T15837-snapshot/tests/database/Mvc/Model/GetSnapshotDataCest.php#L51

and let me know if I understand correctly the issue?

Nialld commented 2 years ago

this issue is happening with caching involved

i have some steps to reproduce

$somemodel = SomeModel::findFirst(1); // SomeModel descends from \Phalcon\Mvc\Model .. reads the data from the DB

/ in the onConstruct of SomeModel public function onConstruct() { $this->getModelsManager()->keepSnapshots($this, $true); } /

$cache = Di::getDefault()->getShared('cache'); / @var $cache \Phalcon\Cache\Adapter\Libmemcached / $cache->set('key', $somemodel, 100); $newModel = $cache->get('key'); $changedFields = $newModel->getChangedFields(); // BOOM

Nialld commented 2 years ago

should also say, i was able to work around this by overriding unserialize in my class that extends \Phalcon\Mvc\Model

/**
* could be fixed by https://github.com/phalcon/cphalcon/issues/15837
* @param $data
* @return void
*/
public function unserialize($data)
{
parent::unserialize($data);
if (is_null($this->getSnapshotData())) {
    $this->setSnapshotData([]); // fix the snapshot data
}
}
niden commented 2 years ago

should also say, i was able to work around this by overriding unserialize in my class that extends \Phalcon\Mvc\Model

/**
* could be fixed by https://github.com/phalcon/cphalcon/issues/15837
* @param $data
* @return void
*/
public function unserialize($data)
{
parent::unserialize($data);
if (is_null($this->getSnapshotData())) {
  $this->setSnapshotData([]); // fix the snapshot data
}
}

@Nialld In my branch this will not happen since I removed the null from snapshotData it is an empty array if it is empty.

I will run your example in a test and see what I get. Stay tuned.

Nialld commented 2 years ago

this issue still exists, ive moved the fix into a custom serialiser as such

class CustomPhp extends \Phalcon\Storage\Serializer\Php
{
    public function unserialize($data): void
    {
        parent::unserialize($data);
        if ($this->data instanceof Model) {
            if (is_null($this->data->getSnapshotData())) {
                $this->data->setSnapshotData([]);
            }
        }
    }
}

and when initializing my cache i add this class using setDefaultSerializer, probably not the best way but it works for me

Drummi42 commented 1 year ago

A similar problem. Will it be fixed, @niden? My observations: The default property of the snapshot model is []. During serialize, if there were no changes to the model (for example, they just got it from the database) snapshot = null and when this property is taken from the cache, null is assigned to it ($model->getSnapshotData() === null) provided that $this->ModelManager->isKeepingSnapshots($model) === true.

During unserialize, this null is written to the model. What prevents you from using $this->getChangedFields();, where the snapshot property is checked for an array https://github.com/phalcon/cphalcon/blob/master/phalcon/Mvc/Model.zep#L1823

We get the error "The 'keepSnapshots' option must be enabled to track changes".

The cache is stored in radis, I work through symfony/cache.

To Reproduce

class A extends Model
{
    public $prop;

    public function initialize()
    {
        $this->keepSnapshots(true);
    }
}
// fail
$a = A::findFirst(1);
$this->getCache()->set('test', $a);
$cachedModel = $this->getCache()->get('test');
$cachedModel->getSnapshotData(); // null here
$this->getChangedFields(); // get error

Details

Drummi42 commented 1 year ago

__serialization sets snapshot = null in the cache model if attributes are not changed. https://github.com/phalcon/cphalcon/blob/5.0.x/phalcon/Mvc/Model.zep#L384 If the snapshot is cached, then __unserialize set snapshot = cachedSnapshot. And this condition does not work https://github.com/phalcon/cphalcon/blob/5.0.x/phalcon/Mvc/Model.zep#L603 temporary fix in the model:

public function __serialize(): array
{
    $arr = parent::__serialize();
    if ($arr['snapshot'] === null) {
        unset($arr['snapshot']);
    }
    return $arr;
}