Criteria needs a __wakeup() method? #613

Open
oschonrock opened this Issue Feb 18, 2013 · 8 comments

4 participants

@oschonrock

Slightly unusual use-case, but I am constructing a complex criteria object in one process and then passing it via serialization to another process, which runs the query using the unserialized criteria. The same would apply if storing criteria in session.

When the criteria wakes up in the 2nd process, it will sometimes not work (query fails in BasePeer with "Cannot fetch TableMap for undefined table blah"). This is because in the new process the Peer class for that table has not necessarily been "required" yet and therefore buildTableMap() has not run.

In the 1st process this is not an issue, because the Peer will always have been "required" in the process of building the criteria.

I played around with the contents of the woken up criteria to figure out which peer classes need to be "required". I wrote the utility method below which works for me. Could this realistically be implemented as __wakeup on criteria? If so the Peer class name construction below needs improving (it currently uses one of my utility functions: camel_case and there might be issues if the php Peer class name is different to the table name. The latter is never the case in my model.).

  /**                                                                                                                                                                                 
   * necessary to call this when a criteria has been passed from one process to another.                                                                                              
   * eg during serilization and de-serialization of passing as a param to a forkedJob                                                                                                 
   * it kind of should be implemented as a __wakeup on criteria really                                                                                                                
   *                                                                                                                                                                                  
   */
  public static function wakeupCriteria(Criteria $criteria)
  {
    foreach ($criteria->getJoins() as $join)
    {
      // essentially require the Peer classes which are used on the joins of the criteria (includes primary table)                                                                    
      // the end of each Peer runs eg buildTableMap() which we need for the criteria to work                                                                                          
      // we do not mind repeats because class_exists will just return immediately                                                                                                     
      if (($table = $join->getLeftTableName()) !== null) class_exists(camel_case($table, true) . 'Peer');
      if (($table = $join->getRightTableName()) !== null) class_exists(camel_case($table, true) . 'Peer');
    } 
  }
@marcj
Propel member

@oschonrock, what do you mean by process? Real two different php processes?
However, if you want to deserialize a php object, no matter which class, then you should really already have setup the required environment (loaded all needed classes etc). So, I think that code shouldn't be a part of propel's base.

necessarily been "required" yet

Then you should probably make sure that all required classes have been require()'d.

@oschonrock

Hi, thanks for response.

Yes, 2 proper separate php processes. I have provided some much simplified code below that reproduces the issue.

All classes are loaded/required by autoload: Propel's own autoload and several others. This is a very large application with more than 1000 classes.

The issue happens because in proc1.php (see code below) OrganisationGroupPeer is autoloaded when we refer to the constant while defining the criteria. That file, om/BaseOrganisationGroupPeer.php, also (importantly) runs

BaseOrganisationGroupPeer::buildTableMap();

The criteria works in proc1. In proc2 no code ever refers to OrganisationGroupPeer, so it is not autoloaded and buildTableMap is never run. This is why the exception is thrown.

It is not possible for proc2 to know which class files it needs to load, without examining the contents of the criteria. This breaks encapsulation. The serialized criteria object has internal class loading dependencies, but fails to trigger Propel's autoload when "waking up" from a serialized state. My proposed "__wakeup" method solves this by triggering Propel's autoload for those tables that are contained in the Criteria.

If I include a call to the proposed wakeupCriteria in proc2 before using the criteria object, the necessary classes get loaded and the exception in BasePeer is avoided. I have shown the use of my proposed wakeupCriteria method in a separate proc2fixed.php. My suggestion is to put that code into Criteria::__wakeup so it happens automatically when criteria in unserialized. Not as good, but still useful, would be to implement that method as an instance method on criteria so that you call:

$c = unserialize(file_get_contents('/tmp/crit'));
$c->wakeup();

I hope that makes the issue clearer. Below is the sample code for proc1 and proc2 and the result as run from command line.

proc1.php

