Add Model checkpoint functions

[samsouthardjr]

I developed the checkpoint ability to address an issue with the UserAction validateBeforeExecute method, which uses the getDirtyRef method to determine if something has changed in the model in a field where it shouldn't have changed. The problem is that things other than the UserAction could have made those changes. For example, consider:

class Dummy extends Model
{
    public $table = 'dummy';

    protected function init(): void
    {
        parent::init();
        $this->addField('name');
        $this->addField('foo', ['never_persist' => true]);

        $this->onHook(Model::HOOK_AFTER_LOAD, function($m) {
           $m->set('foo', 'crash'); 
        });
    }
}

using a standard CRUD edit or even delete UserAction will cause an exception, since validateBeforeExecute will detect the change in the 'foo' field.

The new checkpoint and getDirtyVsCheckpoint methods allow the validateBeforeExecute method to detect only the changes since the checkpoint was made, which by default is after the record is entirely loaded -- after the HOOK_AFTER_LOAD hook has been executed. The methods are general enough to be used in other circumstances as well, and support an arbitrary number of checkpoints.

[mvorisek]

I developed the checkpoint ability to address an issue with the UserAction validateBeforeExecute method

please cover this functionality with a test

we can then discuss the solution, I would highly prefer to not store any extra data structure in the model, as it complicates things and currently, it does solve single model "checkpointing" only

[samsouthardjr]

please cover this functionality with a test

we can then discuss the solution, I would highly prefer to not store any extra data structure in the model, as it complicates things and currently, it does solve single model "checkpointing" only

I've added a test -- I think (I've not done that before, so tell me if I did something wrong). To your points, though: 1) I agree that there shouldn't be extra data in the model. But the question is what is extra? As it works now, the testing that UserAction's validateBeforeExecute method doesn't work -- it doesn't distinguish between changes made by something else (including the model itself in my example) and changes made by the UserAction itself. If you're to distinguish between those two cases, you need the information on those two cases. The alternative would be to either exclude never_persist fields from the testing that validateBeforeExecute does, but that appears arbitrary, and there could be other items that should be excluded (i.e., what about never_save fields?) 2) As to single model "checkpointing" only being solved, I'm not sure what you're referring to. Anywhere that getDirtyRef is currently used, getDirtyVsCheckpoint could be used. I don't have any experience with nested Models, if that's what you mean, but if you could provide an example of where this approach would fall down in that case I'd be happy to look at extending it.

[mvorisek]

As it works now, the testing that UserAction's validateBeforeExecute method doesn't work -- it doesn't distinguish between changes made by something else (including the model itself in my example) and changes made by the UserAction its

please add a test for this UserAction usecase, I think we can store the checkpoint in UA instead

[samsouthardjr]

please add a test for this UserAction usecase, I think we can store the checkpoint in UA instead

I'm not sure how to write a test case for that. The issue is that the UserAction ends up generating an exception and halting execution in the example I provided.

As to adding it to UserAction, I initially looked for that approach and couldn't find the right place to record the checkpoint. However, since then I've studied ATK4/core a bit more and I think that the right place to do it would be in overriding the setOwner method from TrackableTrait. Does that seem right to you?

However, if I took that approach, I'd need to use Model's getDirtyRef from UserAction, and getDirtyRef is marked as internal only. Is it considered acceptable to use getDirtyRef from another portion of atk4/data (i.e., UserAction), or should getDirtyRef be used only within Model?

[mvorisek]

I'm not sure how to write a test case for that. The issue is that the UserAction ends up generating an exception and halting execution in the example I provided.

you can test exception using expectException, but is an exception wanted, shouldn't this PR fix it?

overriding the setOwner method from TrackableTrait

it might work /w cloned owner & reset dirty, however any owner change is unwanted, as an owner might be used from the code/hook and it must be correct/original

[samsouthardjr]

please add a test for this UserAction usecase, I think we can store the checkpoint in UA instead

I played around with this, and I don't think moving it to UA will work: 1) When the UserAction is first associated with a model, it's still a model. So checkpointing it at that point doesn't have any changes to a record, not even changes that would be triggered by a HOOK_AFTER_LOAD in my example (no record is loaded). 2) By the next time UserAction code is involved, all the changes have been made. For example, if you're editing a record the changes the user inputs to the form are applied to the model and then, when the user pushes the save button, that's when the UserAction is executed. But at that point it's too late to save a checkpoint -- everything's already been applied to the model. So a checkpoint would save the current state, and nothing would ever be considered dirty.

