Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BindingResult should not hold on to ConversionService when serialized in session [SPR-8282] #12930

Closed
spring-issuemaster opened this issue Apr 28, 2011 · 17 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Apr 28, 2011

Darren Mills opened SPR-8282 and commented

We have a WebSphere cluster configured with session replication between each node. In order to accomplish this, all objects that end up in the session must be Serializable. We are using the Joda Date/Time Converters in our domain objects which could be contained within a Web Form. Although all of OUR objects are Serializable, our production logs are complaining about the FormattingConversionService not being Serializable. Another poster confirmed they are seeing the same exception and provided screenshots of their session attributes which included the FormattingConversionService.

Possible solutions may be either making all ConversionService implementations serializable, or changing the Spring behavior of storing the service bean in session, for example, in ServletContext.


Affects: 3.0.5

Reference URL: http://forum.springsource.org/showthread.php?p=359434

Referenced from: commits 74b6a5b, 9472025

7 votes, 14 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 22, 2012

steve bread commented

I encountered the same error and looking at the screenshots in http://forum.springsource.org/showthread.php?102054-NotSerializableException-with-FormattingConversionService it is the same cause i.e. BindingResult has a reference to ConversionService which is not serializable. In my case I am trying to add the Errors object to the flash so that I can use it in a post-redirect-get scenario.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 22, 2012

steve bread commented

To add to my previous comment, the specific type of the Error object is BeanPropertyBindingResult which implements Serializable but extends AbstractPropertyBindingResult which has a reference to DefaultFormattingConversionService which is not Serializable.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 15, 2012

Rossen Stoyanchev commented

We are using the Joda Date/Time Converters in our domain objects which could be contained within a Web Form. Although all of OUR objects are Serializable, our production logs are complaining about the FormattingConversionService not being Serializable.

Darren, it sounds like your domain objects maintain references to Converter implementations. This is actually not that common. Typically converters get used transparently when you declare @RequestParam or @ModelAttribute method arguments and there is no need for you to call them.

I encountered the same error ... it is the same cause i.e. BindingResult has a reference to ConversionService which is not serializable

Steve, you have to find out how a BindingResult makes it into the session. A BindingResult is typically added to the Model so it is available for rendering but it's never stored in the session.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 30, 2012

steve bread commented

Hi Rossen, in my application the submitted form is validated on POST and if there are errors the page is reloaded using a redirect to GET. This is so that a back button does not resubmit the form. To have the BindingResult object available after the redirect I was adding it to the Flash which puts it in the session. I got around the serializability issue by adding the contained list of ObjectError objects to the flash instead.

On POST

copyErrorsToFlash(Model model, RedirectAttributes redirectAttrs) {
  Map<String, Object> modelMap = model.asMap();
  for (Map.Entry<String, Object> entry : modelMap.entrySet()) {
    if (entry.getValue() instanceof Errors) {
      redirectAttrs.addFlashAttribute(FLASH_BINDINGRESULT_PREFIX + entry.getKey(),
          ((Errors) entry.getValue()).getAllErrors());
    }
  }
}

On GET

copyErrorsFromFlash(Model model) {
  Map<String, Object> modelMap = model.asMap();
  for (Map.Entry<String, Object> entry : modelMap.entrySet()) {
    if (entry.getKey().startsWith(FLASH_BINDINGRESULT_PREFIX)) {
      List<ObjectError> errors = (List<ObjectError>) entry.getValue();
      String bindingResultKey = entry.getKey().substring(FLASH_BINDINGRESULT_PREFIX.length());
      BindingResult bindingResult = (BindingResult) modelMap.get(bindingResultKey);
      for (ObjectError objectError : errors) {
        bindingResult.addError(objectError);
      }
    }
  }
}

FLASH_BINDINGRESULT_PREFIX is just a constant I defined.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 17, 2012

Andrzej Wisłowski commented

My proposition is to define field conversionService as transient in the AbstractPropertyBindingResult class.
Then BindingResult will not be serialized.

