Switchable ReadOnly-Editable object

Switchable ReadOnly-Editable object

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


ben posted on Tuesday, January 08, 2008

Hi,

 

We’re having a scenario a little bit special. We’ve got an EditableRootListBase with editable business objects. Those items can be edited until they get a special state since which they must become ReadOnly (they can’t be deleted or edited).

 

We are thinking about adding an “IsReadOnly” property to the Csla.Core.BusinessBase class that let us set our business objects as ReadOnly. This property may be checked in the “BeginEdit” method to avoid editions if the object is ReadOnly. It also may be checked in the IsSavable property to return false if the object is ReadOnly.

 

Does anybody have done something like this?

Does anybody think it may be a problematic approach?

 

Thanks in advance

 

Benjamin

ajj3085 replied on Tuesday, January 08, 2008

I think a better way to do this would be to have some kind of flag or method that determines if the instance is readonly or not.  Then override CanWriteProperty and if that flag is true, always return false.  You can still expose a property to determine if the object as a whole is editable.  I do this myself.  Quotes and orders can be locked to prevent editing.  They can be unlocked as well, which takes a snapshot at the time of unlocking for history reasons.

ben replied on Tuesday, January 08, 2008

Thanks for your answer ajj3085. Your solution seems quite nice but it presents some inconvenients:

 

-         We’re not using the CSLA property level security system because we don`t need it, so our properties accessors do not call CanReadProperty() or CanWriteProperty() methods. We had to change all our objects’ properties to implement it. We’re using code generation so this may be resolved.

-         Thinking about performance, it seems much faster to check whether an object is ReadOnly just once before trying to edit it and not every time the user try to edit one property.

-         When the user tries to edit a ReadOnly instance the exception raised shouldn’t be a security exception with a generic message. It should be a System.ReadOnlyException (or a BusinessLogicException) with a message telling the reason why the object is ReadOnly. For instance: “This delivery note can’t be edited because of it is invoiced”). There may be many business reasons to set a property as ReadOnly so it is very helpful to the user knowing why he/she can’t edit a specific property.

 

I can see that your choice is more flexible since it let you set just some properties as ReadOnly and other may still be editable but we don’t really need that behaviour. We want to lock our delivery notes (all their properties) when they get invoiced.

 

We have thought in that approach because it seems easer to implement and simpler. It also allow us to modify a base DataForm to check if its data source object is ReadOnly (just checking a generic property) and set all its controls as ReadOnly. With only one check we get all controls disabled and the user can see the DataForm in ReadOnly mode.

 

The only reason I think your choice may be better than ours is because you are not modifying the framework and that makes easer to update it with new versions. Why do you think your choice is better? Is there something I’m messing?

 

We are working in developing this new feature. If there is some one interested we may publish it here when it is finished. If more people think it is useful then Rocky may consider including it in the framework in some way.

 

Thanks again.

 

Benjamin Moles

JoeFallon1 replied on Tuesday, January 08, 2008

Ben,

I am not sure why you are modifying CSLA directly. Do you have Base classes which inherit from CSLA? Then all your BOs inherit from your Base classes. This way you can add functionality to your Base class and all your BOs will pick it up. This design has been highly recommended for quite a while now.

Joe

 

ajj3085 replied on Tuesday, January 08, 2008

ben:
We’re not using the CSLA property level security system because we don`t need it, so our properties accessors do not call CanReadProperty() or CanWriteProperty() methods. We had to change all our objects’ properties to implement it. We’re using code generation so this may be resolved.


If you code gen than this shouldn't really be an issue.  Even if you're not using property level security you should probably have these checks though.. see my next statement for more.

ben:
Thinking about performance, it seems much faster to check whether an object is ReadOnly just once before trying to edit it and not every time the user try to edit one property.


Well there's no reason you still can't do this.  I recommend you still have the CanWriteProperty override though... beware the UI developer that DOESN'T check if the entire object IsReadOnly and happily lets the user modify the object and save it.

