i can do that, but org.roller.presentation.velocity.pojos *is* a new
sub-package. maybe org.roller.presentation.velocity.wrappers would be
more clear?
-- Allen
Lance Lavandowska wrote:
Just one suggestion, put the wrappers in a sub-package, perhaps
org.roller.presentation.velocity.pojos.wrappers ?
On 7/4/05, Allen Gilliland <[EMAIL PROTECTED]> wrote:
ok, I realize why this happens now. I wasn't thinking clearly on my
last message.
anyways, I verified on the velocity-user list that there isn't really a
way to deal with this other than creating wrapper classes, so it sounds
like that's what we'll have to do. I've tested this with some of the
changes I made for the theme management stuff and it's really not that
tough to do ... just requires extra code which is just a little annoying.
how about a convention like this which basically just contains getXXX()
methods ...
org.roller.presentation.velocity.pojos.<POJO>Wrapper
then when you put something in the velocity context you do ...
context.put("name", new <POJO>Wrapper(<POJO>));
I've created a TemplateWrapper class to try this out and it's working
great. If everyone is comfortable with this approach then I'd like to
keep working to get these wrappers implemented in the 1.3 release
because I think this is pretty important and relatively easy to do.
-- Allen
Allen Gilliland wrote:
hmmm ... well that's a bummer. i'd like to do some testing to double
check that though because that seems like a strange approach on their
part. part of the purpose of polymorphism is to hide methods in
certain cases and if velocity is bypassing that using reflection then
we would never be able to overcome that :(
-- Allen
Dave Johnson wrote:
I think that's a good idea, but (as far as I know) it won't keep
weblog template authors from accessing fields on WebsiteData.
Velocity uses reflection, so they won't be restricted to just the
Website methods/fields.
What's the best way to restrict template authors to just the methods
in the interface? Some sort of proxy class perhaps?
- Dave
On Jul 1, 2005, at 12:50 PM, Allen Gilliland wrote:
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