public abstract class AbstractPropertyBindingResult extends AbstractBindingResult {
transient private ConversionService conversionService;
....

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 23, 2012

Rossen Stoyanchev commented

The issue is that the conversionService will then not be re-initialized after deserialization. In the use case of steve bread that doesn't matter since he only needs access to the errors, so saving the errors only makes sense. Andrzej Wisłowski do you have a different use case in mind?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 22, 2013

Geoff Weatherall commented

This issue also occurs within the Google App Engine. It would be nice to have fixed, but Steve Bread's suggestion can be used to work around the issue. To reiterate the point, it's not domain objects retaining references to the Converter, it's the BindingResult held in Flash during a POST-REDIRECT-GET interaction.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 26, 2013

Simon Oualid commented

This issue is not "minor" IMHO. It is filling the logs of many of my clustered apps for years now. Hope it will be resolved in the next release.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 7, 2016

Nils Schmidt commented

We ran into a similar problem when using a Redis as HttpSessionStore in a clustered environment.

When we put the BindingResult in RedirectAttributes (for POST-REDIRECT-GET pattern) redis tries to serialize AbstractPropertyBindingResult which obviously fails.

The following workaround worked for us: Before saving anything to RedisSessionStore we set the conversionService to null. You might need to adjust the @Before pointcut to match the location when and where you wish to apply the workaround.

package org.springframework.session.data.redis;

import java.util.List;
import java.util.function.Consumer;

import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.Before;
import org.springframework.stereotype.Component;
import org.springframework.util.ReflectionUtils;
import org.springframework.validation.BeanPropertyBindingResult;
import org.springframework.web.servlet.FlashMap;
import org.springframework.web.servlet.support.SessionFlashMapManager;

@Aspect
@Component
public class PreSessionSerializationCleaner {

    @Before("execution (* org.springframework.session.data.redis.RedisOperationsSessionRepository.save(org.springframework.session.data.redis.RedisOperationsSessionRepository.RedisSession)) && args(redisSession)")
    public void beforeSave(RedisOperationsSessionRepository.RedisSession redisSession) {
        forEachBindingResultInFlashScope(this::removeConversionService, redisSession);
    }

    private void forEachBindingResultInFlashScope(Consumer<BeanPropertyBindingResult> consumer,
                                                  RedisOperationsSessionRepository.RedisSession redisSession) {
        final List flashMap = (List) redisSession.getAttribute(SessionFlashMapManager.class.getName() + ".FLASH_MAPS");
        if (flashMap != null && flashMap.size() == 1 && (flashMap.get(0) instanceof FlashMap)) {
            ((FlashMap) flashMap.get(0)).entrySet().forEach(e -> {
                if (e.getValue() instanceof BeanPropertyBindingResult) {
                    consumer.accept((BeanPropertyBindingResult) e.getValue());
                }
            });
        }
    }

    private void removeConversionService(final BeanPropertyBindingResult bindingResult) {
        if (bindingResult != null) {
            ReflectionUtils.doWithFields(BeanPropertyBindingResult.class,
                    field -> {
                        field.setAccessible(true);
                        field.set(bindingResult, null);
                    },
                    field -> field.getName().equals("conversionService")
            );
        }
    }
}
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 11, 2016

Łukasz Glapiński commented

I agree that it's not minor. Nowadays using redis or similar as spring's session backend is very popular. I'm on 4.3.3 and still seeing this issue. There is no way to put BindingResult into flashAttributes for native spring's error management.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 11, 2016

Juergen Hoeller commented

Alright, if we specifically redeclare this ticket to just marking the ConversionService reference as transient (with it not being available anymore on deserialization), I'm happy to roll this into 4.3.4. Sorry that it took so long...

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 12, 2016

Nils Schmidt commented

Thanks Juergen! We highly apreciate it. :-)

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 3, 2017

Erko Hansar commented

I'm a little late to the party, but this is not a fix, it's just hiding the real problem even deeper!

