Re: Persistence methods in the POJOs (was Re: velocity context cleanup)

[Allen Gilliland <Al...@Sun.COM>]

I think you are definitely right about 2 things.

1. there is no reason to worry about what methods are in a pojo now because we are going to have wrappers/proxy for that very soon.

2. we definitely do not want to limit the functionality of our object model and break the natural OO ability to do inheritance, polymorphism, etc.

I totally believe that our pojos should have any application logic method that is relevant to itself.  In fact I just created a whole set of new WebsiteData.getPageByXXX() methods for the theme management code because it makes far more sense to call website.getDefaultPage() rather than userMgr.retrievePage(website.getDefaultPageId()).

However, I would still argue (and not all that strongly) that some pieces of the persistent object class still may not be appropriate where they are currently.

PO.canSave() - this one is probably the one i am most against because i'm not exactly convinced that an object itself should know and be able to determine if/where it can be saved.  an OO example might be ... if someone wants to deposit money in a bank then it is the banks responsiblity to determine who can deposit money and who can't, not the customers responsibility.  for this reason i believe that it is ultimately the manager classes responsiblity to decide if an object can be saved or not and to throw an exception if permission is denied.

PO.save()/PO.remove() - these two i go more back and forth on.  i have almost always used mgr.save(obj) and mgr.delete(obj) in the past and it has always worked fine, so that's what i am used to and comfortable with.  I like the idea of having save() and remove() methods, however i'm not sure it's ever really made sense to me that an object deletes itself.  if i want to close my account at a bank i need to actually go to the bank and say "i want to close my account", i can't just to it myself without involving the bank.

My view of manager classes is that they are meant to be a gateway to whatever persistent store you are using.  They should be the single point of access for persisting your objects and they should ideally be designed so that users of the manager classes don't need to know anything about how the data is persisted.  If they are designed right then they can be extended as many times as you wish to alter your persistence strategy ...

FooManager (interface)
FooManagerImplFilesystem implements FooManagerImpl
FooManagerImplSQLDB extends FooManagerImplFilesystem
FooManagerImplCache extends FooManagerImplSQLDB

and on and on.  whatever objects you persist may have as many application logic methods as needed, but to persist objects you go through a manager.

anyways, i'm rambling now, but in the end i don't think we have any immediate need to change the PersistentObject class.

-- Allen