#!/usr/bin/env php
<?php

// initialises propel and its autoloader, etc...                                                                                                                                       
require dirname(dirname(__FILE__)) . '/lib/base_rwf.php';

echo "this is process #1\n";

$c = new Criteria();
$c->add(OrganisationGroupPeer::ID, 3);
$c->addJoin(OrganisationPeer::ID,OrganisationGroupPeer::ORGANISATION_ID);

file_put_contents('/tmp/crit', serialize($c));

$org = OrganisationPeer::doSelectOne($c);

echo 'org#' . $org->getId() . ' seo_name=' . $org->getSeoName() . "\n";

proc2.php

#!/usr/bin/env php
<?php

// initialises propel and its autoloader, etc...                                                                                                                                       
require dirname(dirname(__FILE__)) . '/lib/base_rwf.php';

echo "this is process #2\n";

$c = unserialize(file_get_contents('/tmp/crit'));

$org = OrganisationPeer::doSelectOne($c);

echo $org->getSeoName();

result when running:

# php cli/proc1.php && php cli/proc2.php
this is process #1
org#3 seo_name=bvsc
this is process #2
PHP Fatal error:  Uncaught exception 'PropelException' with message 'Cannot fetch TableMap for undefined table: organisation_group' in /extlib/propel-1.6.7/runtime/lib/map/Database\
Map.php:127
Stack trace:
#0 /extlib/propel-1.6.7/runtime/lib/adapter/DBAdapter.php(556): DatabaseMap->getTable('organisation_gr...')
#1 /extlib/propel-1.6.7/runtime/lib/util/BasePeer.php(479): DBAdapter->bindValues(Object(PDOStatement), Array, Object(DatabaseMap))
#2 /lib/propel/om/BaseOrganisationPeer.php(502): BasePeer::doSelect(Object(Criteria), Object(PropelPDO))
#3 /lib/propel/om/BaseOrganisationPeer.php(472): BaseOrganisationPeer::doSelectStmt(Object(Criteria), NULL)
#4 /lib/propel/om/BaseOrganisationPeer.php(454): BaseOrganisationPeer::doSelect(Object(Criteria), NULL)
#5 /cli/proc2.php(11): BaseOrganisationPeer::doSelectOne(Object in /extlib/propel-1.6.7/runtime/lib/util/BasePeer.php on line 488

proc2fixed.php

#!/usr/bin/env php
<?php

// inialises propel and its autoloader, etc...                                                                                                                                       
require dirname(dirname(__FILE__)) . '/lib/base_rwf.php';

function wakeupCriteria(Criteria $criteria)
{
  foreach ($criteria->getJoins() as $join)
  {
    // essentially require the Peer classes which are used on the joins of the criteria (includes primary table)                                                                     
    // the end of each Peer runs eg buildTableMap() which we need for the criteria to work                                                                                           
    // we do not mind repeats because class_exists will just return immediately                                                                                                      
    if (($table = $join->getLeftTableName()) !== null) class_exists(camel_case($table, true) . 'Peer');
    if (($table = $join->getRightTableName()) !== null) class_exists(camel_case($table, true) . 'Peer');
  }
}

echo "this is process #2\n";

$c = unserialize(file_get_contents('/tmp/crit'));

wakeupCriteria($c);

$org = OrganisationPeer::doSelectOne($c);

echo 'org#' . $org->getId() . ' seo_name=' . $org->getSeoName() . "\n";

fixed result

php cli/proc1.php && php cli/proc2fixed.php 
this is process #1
org#3 seo_name=bvsc
this is process #2
org#3 seo_name=bvsc
@marcj
Propel member

Thank you really much for your extensive explanation.

Basically, the Criteria classes are standalone. This means, there are no references at all to the actual custom model's classe. This is also the reason, why the autoload is not triggered. You have two ways to go around that:

  • Use the *Query object instead of Criteria in your script 1 :
$c = new OrganisationGroupQuery()
    ->filterById(3)
    ->joinOrganisation();
  • Fire OrganisationPeer::buildTableMap(); before you run doSelectOne in script 2.

Unfortunately, there's no way to detect which table belongs to which model class, due to several reasons:

  • There's no default table => class name map, technically its vice verse: a model says his table name, so you need first to know the class name of this model to know then the table
  • AutoDetection as in your case fails, because it's not always camelCased. Actually, a developer can completely name his class as he want.

I'll try to write a fix for that, but not sure if it will be very elegant. Maybe this will be always a hack in your described scenario.

@marcj marcj added a commit to marcj/Propel that referenced this issue Feb 25, 2013
@marcj marcj Fixed #613. Proves the issue described in #613 and added a kind of fi…
…x for it.
9019d7a
@marcj
Propel member

That referenced feature should do the trick for you, but read the information at the method, please.
It's not best practise to call this method, but sometimes there is no other way (as it seems in your scenario).

@staabm
Propel member

I think this shouldn't be a supported feature by Propel, because it really looks like an edge case.
@oschonrock does the method provided by @marcj work for you?

If so, you could just copy it into a util class of your project?

@niels-nijens
@oschonrock

@marcj I can see what you are trying to do with Popel:buildAllTableMaps. Because there appears to be no realiable generic lookup/conversion from table_name to Peer class name. I tried it, it's slow as you predicted, it also required a slight modification to work for me...the classmap includes the "Base" prefixed names and the normal ones, so I changed the if to this to prevent a "class already defined" error:

          if (substr($class, -4) === 'Peer' && strpos($class, 'Base') !== 0) {

then it works. However the "Base" prefix can be changed at generator time if I am not mistaken, so my fix will probably not work generically.

I also tried with using ModelCriteria Query classes in proc1 and proc2 (I almost exclusively use them everywhere, but for legacy reasons the first example used Criteria)...I also changed the actual query, as the first example made no commercial sense (not that that matters here). I have included below 2 new files proc1Mod.php and proc2Mod.php which use ModelCriteria Query objects. This is nicer for sure, but also suffers from the same problem. see proc1Mod.php and proc2Mod.php below....My wakeupCriteria works (because Query extends Criteria) and your (modified) buildAllTableMaps also works, but without those it fails the same as the original Criteria based example

@niels-nijens and @staabm . You're right, I could extend Criteria with "SerializableCriteria" implementing my __wakeup method, use it in proc1 and proc2, and that would solve the problem for my application. Because my table names all match my Peer classes (via camelCase), I don't need marcj's sledgehammer solution. However, I already have a solution for me ;-) . It's running in production and works fine.

The reason for posting the issue, is to benefit other users. In general we find that when you serialialize Propel classes, all sorts of funky things start to happen and you need to be very careful. We do it a lot, because we have been using Propel since ~2004 and have an advanced application that needs this of thing.

Having said that, if you think it's beyond what propel wants to support and should be considered "bloat", that's fine with me and we can just leave this issue here as "Googlebait" so other users can use the ideas in it if they come across this problem.

You could even consider implementing __wakeup in Criteria and throwing an Exception with a message linking to this issue, because, in truth it doesn't really work right now, unless you bend over backwards a bit. What happened to us, is that it broke in production because it actually only fails "sometimes". It depends on the Criteria contents and classes already loaded in proc2. So always breaking on __wakeup is better in my opinion, because then you can do something about it before deploying to production.

@niels-nijens and @marcj the PHPBuilder classes (ie generator time) must know about the table_name to Peer class mapping. Is there not a way to expose this knowledge to runtime?

Here is the modified example using ModelCriteria Queries (and a change in the actual query)

proc1Mod.php

#!/usr/bin/env php
<?php

// inialises propel and its autoloader, etc...                                                                                                                                       
require dirname(dirname(__FILE__)) . '/lib/base_rwf.php';

echo "this is process #1\n";

$query = OrganisationQuery::create()
  ->useTaxClassQuery()
    ->filterById(2)
  ->endUse();

file_put_contents('/tmp/crit', serialize($query));

$org = $query->findOne();

echo 'org#' . $org->getId() . ' seo_name=' . $org->getSeoName() . "\n";

proc2Mod.php

#!/usr/bin/env php
<?php

// inialises propel and its autoloader, etc...                                                                                                                                       
require dirname(dirname(__FILE__)) . '/lib/base_rwf.php';

function wakeupCriteria(Criteria $criteria)
{
  foreach ($criteria->getJoins() as $join)
  {
    // essentially require the Peer classes which are used on the joins of the criteria (includes primary table)                                                                     
    // the end of each Peer runs eg buildTableMap() which we need for the criteria to work                                                                                           
    // we do not mind repeats because class_exists will just return immediately                                                                                                      
    if (($table = $join->getLeftTableName()) !== null) class_exists(camel_case($table, true) . 'Peer');
    if (($table = $join->getRightTableName()) !== null) class_exists(camel_case($table, true) . 'Peer');
  }
}

echo "this is process #2\n";

$query = unserialize(file_get_contents('/tmp/crit'));

// does not work without calling this                                                                                                                                                
// wakeupCriteria($query);                                                                                                                                                           

// or this                                                                                                                                                                           
// Propel::buildAllTableMaps();                                                                                                                                                      

$org = $query->findOne();

echo 'org#' . $org->getId() . ' seo_name=' . $org->getSeoName() . "\n";
@oschonrock

I hacked around in the PropelCovertConfTask to generate a map from tables to Base Peer classes.. this is the resulting diff (not polished at all), if that's useful:

diff -u extlib/propel-1.6.7/generator/lib/task/PropelConvertConfTask.php ~/
--- extlib/propel-1.6.7/generator/lib/task/PropelConvertConfTask.php    2012-07-30 08:58:15.000000000 +0100
+++ /home/oliver/PropelConvertConfTask.php      2013-02-26 13:52:47.000000000 +0000
@@ -152,6 +152,14 @@
         $output .= var_export($phpconfClassmap, true);
         $output .= ";";
 
+        $phpconfTablemap = $this->getTableMap();
+        $outfile = new PhingFile($this->outputDirectory, 'table_map');
+        $output = '<' . '?' . "php\n";
+        $output .= "// This file generated by Propel " . $phpconf['generator_version'] . " convert-conf target".($this->getGeneratorConfig()->getBuildProperty('addTimestamp') ? " on " . strftime("%c") : '') . "\n";
+        $output .= "return ";
+        $output .= var_export($phpconfTablemap, true);
+        $output .= ";";
+
         $mustWriteClassMap = true;
         if (file_exists($outfile->getAbsolutePath())) {
             $currentClassmap = file_get_contents($outfile->getAbsolutePath());
@@ -393,4 +401,38 @@
 
         return $phpconfClassmap;
     }
+
+    /**
+     * Lists data model classes and builds an associative array className => classPath
+     * To be used for autoloading
+     * @return array
+     */
+    protected function getTableMap()
+    {
+        $phpconfTablemap = array();
+
+        $generatorConfig = $this->getGeneratorConfig();
+
+        foreach ($this->getDataModels() as $dataModel) {
+
+            foreach ($dataModel->getDatabases() as $database) {
+
+                $classMap = array();
+
+                foreach ($database->getTables() as $table) {
+
+                    if (!$table->isForReferenceOnly()) {
+
+                      $builder = $generatorConfig->getConfiguredBuilder($table, 'peer');
+                      $phpconfTablemap[$table->getName()] = $builder->getFullyQualifiedClassname();
+                    }
+                }
+            }
+        }
+
+        // sort the classmap by class name, to avoid discrepancies between OS
+        ksort($phpconfTablemap);
+
+        return $phpconfTablemap;
+    }
 }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment