cphalcon icon indicating copy to clipboard operation
cphalcon copied to clipboard

[BUG]: getSnapshotData returns null

Open Nialld opened this issue 3 years ago • 7 comments

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

  • Phalcon version: 5.0.0alpha7
  • PHP Version: 8.0.14
  • Operating System: Ubuntu 20.04
  • Installation type: Compiled from source
  • Zephir version (if any):
  • Server: Nginx

Nialld avatar Dec 23 '21 09:12 Nialld

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

Using your example (the below is a test):

  • Insert a record in the DB
  • Get the record from the db with a keepSnapshots on
  • Serialize it
  • Unserialize it
  • Call getChangedFields()

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?

niden avatar Feb 10 '22 23:02 niden

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 avatar Feb 11 '22 12:02 Nialld

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 avatar Feb 11 '22 12:02 Nialld

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.

niden avatar Feb 11 '22 15:02 niden

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

Nialld avatar Sep 03 '22 17:09 Nialld

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

  • Phalcon version: 5.1.4
  • PHP Version: PHP 8.1.15
  • Operating System: linux
  • Installation type: pecl
  • Server: Nginx

Drummi42 avatar Feb 07 '23 08:02 Drummi42

__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;
}

Drummi42 avatar Mar 01 '23 08:03 Drummi42