yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

ActiveRecord needs a method to explicitly load specified relations #11000

Open PowerGamer1 opened 8 years ago

PowerGamer1 commented 8 years ago

Consider a following situation ($project is an instance of ActiveRecord based model with hasOne / hasMany relations; the web form allows to change evaluator of a project):

$ok = false;
$transaction = \Yii::app->getDb()->beginTransaction(\yii\db\Transaction::SERIALIZABLE);
try
{
    $project = $this->findProject($id);
    $oldEvaluator = $project->evaluator;
    if($project->load(\Yii::app->getRequest()->post()) && $project->validate())
    {
        $project->update(false);
        // Since db record successfully updated need to prepare email, but it needs both $project model and related model $project->language. Both models must be loaded in the same transaction.

        // This is currently the only way in Yii2 to load related record - very obscure!
        $project->language;

        // Here a new version (rather than cached by Yii2) of related record must be loaded (hence the unset) even more obscure!
        unset($project->evaluator);
        $project->evaluator;

        $ok = true;
    }

    $transaction->commit();
}
catch(\Exception $e)
{
    $transaction->rollBack();
    throw $e;
}
if($ok)
{
    // Keep the lengthy process (of email creation, etc) outside transaction.
    // Email text will use $project->language, $project->evaluator and $oldEvaluator
    if($oldEvaluator->id != $project->evaluator->id)
        app()->getMailer()->compose('evaluatorReassigned', ['project' => $project, 'oldEvaluator' => $oldEvaluator]);
}

So instead of writing obscure things like:

$project->language;
unset($project->evaluator);
$project->evaluator;

I propose to make new method in Active Record to load related models:

$project->loadRelations(['language', 'evaluator']);

Possible implementation of ActiveRecord::loadRelations:

public function loadRelations(array $relations)
{
    foreach($relations as $relation)
    {
          unset($this->relation);
          $this->relation;
   }
}
mdmunir commented 8 years ago

I am unsure what are you trying to do. But if you need language in evaluator relation. You can call it directly.

public function getEvaluator()
{
    $language = $this->language;
    // do something with language
    return $this->hasOne(Evaluator::className(),[..]);
}
PowerGamer1 commented 8 years ago

I see I missed one important factor in my original post (related models are used outside the transaction). I will edit it now. Basically I need something like this:

// DB transaction start
$project = Project::findOne($id);
if(/* some condition */)
{
    $project->language;
    $project->evaluator;
    $ok = true;
}
// DB transaction end
if($ok)
    // pass $project and both related models somewhere else for further processing:
    doSomething($project);
  1. Both relations (language and evaluator) need to be loaded within the same transaction as main model Project.
  2. But language and evaluator must be loaded only if some condition is met (so I can't do $project->find()->where($id)->with(['language', 'evaluator'])->one();).
  3. Later on I pass all three models to some function for further processing. Since doSomething is called outside the transaction (it may do some lengthy processing and must not keep transaction open for long) related models language and evaluator must already have been loaded. Since language and evaluator models are related to Project model it would make sense to pass only an instance of $project and then doSomething will access the related models as $project->language and $project->evaluator.
SamMousa commented 8 years ago

Project::find()->with(['language', 'evaluator'])->where(['id' => $id])->one() ?

PowerGamer1 commented 8 years ago

@SamMousa

Project::find()->with(['language', 'evaluator'])->where(['id' => $id])->one() ?

Did you even read what I wrote? Twice I tried to explain that there are situations when first you have to do Project::find()->where(['id' => $id])->one() and only later you may or may not additionally need to do the ->with(['language', 'evaluator']) part, which cannot be done (nicely at least) separately from Project::find().

SamMousa commented 8 years ago

Yes, but you wrote a lot, obviously I missed your number 2. I'm unclear on your number 1 requirement though; since reading within your transaction will yield the same results as reading after your transaction has been committed; unless you are rolling back the transaction, in which case you are probably not going to do the "lenghty` processing.

PowerGamer1 commented 8 years ago

@SamMousa

since reading within your transaction will yield the same results as reading after your transaction has been committed; unless you are rolling back the transaction

Do you honestly believe that something like:

$transaction = \Yii::app->getDb()->beginTransaction(\yii\db\Transaction::SERIALIZABLE);
$project1 = Project::find()->where(['id' => 123])->one();
$transaction->commit();
$project2 = Project::find()->where(['id' => 123])->one();

will ALWAYS result in $project1 having the same data from database as $project2 ? No offense, but you are gravely mistaken in this regard.

SamMousa commented 8 years ago

Ah, didn't know you were using that isolation level, the standard isolation level is lower and does not lock for reads.

So what happens to your system when the application stops / crashes after committing the transaction but before doing the lengthy processing? What happens if doSomething traverses relations defined in your Elevator and Language model, what version of data do you expect it to get exactly? Obviously I don't know your specific use case but as far as I can tell this might not be the best architectural choice...

Regardless, there are several (all ugly) ways to manually load the relations:

$model->language;
$model->__get('language');

You could of course implement a wrapper the part of __get() that loads a relation and call it load().

PowerGamer1 commented 8 years ago

@SamMouse

Ah, didn't know you were using that isolation level, the standard isolation level is lower and does not lock for reads.

Serialization level does not matter in that example at all. It could have been the lowest one and data in $project1 could still be different from $project2.

So what happens to your system when the application stops / crashes after committing the transaction but before doing the lengthy processing?

If it stops / crashes AFTER committing the transaction the data in DB will be correct with no integrity loss, nothing else would matter.

What happens if doSomething traverses relations defined in your Elevator and Language model, what version of data do you expect it to get exactly?

doSomething must get the version of Elevator and Language that would exist in database at the time of read from project table - exactly what SERIALIZABLE isolation level guarantees.

Regardless, there are several (all ugly) ways to manually load the relations:

$model->language;
$model->__get('language');

You could of course implement a wrapper the part of __get() that loads a relation and call it load().

Man, you really didn't read my posts at all. I pointed out that exact ugliness in my very first post and proposed to implement a method in the framework to do such loading explicitly.

SamMousa commented 8 years ago

It matters, since if there is no lock on the other tables data can be written to by other queries and it can be different regardless if running it in or outside the transaction; and if that is the case then it doesn't matter where you do the select since you can never depend on it returning a specific value...

Anyway good luck with your application architecture, obviously using AR to create a snapshot of data over multiple relations at some other point in time is the way to go to keep your concerns separated... On Mar 10, 2016 2:51 PM, "PowerGamer1" notifications@github.com wrote:

@SamMouse

Ah, didn't know you were using that isolation level, the standard isolation level is lower and does not lock for reads.

Serialization level does not matter in that example at all. It could have been the lowest one and data in $project1 could still be different from $project2.

So what happens to your system when the application stops / crashes after committing the transaction but before doing the lengthy processing?

If it stops / crashes AFTER committing the transaction the data in DB will be correct with no integrity loss, nothing else would matter.

What happens if doSomething traverses relations defined in your Elevator and Language model, what version of data do you expect it to get exactly?

doSomething must get the version of Elevator and Language that would exist in database at the time of read from project table - exactly what SERIALIZABLE isolation level guarantees.

Regardless, there are several (all ugly) ways to manually load the relations:

$model->language; $model->__get('language');

You could of course implement a wrapper the part of __get() that loads a relation and call it load().

Man, you really didn't read my posts at all. I pointed out that exact ugliness in my very first post and proposed to implement a method in the framework to do such loading explicitly.

— Reply to this email directly or view it on GitHub https://github.com/yiisoft/yii2/issues/11000#issuecomment-194848594.

SamMousa commented 8 years ago

So what happens to your system when the application stops / crashes after committing the transaction but before doing the lengthy processing?

If it stops / crashes AFTER committing the transaction the data in DB will be correct with no integrity loss, nothing else would matter.

So the lengthy processing that is dependent on data that no longer exists (since your transaction has been committed) doesn't matter?

PowerGamer1 commented 8 years ago

So the lengthy processing that is dependent on data that no longer exists (since your transaction has been committed) doesn't matter?

I guess by "no longer exists" you meant "could have changed". That is not a problem, the "lengthy" process is SUPPOSED to work with the data that existed in database at the moment transaction was executing, otherwise you can end up generating email mentioning the wrong project language, or sending email to wrong person. I recommend you start thinking about concurrent execution of the code, data integrity and the role of transactions to better understand this. Also, lets not get too off-topic please.

SamMousa commented 8 years ago

Your reasoning is wrong, but anyway enough of my time down the sink on this issue. On Mar 10, 2016 5:11 PM, "PowerGamer1" notifications@github.com wrote:

So the lengthy processing that is dependent on data that no longer exists (since your transaction has been committed) doesn't matter?

I guess by "no longer exists" you meant "could have changed". That is not a problem, the "lengthy" process is SUPPOSED to work with the data that existed in database at the moment transaction was executing, otherwise you can end up generating email mentioning the wrong project language, or sending email to wrong person. I recommend you start thinking about concurrent execution of the code, data integrity and the role of transactions to better understand this. Also, lets not get too off-topic please.

— Reply to this email directly or view it on GitHub https://github.com/yiisoft/yii2/issues/11000#issuecomment-194928572.