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.
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:
..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.
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?
...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
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
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);
}
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.
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
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
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 >".
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
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
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
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
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