Issues with updating a JPA entity from a Spring mvc controller method and using hidden input fields to store an ID

Question

Say I have an entity/javabean that has a substantial number of properties.

Furthermore, I have a html form (in jsp or thymeleaf) that I use to update that entity.

As my application stands here is how I proceed to update the entity:

I set the JPA ID of the entity in a hidden html field in the form
in the Spring controller, I retrieve the entity from the database using that hidden ID
still in the controller method I then set each field of the previously retrieved entity using the fields of the spring mvc ModelAttribute passed as an argument to the controller method.
I then persist/update the entity using the entityManager.

Here is a sample from my controller method:

@RequestMapping(value = "/family/advertisement/edit", method = RequestMethod.POST, produces = "text/html")
    public String editFamilyAdvertisement(@ModelAttribute @Validated(value = Validation.AdvertisementCreation.class) FamilyAdvertisement familyAdvertisement,
            BindingResult bindingResult, Model model) {
        FamilyAdvertisement advertisementForUpdate = advertisementService.findFamilyAdvertisement(familyAdvertisement.getId());
        if (bindingResult.hasErrors()) {
            populateModel(model, familyAdvertisement);
            return "family/advertisement/edit";
        }
        advertisementForUpdate.setNeeds(familyAdvertisement.getNeeds());
        advertisementForUpdate.setChildcareTypes(familyAdvertisement.getChildcareTypes());
        advertisementForUpdate.setDayToTimeSlots(familyAdvertisement.getDayToTimeSlots());
        ...
        advertisementService.editFamilyAdvertisement(advertisementForUpdate);
        return "redirect:/some/url";
    }

I have two problems with the application as it currently stands:

Firstly a clever hacker can easily tamper with the ID and update someone else's advertisement.
Secondly, I have to update each field of the attached entity manually using those from the spring mvc model attribute: this is tedious and ugly.

Can anyone please suggest a better pattern or solution?

edit 1: I followed the provided advice.

Here is my modified controller method:

@RequestMapping(value = "/family/advertisement/edit", method = RequestMethod.POST, produces = "text/html")
    public String editFamilyAdvertisement(@ModelAttribute @Validated(value = Validation.AdvertisementCreation.class) FamilyAdvertisementInfo familyAdvertisementInfo,
            BindingResult bindingResult, Model model) {
        Member member = memberService.retrieveCurrentMember();
        FamilyAdvertisement advertisementForCheck = advertisementService.findFamilyAdvertisement(familyAdvertisementInfo.getFamilyAdvertisement().getId());
        if (!member.getAdvertisements().contains(advertisementForCheck)) {
            throw new IllegalStateException("advertisement does not belong to member");
        }
        if (bindingResult.hasErrors()) {
            populateModel(model, familyAdvertisementInfo);
            return "family/advertisement/edit";
        }
        advertisementService.editFamilyAdvertisement(familyAdvertisementInfo.getFamilyAdvertisement());
        return "redirect:/family/advertisement/edit/" + familyAdvertisementInfo.getFamilyAdvertisement().getId();
    }

You see that I have to fetch the Family advertisement entity from db in order to check it belongs to the current member in session. Then when I try to save the entity as advised I get a StaleObjectStateException as follows:

SEVERE: Servlet.service() for servlet [bignibou] in context with path [/bignibou] threw exception [Request processing failed; nested exception is org.springframework.orm.jpa.JpaOptimisticLockingFailureException: org.hibernate.StaleObjectStateException: Row was updated or deleted by another transaction (or unsaved-value mapping was incorrect): [com.bignibou.domain.FamilyAdvertisement#1]; nested exception is javax.persistence.OptimisticLockException: org.hibernate.StaleObjectStateException: Row was updated or deleted by another transaction (or unsaved-value mapping was incorrect): [com.bignibou.domain.FamilyAdvertisement#1]] with root cause
org.hibernate.StaleObjectStateException: Row was updated or deleted by another transaction (or unsaved-value mapping was incorrect): [com.bignibou.domain.FamilyAdvertisement#1]
    at org.hibernate.event.internal.DefaultMergeEventListener.entityIsDetached(DefaultMergeEventListener.java:303)
    at org.hibernate.event.internal.DefaultMergeEventListener.onMerge(DefaultMergeEventListener.java:151)
    at org.hibernate.event.internal.DefaultMergeEventListener.onMerge(DefaultMergeEventListener.java:76)
    at org.hibernate.internal.SessionImpl.fireMerge(SessionImpl.java:903)
    at org.hibernate.internal.SessionImpl.merge(SessionImpl.java:887)
    at org.hibernate.internal.SessionImpl.merge(SessionImpl.java:891)
    at org.hibernate.ejb.AbstractEntityManagerImpl.merge(AbstractEntityManagerImpl.java:879)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:601)
    at org.springframework.orm.jpa.ExtendedEntityManagerCreator$ExtendedEntityManagerInvocationHandler.invoke(ExtendedEntityManagerCreator.java:366)
    at com.sun.proxy.$Proxy120.merge(Unknown Source)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:601)
    at org.springframework.orm.jpa.SharedEntityManagerCreator$SharedEntityManagerInvocationHandler.invoke(SharedEntityManagerCreator.java:241)
    at com.sun.proxy.$Proxy119.merge(Unknown Source)
    at org.springframework.data.jpa.repository.support.SimpleJpaRepository.save(SimpleJpaRepository.java:345)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:601)
    at org.springframework.data.repository.core.support.RepositoryFactorySupport$QueryExecutorMethodInterceptor.executeMethodOn(RepositoryFactorySupport.java:334)
    at org.springframework.data.repository.core.support.RepositoryFactorySupport$QueryExecutorMethodInterceptor.invoke(RepositoryFactorySupport.java:319)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocatio

