Error in ApplyEdit() if child is added after BeginEdit()

Error in ApplyEdit() if child is added after BeginEdit()

Old forum URL: forums.lhotka.net/forums/t/6421.aspx


oscarmorasu posted on Friday, February 13, 2009

Hello.

I'm using CSLA 3.6.1. All my business objects inherit from BusinessBase<T>. I have a parent object "P" that contains a child object "C".

In one scenario an instance of P already exists, and I call BeginEdit(). Then I create an instance of C and assign it to P.

When I call P.ApplyEdit() I get an UndoException in AcceptChanges. Looking at the implementation of UndoableBase. AcceptChanges() I see:

if (this.EditLevel - 1 != parentEditLevel)

throw new UndoException(string.Format(Resources.EditLevelMismatchException, "AcceptChanges"));

if (EditLevel > 0)

{...

I get the exception because the logic first compares the current instance's EditLevel against the parent's. Since C was created after P.BeginEdit() those levels don't match and I get the exception. Is that an expected behavior? Is there a way to synchronize C's EditLevel?

Now, in the UndoableBase.UndoChanges() I see the opposite:

if (EditLevel > 0)

{

if (this.EditLevel - 1 != parentEditLevel)

throw new UndoException(string.Format(Resources.EditLevelMismatchException, "UndoChanges"));

In this case the logic first checks for EditLevel > 0 followed by the parent EditLevel validation. If ApplyEdit() followed this order I wouldn't get the exception.

Any thoughts?

Thanks.

RockfordLhotka replied on Friday, February 13, 2009

If you use a managed backing field to reference your child then CSLA should set the edit level of the child to match the parent.

If you use a private backing field to reference your child then you must manage edit levels of the child yourself (that's no different than any pre-3.6 code you'd have written in this case).

If you are using a managed backing field and you are still seeing this issue, then please share the code where you declare the child property in the parent, and where you create/add the child object.

oscarmorasu replied on Monday, February 23, 2009

Thanks for your answer Rocky, I'll look deeper into managed fileds.

rsbaker0 replied on Monday, February 23, 2009

oscarmorasu:

..In one scenario an instance of P already exists, and I call BeginEdit(). Then I create an instance of C and assign it to P....

If you can do this with a managed property per Rocky, then if you call LoadProperty() to assign the value it will automatically set the edit level properly to match the containing parent object's edit level.

Edit level issues were the #1 reason I went almost completely to managed properties for child objects. There are some potential gotchas in complicated undo scenarios, but so far I find they work very well.

Andreas replied on Friday, May 08, 2009

Unfortunately LoadProperty doesn't set the child level to its parent level. Only SetProperty() does this job. But that's probably not what you realy want if you think in terms of "lazy loading" child properties because SetProperty() marks you parent object as dirty, fires OnPropertyChanged-Events and checking validation and authorisation rules etc. So how do we implement lasy loading child properties on any parent EditLevel value without corupting the object tree?

Andreas replied on Friday, May 08, 2009

...I need to add, that the behavior depends on which LoadProperty() member you call:
protected void LoadProperty<P>(PropertyInfo<P> propertyInfo, P newValue)
works the way rsbaker0 described

but

protected void LoadProperty(IPropertyInfo propertyInfo, object newValue)
works the way I described (the trap I steped into).

For implementing child lazy loading the right way you have make sure to call 
protected void LoadProperty<P>(PropertyInfo<P> propertyInfo, P newValue)

Rocky, can you comment why they work different?

Andreas

 

 

RockfordLhotka replied on Saturday, May 09, 2009

This seems like a bug.

 

The late-bound overload that accepts an object parameter was added very late in the process to accommodate data load scenarios where types aren’t known. Obviously you take a performance hit for using that overload, but it allows for looser coupling of a sort.

 

Rocky

 

 

From: Andreas [mailto:cslanet@lhotka.net]
Sent: Friday, May 08, 2009 4:42 PM
To: rocky@lhotka.net
Subject: Re: [CSLA .NET] Error in ApplyEdit() if child is added after BeginEdit()

 

...I need to add, that the behavior depends on which LoadProperty() member you call:
protected void LoadProperty<P>(PropertyInfo<P> propertyInfo, P newValue)
works the way rsbaker0 described

but

protected void LoadProperty(IPropertyInfo propertyInfo, object newValue)
works the way I described (the trap I steped into).

For implementing child lazy loading the right way you have make sure to call 
protected void LoadProperty<P>(PropertyInfo<P> propertyInfo, P newValue)

Rocky, can you comment why they work different?

Andreas

 

 



Andreas replied on Wednesday, May 13, 2009

Here is a possible fix.

 

Two new, not-generic members of LoadProperty. The only differnce is that they use the not-generic members Fieldmanager.LoadFiledData and Fieldmanager.SetFieldData. The existing LoadPropertyValue<P>() and LoadProperty<P>() now refer to these new not-generic members.

This change is requried if someone needs a custom PropertyInfo implementation (e.g. derived from PropertyInfo<P>). In this case IPropertInfo is needed to interface with BusinessBase and its LoadProperty members. With this change CSLA now handles child objects correctly.

Will this be fixed in 3.6.3?

 

    //private void LoadPropertyValue<P>(PropertyInfo<P> propertyInfo, P oldValue, P newValue, bool markDirty)

    private void LoadPropertyValue(IPropertyInfo propertyInfo, object oldValue, object newValue, bool markDirty)

    {

        var valuesDiffer = false;

        if (oldValue == null)

            valuesDiffer = newValue != null;

        else

            valuesDiffer = !(oldValue.Equals(newValue));

 

        if (valuesDiffer)

        {

 

            IBusinessObject old = oldValue as IBusinessObject;

            if (old != null)

                RemoveEventHooks(old);

            IBusinessObject @new = newValue as IBusinessObject;

            if (@new != null)

                AddEventHooks(@new);

 

 

            if (typeof(IEditableBusinessObject).IsAssignableFrom(propertyInfo.Type))

            {

                // remove old event hook

                //if (oldValue != null)

                //{

                //  INotifyPropertyChanged pc = (INotifyPropertyChanged)oldValue;

                //  pc.PropertyChanged -= new PropertyChangedEventHandler(Child_PropertyChanged);

                //}

                if (markDirty)

                {

                    OnPropertyChanging(propertyInfo.Name);

                    //FieldManager.SetFieldData<P>(propertyInfo, newValue);

                    FieldManager.SetFieldData(propertyInfo, newValue);

                    PropertyHasChanged(propertyInfo.Name);

                }

                else

                {

                    //FieldManager.LoadFieldData<P>(propertyInfo, newValue);

                    FieldManager.LoadFieldData(propertyInfo, newValue);

                }

                IEditableBusinessObject child = (IEditableBusinessObject)newValue;

                if (child != null)

                {

                    child.SetParent(this);

                    // set child edit level

                    UndoableBase.ResetChildEditLevel(child, this.EditLevel, this.BindingEdit);

                    // reset EditLevelAdded

                    child.EditLevelAdded = this.EditLevel;

                    // hook child event

                    //INotifyPropertyChanged pc = (INotifyPropertyChanged)newValue;

                    //pc.PropertyChanged += new PropertyChangedEventHandler(Child_PropertyChanged);

                }

            }

            else if (typeof(IEditableCollection).IsAssignableFrom(propertyInfo.Type))

            {

                // remove old event hooks

                //if (oldValue != null)

                //{

                //  IBindingList pc = (IBindingList)oldValue;

                //  pc.ListChanged -= new ListChangedEventHandler(Child_ListChanged);

                //}

                if (markDirty)

                {

                    OnPropertyChanging(propertyInfo.Name);

                    //FieldManager.SetFieldData<P>(propertyInfo, newValue);

                    FieldManager.SetFieldData(propertyInfo, newValue);

                    PropertyHasChanged(propertyInfo.Name);

                }

                else

                {

                    //FieldManager.LoadFieldData<P>(propertyInfo, newValue);

                    FieldManager.LoadFieldData(propertyInfo, newValue);

                }

                IEditableCollection child = (IEditableCollection)newValue;

                if (child != null)

                {

                    child.SetParent(this);

                    IUndoableObject undoChild = child as IUndoableObject;

                    if (undoChild != null)

                    {

                        // set child edit level

                        UndoableBase.ResetChildEditLevel(undoChild, this.EditLevel, this.BindingEdit);

                    }

                    //IBindingList pc = (IBindingList)newValue;

                    //pc.ListChanged += new ListChangedEventHandler(Child_ListChanged);

                }

            }

            else

            {

                if (markDirty)

                {

                    OnPropertyChanging(propertyInfo.Name);

                    //FieldManager.SetFieldData<P>(propertyInfo, newValue);

                    FieldManager.SetFieldData(propertyInfo, newValue);

                    PropertyHasChanged(propertyInfo.Name);

                }

                else

                {

                    //FieldManager.LoadFieldData<P>(propertyInfo, newValue);

                    FieldManager.LoadFieldData(propertyInfo, newValue);

                }

            }

        }

    }

 

    /// <summary>

    /// Loads a property's managed field with the

    /// supplied value calling PropertyHasChanged

    /// if the value does change.

    /// </summary>

    /// <param name="propertyInfo">

    /// PropertyInfo object containing property metadata.</param>

    /// <param name="newValue">

    /// The new value for the property.</param>

    /// <remarks>

    /// No authorization checks occur when this method is called,

    /// and no PropertyChanging or PropertyChanged events are raised.

    /// Loading values does not cause validation rules to be

    /// invoked.

    /// </remarks>

    //protected void LoadProperty<P>(PropertyInfo<P> propertyInfo, object newValue)

    protected void LoadProperty(IPropertyInfo propertyInfo, object newValue)

    {

        try

        {

            object oldValue;

            var fieldData = FieldManager.GetFieldData(propertyInfo);

            if (fieldData == null)

            {

                oldValue = propertyInfo.DefaultValue;

                //fieldData = FieldManager.LoadFieldData<P>(propertyInfo, oldValue);

                fieldData = FieldManager.LoadFieldData(propertyInfo, oldValue);

            }

            else

            {

                //var fd = fieldData as FieldManager.IFieldData<P>;

                var fd = fieldData as FieldManager.IFieldData;

                if (fd != null)

                    oldValue = fd.Value;

                else

                    oldValue = fieldData.Value;

            }

            //LoadPropertyValue<P>(propertyInfo, oldValue, newValue, false);

            LoadPropertyValue(propertyInfo, oldValue, newValue, false);

        }

        catch (Exception ex)

        {

            throw new PropertyLoadException(

              string.Format(Properties.Resources.PropertyLoadException, propertyInfo.Name, ex.Message), ex);

        }

    }

 

    protected void LoadProperty<P>(PropertyInfo<P> propertyInfo, P newValue)

    {

        this.LoadProperty(propertyInfo as IPropertyInfo, newValue);

    }

 

    private void LoadPropertyValue<P>(PropertyInfo<P> propertyInfo, P oldValue, P newValue, bool markDirty)

    {

        this.LoadPropertyValue(propertyInfo as IPropertyInfo, oldValue, newValue, markDirty);

    }

 

 

RockfordLhotka replied on Monday, May 18, 2009

We surely don't want the generic LoadProperty<T>() to invoke a non-generic LoadProperty().

There's a lot of complex stuff going on in the field manager to avoid boxing/unboxing of values when the generic methods are used. If you use the non-generic methods you pay a performance penalty, but get looser behaviors (and more agressive type coercion).

In other words, I basically implemented two code paths that do almost the same thing - on purpose. The generic code path (and IPropertyInfo<T> and FieldData<T>) all exist to avoid boxing and type coercion. The non-generic code path should have the same behavior, but with boxing and type coercion.

So the bug here is that the non-generic LoadProperty() isn't doing the right thing - but that needs to be solved without causing the generic implementation to use a less efficient code path.

Andreas replied on Tuesday, May 19, 2009

Yes, Rocky everything you said makes perfektly sense. With IPropertyInfo<T> I could fix the problem much cleaner....but where do I find IPropertyInfo<T>? There is only PropertyInfo<T> and IFieldData<T>....?

Andreas

 

RockfordLhotka replied on Tuesday, May 19, 2009

Sorry, you are right, there is the IPropertyInfo interface, and the PropertyInfo<T> class. But that is all you need.

 

Remember that you create the PropertyInfo<T> instance in your RegisterProperty() method call. You can create any IPropertyInfo object you choose when you call RegisterProperty() – it only cares about the interface. So that object can be generic or non-generic as you choose. It doesn’t even have to be the same type for every property if you wanted variety.

 

Your IPropertyInfo implementation object (normally PropertyInfo<T>) is responsible for creating the IFieldData<T> instance (normally FieldData<T>), and so you choose exactly what class to use when creating your field data container object.

 

I must confess that I’ve lost the reason we’re discussing this.

 

There’s a bug in LoadProperty() (non-generic overload) that needs to be addressed – but how did we get off on replacing the PropertyInfo<T> and FieldData<T> types?

 

Rocky

Andreas replied on Tuesday, May 19, 2009

Not an easy question, how to get of replacing the generic types. I asked this question myself before I implemented the non-generic LoadProperty member. I have no answer to it and I only think that IPropertyInfo is simply missing in the framework.
My suggestion would be that BusinessBase can have both, non-generic and generic LoadProperty, executing more or less the same code. The only difference is that one of them refer to the generic and the other to the non-generic LoadFieldData and SetFieldData of FieldManager. But for performance reasons the framework uses the generic ones internally only. By the way: Why did you provide non-generic LoadFieldData and SetFieldData of FieldManager when you think boxing and unboxing will lead to an inacceptable performance penalty?
I think we end up with a little CSLA design problem here, don't we? If you really wanted to prevent boxing and unboxing you probably should have introduce IPropertyInfo in the first place just like IFieldData.

Andreas

Andreas replied on Tuesday, May 19, 2009

Sorry, but IE8 swallowed all "<T>" in my last post:

Not an easy question, how to get of replacing the generic types. I asked this question myself before I implemented the non-generic LoadProperty member. I have no answer to it and I only think that "IPropertyInfo< T >" is simply missing.
My suggestion would be that BusinessBase can have both, non-generic and generic LoadProperty, executing more or less the same code. The only difference is that one of them refer to the generic and the other to the non-generic LoadFieldData and SetFieldData of FieldManager. But for performance reasons the framework uses the generic ones internally only. By the way: Why did you provide non-generic LoadFieldData and SetFieldData of FieldManager when you think boxing and unboxing will lead to an inacceptable performance penalty?
I think we end up with a little CSLA design problem here, don't we? If you really wanted to prevent boxing and unboxing you probably should have introduce "IPropertyInfo< T >" in the first place just like "IFieldData< T >".

RockfordLhotka replied on Tuesday, May 19, 2009

I should point out too, that CSLA doesn’t need an IPropertyInfo<T> to avoid boxing or type coercion.

 

The only place where boxing and type coercion come into play is when interacting with the field data container objects (IFieldData<T>). You might notice that there are both IFieldData and IFieldData<T>, and CSLA will try to use the generic one when possible, falling back to the non-generic when necessary.

 

But there’s really no need for an IPropertyInfo<T>, because the framework doesn’t need type information from the property metadata container, only the field data container.

 

Rocky

RockfordLhotka replied on Tuesday, May 19, 2009

The non-generic LoadProperty() method exists purely to support loosely
coupled data access layer implementations.

Some people implement their DAL by calling a service or other external
entity that returns a DTO, and their DAL doesn't actually know the type of
the DTO. Using various techniques (reflection, dynamic types, etc) it is
possible to load the business object's properties.

In that case a compile-time concept like generics is not useful because no
type information is actually available until runtime, hence the non-generic
LoadProperty().

As I mentioned earlier, this was not part of the original design. This was
something added later in the development process based on community
feedback.

And this is (to some degree) why the editlevel bug exists - because if you
use the non-generic LoadProperty() in its intended usage scenario you'd
never encounter a case where the edit level needs to be set.

All that said, it does seem reasonably valid that someone might use the
non-generic LoadProperty() outside the DAL, and would then expect it to have
the same semantic behaviors as the generic implementation - which is why I
do think this is a bug.

Rocky

Andreas replied on Tuesday, May 19, 2009

Rocky, I don't want to stress this to much but I can't hardly believe, that DAL scenario was the only use case and motivation for adding the non-generic LoadProperty - which was of cause not the original design. But what about custom or derived PropertyInfo<T> implementations? This is another use case, where a non-generic LoadProperty(IPropertyInfo,..) is needed, in any case you want to call LoadProperty. This is (from what I know so far) definitely an intended use case from the beginning, isn't it?

Will this bug be fixed in 3.6.3?

 

Andreas

 

 

 

RockfordLhotka replied on Tuesday, May 19, 2009

Three things.

 

First, the design scenario for the non-generic LoadProperty() was something the community came up with late in the development cycle, and is purely centered around the DAL. I guess in the end you can believe what you would like, but as I’m the guy who decided to create the overload for that purpose I’m pretty sure that was the only thing in my mind at the time.

 

Second, I think I’ll fix LoadProperty() regarding the editlevel issue in 3.6.3, but I can’t say for sure. It might have to wait until 3.6.4 or something. I’m only one guy, and I also do work where I make money and that takes precedence over the wish list. Alternately I can hold 3.6.3 until this feature is done, but that really won’t change the timeframe in which I can get to it, and it would prevent valuable bug fixes from being released in the shorter term.

 

Third, I think you are talking about a different issue, and I confess I don’t understand the issue you are talking about. Clearly it is something other than the editlevel bug in LoadProperty(), and has something to do with a custom PropertyInfo<T> equivalent. I think it would be helpful if you would clearly explain the scenario you are facing.

 

Rocky

Andreas replied on Wednesday, May 20, 2009

Here is my scenario:

1. I decided to create my custom PropertyInfo class (class MyPropertyInfo implementing IPropertyInfo). The decision came out from the following discussion: http://forums.lhotka.net/forums/thread/28071.aspx
2. Then I wanted to implement lazy loading of some properties. Before the lazy loading starts the owning parent already can have a certain Editlevel > 0.  Here is the implementation of the property getter

get
{
   //ChildPropertyInfo is of type MyPropertyInfo
   if(FieldManager.FieldExists(this.ChildProperty)==false)
   {
      ChildType childValue = DataPortal.Fetch<ChildType>.Fetch(new SingelCriteria<ChildType, int>(this.id);
      this.LoadProperty(ChildProperty, childValue); //LoadProperty(IPropertyInfo, object value) is called and hits me with the EditLevel bug...
      this.OnPropertyChanged(this.ChildProperty.Name);
   }
   return this.GetProperty(this.ChildProperty);
}

By reviewing my code during this discussion I changed my MyPropertyInfo implemenation which now derives from PropertyInfo<T> instead of implementing IPropertyInfo from the ground up. Now I can force the code to call the LoadProperty<T>() member. So this will work for me even if you decided to fix it in version >3.6.3. Thanks for the valuable discussion!

Andreas

 

RockfordLhotka replied on Wednesday, May 20, 2009

So the only reason for creating a custom IPropertyInfo is to provide a more advanced field data container? If that’s true, then you should subclass PropertyInfo<T> as a solution.

 

There are two primary scenarios for creating a custom IPropertyInfo

 

1.       To override the method that creates the field data container object for a property

2.       To add more metadata properties describing the property (such as values describing how this property should be persisted)

 

Both scenarios can be accomplished by subclassing PropertyInfo<T>.

 

I don’t have a scenario at this time, where a raw implementation of IPropertyInfo would be necessary.

 

Rocky

Copyright (c) Marimer LLC