Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveRecord: transaction support #226

Closed
resurtm opened this issue May 11, 2013 · 18 comments · Fixed by #253
Closed

ActiveRecord: transaction support #226

resurtm opened this issue May 11, 2013 · 18 comments · Fixed by #253
Labels
status:under development Someone is working on a pull request. type:enhancement
Milestone

Comments

@resurtm
Copy link
Contributor

resurtm commented May 11, 2013

Consider we've designed the following AR model:

/**
 * @property integer $id
 * @property string $username
 * @property string $password_digest
 *
 * @property Post[] $posts
 * @property integer[] $postIds
 */
class User extends ActiveRecord
{
    public function afterSave($insert)
    {
        parent::afterSave($insert);

        if ($this->isAttributeSafe('postIds')) {
            foreach ($this->posts as $post) {
                $this->unlink('posts', $post);
            }
            if (is_array($this->getPostIds())) {
                foreach ($this->getPostIds() as $postId) {
                    $post = Post::find()
                        ->where(array('id' => $postId))
                        ->one();
                    $this->link('posts', $post);
                }
            }
        }
    }

    /**
     *
     */
    public function getPosts()
    {
        return $this->hasMany('Post', array('author_id' => 'id'));
    }

    /**
     * @var integer[]
     */
    private $_postIds;

    /**
     * @return integer[]
     */
    public function getPostIds()
    {
        if ($this->_postIds === null) {
            $this->_postIds = array();
            foreach ($this->posts as $post) {
                $this->_postIds[] = $post->id;
            }
        }
        return $this->_postIds;
    }

    /**
     * @param integer[] $postIds
     */
    public function setPostIds($postIds)
    {
        if (is_array($postIds)) {
            $this->_postIds = $postIds;
        }
    }

    public function save($runValidation = true, $attributes = null)
    {
        $transaction = $this->getDb()->beginTransaction();
        try {
            $result = parent::save($runValidation, $attributes);
            if ($result) {
                $transaction->commit();
            } else {
                $transaction->rollback();
            }
        } catch (\Exception $e) {
            $transaction->rollback();
            throw $e;
        }
        return $result;
    }
}

User::$postIds property used in a view form in checkbox list field. Note that User::save() method was slightly enhanced. On what basis this decision made?

1. We could work with transactions inside controller action, but this approach violates fat model—thin controller principle. Everyone agree we shouldn't deal with the database transactions inside controller layer? This way is conceptually incorrect.

2. Create new abstraction layer (behavior, helper, whatever) which would deal with the related records and it could use database transactions to save main AR model with related AR models atomically. I don't think i have to introduce additional abstraction layer whilst it can be solved in a more lightweight fashion.

3. Enhance AR class itself. I did it in my own AR model but i propose to do this in yii\db\ActiveRecord as well. The solution would be to introduce third parameter (called something like $wrapWithTransaction) in ActiveRecord::insert(), ActiveRecord::update() and ActiveRecord::save() methods.

What do you think about my proposal? Patch itself is straightforward and ready to be merged.

I can say that this situation happens to me very often and it worth implementing it in the core.

@creocoder
Copy link
Contributor

If do this change ActiveRecord::delete() method should have this param too.

I can say that this situation happens to me very often and it worth implementing it in the core.

For me too, happens in every application.

@resurtm
Copy link
Contributor Author

resurtm commented May 11, 2013

OK, preparing PR to be more convincing. :-)

@Ragazzo
Copy link
Contributor

Ragazzo commented May 11, 2013

btw, guys would it be some auto-transaction for db-component maybe, i think it is useful?

@samdark
Copy link
Member

samdark commented May 11, 2013

@Ragazzo what do you mean?

@Ragazzo
Copy link
Contributor

Ragazzo commented May 11, 2013

I mean that if i set something like this:

//in config for example
components => array(
    'db'=> array(
        'dsn' => '...',
       'user'=> '...',
       'charset' => 'utf-8',
       'autoTransactional' => 'true',
    ),
),

then all queries for insert/delete in this connection will be in one transaction, and will only applied on the application end. can be useful in some cases, but dont know how to deal with select queries in that way. There were some requests for this feature in russian forums, but in Yii1.1 it is hard to implement and need to think how to do this, simple onBeginRequest/onEndRequest i think will not be enough.

@samdark
Copy link
Member

samdark commented May 11, 2013

I don't think there's any sense in wrapping single queries in transactions.

@creocoder
Copy link
Contributor

@Ragazzo Looks are not only useless, but also will be very disturbed.

@resurtm
Copy link
Contributor Author

resurtm commented May 11, 2013

PHPDocs (inside appropriate pull request) are also explains my proposal. :-)

@resurtm
Copy link
Contributor Author

resurtm commented May 11, 2013

@Ragazzo, it somehow reminds me Rails3 sandboxed environment/console. Not that meaningless as for me.

@bwoester
Copy link
Contributor

Can't this be implemented by attaching to onBeginRequest() / onEndRequest() and starting / committing transactions there?

@Ragazzo
Copy link
Contributor

Ragazzo commented May 11, 2013

@resurtm yes, "idea" was taken from there. just thoughts, if this can be useful, anyway.

@creocoder
Copy link
Contributor

@bwoester If you ask about @resurtm feature, then no, it cant be be implemented by attaching to onBeginRequest() / onEndRequest() and starting / committing transactions there. For one request there can be X transactions with Y operations inside every.

@resurtm
Copy link
Contributor Author

resurtm commented May 11, 2013

@creocoder, to be even more pedantic it (@bwoester's sentence) could be achieved through nested transactions/savepoints (RDBMS specific though). But in my humble opinion i see no real benefits of that too. :-)

@samdark
Copy link
Member

samdark commented May 11, 2013

We're going to implement transactions support on Connection level. Not sure what's the benefit on doing it at AR level.

@qiangxue
Copy link
Member

I'm not quite convinced with this approach. Note that only the AR model itself knows whether transaction should be used when saving/deleting a record of it. With this $transaction parameter, the AR model is counting on some external code (e.g. controller) to trigger the transaction while this external code may not have knowledge whether the transaction should be enabled or not.

So my opinion is that if an AR saves some additional related records in afterSave(), it's better its save() method is explicitly overwritten to enable the transaction.

@resurtm
Copy link
Contributor Author

resurtm commented May 12, 2013

@qiangxue, there is second cleaner way of changing AR object lifecycle atomicity characteristic. See #244.

@qiangxue
Copy link
Member

Yes, this second approach is better. It still has the following problems though:

  1. The three actions insert(), update(), delete() may not all need transactions. What if only update() needs transaction support?
  2. A more important and difficult problem is about nested transaction. For example, if model A starts a transaction when it is being saved together with B, and B also starts a transaction by itself. How will this problem be solved? Note that most DBMS don't support nested transactions.

@resurtm
Copy link
Contributor Author

resurtm commented May 12, 2013

Note that most DBMS don't support nested transactions.

Interesting results after ten minutes of googling:

  1. MySQL: nested transactions can be emulated through savepoints. I used this way in a real world project—worked fine for me.
  2. MSSQL: nested transactions supported natively and savepoints are also available.
  3. PgSQL: nested transactions can also be emulated through savepoints. Didn't tested it myself though.
  4. Oracle: nested transactions supported natively and savepoints are also available.
  5. SQLite3: savepoints. Didn't tested it too.

However i think that one transaction is enough to wrap together both outer (controller, component, whatever) and inner (model) DB operations in most cases (85—90 % of them).

Interesting to know the other developers' experience on emulating nested transactions through savepoints (mostly for MySQL, PgSQL and SQLite3).

For example, if model A starts a transaction when it is being saved together with B, and B also starts a transaction by itself.

Yes and for this case i've made outer transaction detection. :-)

The three actions insert(), update(), delete() may not all need transactions. What if only update() needs transaction support?

I'll enhance my PR in a moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:under development Someone is working on a pull request. type:enhancement
Projects
None yet
6 participants