edit 2: the issue I have if I don't fetch the entity from the db, is that the above call to contains is always going to evaluate to false because it uses the equals method internally and the entity may have changed (after all that's the purpose of the method).

if (!member.getAdvertisements().contains(familyAdvertisementInfo.getFamilyAdvertisement())) {
            throw new IllegalStateException("advertisement does not belong to member");
        }

edit 3:

I still have the same issue with the StaleObjectStateException because it seems that my controller method does two saves/transactions.

@RequestMapping(value = "/family/advertisement/edit", method = RequestMethod.POST, produces = "text/html")
    public String editFamilyAdvertisement(@ModelAttribute @Validated(value = Validation.AdvertisementCreation.class) FamilyAdvertisementInfo familyAdvertisementInfo,
            BindingResult bindingResult, Model model) {
        Member member = memberService.retrieveCurrentMember();//ONE
        if (!advertisementService.advertisementBelongsToMember(familyAdvertisementInfo.getFamilyAdvertisement(), member)) {
            throw new IllegalStateException("advertisement does not belong to member");
        }
        if (bindingResult.hasErrors()) {
            populateModel(model, familyAdvertisementInfo);
            return "family/advertisement/edit";
        }
        familyAdvertisementInfo.getFamilyAdvertisement().setMember(member);
        advertisementService.editFamilyAdvertisement(familyAdvertisementInfo.getFamilyAdvertisement());//TWO
        return "redirect:/family/advertisement/edit/" + familyAdvertisementInfo.getFamilyAdvertisement().getId();
    }

See exception:

org.hibernate.StaleObjectStateException: Row was updated or deleted by another transaction (or unsaved-value mapping was incorrect): [com.bignibou.domain.FamilyAdvertisement#1]
    org.hibernate.event.internal.DefaultMergeEventListener.entityIsDetached(DefaultMergeEventListener.java:303)
    org.hibernate.event.internal.DefaultMergeEventListener.onMerge(DefaultMergeEventListener.java:151)
    org.hibernate.event.internal.DefaultMergeEventListener.onMerge(DefaultMergeEventListener.java:76)
    org.hibernate.internal.SessionImpl.fireMerge(SessionImpl.java:903)
    org.hibernate.internal.SessionImpl.merge(SessionImpl.java:887)
    org.hibernate.internal.SessionImpl.merge(SessionImpl.java:891)
    org.hibernate.ejb.AbstractEntityManagerImpl.merge(AbstractEntityManagerImpl.java:879)
    sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    java.lang.reflect.Method.invoke(Method.java:601)
    org.springframework.orm.jpa.ExtendedEntityManagerCreator$ExtendedEntityManagerInvocationHandler.invoke(ExtendedEntityManagerCreator.java:366)
    com.sun.proxy.$Proxy123.merge(Unknown Source)
    sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    java.lang.reflect.Method.invoke(Method.java:601)
    org.springframework.orm.jpa.SharedEntityManagerCreator$SharedEntityManagerInvocationHandler.invoke(SharedEntityManagerCreator.java:241)
    com.sun.proxy.$Proxy122.merge(Unknown Source)
    org.springframework.data.jpa.repository.support.SimpleJpaRepository.save(SimpleJpaRepository.java:345)
    sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    java.lang.reflect.Method.invoke(Method.java:601)
    org.springframework.data.repository.core.support.RepositoryFactorySupport$QueryExecutorMethodInterceptor.executeMethodOn(RepositoryFactorySupport.java:334)
    org.springframework.data.repository.core.support.RepositoryFactorySupport$QueryExecutorMethodInterceptor.invoke(RepositoryFactorySupport.java:319)
    org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:110)
    org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    org.springframework.dao.support.PersistenceExceptionTranslationInterceptor.invoke(PersistenceExceptionTranslationInterceptor.java:155)
    org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    org.springframework.data.jpa.repository.support.LockModeRepositoryPostProcessor$LockModePopulatingMethodIntercceptor.invoke(LockModeRepositoryPostProcessor.java:91)
    org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    org.springframework.aop.interceptor.ExposeInvocationInterceptor.invoke(ExposeInvocationInterceptor.java:91)
    org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:204)
    com.sun.proxy.$Proxy129.save(Unknown Source)
    com.bignibou.service.AdvertisementServiceImpl_Roo_Service.ajc$interMethod$com_bignibou_service_AdvertisementServiceImpl_Roo_Service$com_bignibou_service_AdvertisementServiceImpl$updateFamilyAdvertisement(AdvertisementServiceImpl_Roo_Service.aj:58)
    com.bignibou.service.AdvertisementServiceImpl.updateFamilyAdvertisement(AdvertisementServiceImpl.java:1)
    com.bignibou.service.AdvertisementServiceImpl_Roo_Service.ajc$interMethodDispatch1$com_bignibou_service_AdvertisementServiceImpl_Roo_Service$com_bignibou_service_AdvertisementServiceImpl$updateFamilyAdvertisement(AdvertisementServiceImpl_Roo_Service.aj)
    com.bignibou.service.AdvertisementServiceImpl.editFamilyAdvertisement(AdvertisementServiceImpl.java:27)
    com.bignibou.controller.AdvertisementController.editFamilyAdvertisement(AdvertisementController.java:85)

