okay, so it sounds like everyone agrees that this is a good thing to do at some
point. i'll go ahead and work on a more formal proposal so we can see exactly
what exact changes we would make.
in addition to removing some really dangerous methods like website.remove() i
think we'll also need a plan to prevent users from having access to
<object>.setXXX(). i've run the same tests as i did with website.remove() and
basically any user can set any property of any of the objects in the velocity
context to whatever they want and these changes will go directly back to the
db. while this would be a pretty stupid and non-productive thing for a user to
do it is still a potential risk that i think we should try and avoid.
what i am now thinking may be the best idea is to setup either interfaces or
base/abstract versions of each pojo that will go into the velocity context, and
then when we are doing the loading of the velocity context we can simply cast
the real pojos into their more limited versions. here's an example of what i
mean ...
- create interface Website which only contains getXXX() methods
- the velocity context gets, ctx.put("website", (Website) WebsiteDataObject)
- XXXManager classes still pass around XXXData objects as usual
this would mean we could control what methods are available to users via
Velocity without actually changing any of our existing pojos. the only work
would be to create the new interfaces, have each pojo implement it's respective
interface, then modify the context loading methods to cast the pojos into the
interface.
the other cool thing is that this is not intrusive on any existing
functionality, so i believe i could start adding this in for the 1.3 release
without causing any problems for Dave in his 2.0 branch.
what do people think?
-- Allen
On Fri, 2005-07-01 at 08:18, Anil Gangolli wrote:
> I agree with this general direction of separating the persistence behavior
> from the data holder beans; let pojos be pojos.
>
> Putting the logic in managers does necessitate the need always to know about
> the association between the class and its manager. In
> methods that deal with multiple different classes of objects we do persist,
> that can become an issue.
>
> However, if we decide we want to be able to deal with instances that know how
> to save themselves, or determining if the current user
> can save them, etc, that behavior should really be in classes that derive
> from or wrap/delegate to the pojo data holders, not the
> other way around as it is now.
>
> So Allen, +1 to this idea
>
> --a.
>
>
> ----- Original Message -----
> From: "David M Johnson" <[EMAIL PROTECTED]>
> To: <[email protected]>
> Sent: Friday, July 01, 2005 7:09 AM
> Subject: Re: velocity context cleanup
>
>
> > +1!!!
> >
> > I'm guilty of moving methods like save(), remove(), etc. into the POJOs. I
> > think everything you've outlined below is basically a
> > good idea, but I'd like to hold off on it until we merge 2.0 back in.
> >
> > - Dave
> >
> >
> > On Jun 30, 2005, at 10:50 AM, 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"
> >>>> <[EMAIL PROTECTED]>
> >>>> To: <[email protected]>
> >>>> Sent: Thursday, June 30, 2005 8:24 AM
> >>>> Subject: Re: velocity context cleanup
> >>>>
> >>>>
> >>>>
> >>>>> On 6/29/05, Allen Gilliland <[EMAIL PROTECTED]> 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
> >>>>
> >>>
> >
>