Unless I missed something in how/when the UserAction is created/triggered, there's just no place where UserAction code is triggered where you can meaningfully save a checkpoint. I don't see how, with the current UserAction design, this can be easily changed.

In the Model, however, you can easily do this, and I've rolled back my changes to implement it in the Model class again. I get why you'd rather it be implemented in the UserAction, without storing the data in the Model class, but I don't think it's possible right now, and I think the added functionality of checkpoint is worth it -- even if you don't count that it fixes an otherwise unfixable issue with UserAction.

[mvorisek]

The new checkpoint and getDirtyVsCheckpoint methods allow the validateBeforeExecute method to detect only the changes since the checkpoint was made

Am I correct you want to detect changes between two points?

Am I correct you want this solely for the developer/you and to not change any atk4/{data, ui} behaviour on it?

About the implementation. I do not like how dirty buffer is implemented for ages as it is recalculated on every Model::set(). I would prefer to simply store copy of the loaded values. The we can simply calculate the changed values when needed, /w the stored "loaded snapshot" or "snapshot made by an user". This is also how Doctrine ORM calculates changes.

This would also solve an issue /w the proposed design which introduces "named" (int $point) snapshots. Instead of saving a snapshot under some "name" in Model, snapshot can be stored in a php variable and passed later to a "calculate changed values" method. For one field, this is what Field::compare() method does :)

As objects are mutable, when implementing this, care must be taken when snapshoting values like instance of DateTime.

[samsouthardjr]

The new checkpoint and getDirtyVsCheckpoint methods allow the validateBeforeExecute method to detect only the changes since the checkpoint was made

Am I correct you want to detect changes between two points?

Am I correct you want this solely for the developer/you and to not change any atk4/{data, ui} behaviour on it?

You're not quite correct. In the initial post description I give an example that results in what I believe is an atk4/{data, ui} bug that this solves -- the UserAction doesn't/can't distinguish between a change the model itself makes and changes the UserAction makes and as a result throws an exception on what should be perfectly allowable code.

As a further example, take the testFields() test in UserActionTest.php method testFieldsTooDirty1(), at line 204:

    $client = new UaClient($this->pers);
    $client->addUserAction('change_details', ['callback' => 'save', 'fields' => ['name']]);

    $client = $client->load(1);

    $this->assertNotSame('Peter', $client->get('name'));
    $client->set('name', 'Peter');
    $client->getUserAction('change_details')->execute();
    $this->assertSame('Peter', $client->get('name'));

If the UaClient model had a HOOK_AFTER_LOAD like my example, then this test would fail -- an exception would be generated because the current UserAction code would detect that the 'foo' field -- changed by the Model's hook, not by anything else -- had changed.

About the implementation. I do not like how dirty buffer is implemented for ages as it is recalculated on every Model::set(). I would prefer to simply store copy of the loaded values. The we can simply calculate the changed values when needed, /w the stored "loaded snapshot" or "snapshot made by an user". This is also how Doctrine ORM calculates changes.

This would also solve an issue /w the proposed design which introduces "named" (int $point) snapshots. Instead of saving a snapshot under some "name" in Model, snapshot can be stored in a php variable and passed later to a "calculate changed values" method. For one field, this is what Field::compare() method does :)

As objects are mutable, when implementing this, care must be taken when snapshoting values like instance of DateTime.

That's basically what I'm doing with this checkpointing code. The checkpoints are saving the original data whenever there's a deviation from the model. As to whether or not that should be the baseline behavior, you make a case for it, but that's beyond what I am attempting to do, and would involve more changes than I am comfortable making.

However, I will point out that what you propose would not fix the big that I'm trying to fix. There's no point (that I can find) in the UserAction code that you could use to set the checkpoint after the record is loaded but before (in my example of a CRUD button for an edit) the fields have been modified by the form's submit. That point only exists currently within the Model code, so fixing this bug requires something in the Model code.

[mvorisek]

So the primary objective you want to address is this check: https://github.com/atk4/data/blob/9c59a5a137b5e12fc59d82d14f31cdab4b8f3f78/src/Model/UserAction.php#L168-L178

the check was introduced in https://github.com/atk4/data/commit/95091d9858213f142789f8b6722e15aabdea098a#diff-67e2309cee62130b41ca90d06587bb7669f6a8a1bda8ad59ebda5e3eed08d83bR65

the check if optional if UserAction::fields are specified

for me, the prop. name is very confiusing (as on the first sight) it seems it does not define UA fields/args, but only fields that are required to be unchanged (non-dirty) - is this correct?

given this info, why you want to introduce "another dirty state"?

def. dirty = value different than loaded from DB