Did you override the hashCode()/equals() methods? Maybe they should only use the id. — WilQu, Apr 03 '13 at 16:01
Using only the ID in hashcode/equals is not really recommended... — balteo, Apr 03 '13 at 16:06
Why is it not recommended? It seems to make sense in this case. But if that's a problem, then instead of calling contains() you can compare the IDs one by one. — WilQu, Apr 03 '13 at 16:12
See my edit #3. The `advertisementBelongsToMember` method does the comparison one by one... — balteo, Apr 03 '13 at 16:59
What does the editFamilyAdvertisement do? I think you should ask another question since it is not releted to your first questions. — WilQu, Apr 04 '13 at 07:25
You're right. That's what I have done [here](http://stackoverflow.com/questions/15795819/preventing-the-hibernate-staleobjectstateexception-from-occurring). I am marking your answer as accepted. — balteo, Apr 04 '13 at 07:28
You could also add the id field as a path variable like so `@RequestMapping(value = "/family/advertisement/{id}/edit", method = RequestMethod.POST, produces = "text/html") public String editFamilyAdvertisement(@PathVariable Integer id, @ModelAttribute @Validated(value = Validation.AdvertisementCreation.class) FamilyAdvertisementInfo familyAdvertisementInfo, BindingResult bindingResult, Model model)`. From there you can set the id of your entity in the controller to guarantee they are not hacking another id. A lot of boilerplate code but works. — dukethrash, Apr 17 '13 at 01:59

score 7 · Accepted Answer · answered Apr 03 '13 at 14:59

7

First question: when you retrieve the entity from the database, specify the id and the user. If no entity is found with the id and the user, it means that the user doesn't own the entity.

Second question: several solutions, depending of your requierements

Expose your entity directly instead of a dedicated form object
Encapsulate your entity in the form and use delegate methods (your IDE can generate them)
Use Dozer

answered Apr 03 '13 at 14:59

WilQu

7,131
6
30
38

Hi WilQu. Thanks for the reply. Good point regarding the first question! Now regarding the second question, I already expose the entity directly but the object coming from the controller model attribute is detached so I still need to update each field as explained in my post... Do you see my point? – balteo Apr 03 '13 at 15:12
By the way, Dozer looks nice, but I'd rather not introduce another dependency if possible. – balteo Apr 03 '13 at 15:13
1

@balteo why not just reattach it then? – WilQu Apr 03 '13 at 15:19
Thanks for your last comment. I am editing my post accordingly. – balteo Apr 03 '13 at 15:41
1

If your entity inherits from an abstract class and contains an id field in the abstract class you can create a method to wrap the `entityManager.merge` which first does a `Object ref = this.entityManager.find(persistentClass, entity.getId()); if (ref != null) { BeanUtils.copyProperties(entity, ref);}` You can skip the copying of the id field in the `BeanUtils.copyProperties` by passing in ignored fields. – dukethrash Apr 17 '13 at 01:53
Hi Dukethrash: I have actually used BeanUtils.copyProperties. I just forgot to add a comment here or update the post... Thanks a lot. – balteo Apr 17 '13 at 13:46

Issues with updating a JPA entity from a Spring mvc controller method and using hidden input fields to store an ID

1 Answers1