ben:
When the user tries to edit a ReadOnly instance the exception raised shouldn’t be a security exception with a generic message. It should be a System.ReadOnlyException (or a BusinessLogicException) with a message telling the reason why the object is ReadOnly. For instance: “This delivery note can’t be edited because of it is invoiced”). There may be many business reasons to set a property as ReadOnly so it is very helpful to the user knowing why he/she can’t edit a specific property.


In your custom BusinessBase class sitting between your BOs and Csla.BusinessBase (you DO have one, right?) just add an overload of CanWriteProperty that throws a different exception of your choosing.

 

ben:
I can see that your choice is more flexible since it let you set just some properties as ReadOnly and other may still be editable but we don’t really need that behaviour. We want to lock our delivery notes (all their properties) when they get invoiced.


This is one of those things that you may never have, but also the cost of including it is low.  The per-property method with CanWriteProperty overload will make coding of your properties simplier (you'd likely have a method that throws the appropriate exception anyway instead of hardcoding that exception throw in every setter).  You can make things even a bit more performant by always returning true or false from the CanWriteProperty override instead of delegating back to the base class.


 

ben:
We have thought in that approach because it seems easer to implement and simpler. It also allow us to modify a base DataForm to check if its data source object is ReadOnly (just checking a generic property) and set all its controls as ReadOnly. With only one check we get all controls disabled and the user can see the DataForm in ReadOnly mode.


Yes, you can still do this.  The advantage of overriding CanWriteProperty in addition is that you won't have to worry about a developer that forgets to have his from inherit from DataForm.

 

ben:
The only reason I think your choice may be better than ours is because you are not modifying the framework and that makes easer to update it with new versions. Why do you think your choice is better? Is there something I’m messing?


Its not an xor choice. I would say do both; one to keep things simplier in your DataForm, the other to catch UI programmers that make mistakes (we all make mistakes, after all).  I wouldn't directly modify Csla itself though unless you absolutely have to so that you can keep up with the latest version, or at the very least cause less pain when you do upgrade  your Csla version.

HTH
Andy

ben replied on Wednesday, January 09, 2008

Hi Joe and Andy,

 

To avoid the possibility someone may edit an object by mistake we thought using the BeginEdit method:

 

Public Sub BeginEdit() Implements IEditableBusinessObject.BeginEdit

 

   If Me.IsReadOnly Then

      Throw New ReadOnlyException(Me.ReadOnlyReason)

   End If

 

   If Not mHasBeenTouch Then

      OnFirstEdition()

      mHasBeenTouch = True

   End If

   CopyState(Me.EditLevel + 1)

End Sub

 

As the BeginEdit method is not overridable (I don’t know why) we thought changing the Csla.Core.BusinessBase and Csla.BusinessListBase classes. This is the reason why we decide it.

 

After reading your post, we have changed our implementation. Now, we have make “BeginEdit” overridable and we have moved all our new code to our own BusinessBase and BusinessListBase classes (from which all our business objects inherit). Now we have the following code in our base classes:

 

Public Overrides Sub BeginEdit()

   If Me.IsReadOnly Then

      Throw New ReadOnlyException(Me.ReadOnlyReason)

   End If

   MyBase.BeginEdit()

End Sub

 

By overriding the “BeginEdit” method we avoid any risk of someone may save data into the database but, Andy you are right, in the properties “Set” block there may be code that should be protected against wrong access. And we are not talking about a XOR chance so we will override methods “CanReadProperty”, “CanWriteProperty” and “CanExecuteMethod”.

 

Andy, I’ve seen method “CanWriteProperty(propertyName, throwOnFalse)” is not overridable and also you said “overload” instead of “overrides”. I think overloading is good because of you’re not modifying the CSLA framework but it is not very safe due to any programmer may cast a business object as Csla.Core.BusinessBase and then a call to this method would not be consistent. What do you think about making this method overridable? (it would be a small change to the CSLA framework).

 

Thanks to both for your time. Your contribution is very helpful.

 

Ben

ajj3085 replied on Wednesday, January 09, 2008

Have you tested this new implementation with databinding?  I think that databinding may end up calling it, and it won't know about your object level read-only flag.  Also, semantically it seems "odd."  I don't think anything expects an exception when calling BeginEdit, and remember that is something called from databinding.

You're right that someone could call the wrong overload of CanWriteProperty.  I think that risk is low though, because you really want to use your overloaded version in property setters, which you said would all be code-gened.  So the risk there is minimal.  Further reducing the risk would rely on education of your developers.  You can only hand hold them so much though.. Smile [:)]  In all seriousness, there's nothing technically stopping your developers from using reflection to set a field directly instead of through one of your property setters.  Again, you can only do so much.. anything beyond will probably needlessly add complexity to your system and give you a harder time if you want to upgrade to a newer version of Csla.

The only other place that could happen is from the UI, but there's really no reason for the UI to call a either version of CanWriteProperty that would throw an exception for you.  It should call the version that just returns the result, so it can make controls readonly and / or disabled.

What you ultimately is of course up to you.  You need to decide what things are going to be important moving forward; ease of upgrade to the newest Csla, keeping developers from doing "bad" things in code vs. by education, etc.

HTH
Andy

ben replied on Wednesday, January 09, 2008

We are using “DisableIEditableObject = True” so we are handling the edition process in our framework (extends the CSLA). Databinding calls are not a problem for us. Our code is ready to catch these exceptions.

 

I don’t understand why you say that semantically it seems “odd”. In my opinion, the best moment to tell someone that an object is ReadOnly and it can’t be edited is when the object is about to be edited. When any process or user wants to edit an object it must call BeginEdit so, if this process/user forgot to check the IsReadOnly property then the BeginEdit method is the natural place to raise an exception. Databinding is designed thinking in DataSets so it may not support many things (like n-level undo) and we have to live with it, but that is a different issue.

 

I think you are right again when you say that the risk is very low overloading instead of overriding the “CanWriteProperty” method. Furthermore, we can change it at any moment if we need to, so we’ll overload it.

 

Thanks.

 

Ben

ajj3085 replied on Wednesday, January 09, 2008

ben:
We are using “DisableIEditableObject = True” so we are handling the edition process in our framework (extends the CSLA). Databinding calls are not a problem for us. Our code is ready to catch these exceptions.


DisableIEditableObject doesn't stop Windows forms controls (like the BindingSource) from calling BeginEdit.  Do you actually have this working?

 

ben:
I don’t understand why you say that semantically it seems “odd”. In my opinion, the best moment to tell someone that an object is ReadOnly and it can’t be edited is when the object is about to be edited.


Well if your coding the UI, I'd think you would check the flag and disable editing controls, or use ReadWriteAuthorization component from Csla.  BeginEdit will just serialize the object and push it onto the stack.  You've set a flag so this won't happen, so given all this I'm not sure why an exception here is needed.


ben:
When any process or user wants to edit an object it must call BeginEdit so, if this process/user forgot to check the IsReadOnly property then the BeginEdit method is the natural place to raise an exception. Databinding is designed thinking in DataSets so it may not support many things (like n-level undo) and we have to live with it, but that is a different issue.


That's not true at all.  Your client program can attempt to set properties without ever calling BeginEdit.  That's strickly for a databinding scenario, but that's certainly not the only way to interact with the object.
 

ben:
I think you are right again when you say that the risk is very low overloading instead of overriding the “CanWriteProperty” method. Furthermore, we can change it at any moment if we need to, so we’ll overload it.


I think that's probably your best bet.  Again though, at the end of the day you need to do what works for your application.

Andy

Copyright (c) Marimer LLC