Now when we use POST-REDIRECT-GET pattern in a clustered environment (sessions replicated via Spring Session), and we have a validation error that we want to transfer via Errors and RedirectAttributes, we don't anymore get an exception when session gets serialized (because the field is now transient), but the redirect view drawing is broken, because on the next request the session is deserialized and conversionService is null and controller's WebDataBinder editors (for example CustomDateEditor) don't kick in anymore :(

So what is the official suggestion here: don't use REDIRECT-GET when validation errors are present :(?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 6, 2017

Erko Hansar commented

Add-on to my last comment (can't edit comments):

I was mistaken - our redirect view rendering in jsp-s is broken not because conversionService is null, but because BeanPropertyBindingResult.beanWrapper field is also declared as transient and serialization + deserialization looses the reference to the original beanWrapper which contains the custom editors from @InitBinder method.

So I assume that the BindingResult was never intended to be put into flash attributes (session) (required for POST-REDIRECT-GET) and when you have binding or validation errors on POST you need to return a "local" view, not a "redirect view with a new GET request". In that case it's really misleading that BeanPropertyBindingResult, AbstractBindingResult, AbstractErrors and others are marked as implements Serializable.

Inspired by steve bread's workaround I wrote a temporary fix for our project (until we rewrite and retest all the POST handlers).
All affected POST mapping methods need to call

Spr8282WorkaroundInterceptor.storeForRedirectGet(redirectAttrs, "exampleForm", exampleForm, errors);

if they have validation errors and want to redirect.

And then a handler interceptor runs after the following GET request handler to merge original errors into a new "clean" BindingResult (created by ModelFactory updateModel -> updateBindingResult):

@Slf4j
public class Spr8282WorkaroundInterceptor extends HandlerInterceptorAdapter {

    private static final String SPR8282_WORKAROUND_SUFFIX = "_SPR8282_WORKAROUND";

    public static void storeForRedirectGet(RedirectAttributes redirectAttrs, String formName, Serializable formObject, Errors errors) {
        redirectAttrs.addFlashAttribute(formName, formObject);
        redirectAttrs.addFlashAttribute(BindingResult.MODEL_KEY_PREFIX + formName + SPR8282_WORKAROUND_SUFFIX, errors);
    }

    @Override
    public void postHandle(HttpServletRequest request, HttpServletResponse response, Object handler, ModelAndView modelAndView) throws Exception {
        if (modelAndView != null && HttpMethod.GET.matches(request.getMethod())) {
            restoreAfterRedirectGet(modelAndView.getModelMap());
        }
    }

    private static void restoreAfterRedirectGet(Map<String, Object> modelMap) {
        List<String> keys = modelMap.keySet().stream().filter(k -> k.endsWith(SPR8282_WORKAROUND_SUFFIX)).collect(Collectors.toList());
        for (String key : keys) {
            String liveKey = StringUtils.removeEnd(key, SPR8282_WORKAROUND_SUFFIX);
            Errors origErrors = (Errors)modelMap.remove(key);
            Errors liveErrors = (Errors)modelMap.get(liveKey);
            if (liveErrors == null) {
                log.error("Did not find a live binding result for key: " + liveKey);
            } else {
                liveErrors.addAllErrors(origErrors);
                log.debug("Restored errors after redirect: " + liveKey);
            }
        }
    }

}
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 6, 2017

Rossen Stoyanchev commented

don't use REDIRECT-GET when validation errors are present ?

Indeed it's what this has been designed for and it's easiest to use:

@PostMapping("/form")
public String handle(FormBean bean, BindingResult result) {
    if (result.hasErrors()) {
        // FormBean and BindingResult already in model so just return back to the form...
        return "form";
    }
    // Process bean...
    return "redirect:/success";
}

Any reason to deviate from that pattern?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 6, 2017

Erko Hansar commented

Real world projects almost always need extra stuff (in addition to the FormBean) in the model to render the "form". Thus we need to duplicate the model populating code for GET and "failed" POST handlers.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 6, 2017

Rossen Stoyanchev commented

Isn't this done with @ModelAttribute methods anyway? Alternatively just shared methods on the controller populating the model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.