cphalcon: [BUG]: Save, saves related data also (not changed)

I would say default. Developers can then change the behavior if they need to.

This is a bug. It appeared in 4.1 branch. To be precise now we are running 4.1.2. There is no such behavior in php7.4-phalcon_4.0.4-908. We even fixed this package in our Docker but now we had to update and got a lot issues.

Lets assume we have a code

class Spendings extends BaseModel {
....
public function initialize(): void {
    $this->hasOne('spendings_categories_id',
            SpendingsCategories::class,
            'id',
            [
                'reusable' => true,
                'alias'    => 'Category',
            ]);

$sp = Spendings::findFirst(627979);
echo  $sp->Category->name; //Relation
$sp->amount += 0.01;
sp->save();

Queries will be:

       START TRANSACTION
      UPDATE `spendings` SET `status` = 1, `.... WHERE `id` = 627979
(!!!) UPDATE `spendings_categories` SET `name` = 'test', `.... WHERE `id` = 12601
       COMMIT

On the one hand let it be, just additional update. But if we changed spendings_categories_id in spendings to another the second update will wrongly update a row in spendings_categories. For example you had spendings_categories: id | name 1 | category 1 2 | category 2 You change spendings.spending_category_id from 1 to 2, now you have 1 | category 2 2 | category 2

There is workaround with switches off relation update

public function save(): bool {
        $this->related = [];
        return parent::save();
    }

Do not hesitate to contact me if you need more details or tests.

This has been one of the most painful areas of the framework to be honest. This particular discussion goes back to even v3. When to update? That is the question.

Personally, I don’t let the ORM update things for me, I do the updates where I need them to, wrapping the whole operation in a transaction. But that is just me.

I like the idea from @noone-silent. We can go with that and set the behavior for updates once and for all.

@rudiservo I did some research to your last comment. This issue for us has nothing to do with dynamicUpdate. This issue is that the Phalcon Framwework is marking models as “dirty” while their fields have not changed.

An yes I guess the dynamicUpdate is also a bit broken then, since it should not store any fields. But my biggest concern is the Dirty state not being set correctly when fetching related models. For example this is now the case in our code base:

$someModel = SomeModel::query->andWhere('id = 1')->execute();

$someRelatedModel = $someModel->someRelatedModel;
$someOtherRelatedModel = $someRelatedModel->someOtherRelatedModel;

if ($someRelatedModel->status = 'VALID' && $someOtherRelatedModel->status = 'VALID') {
    $someModel->name = 'A new name';
    $someModel->save();
}

// What happens (looking at our database logging in combination with Clockwork:
// 1: someModel SELECT
// 2: someRelatedModel SELECT
// 3: someOtherRelatedModel SELECT

// So far so good, but this is where it's getting weird:
// someOtherRelatedModel UPDATE some fields, but no new information
// someRelatedModel UPDATE some fields, but no new information
// someModel UPDATE name field.

The 2 related models should not go to the database. They were only read and never written. I fixed the issue for us by overwriting the collectRelatedToSave function.

Hope that helps to clear up the confusion that’s it’s about Dynamic Updates. (It’s not)

To be honest I think prepareSave, while it has good intentions of enabling making a lot of changes just before the model gets saved, it is a bad practice if you rely on it for data consistently. Because the ORM can only run the model once, if devs depend on prepareSave for calculations or data consistency and are not using their own logic and functions before triggering a save(), it will lead to data not being changed because of the type and complexity of relations.

I have a very complex project that relations can go to 5 or 6 layers deep, don’t like it but it’s the way the customer needs it.

We could however have a dirty cache inside a thread cache that when a save gets triggered it runs normally then checks the cache for unsaved models, this adds more cycles and checks every time a model gets changed or save, and might even be unwanted behaviour, I don’t recommend it like this.

So my proposal is dirty_state execution flags to avoid loops, resultset with added logic to append/add and persist models with a clear way for dev to program to make sure that it’s get triggered by save(), and avoid magic has much has possible.

I am working on it has we speak, it’s slow progress because I am trying not to break previous behaviour, i.e. saving hasMany that is an array instead of a resultset object, keeping collectRelatedToSave function. The hardest part of them all is when you delete or remove a related model, this is the one thing that will drive me nuts, because the model still exists in the database, but you do not want it to be used in a resultset because you deleted it, just not persisted it.

Then this issue is not fixed. This issue is about saving a model will also save all related models. Not about making a config setting as default. The config setting as default is just a side effect needed for this ticket.

@rudiservo You complicate things too much here. There have been made many supposed fixes for this already, all using the dynamic update setting. You have concerns about performance, but performance is not the main aspect someone uses ORM. We use ORM because of convenience. If we need pure performance or have hundreds or thousands of queries, we should act like @niden does and do transactions directly, or if we only work like this, don’t use ORM at all.

Nobody is using 200 relations and all these relations also use relations of the hundreds. Yes, then performance and even resources would be a problem. But not in those simple use cases we have here.

@niden DynamicUpdates and snapshots by default or mandatory?

FYI, this is just quick fix, still I my advice would be to only run save on relations that we set i.e. $a->b = $b (WIP), I will create an issue for this.

It’s the same behavior, no matter if dynamicUpdates/Snapshots are activated or not. As soon there is access to a related model, the model is getting saved with all fields, even there was no change on the related model data.

It was just a guess, that if dynamicUpdates are enabled this bug would not trigger. But this bug triggers, regardless what settings have been made.

And again: This bug also appears in Phalcon 4, where there is no dirty state setting. So the real bug is somewhere else I guess

ok, so two different situations @borisdelev asked about the behaviour of saving and calling save on every relation.

@noone-silent and @larsverp have the dynamic update but the model get saved and all fields get saved on that update.

Is this right?

@borisdelev do you have dynamicUpdate set? because if you do and it is executing an update, that behaviour should not happen. Can you debug and tell us what are types of the fields getting updated, or is it all of them?

@larsverp Will only work if Dynamic update is set, so you have to put it in the doLowUpdate function. Also there is a bit of an issue, with field types. In do lowUpdate you have to cast the values to compare in order to assert if the values are what you want.

i.e. “4” and “4.0” are not exactly the same for PHP, a lot of dev’s do not cast the value on set, so this needs to be done.

So the getChangedFields, might not get all changed fields, the function needs a revamp and doLowUpdate needs a revamp to.

The weird part is where you say you have dynamicUpdates, and still saves the model, it should not unless you have an issue in your snapshots, or the type of data you have in your database and model.

Check this to see if there is something different about your model so we can do something about it. https://github.com/phalcon/cphalcon/blob/5.0.x/phalcon/Mvc/Model.zep#L3911

@larsverp well dynamic updates will save writes to your DB, also they will not issue an update for all the fields of the model, only the ones that have changed, making the update query smaller. It’s easier to scale up php workers than it is to scale up the DB, with Relational DB clusters the writing usually happens always in the same node, so pulling easing off the writing will improve the database by a lot.

Test it and give your feedback on how it went.

The solution for this without using dynamic updates, is to forget about “dirty state” on relations has you have to always check the relations for changes and just use the dirty state only for changed fields.

I have also replicated this issue in our application. We have installed ClockWork that allows us to see all database calls made in a call. I ran this code:

public function phalconAction()
    {
        /** @var EventTicket $ticket */
        $ticket = EventTicket::query()->andWhere('uuid = "93b006a8-188b-4c2b-bd3c-64f8c6f3101e"')->execute()->getFirst();

        $orderItem   = $ticket->orderItem;
        $orderStatus = $orderItem->order->status;

        if ($orderStatus === OrderStatus::COMPLETE) {
            $ticket->status = EventTicketStatus::RESERVED;
            $ticket->save();
        }
    }

That results in the following ClockWork result:

The EventTicket Query is not on the screenshot, but you can see that 2 UPDATE queries are done for Order & OrderItem. The data in the UPDATE query is not different from what is currently in the database.

The example code I ran is not too bad, but in our production application this creates a massive performance issue.

TLDR: this happens in collectRelatedToSave()

Starts in the Model::__get

if typeof relation == "object" {
            /**
             * There might be unsaved related records that can be returned
             */
            if isset this->dirtyRelated[lowerProperty] {
                return this->dirtyRelated[lowerProperty];
            }

            /**
             * Get the related records
             */
            return this->getRelated(lowerProperty);
        }

if you fetch anything it does not get set in the dirtyRelated, I think the ideas was to only execute presaverelated() and postsaverelated() if hasRelated() and collectRelatedToSave()

Issue is in the collectRelatedToSave()

for name, record in related {
            if isset dirtyRelated[name] {
                continue;
            }

            if typeof record !== "object" || !(record instanceof ModelInterface) {
                continue;
            }

            record->setDirtyState(self::DIRTY_STATE_TRANSIENT);
            let dirtyRelated[name] = record;
        }

so the getRelated() will set this->related[lower_property] = related and the collectRelatedToSave() will iterate on the related But since the model is not in the this->dirtyRelated it will be set Transient with record->setDirtyState(self::DIRTY_STATE_TRANSIENT); So it can be executed in preSaveRelatedRecord with if record->dirtyState !== Model::DIRTY_STATE_PERSISTENT && !record->save()

Right now it’s a feature because probably there “is” a bug So lets suppose you have C belongs to B and B belongs to A

if we A->B->C->setSomething(value); C will be in a transient state, but B will not So saving A will not save C unless B is also in a transient State

I can think of 3 ways to fix this:

1. has a static “cache” of real dirtyModels to be executed, this could trigger a save on unwanted and unrelated models.
2. cascade dirtyState to parents, this will consume a lot of cycles since you have to check if there is a loaded relation and cascade on it everytime you do a __set
3. check all loaded relations recursively for any transient models on save, this could trigger infinite loops (my guess is it does already) so an EXECUTED and EXECUTED_TRANSIENT states will/might be needed to differentiate and avoid infinite save loops, this might be possible without a First Level Cache, I am not sure.

FYI to trigger an infinite loop ATM you just have to

A->B
B->setA(A)
A->save()

I am working currently on this in my repo while trying to add FirstLevelCache… not easy without breaking a few things, sorry.