On Tue, 2005-07-12 at 15:14, Dave Johnson wrote:
> I've rethought my position on this one.
> 
> I'm -1 on removing persistence and application logic methods from the 
> persistent object (PO) classes because I don't believe we gain anything 
> by making he change.
> 
> Here's why:
> 
> Originally, the POs were just Data Transfer Objects (a workaround 
> pattern for EJB) and they were generated by XDoclet. They were just 
> dumb JavaBeans. We decided that we wanted those objects to become 
> full-fledged objects so that we could use associations and inheritance 
> and other object oriented concepts. We stopped generating the *Data 
> classes and started putting application logic and associations into 
> those objects.
> 
> What do we gain by moving methods out of the objects to which the 
> belong?
> 
> Why move from this:
> 
>     MyManager mgr = MyManagerFactory.getMyManager();
>     MyObject a = mgr.getMyObject();
>     a.doSomeStuff();
> 
> To this:
> 
>     MyManager mgr = MyManagerFactory.getMyManager();
>     MyObject a = mgr.getMyObject();
>     mgr.doSomeStuff(a);
> 
> 
> I don't think we gain anything and what's more, we lose the opportunity 
> to use inheritance/polymorphism. I'd like to have as close to a real 
> object model as possible. When you need to remove an object you call 
> its remove method. What if you want to override doSomeStuff() in some 
> of your objects? Do you put a but if-then-else control in there and 
> call instanceof? Actually, that's not a hypothetical question. We 
> already have a object.canSave() method with different behaviors for 
> different objects.
> 
> We'll still need manager classes, but only for object creation and 
> queries (just like an EJBHome interface). That was our direction before 
> so why should we change it?
> 
> With that approach we still have good persistence engine independence, 
> which was always one of our goals (i.e. we supported both Castor JDO 
> and Hibernate). And, our objects are protected from malicious user 
> calls from Velocity by Allen's new object wrappers.
> 
> 
> So, change my mind. Tell me what are the specific benefits of this 
> refactoring?
> 
> - Dave
> 
> 
> 
> 
> On Jun 30, 2005, at 1:50 PM, Allen Gilliland wrote:
> 
> > Okay, so it sounds like a few other people have given this a little 
> > thought and think that it may be beneficial to make some changes to 
> > the way the Pojos and PersistentObjects work.  I think it would help 
> > to add a little more detail to the discussion so we know what we are 
> > talking about.  Here's my stab at what changes I would think about 
> > making ...
> >
> > - move PersistentObject.save() into Manager classes only
> > - move PersistentObject.remove() into Manager classes only
> >
> > I think those 2 changes would go a long ways toward making it less 
> > dangerous to make Pojos directly available to users via the velocity 
> > context.  I am in partial agreement that we may not need the 
> > PersistentObject class at all.  Right now I would also consider doing 
> > ...
> >
> > - remove PersistentObject.get/setId() (these are not necessarily part 
> > of all objects)
> > - remove PersistentObject.setData() (this can easily be done elsewhere)
> > - remove PersistentObject.canSave() (i don't fully understand how this 
> > is used, but i believe this logic can be in the Manager classes 
> > save/remove methods)
> >
> > If we also want to do those last few items then the PersistentObject 
> > class would basically be useless.  I think the first 2 are pretty 
> > important, but the last 3 are optional.  Personally I would probably 
> > go ahead and ditch the PersistentObject class just because I don't 
> > think we really need it.
> >
> > what do others think?
> >
> > Remember, we are just talking about this right now so please speak up 
> > and voice your opinion.  We aren't going to make any changes right 
> > away, especially with the fact that Dave has a lot of data model work 
> > going on for the 2.0 release and we don't want to mess with what he 
> > has done so far.  Once we get a bit more consenus then I will 
> > formalize a Proposal that can be reviewed again.
> >
> > -- Allen
> >
> >
> > On Thu, 2005-06-30 at 10:12, Rudman Max wrote:
> >> I just wanted to chime in that I really dislike persistence methods
> >> being in POJOs also and would be willing to pitch in with moving
> >> those out to the appropriate manager classes. In fact, I would even
> >> like to see PersistenceObject go as having to extend data objects
> >> from it pretty much negates one of the key benefits of Hibernate --
> >> its non-intrusiveness into the object model.
> >>
> >> Max
> >>
> >> On Jun 30, 2005, at 11:53 AM, Anil Gangolli wrote:
> >>
> >>>
> >>> The remove() method is used in several cases to do some of the
> >>> cascading needed to maintain consistency properties.  Just make
> >>> sure to preserve that logic; if you take out the remove() methods,
> >>> this logic needs to be moved into the corresponding manager methods.
> >>>
> >>> --a.
> >>>
> >>>
> >>> ----- Original Message ----- From: "Lance Lavandowska"
> >>> <la...@gmail.com>
> >>> To: <ro...@incubator.apache.org>
> >>> Sent: Thursday, June 30, 2005 8:24 AM
> >>> Subject: Re: velocity context cleanup
> >>>
> >>>
> >>>
> >>>> On 6/29/05, Allen Gilliland <Al...@sun.com> wrote:
> >>>>
> >>>>
> >>>>> *Data.remove() is available to users (try $website.remove() in a
> >>>>> template)
> >>>>>
> >>>>
> >>>> This method should probably be removed from the classes.  While I
> >>>> think even POJOs should contain some business logic, I don't feel
> >>>> that
> >>>> persistence-related methods are appropriate.  Because this is only 
> >>>> my
> >>>> personal gut-check, I've never objected.
> >>>>
> >>>>
> >>>>> PageHelper.evaluateString() is available to users (this one
> >>>>> actually bit us in the ass already and a user caught themself in
> >>>>> a recursive loop which killed the server)
> >>>>>
> >>>>
> >>>> I'm the one guilty of creating that monstrosity, and I say "get
> >>>> rid of
> >>>> it".  I doubt it is in my real use - but you may break a few pages 
> >>>> by
> >>>> removing it.  Perhaps change it to print "THIS MACRO HAS BEEN
> >>>> REMOVED"?  Note: this is a misguided macro, not a Context value.
> >>>>
> >>>>
> >>>>> Some of these may be a simple case of updating the public,
> >>>>> protected, private access levels on methods, but some cases may
> >>>>> mean removing objects from the Context and/or removing methods
> >>>>> from objects that are part of the Context.
> >>>>>
> >>>>
> >>>> All of the objects placed into the Context are done so to achieve an
> >>>> objective in the *.vm templates or the Page templates.  As I implied
> >>>> above, let's look at what is being exposed by these objects that may
> >>>> be 'dangerous' instead.
> >>>>
> >>>> Lance
> >>>
> >>
> >
>