You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@roller.apache.org by Dave <sn...@gmail.com> on 2007/01/18 19:30:36 UTC
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk: src/org/apache/roll...
Hi Mitesh,
I've CC'ed the roller-dev list on this. Others might want to comment.
> On Jan 18, 2007, at 1:10 PM, Mitesh Meswani wrote:
> + // Form bean workaround: empty string is never a valid id
> + if (id != null && id.trim().length() == 0) return;
>
> I am curious what does the comment mean? Can you please give me some more details.
I'm not sure that is the best solution, but the problem is that the
Struts form beans all use a generated copyTo() method to copy fields
from the web form to the POJO object. Currently, the form beans are
not smart enough to know that the empty string is not a valid ID so
they copy the empty string into the ID field of each object. That
confuses Hibernate's saveOrUpdate() method -- it sees a non-null ID
and considers the object to be an already existing object in the
database.
A better solution might be to change the generated form beans so that
they are smarter about ID fields (or ditch generated form beans and
upgrade to Struts2), but I wasn't ready to fight XDoclet to make that
happen (yet).
Any opinions / suggestions on this problem?
- Dave
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk: src/org/apache/roll...
Posted by Dave <sn...@gmail.com>.
On 1/18/07, Allen Gilliland <al...@sun.com> wrote:
> Regardless of what's actually happening, the correct way (which we
> *must* fix) is to fix the way the current StrutsForm -> Pojo mapping
> happens and make sure that id values cannot be copied. As Mitesh said,
> we should *never* allow an id on a pojo to be manually set when that
> pojo is allowing the framework to manage the id. In fact, in these
> cases I think the correct thing to do is to make the setId() method
> private, which is actually suggested in the Hibernate In Action book
> somewhere.
>
> So what this entails is modifying the pojos to make setId() private
OK. I will do that ASAP.
> fixing the way the struts actions/forms work to make sure all actions
> which modify an existing pojo actually query for that pojo first, then
> mutate it.
I don't think that's an issue any more, but we should check.Now that
we're using the simplified store() method, any parts of the UI that
still do that are now broken.
> >> Technically, the call to saveFoo() isn't necessary with Hibernate or
> >> JPA because FooData is a persistent/managed object, but perhaps other
> >> persistence frameworks (iBatis?) might beed that final call to
> >> FooManager.saveFoo().
> > We should distinguish between save and update. Thus in above case
> > instead of calling FooManager.saveFoo(), we should call
> > FooManager.updateFoo() which will be implemented as a noop for JPA or
> > Hibernate. Other frameworks can implement it as required.
>
> I disagree. There is no real reason why we must treat saves and updates
> separately. I think this is something that the framework can take care
> of on it's own and it's actually handled in a very simplistic way ...
>
> if(object.id != null)
> update()
> else
> save()
>
> and the framework manages what attribute of the pojo is the id through
> its mapping files. so I don't think we need to do this.
Yes. I believe you are correct Allen. In our current architecture,
there is no reason to support the saving of detached objects. So we
can assume that an object with an ID is a "managed" object.
- Dave
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk:
src/org/apache/roll...
Posted by Mitesh Meswani <Mi...@Sun.COM>.
Allen Gilliland wrote:
>
>
> Mitesh Meswani wrote:
>>
>>
>> Allen Gilliland wrote:
>>>
>>>
>>> Mitesh Meswani wrote:
>>>>
>>>>
>>>> Dave wrote:
>>>>> On 1/18/07, Mitesh Meswani <Mi...@sun.com> wrote:
>>>>>> > if(object.id != null)
>>>>>> > update()
>>>>>> > else
>>>>>> > save()
>>>>>
>>>>>> This would be true if the id is generated as is the case with
>>>>>> Roller.
>>>>>> For objects where id is user supplied, above algorithm can not be
>>>>>> used.
>>>>>> Thats the reason some frameworks (for example JPA) does not provide
>>>>>> equivalent of saveOrUpdate() method.
>>>>>
>>>>> Yes, that is true and it is the case with the property object, whose
>>>>> ID is the name property.
>>>>>
>>>>> And, we can't rely on object.id because we don't know which field is
>>>>> the id. So, for JPA you'd have to do something like this in the store
>>>>> method:
>>>>>
>>>>> EntityManager em = getEntityManager(true);
>>>>> if (!em.contains(obj)) {
>>>>> // If entity is not managed we can assume it is new
>>>>> em.persist(obj);
>>>>> }
>>>>> return obj;
>>>> Above has a performance penalty to pay and it assumes that no
>>>> detached object will be ever passed to store(). Currently, I don't
>>>> think that is true. For example, I found following with a quick search
>>>> Class MaintenanceAction {
>>>>
>>>> ....
>>>> public ActionForward flushCache(
>>>> ActionMapping mapping,
>>>> ActionForm actionForm,
>>>> HttpServletRequest request,
>>>> HttpServletResponse response)
>>>> throws IOException, ServletException {
>>>> try {
>>>> RollerRequest rreq =
>>>> RollerRequest.getRollerRequest(request);
>>>> -----> WebsiteData website = rreq.getWebsite(); // I think this
>>>> is a detached object
>>>> RollerSession rses =
>>>> RollerSession.getRollerSession(request);
>>>> if ( rses.isUserAuthorizedToAdmin(website) ) {
>>>> // some caches are based on weblog
>>>> last-modified, so update it
>>>> website.setLastModified(new Date());
>>>> try {
>>>> UserManager umgr =
>>>> RollerFactory.getRoller().getUserManager();
>>>> -----> umgr.saveWebsite(website); // The save
>>>> will not work as it will end up calling persist
>>>
>>>
>>> Actually, that shouldn't be a detached object because the
>>> RollerRequest class should be querying for the object on each
>>> request.In any case though, the main point is as Dave suggested
>>> before, we will *NOT* support the saving of detached objects anymore
>>> and anywhere that does it now is considered a bug and needs to be
>>> fixed.
>> Great. So, the approach suggested by Dave should work. I remember
>> seeing detached objects being used in the test code which we can fix
>> as required.
>>
>> But, I still think that the code will be cleaner if we distinguish
>> between saving a new object vs. updating an existing object. Most of
>> the calls to Manager#save() are on managed object and can be removed.
>> The JPA implementation currently use the information about whether
>> save is called for insert or update to maintain relationships as below.
>> Class DatamapperWeblogManagerImpl {
>> .....
>> public void saveWeblogCategory(WeblogCategoryData cat)
>> throws RollerException {
>> ......
>> if(!PersistentObjectHelper.isObjectPersistent(cat)) { <<-- A
>> workaround to findout whether we are here for insert or update
>> // Newly added object. If it has a parent,
>> // maintain relationship from both sides
>> WeblogCategoryData parent = cat.getParent();
>> if(parent != null) {
>> parent.getWeblogCategories().add(cat);
>> }
>> }
>> We will need to find another workaround for above if the save() call
>> comes for both insert and update
>
>
> I suppose it's debatable which approach is cleaner, but my preference
> is to maintain the current code the way it is and I think that will
> work fine for any backend implementation.
>
> One thing that I am not okay with is removing the need for a call to
> XXXManager.save() for updating managed objects. The problem with
> doing that is that it requires the backend implementation to use some
> kind of ORM type object management solution. i.e. If I wanted to take
> the current Roller code and rip out Hibernate and create my own
> manager impls using raw jdbc coding then that is possible right now
> and it wouldn't be if we removed calls to save() for all update action.
I see the point about not doing away with save(). But it is also not a
good idea to depend on the "magic" that saveOrUpdate() performs to
determine whether its an insert or update to database. For example, the
manager using raw jdbc will either need a trip to database or maintain
some cache to determine whether to issue an insert or update in response
to save(). How about separating save() into two methods say persist() to
insert objects and update() to update objects and call appropriate
method from the frontend.
>
> For your example, I think we should look more closely at the need for
> maintaining the relationship on both ends, but regardless of that I
> think the example may not be doing the right thing. When I want to
> add a new category X to a parent category Y then I think the proper
> code is ...
>
> Category X = new Category();
> Y.addCategory(X); (this triggers getWeblogCategories().add(X) and
> X.setParent(Y))
> mgr.saveCategory(Y);
>
> As I understand it, that is how Hibernate and other ORM solutions want
> you to handle the persistence of parent/child relationships.
I agree. The code in Datamapper*Manager is a workaround. Current roller
frontend code needs to change to use above pattern
-Mitesh
>
> -- Allen
>
>
>>
>> -Mitesh
>>
>>>
>>> -- Allen
>>>
>>>
>>>>
>>>> -Mitesh
>>>>>
>>>>> - Dave
>>>>
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk:
src/org/apache/roll...
Posted by Allen Gilliland <al...@sun.com>.
Mitesh Meswani wrote:
>
>
> Allen Gilliland wrote:
>>
>>
>> Mitesh Meswani wrote:
>>>
>>>
>>> Dave wrote:
>>>> On 1/18/07, Mitesh Meswani <Mi...@sun.com> wrote:
>>>>> > if(object.id != null)
>>>>> > update()
>>>>> > else
>>>>> > save()
>>>>
>>>>> This would be true if the id is generated as is the case with Roller.
>>>>> For objects where id is user supplied, above algorithm can not be
>>>>> used.
>>>>> Thats the reason some frameworks (for example JPA) does not provide
>>>>> equivalent of saveOrUpdate() method.
>>>>
>>>> Yes, that is true and it is the case with the property object, whose
>>>> ID is the name property.
>>>>
>>>> And, we can't rely on object.id because we don't know which field is
>>>> the id. So, for JPA you'd have to do something like this in the store
>>>> method:
>>>>
>>>> EntityManager em = getEntityManager(true);
>>>> if (!em.contains(obj)) {
>>>> // If entity is not managed we can assume it is new
>>>> em.persist(obj);
>>>> }
>>>> return obj;
>>> Above has a performance penalty to pay and it assumes that no
>>> detached object will be ever passed to store(). Currently, I don't
>>> think that is true. For example, I found following with a quick search
>>> Class MaintenanceAction {
>>>
>>> ....
>>> public ActionForward flushCache(
>>> ActionMapping mapping,
>>> ActionForm actionForm,
>>> HttpServletRequest request,
>>> HttpServletResponse response)
>>> throws IOException, ServletException {
>>> try {
>>> RollerRequest rreq =
>>> RollerRequest.getRollerRequest(request);
>>> -----> WebsiteData website = rreq.getWebsite(); // I think this
>>> is a detached object
>>> RollerSession rses = RollerSession.getRollerSession(request);
>>> if ( rses.isUserAuthorizedToAdmin(website) ) {
>>> // some caches are based on weblog
>>> last-modified, so update it
>>> website.setLastModified(new Date());
>>> try {
>>> UserManager umgr =
>>> RollerFactory.getRoller().getUserManager();
>>> -----> umgr.saveWebsite(website); // The save will
>>> not work as it will end up calling persist
>>
>>
>> Actually, that shouldn't be a detached object because the
>> RollerRequest class should be querying for the object on each
>> request.In any case though, the main point is as Dave suggested
>> before, we will *NOT* support the saving of detached objects anymore
>> and anywhere that does it now is considered a bug and needs to be fixed.
> Great. So, the approach suggested by Dave should work. I remember seeing
> detached objects being used in the test code which we can fix as required.
>
> But, I still think that the code will be cleaner if we distinguish
> between saving a new object vs. updating an existing object. Most of the
> calls to Manager#save() are on managed object and can be removed.
> The JPA implementation currently use the information about whether save
> is called for insert or update to maintain relationships as below.
> Class DatamapperWeblogManagerImpl {
> .....
> public void saveWeblogCategory(WeblogCategoryData cat)
> throws RollerException {
> ......
> if(!PersistentObjectHelper.isObjectPersistent(cat)) { <<-- A
> workaround to findout whether we are here for insert or update
> // Newly added object. If it has a parent,
> // maintain relationship from both sides
> WeblogCategoryData parent = cat.getParent();
> if(parent != null) {
> parent.getWeblogCategories().add(cat);
> }
> }
> We will need to find another workaround for above if the save() call
> comes for both insert and update
I suppose it's debatable which approach is cleaner, but my preference is
to maintain the current code the way it is and I think that will work
fine for any backend implementation.
One thing that I am not okay with is removing the need for a call to
XXXManager.save() for updating managed objects. The problem with doing
that is that it requires the backend implementation to use some kind of
ORM type object management solution. i.e. If I wanted to take the
current Roller code and rip out Hibernate and create my own manager
impls using raw jdbc coding then that is possible right now and it
wouldn't be if we removed calls to save() for all update action.
For your example, I think we should look more closely at the need for
maintaining the relationship on both ends, but regardless of that I
think the example may not be doing the right thing. When I want to add
a new category X to a parent category Y then I think the proper code is ...
Category X = new Category();
Y.addCategory(X); (this triggers getWeblogCategories().add(X) and
X.setParent(Y))
mgr.saveCategory(Y);
As I understand it, that is how Hibernate and other ORM solutions want
you to handle the persistence of parent/child relationships.
-- Allen
>
> -Mitesh
>
>>
>> -- Allen
>>
>>
>>>
>>> -Mitesh
>>>>
>>>> - Dave
>>>
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk:
src/org/apache/roll...
Posted by Mitesh Meswani <Mi...@Sun.COM>.
Allen Gilliland wrote:
>
>
> Mitesh Meswani wrote:
>>
>>
>> Dave wrote:
>>> On 1/18/07, Mitesh Meswani <Mi...@sun.com> wrote:
>>>> > if(object.id != null)
>>>> > update()
>>>> > else
>>>> > save()
>>>
>>>> This would be true if the id is generated as is the case with Roller.
>>>> For objects where id is user supplied, above algorithm can not be
>>>> used.
>>>> Thats the reason some frameworks (for example JPA) does not provide
>>>> equivalent of saveOrUpdate() method.
>>>
>>> Yes, that is true and it is the case with the property object, whose
>>> ID is the name property.
>>>
>>> And, we can't rely on object.id because we don't know which field is
>>> the id. So, for JPA you'd have to do something like this in the store
>>> method:
>>>
>>> EntityManager em = getEntityManager(true);
>>> if (!em.contains(obj)) {
>>> // If entity is not managed we can assume it is new
>>> em.persist(obj);
>>> }
>>> return obj;
>> Above has a performance penalty to pay and it assumes that no
>> detached object will be ever passed to store(). Currently, I don't
>> think that is true. For example, I found following with a quick search
>> Class MaintenanceAction {
>>
>> ....
>> public ActionForward flushCache(
>> ActionMapping mapping,
>> ActionForm actionForm,
>> HttpServletRequest request,
>> HttpServletResponse response)
>> throws IOException, ServletException {
>> try {
>> RollerRequest rreq =
>> RollerRequest.getRollerRequest(request);
>> -----> WebsiteData website = rreq.getWebsite(); // I think this
>> is a detached object
>> RollerSession rses = RollerSession.getRollerSession(request);
>> if ( rses.isUserAuthorizedToAdmin(website) ) {
>> // some caches are based on weblog
>> last-modified, so update it
>> website.setLastModified(new Date());
>> try {
>> UserManager umgr =
>> RollerFactory.getRoller().getUserManager();
>> -----> umgr.saveWebsite(website); // The save will
>> not work as it will end up calling persist
>
>
> Actually, that shouldn't be a detached object because the
> RollerRequest class should be querying for the object on each
> request.In any case though, the main point is as Dave suggested
> before, we will *NOT* support the saving of detached objects anymore
> and anywhere that does it now is considered a bug and needs to be fixed.
Great. So, the approach suggested by Dave should work. I remember seeing
detached objects being used in the test code which we can fix as required.
But, I still think that the code will be cleaner if we distinguish
between saving a new object vs. updating an existing object. Most of the
calls to Manager#save() are on managed object and can be removed.
The JPA implementation currently use the information about whether save
is called for insert or update to maintain relationships as below.
Class DatamapperWeblogManagerImpl {
.....
public void saveWeblogCategory(WeblogCategoryData cat)
throws RollerException {
......
if(!PersistentObjectHelper.isObjectPersistent(cat)) { <<-- A
workaround to findout whether we are here for insert or update
// Newly added object. If it has a parent,
// maintain relationship from both sides
WeblogCategoryData parent = cat.getParent();
if(parent != null) {
parent.getWeblogCategories().add(cat);
}
}
We will need to find another workaround for above if the save() call
comes for both insert and update
-Mitesh
>
> -- Allen
>
>
>>
>> -Mitesh
>>>
>>> - Dave
>>
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk:
src/org/apache/roll...
Posted by Allen Gilliland <al...@sun.com>.
Mitesh Meswani wrote:
>
>
> Dave wrote:
>> On 1/18/07, Mitesh Meswani <Mi...@sun.com> wrote:
>>> > if(object.id != null)
>>> > update()
>>> > else
>>> > save()
>>
>>> This would be true if the id is generated as is the case with Roller.
>>> For objects where id is user supplied, above algorithm can not be used.
>>> Thats the reason some frameworks (for example JPA) does not provide
>>> equivalent of saveOrUpdate() method.
>>
>> Yes, that is true and it is the case with the property object, whose
>> ID is the name property.
>>
>> And, we can't rely on object.id because we don't know which field is
>> the id. So, for JPA you'd have to do something like this in the store
>> method:
>>
>> EntityManager em = getEntityManager(true);
>> if (!em.contains(obj)) {
>> // If entity is not managed we can assume it is new
>> em.persist(obj);
>> }
>> return obj;
> Above has a performance penalty to pay and it assumes that no detached
> object will be ever passed to store(). Currently, I don't think that is
> true. For example, I found following with a quick search
> Class MaintenanceAction {
>
> ....
> public ActionForward flushCache(
> ActionMapping mapping,
> ActionForm actionForm,
> HttpServletRequest request,
> HttpServletResponse response)
> throws IOException, ServletException {
> try {
> RollerRequest rreq = RollerRequest.getRollerRequest(request);
> -----> WebsiteData website = rreq.getWebsite(); // I think this is a
> detached object
> RollerSession rses = RollerSession.getRollerSession(request);
> if ( rses.isUserAuthorizedToAdmin(website) ) {
> // some caches are based on weblog
> last-modified, so update it
> website.setLastModified(new Date());
> try {
> UserManager umgr =
> RollerFactory.getRoller().getUserManager();
> -----> umgr.saveWebsite(website); // The save will
> not work as it will end up calling persist
Actually, that shouldn't be a detached object because the RollerRequest
class should be querying for the object on each request. In any case
though, the main point is as Dave suggested before, we will *NOT*
support the saving of detached objects anymore and anywhere that does it
now is considered a bug and needs to be fixed.
-- Allen
>
> -Mitesh
>>
>> - Dave
>
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk:
src/org/apache/roll...
Posted by Mitesh Meswani <Mi...@Sun.COM>.
Dave wrote:
> On 1/18/07, Mitesh Meswani <Mi...@sun.com> wrote:
>> > if(object.id != null)
>> > update()
>> > else
>> > save()
>
>> This would be true if the id is generated as is the case with Roller.
>> For objects where id is user supplied, above algorithm can not be used.
>> Thats the reason some frameworks (for example JPA) does not provide
>> equivalent of saveOrUpdate() method.
>
> Yes, that is true and it is the case with the property object, whose
> ID is the name property.
>
> And, we can't rely on object.id because we don't know which field is
> the id. So, for JPA you'd have to do something like this in the store
> method:
>
> EntityManager em = getEntityManager(true);
> if (!em.contains(obj)) {
> // If entity is not managed we can assume it is new
> em.persist(obj);
> }
> return obj;
Above has a performance penalty to pay and it assumes that no detached
object will be ever passed to store(). Currently, I don't think that is
true. For example, I found following with a quick search
Class MaintenanceAction {
....
public ActionForward flushCache(
ActionMapping mapping,
ActionForm actionForm,
HttpServletRequest request,
HttpServletResponse response)
throws IOException, ServletException {
try {
RollerRequest rreq = RollerRequest.getRollerRequest(request);
-----> WebsiteData website = rreq.getWebsite(); // I think this is a
detached object
RollerSession rses = RollerSession.getRollerSession(request);
if ( rses.isUserAuthorizedToAdmin(website) ) {
// some caches are based on weblog last-modified, so
update it
website.setLastModified(new Date());
try {
UserManager umgr =
RollerFactory.getRoller().getUserManager();
-----> umgr.saveWebsite(website); // The save will
not work as it will end up calling persist
-Mitesh
>
> - Dave
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk:
src/org/apache/roll...
Posted by Allen Gilliland <al...@sun.com>.
Dave wrote:
> On 1/18/07, Mitesh Meswani <Mi...@sun.com> wrote:
>> > if(object.id != null)
>> > update()
>> > else
>> > save()
>
>> This would be true if the id is generated as is the case with Roller.
>> For objects where id is user supplied, above algorithm can not be used.
>> Thats the reason some frameworks (for example JPA) does not provide
>> equivalent of saveOrUpdate() method.
>
> Yes, that is true and it is the case with the property object, whose
> ID is the name property.
>
> And, we can't rely on object.id because we don't know which field is
> the id. So, for JPA you'd have to do something like this in the store
> method:
>
> EntityManager em = getEntityManager(true);
> if (!em.contains(obj)) {
> // If entity is not managed we can assume it is new
> em.persist(obj);
> }
> return obj;
I agree. I think the example above is a very appropriate way to deal
with scenarios for user managed pojo ids like the PropertyData class.
-- Allen
>
> - Dave
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk: src/org/apache/roll...
Posted by Dave <sn...@gmail.com>.
On 1/18/07, Mitesh Meswani <Mi...@sun.com> wrote:
> > if(object.id != null)
> > update()
> > else
> > save()
> This would be true if the id is generated as is the case with Roller.
> For objects where id is user supplied, above algorithm can not be used.
> Thats the reason some frameworks (for example JPA) does not provide
> equivalent of saveOrUpdate() method.
Yes, that is true and it is the case with the property object, whose
ID is the name property.
And, we can't rely on object.id because we don't know which field is
the id. So, for JPA you'd have to do something like this in the store
method:
EntityManager em = getEntityManager(true);
if (!em.contains(obj)) {
// If entity is not managed we can assume it is new
em.persist(obj);
}
return obj;
- Dave
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk:
src/org/apache/roll...
Posted by Mitesh Meswani <Mi...@Sun.COM>.
Allen Gilliland wrote:
> I have a couple comments below ...
>
>
> Mitesh Meswani wrote:
>>
>>
>> Dave wrote:
>>> On 1/18/07, Mitesh Meswani <Mi...@sun.com> wrote:
>>>> I think it is not a good idea to mutate Primary Key (ID) field of
>>>> any pojo.
>>>
>>> I agree.
>>>
>>>> Many persistence providers might choke if a pk field is mutated.
>>>> Pardon my ignorance on Struts - How are these forms used at
>>>> runtime? Do
>>>> they correspond to any screen in UI?
>>>
>>> Yes, here's how it works:
>>>
>>> When a HTTP POST comes in to update a FooData object, Struts hands us
>>> a FooForm object containing the data from the posted web form.
>>>
>>> We use FooForm.id to load the corresponding FooData object from the
>>> database, use FooForm.copyTo() to copy all updated properties to the
>>> FooData object
>> Thanks. Now I understand :)
>
> I think this not entirely true. I think the reason the old store()
> method was wacky is specifically because this is not how it was
> happening.
>
> What happens when a form is posted is that the form copied *all* data
> into a new (non-persistent) pojo object, *including* the id value. So
> the struts actions would often not actually query for a pojo and
> mutate it, instead they would construct a new pojo and copy all
> attributes of the pojo from the form, which is not a good way to do
> things.
>
> I am suspecting that you are getting around this problem now by having
> the forms do the same thing and Hibernate's saveOrUpdate() method is
> being clever and doing the same thing the old store() method did.
> i.e. if you try and save a pojo that is not currently managed by
> Hibernate then it tries to use the id value to load that object, then
> update it.
>
> Regardless of what's actually happening, the correct way (which we
> *must* fix) is to fix the way the current StrutsForm -> Pojo mapping
> happens and make sure that id values cannot be copied. As Mitesh
> said, we should *never* allow an id on a pojo to be manually set when
> that pojo is allowing the framework to manage the id. In fact, in
> these cases I think the correct thing to do is to make the setId()
> method private, which is actually suggested in the Hibernate In Action
> book somewhere.
>
> So what this entails is modifying the pojos to make setId() private
> and fixing the way the struts actions/forms work to make sure all
> actions which modify an existing pojo actually query for that pojo
> first, then mutate it.
>
>
>>> and then we call FooManager.saveFoo() to persist those
>>> chances.
>>>
>>> Technically, the call to saveFoo() isn't necessary with Hibernate or
>>> JPA because FooData is a persistent/managed object, but perhaps other
>>> persistence frameworks (iBatis?) might beed that final call to
>>> FooManager.saveFoo().
>> We should distinguish between save and update. Thus in above case
>> instead of calling FooManager.saveFoo(), we should call
>> FooManager.updateFoo() which will be implemented as a noop for JPA or
>> Hibernate. Other frameworks can implement it as required.
>
> I disagree. There is no real reason why we must treat saves and
> updates separately. I think this is something that the framework can
> take care of on it's own and it's actually handled in a very
> simplistic way ...
>
> if(object.id != null)
> update()
> else
> save()
This would be true if the id is generated as is the case with Roller.
For objects where id is user supplied, above algorithm can not be used.
Thats the reason some frameworks (for example JPA) does not provide
equivalent of saveOrUpdate() method.
-Mitesh
>
> and the framework manages what attribute of the pojo is the id through
> its mapping files. so I don't think we need to do this.
>
> -- Allen
>
>
>>
>> Regards,
>> Mitesh
>>>
>>> - Dave
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk:
src/org/apache/roll...
Posted by Allen Gilliland <al...@sun.com>.
I have a couple comments below ...
Mitesh Meswani wrote:
>
>
> Dave wrote:
>> On 1/18/07, Mitesh Meswani <Mi...@sun.com> wrote:
>>> I think it is not a good idea to mutate Primary Key (ID) field of
>>> any pojo.
>>
>> I agree.
>>
>>> Many persistence providers might choke if a pk field is mutated.
>>> Pardon my ignorance on Struts - How are these forms used at runtime? Do
>>> they correspond to any screen in UI?
>>
>> Yes, here's how it works:
>>
>> When a HTTP POST comes in to update a FooData object, Struts hands us
>> a FooForm object containing the data from the posted web form.
>>
>> We use FooForm.id to load the corresponding FooData object from the
>> database, use FooForm.copyTo() to copy all updated properties to the
>> FooData object
> Thanks. Now I understand :)
I think this not entirely true. I think the reason the old store()
method was wacky is specifically because this is not how it was happening.
What happens when a form is posted is that the form copied *all* data
into a new (non-persistent) pojo object, *including* the id value. So
the struts actions would often not actually query for a pojo and mutate
it, instead they would construct a new pojo and copy all attributes of
the pojo from the form, which is not a good way to do things.
I am suspecting that you are getting around this problem now by having
the forms do the same thing and Hibernate's saveOrUpdate() method is
being clever and doing the same thing the old store() method did. i.e.
if you try and save a pojo that is not currently managed by Hibernate
then it tries to use the id value to load that object, then update it.
Regardless of what's actually happening, the correct way (which we
*must* fix) is to fix the way the current StrutsForm -> Pojo mapping
happens and make sure that id values cannot be copied. As Mitesh said,
we should *never* allow an id on a pojo to be manually set when that
pojo is allowing the framework to manage the id. In fact, in these
cases I think the correct thing to do is to make the setId() method
private, which is actually suggested in the Hibernate In Action book
somewhere.
So what this entails is modifying the pojos to make setId() private and
fixing the way the struts actions/forms work to make sure all actions
which modify an existing pojo actually query for that pojo first, then
mutate it.
>> and then we call FooManager.saveFoo() to persist those
>> chances.
>>
>> Technically, the call to saveFoo() isn't necessary with Hibernate or
>> JPA because FooData is a persistent/managed object, but perhaps other
>> persistence frameworks (iBatis?) might beed that final call to
>> FooManager.saveFoo().
> We should distinguish between save and update. Thus in above case
> instead of calling FooManager.saveFoo(), we should call
> FooManager.updateFoo() which will be implemented as a noop for JPA or
> Hibernate. Other frameworks can implement it as required.
I disagree. There is no real reason why we must treat saves and updates
separately. I think this is something that the framework can take care
of on it's own and it's actually handled in a very simplistic way ...
if(object.id != null)
update()
else
save()
and the framework manages what attribute of the pojo is the id through
its mapping files. so I don't think we need to do this.
-- Allen
>
> Regards,
> Mitesh
>>
>> - Dave
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk:
src/org/apache/roll...
Posted by Mitesh Meswani <Mi...@Sun.COM>.
Dave wrote:
> On 1/18/07, Mitesh Meswani <Mi...@sun.com> wrote:
>> I think it is not a good idea to mutate Primary Key (ID) field of
>> any pojo.
>
> I agree.
>
>> Many persistence providers might choke if a pk field is mutated.
>> Pardon my ignorance on Struts - How are these forms used at runtime? Do
>> they correspond to any screen in UI?
>
> Yes, here's how it works:
>
> When a HTTP POST comes in to update a FooData object, Struts hands us
> a FooForm object containing the data from the posted web form.
>
> We use FooForm.id to load the corresponding FooData object from the
> database, use FooForm.copyTo() to copy all updated properties to the
> FooData object
Thanks. Now I understand :)
> and then we call FooManager.saveFoo() to persist those
> chances.
>
> Technically, the call to saveFoo() isn't necessary with Hibernate or
> JPA because FooData is a persistent/managed object, but perhaps other
> persistence frameworks (iBatis?) might beed that final call to
> FooManager.saveFoo().
We should distinguish between save and update. Thus in above case
instead of calling FooManager.saveFoo(), we should call
FooManager.updateFoo() which will be implemented as a noop for JPA or
Hibernate. Other frameworks can implement it as required.
Regards,
Mitesh
>
> - Dave
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk: src/org/apache/roll...
Posted by Dave <sn...@gmail.com>.
On 1/18/07, Mitesh Meswani <Mi...@sun.com> wrote:
> I think it is not a good idea to mutate Primary Key (ID) field of any pojo.
I agree.
> Many persistence providers might choke if a pk field is mutated.
> Pardon my ignorance on Struts - How are these forms used at runtime? Do
> they correspond to any screen in UI?
Yes, here's how it works:
When a HTTP POST comes in to update a FooData object, Struts hands us
a FooForm object containing the data from the posted web form.
We use FooForm.id to load the corresponding FooData object from the
database, use FooForm.copyTo() to copy all updated properties to the
FooData object and then we call FooManager.saveFoo() to persist those
chances.
Technically, the call to saveFoo() isn't necessary with Hibernate or
JPA because FooData is a persistent/managed object, but perhaps other
persistence frameworks (iBatis?) might beed that final call to
FooManager.saveFoo().
- Dave
Re: svn commit: r497459 [1/2] - in /incubator/roller/trunk:
src/org/apache/roll...
Posted by Mitesh Meswani <Mi...@Sun.COM>.
I think it is not a good idea to mutate Primary Key (ID) field of any
pojo. Many persistence providers might choke if a pk field is mutated.
Pardon my ignorance on Struts - How are these forms used at runtime? Do
they correspond to any screen in UI?
-Mitesh
Dave wrote:
> Hi Mitesh,
>
> I've CC'ed the roller-dev list on this. Others might want to comment.
>
>> On Jan 18, 2007, at 1:10 PM, Mitesh Meswani wrote:
>> + // Form bean workaround: empty string is never a valid id
>> + if (id != null && id.trim().length() == 0) return;
>>
>> I am curious what does the comment mean? Can you please give me some
>> more details.
>
> I'm not sure that is the best solution, but the problem is that the
> Struts form beans all use a generated copyTo() method to copy fields
> from the web form to the POJO object. Currently, the form beans are
> not smart enough to know that the empty string is not a valid ID so
> they copy the empty string into the ID field of each object. That
> confuses Hibernate's saveOrUpdate() method -- it sees a non-null ID
> and considers the object to be an already existing object in the
> database.
>
> A better solution might be to change the generated form beans so that
> they are smarter about ID fields (or ditch generated form beans and
> upgrade to Struts2), but I wasn't ready to fight XDoclet to make that
> happen (yet).
>
> Any opinions / suggestions on this problem?
>
> - Dave