Doctrine 1
  1. Doctrine 1
  2. DC-620

Unserialize does not add entity to the table entitymap

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.2.2
    • Fix Version/s: 1.2.3
    • Component/s: Record
    • Labels:
      None

      Description

      Hello,

      Considering this test script:

      if (!file_exists("/tmp/serialize.doctrine"))
      {
        $a = Address::getById(1);
        $b = Address::getById(2);
        $c = Address::getById(3);
        $d = Address::getById(4);
        print "Serialize using oid=" . $d->getOid() . "\n";
        file_put_contents("/tmp/serialize.doctrine", serialize($d));
      }
      else
      {
        $d = unserialize(file_get_contents("/tmp/serialize.doctrine"));
        print "unserialized oid = " . $d->getOid() . "\n";
      
        $d2 = Address::getById(4);
        print "new object oid = " . $d2->getOid() . "\n";
      
        $d->housenumber ++;
        print "{$d->housenumber} versus {$d2->housenumber}\n";
      }
      

      Things are going wrong in the unserialization. In this test script, I manage to create two objects holding the same oid, but pointing at different object instances (but for the same database id). The output of this script after running it twice shows me:

      $ php test.php
      Serialize using oid=5
      $ php test.php
      unserialized oid = 5
      new object oid = 5
      24 versus 23
      

      Looking at the unserialize() code in Doctrine_Record, the problem seems to be coming from two issues in there:

      • $this->_oid is generated before the data unserialization. Because of this, the generated $this->_oid is overwritten with whatever was in the serialized data (this is why in above example, the oid became 5 for the unserialized record)
      • The unserialized record is not added to the table's identitymap, making it kind of invisible for the table's getRecord() code, resulting in two different objects that both represent the entity with id = 4 (as you can see in the "24 versus 23" output).

      See the attached patch for a fix that I did on our code tree to make the test work.

      Note that I added some extra code in the unserialize method to cleanup any existing entitymap and table repository entry for the unserialized object. This is of course not the best route, but it helps with some unit testing code where a lot of serialize/unserialize handling is going on and objects got mixed up when not doing this cleanup.
      IMO, better would be to add an unserialization method to the Doctrine_Table, which would, like find(), act as a factory for turning unserialized record data into a Doctrine_Record object, possibly making use of already loaded entities that are available in the table's internal caches. With unserialize being handled directly from the Doctrine_Record, there is no way AFAICS to keep up the rule of always having exactly one object in a scope that represents a certain object in the database.

      When running above script with this patch applied and with removing the $a, $b and $c assignments (just for making the oid's different between the two script runs), we get the following output:

      $ php test.php
      Serialize using oid=2
      $ php test.php
      unserialized oid = 5
      new object oid = 5
      24 versus 24
      

      So here, unserializing an object and then reloading the object through the table object gives use two times the same object, representing db object with id = 4;

      When doing things in a different order, we still can force an issue, but this is due to the things mentioned above: to cleanly handle this, unserialization should be handled from a factory method on Table. This script shows the behavior:

      if (!file_exists("/tmp/serialize.doctrine"))
      {
        $d = Address::getById(4);
        print "Serialize using oid=" . $d->getOid() . "\n";
        file_put_contents("/tmp/serialize.doctrine", serialize($d));
      }
      else
      {
        $d2 = Address::getById(4);
        print "new object oid = " . $d2->getOid() . "\n";
      
        $d = unserialize(file_get_contents("/tmp/serialize.doctrine"));
        print "unserialized oid = " . $d->getOid() . "\n";
      
        $d3 = Address::getById(4);
        print "new object oid = " . $d3->getOid() . "\n";
      
        $d->housenumber ++;
        print "{$d->housenumber} versus {$d2->housenumber} versus {$d3->housenumber}\n";
      }
      

      The output of the second run being:

      $php test.php
      new object oid = 2
      unserialized oid = 5
      new object oid = 5
      24 versus 23 versus 24
      

      The first object that was created is still different from the unserialized object, but at least the third object that is created, is the object that was unserialized.

      I hope that this makes the issue clear and that my input helps in fixing things. This issue provided us with some really unexpected behavior and we're glad that we were able to track it down to here.

      1. Doctrine_Record.unserialize.patch
        0.9 kB
        Maurice Makaay
      2. Doctrine_Record.unserialize.patch
        1 kB
        Maurice Makaay

        Activity

        Hide
        Maurice Makaay added a comment -

        FYI: we removed the "Remove existing record from the repository and table entity map" code from our patch. We had some issues with unit testing without this bit, but we updated the unit tests to work without this bit of code. The "Add the unserialized record to repository and entity map' part stays of course. I think you can ignore the cleanup code in the patch. As long as scripts unserialize data before working with it, things should be fine without it.

        Show
        Maurice Makaay added a comment - FYI: we removed the "Remove existing record from the repository and table entity map" code from our patch. We had some issues with unit testing without this bit, but we updated the unit tests to work without this bit of code. The "Add the unserialized record to repository and entity map' part stays of course. I think you can ignore the cleanup code in the patch. As long as scripts unserialize data before working with it, things should be fine without it.
        Hide
        Maurice Makaay added a comment -

        New version of the patch, without the cleanup code in it.

        Show
        Maurice Makaay added a comment - New version of the patch, without the cleanup code in it.
        Hide
        Maurice Makaay added a comment -

        Trying to always use the same object for the same entity in the database, we came up with a factory method that we put on our record class as a static public. Maybe this is an idea that you want to include in Doctrine as well?

          static public function fromSerialized($serialized)
          {
            $entity = unserialize($serialized);
        
            if (!($entity instanceof Doctrine_Record)) throw new Exception(
              __METHOD__ . ': serialized object is not a Doctrine_Record object'
            );
        
            // If the unserialized object is a persisted entity, then we must
            // check if there is already an object for that entity available in
            // Doctrine's table repository.
            if ($entity->exists())
            {
              // Retrieve the entity through the table repository.
              $table = $entity->getTable();
              $repository_entity = $table->find($entity->id);
        
              // If a different object was returned than our unserialized
              // object, then there was an object loaded before unserialization.
              // We will merge the data from the unserialized object with
              // the existing object and return the existing object to the caller.
              if ($entity->getOid() !== $repository_entity->getOid()) {
                $repository_entity->merge($entity);
                $entity = $repository_entity;
              }
            }
        
            return $entity;
          }
        

        For us, it's working wonders in combination with above Doctrine_Record::unserialize() patch. When letting this code handle our unserialization, the same object (checked by its oid) is used for referencing the same entity throughout the code. We call the code like this:

        $object = Doctrine_Record::fromSerialized($serialized_data);
        
        Show
        Maurice Makaay added a comment - Trying to always use the same object for the same entity in the database, we came up with a factory method that we put on our record class as a static public. Maybe this is an idea that you want to include in Doctrine as well? static public function fromSerialized($serialized) { $entity = unserialize($serialized); if (!($entity instanceof Doctrine_Record)) throw new Exception( __METHOD__ . ': serialized object is not a Doctrine_Record object' ); // If the unserialized object is a persisted entity, then we must // check if there is already an object for that entity available in // Doctrine's table repository. if ($entity->exists()) { // Retrieve the entity through the table repository. $table = $entity->getTable(); $repository_entity = $table->find($entity->id); // If a different object was returned than our unserialized // object, then there was an object loaded before unserialization. // We will merge the data from the unserialized object with // the existing object and return the existing object to the caller. if ($entity->getOid() !== $repository_entity->getOid()) { $repository_entity->merge($entity); $entity = $repository_entity; } } return $entity; } For us, it's working wonders in combination with above Doctrine_Record::unserialize() patch. When letting this code handle our unserialization, the same object (checked by its oid) is used for referencing the same entity throughout the code. We call the code like this: $object = Doctrine_Record::fromSerialized($serialized_data);

          People

          • Assignee:
            Jonathan H. Wage
            Reporter:
            Maurice Makaay
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: