Nested transactions or other refactoring method

Not for 'how-to' coding questions but PHP theory instead, this forum is here for those of us who wish to learn about design aspects of programming with PHP.

Moderator: General Moderators

matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Nested transactions or other refactoring method

Post by matthijs »

Below is an example with 3 model classes which have a kind of hierarchical relationship. One country contains many regions. One region contains many cities. Each class will have setters to make it possible to set the needed properties. And then each class will have different CRUD methods. In the example below I just keep it to save for now. Now as you skim through the code you see there's a lot of repetition.
- Region is performing a piece of code (logic+queries) from the save() method in Country and then some more
- City is performing a piece of code from the save() method in region and then some more
If I were to also have a class for, say neighborhoods, the nesting and repetition of code would be even worse.

The problem is, in this example I use transactions. These are important because I may make several updates to different tables and I want to rollback in case anything goes wrong. And since nested transactions are not possible (yet?) I cannot just reuse the save methods of one class in the other. So I can not let the City's save() method use the Region's save() method instead of - as it is now - duplicate all the logic and queries.

What direction would you take to solve this problem?

p.s. the code is incomplete to make it more readable, just enough to show the problem hopefully

Code: Select all

class Country {
    protected $country;
    function __construct(){
        $conn = new PDO('odbc:*', '*', '*', array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION));
    }   
    function save(){
        $conn->beginTransaction();
        try {
            
            // Check if country already exists
            // if not INSERT COUNTRY 
            $this->conn->execute("INSERT INTO countries (country) VALUES ($this->country)");
 
            $conn->commit();
        }
        catch (Exception $e) {
            $conn->rollBack();
        }
    }
}
class Region {
    protected $region;
    protected $country;
    function __construct(){
        $conn = new PDO('odbc:*', '*', '*', array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION));
    }   
    function save(){
        $conn->beginTransaction();
        try {
 
            // Check if country exists
            // if not first INSERT country
            $this->conn->execute("INSERT INTO countries (country) VALUES ($this->country)";
            // then get lastInsertId as $countryId and use that in following query
            $this->conn->execute("INSERT INTO regions (region, countryid) VALUES ($this->region, $countryId)");
 
            $conn->commit();
        }
        catch (Exception $e) {
            $conn->rollBack();
        }
    }
}
class City {
    protected $country;
    protected $region;
    protected $city;
    function __construct(){
        $conn = new PDO('odbc:*', '*', '*', array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION));
    }   
    function save(){
        $conn->beginTransaction();
        try {
            
            // Check if region exists
            // if not first INSERT region
            // but maybe country doesn't yet exists, so first check if country exist
            // if not INSERT country
            $this->conn->execute("INSERT INTO countries (country) VALUES ($this->country)";
            // then get lastInsertId as $countryId and use that in following query
            $this->conn->execute("INSERT INTO regions (region, countryid) VALUES ($this->region, $countryId)");
            // then get lastInsertId as $regionId and use that in following
            $this->conn->execute("INSERT INTO cities (city, regionid, countryid) VALUES ($city, $regionId, $countryId)");
 
            $conn->commit();
        }
        catch (Exception $e) {
            $conn->rollBack();
        }
    }
}
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Re: Nested transactions or other refactoring method

Post by Maugrim_The_Reaper »

I could be oversimplifying...but the problem seems to be the transactions. Is there any way of externalising them? For example, use a parent class to track if a transaction is open, and refuse any further child calls to start a new transaction. Of course you then have to somehow externalise the commits or the transaction will end prematurely. So more tracking and limitations on when a transaction can be committed (for example only the class which starts a transaction can actually commit).

It doesn't sound pretty, but it's the potential solution that jumped to mind quickest.
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: Nested transactions or other refactoring method

Post by matthijs »

Interesting, thanks. That might be something to look into.

The classes I use in my real situation do in fact all extend a single base model class which instantiates the db connection, deal with the getters and setters, etc. So that class might then contain the logic to keep track of what has been started by who.

I am just not too far in design patterns yet and am being careful not to put something together that's too large a mess of interconnected code.. of course repetition is also bad, but still.

I would think this would be quite a common problem but so far I haven't found any good examples of code or design patterns which fit a solution. But I'll try and see what your idea does.
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Re: Nested transactions or other refactoring method

Post by Maugrim_The_Reaper »

If it sounds risky, refactor the tracking into an independent class by itself. Then at least it would be a bit easier to tamper with, have less distribution of the tracking logic that could make change difficult or obscure, and if necessary roll back into the other classes.

And unit test it to death of course ;). I would get a handle on defining use cases and probable scenarios before going much further which could then be translated to a range of acceptance tests to ensure your Model, as a whole, behaves as you expect it to.
koen.h
Forum Contributor
Posts: 268
Joined: Sat May 03, 2008 8:43 am

Re: Nested transactions or other refactoring method

Post by koen.h »

Code: Select all

class Country {
 
    private $id;
 
    private $units = array();
 
    public function addUnit($unit)
    {
        $this->units[] = $unit;
        $unit->setCountryId($this->id);
    }
 
    public function save()
    {
        $conn = $this->getConnection();
        $conn->execute("INSERT INTO countries (country) VALUES ($this->country)");
        foreach($this->units as $unit) {
            $unit->save();
        }
    }
}
 
class Region {
 
    private $id;
 
    private $country_id;
 
    private $units = array();
 
    public function addUnit($unit)
    {
        $this->units[] = $unit;
        $unit->setRegionId($this->id);
        $unit->setCountryId($this->country_id);
    }
 
    public function setCountryId($country_id)
    {
        $this->country_id = $id;
    }
 
    public function save()
    {
        $conn = $this->getConnection();
        $conn->execute("INSERT INTO regions (region, countryid) VALUES ($this->region, $countryId)");
       foreach($this->units as $unit) {
            $unit->save();
        }
    }
 
}
 
class City {
 
    private $id;
 
    private $country_id;
 
    private $region_id;
 
    public function setCountryId($contry_id)
    {
        $this->country_id = $id;
    }
 
   public function setRegionId($contry_id)
    {
        $this->region_id = $id;
    }
 
    public function save()
    {
        $conn = $this->getConnection();
        $conn->execute("INSERT INTO cities (city, regionid, countryid) VALUES ($city, $regionId, $countryId)");
    }
 
}
Still have to factor out all repeating elements into an abstract unit Class and a CompositeUnit class and add rollback capacity :?
Other edit: I would externalize the connection. Eg
getConnection() { /* get db connection in registry or factory or set one if not exists */ }
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: Nested transactions or other refactoring method

Post by matthijs »

@koen.h: Thanks for your reply. I think I understand were you are heading but there are some things in your example not entirely clear.

If I take you city class:

Code: Select all

 
class City {
 
    private $id;
 
    private $country_id;
 
    private $region_id;
 
    public function setCountryId($contry_id)
    {
        $this->country_id = $id;
    }
 
   public function setRegionId($contry_id)
    {
        $this->region_id = $id;
    }
 
    public function save()
    {
        $conn = $this->getConnection();
        $conn->execute("INSERT INTO cities (city, regionid, countryid) VALUES ($city, $regionId, $countryId)");
    }
 
}
The point is, the way I want to use this is something like:

Code: Select all

 
$city = new City;
$city->name = "Washington";
$city->region = "District Columbia";
$city->country = "VS";
$city->save;
 
So the methods you had setCountryId() and setRegionId() are not useful. That was the core of the problem. I set a city name, region name and country name and then the city class has to figure everything out (does the region already exists, if not insert, but then check if the country already exists, etc).
But by doing that it repeats almost all code from other classes.

Maybe I didn't fully understand you example code, if so please explain. I do thank you for helping here.

@Maugrim:
yes, unit tests are a good idea. Actually I am using them. That's why I end up with all the repeating code. I just coded it to make sure my tests pass :) I'll have to add more tests to handle exceptional behavior and after that I can try refactor stuff out.
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Re: Nested transactions or other refactoring method

Post by Chris Corbyn »

Java apps tend to follow a transaction-per-request model:

[java]try {  tx.begin();  runApplicationContext();  tx.commit();} catch (VariousExceptions ex) {  //blah} finally {  if (!tx.wasComitted()) {    tx.rollback();  }}[/java]

The lack of a "finally" block makes it a tad less elegant in PHP though. It probably does help with testing however.
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: Nested transactions or other refactoring method

Post by matthijs »

Yes but were do I place this try-catch block? (in PHP). If I were to put this in my FrontController, putting the Dbtransaction stuff there wouldn't make much sense?
koen.h
Forum Contributor
Posts: 268
Joined: Sat May 03, 2008 8:43 am

Re: Nested transactions or other refactoring method

Post by koen.h »

matthijs wrote:@koen.h: Thanks for your reply. I think I understand were you are heading but there are some things in your example not entirely clear.

If I take you city class:

Code: Select all

 
class City {
 
    private $id;
 
    private $country_id;
 
    private $region_id;
 
    public function setCountryId($contry_id)
    {
        $this->country_id = $id;
    }
 
   public function setRegionId($contry_id)
    {
        $this->region_id = $id;
    }
 
    public function save()
    {
        $conn = $this->getConnection();
        $conn->execute("INSERT INTO cities (city, regionid, countryid) VALUES ($city, $regionId, $countryId)");
    }
 
}
The point is, the way I want to use this is something like:

Code: Select all

 
$city = new City;
$city->name = "Washington";
$city->region = "District Columbia";
$city->country = "VS";
$city->save;
 
So the methods you had setCountryId() and setRegionId() are not useful. That was the core of the problem. I set a city name, region name and country name and then the city class has to figure everything out (does the region already exists, if not insert, but then check if the country already exists, etc).
But by doing that it repeats almost all code from other classes.

Maybe I didn't fully understand you example code, if so please explain. I do thank you for helping here.
The "then the city class has to figure everything out" is something that I wouldn't like in my code. It is not the responability of the City object to see whether it resides in a country that exists. That should be something for the Country class, to declare indepence.

In my example you'd have to do:
$country = new Country($devcountry);
$region = new Region('devregion');
$city = new City('devcity');
$country->addUnit($region); (or addRegion or so)
$region->addUnit($city);
$city->save();

Ok I see now what's missing. I should replace the id's with instances: setCountry(CountryInterface $country) { $this->country=$country }, then city->save() { $this->country->save(); }
Not quite what you want but closer.

I probably had to see the wider application to see what makes most sense here.
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: Nested transactions or other refactoring method

Post by matthijs »

The "then the city class has to figure everything out" is something that I wouldn't like in my code. It is not the responability of the City object to see whether it resides in a country that exists. That should be something for the Country class, to declare indepence
That's true. But the main problem is the transactions.

It would be easy to do something like:

Code: Select all

 
class City {
  public function save(){
    $country = new Country;
    $country->name = $this->countryname;
    $countryid = $country->save(); // inserts if not exists, return id
    
    $region = new Country;
    $region->name = $this->regionname;
    $regionid = $region->save(); // inserts if not exists, return id
 
    $conn->execute("INSERT INTO cities (city, regionid, countryid) VALUES ($city, $regionId, $countryId)");
  }
}
 
But the transactions and not being able to nest them is the problem.

The wider application doesn't work with these specific objects (cities, regions and countries) but I thought they were easier to use as an example.
The user of the application would go to a form and fill in a form for a "City". Including all related info (like region, country, and other info). Then everything should be saved in one go. That's the most important action.

But it should also be possible to add Regions independently.

The real app I'm building is about Climbs (climbing a route). Each Climb is climbing a Route in a certain Area. And each Area in a certain Country. So one country may contain more areas. One area may contain more routes. Each time a user fills in the form for a Climb, the Route table must be updated for the route (if it didn't already exist), the area table must be updated (in case the area not already existed). The country table is one I probably prefill with all countries in the world. Because the integrity of the db is important, I want to use transactions for the queries.

Of course an alternative would be to have the user first fill in a form for the area he climbed in. Let that save the area. And then let the user fill in a form for the Climbs/routes he did in that area. But if I can make it any easier for the user, I'd prefer that. So a user should be able to fill in a form with everything and let that save everything. But (later) a user should also be able to manage area's separately.
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: Nested transactions or other refactoring method

Post by matthijs »

Maybe to clarify it further, in response to koen.h's example

Code: Select all

 
// In my example you'd have to do:
$country = new Country($devcountry);
$region = new Region('devregion');
$city = new City('devcity');
$country->addUnit($region); (or addRegion or so)
$region->addUnit($city);
$city->save();
 
A design like this is simple enough. But the whole issue is:
How would you implement transactions here, to make sure that in case something goes wrong after inserting the region but before inserting the city you can roll everything back?

Maybe transactions are not used much or you might think they are not that important. If so, glad to hear about that as well. We're not talking about a mission critical financial application here. But I'm happy to put a bit of effort in to keep my db sane in the long term.
koen.h
Forum Contributor
Posts: 268
Joined: Sat May 03, 2008 8:43 am

Re: Nested transactions or other refactoring method

Post by koen.h »

I haven't tried this out yet. But it is not possible to use a transaction like this:

Code: Select all

 
somesavemethod() {
 $conn = Registry::getConnection();
 $conn->beginTransaction();
 try {
  $conn->execute("INSERT INTO countries (country) VALUES ($this->country)");
  $a = new WhateverClass();
  $a->save();
 }
 catch (Exception $e) {
          $conn->rollBack();
 }
}
 
class WhateverClass {
 public function save() {
 $conn = Registry::getConnection();
 $conn->execute("INSERT INTO etc");
 }
}
 
? It's the same connection object.
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: Nested transactions or other refactoring method

Post by matthijs »

I would not know but it's worth investigating. Thanks for the tip, I'll try it.
koen.h
Forum Contributor
Posts: 268
Joined: Sat May 03, 2008 8:43 am

Re: Nested transactions or other refactoring method

Post by koen.h »

It's because you talk about nested transactions but in what I created above you don't need to nest since it's the same connection object.
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: Nested transactions or other refactoring method

Post by matthijs »

Yes. I'd probably have to use a singleton for the connection.

One thing that's "problematic" with your idea is that it's not a single nesting level, but even deeper.
And then you still end up with nested transactions, like:

Code: Select all

 
class Climb{
    public function save(){
        $conn->beginTransaction();
        try {
            // insert some code to check if route exists. if not
            $a = new Route();
            $a->save();
            // get lastInsertId and use it in following query
            $conn->execute("INSERT INTO climbs (userid, date, route_id) VALUES ($this->userid, $this->date, $this->routeid)");
            $conn->commit();
        }
        catch(){
            $conn->rollBack();
        }
}
class Route {
    public function save(){
        $conn->beginTransaction();
        try {
            // insert some code to check if area exists. if not
            $a = new Area();
            $a->save();
            // get  lastInsertId and use it in following query
            $conn->execute("INSERT INTO routes (name, area_id) VALUES ($this->name, $area_id)");
            $conn->commit();
        catch(){
            $conn->rollBack();
        }
    }
}
class Area {
    public function save(){
        $conn->beginTransaction();
        try {
            // insert some code to check if country exists. if not
            $a = new Country();
            $a->save();
            // get  lastInsertId and use it in following query
            $conn->execute("INSERT INTO areas (name, countryid) VALUES ($this->area, $countryid)");
            $conn->commit();
        } catch(){
            $conn->rollBack();
        }
    }
}
 
Maybe I can put the insert queries in a separate method in each class, say insert(), not using transactions. Then that can be used by a parent class within the transaction goin on in the parent method.

Then, if you call the child class directly, you could use another method, say save(), which does use transactions and which does use the internal method insert() within it. That way I can at least reuse the query statements.

I'd never would have thought a few models like these would be so hard to deal with :) ....
Post Reply