please provide a usecase /w code example where cleaning UserAction::fields is not a solution

[samsouthardjr]

You're correct about the check that I'm trying to address.

the check if optional if UserAction::fields are specified

for me, the prop. name is very confiusing (as on the first sight) it seems it does not define UA fields/args, but only fields that are required to be unchanged (non-dirty) - is this correct?

This is not correct. I'm not specifying anything about which fields are required to be unchanged. The existing mechanisms are doing that. I'm only defining what they need to be changed from to be considered changed.

given this info, why you want to introduce "another dirty state"?

def. dirty = value different than loaded from DB

The why is because things other than the UserAction (such as the Model itself) could have made those changes. I think it's good that the UserAction is checking that only certain fields are changing. But I think it's bad that the UserAction is counting changes not made by the UserAction as changes. Without recording what the state was before the UserAction got involved (which could be different from the loaded record, and is, in my case), the UserAction can't distinguish between changes it makes and changes something else (like the Model itself) made.

please provide a usecase /w code example where cleaning UserAction::fields is not a solution

I don't think I'm explaining well why the issue needs a fix, so let me try again.

The issue is that the Model itself, not the UserAction, is making a change to a field. However, the code you highlight is (without my change) assuming that every change to every field's value is coming from the UserAction. That's where it breaks. And that's why saving a checkpoint -- after the Model itself has made whatever changes it wants to make -- that the UserAction can compare against (instead of the initially loaded values) is valuable.

While you probably could address the issue using the UserAction::fields property, I don't think that's the best solution, since it requires the code adding that UserAction to know everything about the Model. Whenever the Model was updated, if it added a new field that worked like this, every added UserAction would need to be updated to specify the new field -- when it's the Model itself that changed, not the functionality of the UserAction being added. That doesn't seem object oriented. If it's the Model that's changing, shouldn't the Model handle it?

The exact example I provided has another use case -- it's actually how I found the issue. I have a Model that I'm displaying in a lister, and I wanted that lister to have a more complex descriptive text than I could code in an addExpression. So I added the example HOOK_AFTER_LOAD to determine that descriptive text whenever the Model was loaded. As soon as I did that, I could no longer edit or delete records with the standard CRUD actions -- they detected that the never_persist field was changed, and generated the exception.

That use case illustrates my previous point, too. Assume that I did fix the issue by adding a custom UserAction to edit or delete something, and specified the UserAction::fields property to include the never_persist field. Further assume that later on I decide to add another similarly computed never_persist line of text for another reason, completely independently from initial Lister. That original Lister would now break, because it wouldn't know about the new never_persist field. But there's no reason for that Lister to break -- it's still doing exactly what it was before. It has no need to know about the new field.

[mvorisek]

The why is because things other than the UserAction (such as the Model itself) could have made those changes. I think it's good that the UserAction is checking that only certain fields are changing. But I think it's bad that the UserAction is counting changes not made by the UserAction as changes

I have not coded that check, but I think it is there to assert non-dirty (unchanged) data BEFORE the UA is called, it does NOT check any change made by UA. So if the check should stay (and you are not objecting it), it works as desired.

[samsouthardjr]

The why is because things other than the UserAction (such as the Model itself) could have made those changes. I think it's good that the UserAction is checking that only certain fields are changing. But I think it's bad that the UserAction is counting changes not made by the UserAction as changes

I have not coded that check, but I think it is there to assert non-dirty (unchanged) data BEFORE the UA is called, it does NOT check any change made by UA. So if the check should stay (and you are not objecting it), it works as desired.

That was not my interpretation of it, but in reviewing comments in the code I can see that it's ambiguous. And I can see that there is a logic in that approach, too.

However, if that's the case, then there must be another issue somewhere else, because the standard Model (through the UserActionTrait) sets the UserAction::fields to true for the 'add' and the 'edit' UserActions, and I've not been overriding it. Based on the code in validateBeforeExecute, that means that any field should be able to be modified, and that check shouldn't even be happening -- so something else must be changing UserAction::fields to an array which for some reason is excluding the never_persist fields. Perhaps that's the source of the bug -- I'll chase that down.

One bug I think I see is that the 'delete' action added by the UserActionTrait does not set UserAction::fields to true, which means its defaulting to an empty array -- so no fields would be allowed to be changed. Should the standard 'delete' user action care if any fields have been changed in the record it's going to delete? Shouldn't the standard 'delete' action also set UserAction::fields to true?

[mvorisek]

That was not my interpretation of it, but in reviewing comments in the code I can see that it's ambiguous. And I can see that there is a logic in that approach, too.

