I agree 100% - I saw this early on in the testing of CSLA 2.0 but figured I was the only one doing rule checking based on the State of the BO (IsNew) so I didn't bother to bring this issue to Rocky's attention.
After my templates Fetch the data I call MarkOld and then call ValidationRules.CheckRules. As noted, one of the rules depends on if the object IsNew or Not.
I like the idea of moving the framework call of MarkOld to just *before* DataPortal_Fetch as it allows me to remove the duplicate call from my templates.
Joe
I think MarkOld() should not be called before DataPortal_Fetch(). In DataPortal_Fetch(), after fetching data for the current object, it might need to invoke the appropriate Fetch() method on child collections or objects. What if the validation rules in these child objects behave differently depending on the IsNew property of the parent?
Regards,
William
No, just a convention I've been using. IsDirty would be wrong in my case though if MarkOld was made first, unless I go back and change all my code.. I would think there could be other reasons as well, like those that want new objects to be clean, depending on how it was implemented.
ajj3085:
I typically set data via the properties, not directly into the fields.
But that is in direct contradiction to Rocky's recommended best practices.
How dare you! <g>
Joe
I completly agree with Andrés's original post in that I too was used to calling the MarkOld after a fetch.
In fact I'll go as far as saying that _WE_LOST_ this behavior when we went from CSLA 1.x to 2.0. At least in 1.x we had the flexibility of calling MarkOld or not. In 2.0 we have no choice, unless of course we modify the framework - but this warrants its own threaded discussion.
Hey Andrés, on second thought.... it does not make that much sense to call the MarkOld _before_ the dp_Fetch call because your data has not been feteched yet, so how can it be _old_ , or not dirty.
Why not just call the MarkClean() in your dp_fetch.
![Smile [:)]](/emoticons/emotion-1.gif)
xal:
I don't think that's the problem, whether the data has been fetched or not is unimportant. What matters is that you're trying to retrieve data from the db, so the object is "obviously"** going to be old.
The fact that some people (hey Andy!) use the property setters to load the objects doesn't change the fact that you currently don't have much flexibility. In this scenario, you could easily just call mark clean, which is very easy if you use code generation.
![Wink [;)]](/emoticons/emotion-5.gif)
I realize it might be a pain for people that depend on that call being made after their code runs, but I don't believe it's a problem for the majority of users. (just a wild guess)
** The current implementation has another problem too. Some people (I haven't had the need to do this yet, but it's something some people might want) make the fetch NOT fail if there's no data on the db, and just return a "new" object with that id if it does not exist.
Now, in that scenario, you'd normally want to call MarkNew in the fetch, but it won't do much good because the dp will mark it as old later. So, in any case, I think this things should be called before DataPortal_Fetch, so that we get more flexibility.
Well, again, it is currently inflexible as in you normally would call MarkOld() in the fetch, so it's cool that it's done automatically, but if you need something different like IsNew = True when the object doesn't exist in the db, you can't do it because it will be overriden by the dp.
The flexibility comes in that whatever you do inside Fetch() sticks. Right now, it doesn't matter if you call MarkNew, MarkDirty or whatever, it will all be disregarded because of the MarkOld call done in the dp.
About overriding InvokeCompleted, it would be a pain, because you don't have an easy way of knowing whether the object was retrieved or not from the db unless you set some var indicating that it was loaded or it is new. Inside fetch, you could easily do if (dr.Read()) { ... } else { MarkNew(); }.
That is very straight forward, and doesn't require overriding an "outside method" that doesn't know much about what happened during the fetch (unless you explicitly store that info somewhere).
Perhaps this is an argument against having a Fetch return a new object. I haven't yet come across a case where it made sense to do that, and you're blurring the definition. After all, Fetch means 'load from database,' not 'load from database if possible, new otherwise.'
============================================================
But this is exactly how I define my Fetch code. So now it looks like it is broken under Csla2 because the DP calls Mark Old for you. Hmm. Looks like I might have to comment it out until Rocky changes it to either Before Fetch or uses the Args idea.
Joe
But this is exactly how I define my Fetch code. So now it looks like it is broken under Csla2 because the DP calls Mark Old for you. Hmm. Looks like I might have to comment it out until Rocky changes it to either Before Fetch or uses the Args idea.
But this is exactly how I define my Fetch code. So now it looks like it is broken under Csla2 because the DP calls Mark Old for you. Hmm. Looks like I might have to comment it out until Rocky changes it to either Before Fetch or uses the Args idea.
It isn't so much a requirement as it is that is how I built the Fetch method years ago. In the old forum there were lengthy discussions about this and half the people came down on the side of returning new and the other half wanted to return Nothing or throw an exception. Since I had an app to build I picked one of the methods and now all of my code expects that behavior. If the behavior were to suddenly change I have no idea how many pages would break.
Protected Overrides Sub DataPortal_Fetch(ByVal criteria As Object)
If TypeOf criteria Is CriteriaCode Then
Dim crit As CriteriaCode = DirectCast(criteria, CriteriaCode)
If RecordDoesNotExist(crit.Code) Then
Me.DataPortal_Create(criteria)
Else
DoFetch(criteria)
End If
End If
End Sub
Protected Overridable Sub DoFetch(ByVal criteria As Object)
SetDefaults()
Dim cn As IDbConnection
Try
cn = DAL.CreateConnection
cn.Open()
PreFetchData(criteria, cn)
FetchData(criteria, cn)
PostFetchData(criteria, cn)
FetchChildren(criteria, cn)
Finally
cn.Close()
End Try
ValidationRules.CheckRules()
End Sub
Joe
IsNew should be false before your DP_F code runs.
However, it is quite clear that IsDirty needs to be reset to false after DP_F runs, or untold amounts of code would be badly broken.
So would it work to set IsNew to false before DP_F, and both IsDirty and IsNew to false after DP_F?
No. It would not work. You are still setting IsNew to False AFTER the fetch. In my code it is a breaking change. There were a lot of other people who coded their BOs this way too in the old forum. I didn't just make up this idea of returning New if the record is not found. They may not be participating in this new forum as vocally as some people but this issue should not be discounted.
FWIW - I wasn't aware it was a breaking change in 2.0 until this thread pointed it out.
I am not in Production code yet - but will be soon. So I have only done light testing and missed this point.
What is wrong with this?
set IsNew to false and IsDirty to false before DP_F.
Then if I happen to branch over to DP_Create I will have to call MarkNew
and set IsNew=True and IsDirty=True. Then when Fetch returns there is no call to re-set these values since it happened before the Fetch.
==================================================================
Inside SimpleDataPortalFetch just move one line of code:
MethodCaller.CallMethodIfImplemented(obj, "MarkOld")
' tell the business object to fetch its data
Dim method As MethodInfo = MethodCaller.GetFetchMethod(objectType, criteria)
If TypeOf criteria Is Integer Then
' an "Integer" criteria is a special flag indicating
' that criteria is empty and should not be used
MethodCaller.CallMethod(obj, method)
Else
MethodCaller.CallMethod(obj, method, criteria)
End If
''' mark the object as old
'''MethodCaller.CallMethodIfImplemented(obj, "MarkOld")
I just tested it and the BO is set correctly:
mIsNew = False
mIsDirty=False
=====================================================
Note: the exact same issue exists in SimpleDataPortal_Create
' mark the object as new
MethodCaller.CallMethodIfImplemented(obj, "MarkNew")
' tell the business object to fetch its data
Dim method As MethodInfo = MethodCaller.GetCreateMethod(objectType, criteria)
If TypeOf criteria Is Integer Then
' an "Integer" criteria is a special flag indicating
' that criteria is empty and should not be used
MethodCaller.CallMethod(obj, method)
Else
MethodCaller.CallMethod(obj, method, criteria)
End If
''' mark the object as new
'''MethodCaller.CallMethodIfImplemented(obj, "MarkNew")
================================================================
Joe
PS - some quick tests show the BO is returned in the correct state (as far as IsNew and IsDirty are concerned) for both DP_Create and DP_Fetch if you move the code to before the methods are called. Not sure where you need to call OnUnknowPropertyChanged when fetching.
To be fair, its only a breaking change if you went against the grain so to speak. Implementing your suggestion would break all of my code because I set the object state via properties which changes the IsDirty flag. Many people may have done New in a Fetch, but it doesn't sound like Rocky ever supported doing so..No. It would not work. You are still setting IsNew to False AFTER the fetch. In my code it is a breaking change. There were a lot of other people who coded their BOs this way too in the old forum. I didn't just make up this idea of returning New if the record is not found. They may not be participating in this new forum as vocally as some people but this issue should not be discounted.
FWIW - I wasn't aware it was a breaking change in 2.0 until this thread pointed it out.I am not in Production code yet - but will be soon. So I have only done light testing and missed this point.
What is wrong with this?
set IsNew to false and IsDirty to false before DP_F.
"To be fair, its only a breaking change if you went against the grain so to speak. Implementing your suggestion would break all of my code because I set the object state via properties which changes the IsDirty flag."
To be fair - you are going against the grain here too. Rocky specifically recommends NOT setting state during fetch using Properties. It not only changes the flag, it raises the events and rule checking. If you did it the way Rocky recommends then the recommended change would not be an issue for you plus it would solve the issue for me.
In your case, if Rocky made the change to set IsDirty before the Fetch then your code could be fixed by adding MarkOld to the end of your Fetch. The call would not be needed if you used the fields to set the values.
Joe
I tend to agree. By introducing the MarkOld() call into DP_Fetch, we've introduced a secondary behavior into the method and coupled the Fetch behavior to marking it old. As we've seen in these posts, that may not be the desired behavior.
I'd vote for removing it from DP_Fetch altogether, even though it's a breaking change for 2.0 code. It allows each developer to code the DP_Fetch to his/her needs.
For what it's worth...
So what I'm hearing is that I can do this
- Call MarkOld() before DP_F
- Call MarkClean() after DP_F
And everyone will be happy?
Rocky,
I don't think you can make everyone happy. I think the above would not work for me.
At this point I agree with Mark and Andy that removing the call from the framework is the best thing to do. Unexpected behaviors are eliminated at the cost of having to add MarkOld to the end of Fetch method for a Root BO. As noted in the book - developers need to do this for Child BOs anyway so it is more consistent.
Don't forget that Create has the same issue so if you remove it from Fetch you should also remove the corresponding code from Create.
Don't forget the reason this whole debate started was that the call to CheckRules fails because MarkOld is called after the Fetch routine and the BO is in the wrong state. Since many developers "do not trust the database" they follow the recommendation to call CheckRules after a Fetch (or a Create). This means they need to MarkOld *before* they call CheckRules which makes the framework call to MarkOld redundant.
Joe
Looking for some feedback from Rocky on these comments.
I think I am going to fourth the motion for removing the calls. On the upside, it will make root and child objects consistent. I think that one of the ways of evaluating the value of a framework is by how much it does for you, but in this case I think it might be getting in the way a bit too much.
Just thinking out load here - if the framework sets IsNew to false before calling Fetch, and Fetch throws an exception, the framework doesn't have to worry about resetting the value back to IsNew=true since we won't have a valid object anyway, right?
I'm all in favor of having the framework handle the repetitive, plumbing-type code as much as possible. However, it appears many of us have differing viewpoints on how to use Fetch (I do think it should only be for fetching, not creating, myself). Some call CheckRules() after loading from the db, others trust the data - some set via properties, others via fields, etc, etc. You can't please everyone all of the time, so damn us all and remove the call to MarkOld. :-)
However, as far as I can tell from all the posts, the only method at issue here is Fetch(). I don't see any problem in allowing the DataPortal to continue calling MarkOld() during Insert/Update and MarkNew() during Deletes.
Maybe we need an enhanced version of Fetch (using events, virtual methods, whatever). Maybe BeforeFetch(), DataPortal_Fetch(), AfterFetch() - something like that. The default implementation for BeforeFetch/AfterFetch in CSLA would be <whatever>, and we could override the methods if desired. No changes to existing code if we want the default CSLA behavior.
I too am in favor of keeping the automatic behaviour. I was one of those who forgot to mark my objects as old all the time in the earlier versions.
However, to accommodate all users of the framework, would it be possible to create an attribute which would tell the framework, not to do automatic MarkOld and the likes. Then the developer that needs to handle this manually could mark the DP_F method with this attribute.
It may be a bit of a hack, though
/Henrik
However, to accommodate all users of the framework, would it be possible to create an attribute which would tell the framework, not to do automatic MarkOld and the likes. Then the developer that needs to handle this manually could mark the DP_F method with this attribute.
This sounds like a suggestion I had made; adding an extra parameter to the event arguments for Invoke and invokeComplete so that you can figure out what is going on. The default Invoke / InvokeComplete could then handle those tasks. Maybe the event args could also include the criteria used to invoke the DP method, for Create, Fetch and Delete ajj3085:
I definitely agree with this.
For my opinion, an object is not old while fetching, and cannot be old unless all its data is loaded. Including children objects. Additionally an object is dirty while fetching.
On the other hand, I haven’t defined such dependencies in my design yet.
Introducing the virtual methods is probably the way to go. IMHO, the DataPortal should only be responsible for the supporting the mobile-object concept. It really shouldn't be responsible for setting the state of the object - that should be handled internally by the object itself. The DataPortal can indirectly trigger this - by making calls to pre_fetch, fetch, and post_fetch (or whatever they happened to be called).
Hey guyroch! How was your csla vacation?
I go _under_ during the summer - reconnecting with nature and the family :)
** The current implementation has another problem too. Some people (I haven't had the need to do this yet, but it's something some people might want) make the fetch NOT fail if there's no data on the db, and just return a "new" object with that id if it does not exist.
Now, in that scenario, you'd normally want to call MarkNew in the fetch, but it won't do much good because the dp will mark it as old later. So, in any case, I think this things should be called before DataPortal_Fetch, so that we get more flexibility.
My approach with this would be more to just catch the RecordNotInTableException (or whatever exception you deemed appropriate) and then return a complety "new" object instead of toying toying with the fetch object. I not sure I would have the BO do this though, I guess it depends on the use case.
just my 2 cents here
Copyright (c) Marimer LLC