LazyLoad property and BeginEdit

[amselem]

Hi,
I have a bo with some lazyload properties shown in a windows form app.
Since the lazyload properties are heavy to load , I only load them if the user clicks on their respective tabpage. The user can click in the tabpage before or after beginning an edit in this form.
When the user begins the form edit I call my bo.BeginEdit() so I can call bo.CancelEdit() if the user wants to rollback his changes.
However the Lazyload fields are rolled back only if they were loaded previously (i.e. if the user clicked in the tabpage). If the user begins an edit, then clicks on the tabpage, the lazyload property is not rolled back on an undo operation.

What's the best/correct approach to solve this inconsistent behavior?

Kind regards

[rockfordlhotka]

The easiest solution is to increase the edit level in the method invoked to lazy load the object. Something like this (from memory):

    public static readonly PropertyInfo<PersonEdit> ChildProperty = RegisterProperty<PersonEdit>(nameof(Child));
    public PersonEdit Child
    {
      get => LazyGetPropertyAsync(ChildProperty, async () =>
        {
          if (FieldManager.FieldExists(ChildProperty))
          {
            return Task.FromResult(default(PersonEdit));
          }
         else
          {
            var result = DataPortal.FetchAsync<PersonEdit>("c"));
            for (i = 0; i < this.EditLevel; i++)
              result.BeginEdit();
            return result;
          }
        }
      set => SetProperty(ChildProperty, value);
    }

[amselem]

Hi Rocky,
I dug deeper in the framework and it looks like that code isn't necessary as there's already a ResetChildEditLevel function which equalifies the edit level of the lazy loaded child property with his parent calling CopyState/AcceptChanges where necessary.

I think the problem is that if you have a null lazyload child property (not loaded) , then if you take a snapshot of the BO the null value is saved as the state for this property.
If then you access the lazyload property (so the child property is loaded) and then you UndoChanges the null value is restored instead of cascading to the UndoChanges child property.

In FieldDataManager.UndoChanges I found this code

      var oldItem = state[index];
      var item = _fieldData[index];
      if (item != null)
      {
        var undoable = item.Value as IUndoableObject;
        if (undoable != null)
        {
          // current value is undoable
          if (oldItem != null)
            undoable.UndoChanges(parentEditLevel, parentBindingEdit);
          else
            _fieldData[index] = null;
          continue;
        }
      }

In the case of a child LazyLoad property: oldItem == null, so _fieldData[index] ends with null value instead of cascading to UndoChanges.

I removed the if(oldItem != null) condition and cascaded all the calls to undoable.UndoChanges regardless of oldItem value and the issue dissappears. This is the corrected code:

  var oldItem = state[index];
  var item = _fieldData[index];
  if (item != null)
  {
    var undoable = item.Value as IUndoableObject;
    if (undoable != null)
    {
      // current value is undoable
      undoable.UndoChanges(parentEditLevel, parentBindingEdit);
      continue;
    }
  }

That should be the fix?

[rockfordlhotka]

Thank you for digging into the code. You are right, and I'd forgotten about that behavior.

It seems to me that there are possible scenarios here.

1. In an undo the previous value was null - which your suggested change addresses by retaining the new value - except I can imagine a scenario where it really was null and should return to null - maybe your change needs to only happen if the property is marked for LazyLoad?
2. In an undo the previous value was a different instance - so we should revert to that previous instance? That's maybe unrealistic, since it will always technically be a different instance due to deserialization. Hmm...
3. In an undo the previous value was an instance and the new value is null - so we should revert to that previous instance

[amselem]

For the 1st scenario, although in my case all my null properties are LazyLoad properties I think that you're right, this behavior should only happen on LazyLoad properties. To be honest when fixing the code I already though on adding this condition but I wasn't sure about how to get the PropertyInfo.RelationShipType from the FieldManager.UndoChanges

As for the 3rd case I agree that we should revert to previous instance, but also undo this instance, to cover the following case: bo.BeginEdit -> bo.ChildProperty.SomeField = "blah"-> bo.ChildProperty = null -> bo.UndoEdit. Does it make sense?

I'm not sure about the 2nd ... this seems complex, maybe is beyond Csla scope?

[rockfordlhotka]

I agree that number 2 seems complex, and I am not sure how to do it 🤷‍♀️, so we should ignore it.

I think it should be possible to use _parent and the index value to find the correct IPropertyInfo?

[amselem]

I finally managed to get the PropertyInfo (It was very easy in the end). This is my current fix:

      var oldItem = state[index];
      var item = _fieldData[index];
      if (item != null)
      {
        var undoable = item.Value as IUndoableObject;
        if (undoable != null)
        {
          // current value is undoable
          if (oldItem != null ||
                _propertyList[index]?.RelationshipType == RelationshipTypes.LazyLoad)

            undoable.UndoChanges(parentEditLevel, parentBindingEdit);
          else
            _fieldData[index] = null;
          continue;
        }
      }

As for the Scenario 3 I don't know how to proceed because when you have a Child instance , the CopyState() saves a bool value as the state to indicate that there was a value:

if (item.Value is IUndoableObject child)
          {
            // cascade call to child
            child.CopyState(parentEditLevel, parentBindingEdit);
            // indicate that there was a value here
            state[index] = new FieldData<bool>(item.Name);
          }

So if you set the property to null then call CancelEdit, it tries to restore that bool value as the previuos instance value and throws an exception. Example:

    person.Child = Csla.DataPortal.Create<PersonEdit>();
    person.BeginEdit();
    person.Child = null;
    person.CancelEdit();  // Fails !

Here's some test code for both scenarios:
csla_undo_tests.zip

[amselem]

Hi Rocky, did you give more thoughts on this? I think that at least bugfix for scenario 1 could be implemented

[rockfordlhotka]

I put it on the backlog: #2444

[rockfordlhotka]

@amselem what do you think about this fix:

rockfordlhotka@1199e10