However, if that's the case, then there must be another issue somewhere else, because the standard Model (through the UserActionTrait) sets the UserAction::fields to true for the 'add' and the 'edit' UserActions, and I've not been overriding it. Based on the code in validateBeforeExecute, that means that any field should be able to be modified, and that check shouldn't even be happening -- so something else must be changing UserAction::fields to an array which for some reason is excluding the never_persist fields. Perhaps that's the source of the bug -- I'll chase that down.

please cover all issues with a test cases in this PR, if you cannot fix them no worries

Should the standard 'delete' user action care if any fields have been changed in the record it's going to delete?

I am not the original author of the check, there are reasons for and against :)

[samsouthardjr]

The why is because things other than the UserAction (such as the Model itself) could have made those changes. I think it's good that the UserAction is checking that only certain fields are changing. But I think it's bad that the UserAction is counting changes not made by the UserAction as changes

I have not coded that check, but I think it is there to assert non-dirty (unchanged) data BEFORE the UA is called, it does NOT check any change made by UA. So if the check should stay (and you are not objecting it), it works as desired.

Sorry that it's taken me so long to get back to this (I got busy with other things), but I did track this down and realize I want to push back on this a bit. While asserting non-dirty BEFORE the UA is called may well have been the intent, if it were the intent, I think that intent may be flawed. That intent essentially makes it impossible to use the standard Model UserActions in any Model that changes its own field values for any reason.

At the very least, the standard delete action is currently unusable in those cases. Any call to $m->set($field, 'newval') inside a HOOK_AFTER_LOAD, for any field, will cause the 'delete' UserAction to generate a 'too dirty' exception, since the 'delete' user action defined in UserActionsTrait.php doesn't set 'fields' to true, resulting in the default empty array being used (no fields can be changed).

Similarly, the any call to $m->set($field, 'newval') inside a HOOK_AFTER_LOAD for any never_persist field -- or a system field -- will cause the 'edit' UserAction to generate a 'too dirty' exception, since ultimately the ModalExecutor class's executeModalAction method replaces a 'true' value of 'fields' in the action with the list of 'editable' fields (line 106), from the getFields method of the Model, which depends on isEditable() in Field, which specifically excludes 'never_persist' and 'system' fields from the editable fields (line 456 in Field.php).

Basically it comes down to the intent. How should atk4 behave? Is it considered incorrect to use the set method inside a HOOK_AFTER_LOAD and expect to use the standard UserActions? Doing so, pretty much by definition, will cause the "assert non-dirty (unchanged) data BEFORE the UA is called" to fail. If that's what we want, then everything's working as intended -- any set inside HOOK_AFTER_LOAD will cause that assertion to fail, so a UA will not be callable.

If a call to set() inside a HOOK_AFTER_LOAD is something that should be allowed with the standard Model UserActions, though, then something needs to change. I see three alternatives: 1) My checkpoint approach would work. 2) We could modify the default 'delete' Model UserAction to allow any field to be changed (by setting 'fields' to true, as the 'add' and 'edit' UserActions do) and we could change the Field's isEditable() method to include 'never_persist' and 'system' fields. I'm hesitant to do that, though, as I suspect isEditable may be used in other areas and I wouldn't want to break them. 3) We could modify the 'delete' Model UserAction as above and change the ModalExecutor's executeModelAction method to use a new 'variable' filter on fields, in the place of 'editable'. 'variable' would return a list of any field that was allowed to vary (i.e., every field that wasn't 'read_only').

I believe, based on all of the above and the earlier discussion in this thread that #3 is the preferred choice. But what's the preference of the ATK4 guru's? I'm happy to code, including test cases, any of the above solutions (or another one that anyone suggests). But it seems like I'm running up against a fundamental issue of philosophy/intent. If I'm barking up the wrong tree then no solution I come up will be accepted, and instead of attempting a pull request I should just modify my own classes and not try to make it part of the base ATK4. I think that would be the wrong thing, but ultimately I'm just a peon here, and the gurus have a much better overall knowledge of and vision for how they want the package to work and what should be allowed.

[mvorisek]

The checkpointing feature is too usecase specific and even more questionable to use it in a UA by default. So I will close this PR.

Your usercase with never_persist field modified in HOOK_AFTER_LOAD definitely calls for a solution to flag such field a non-dirty in that hook.

Model::getDirtyRef() method return a reference, so you can easily modify the dirty array in your HOOK_AFTER_LOAD hook. Please note this can change in the future as this method is internal only, but for now, it is a clean solution how to mark the field you update in the hook as non-dirty.