From 5c0fdb05bd78b197bd55a63bc5702a31da6d9415 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Tue, 11 Mar 2014 21:35:36 +0100 Subject: [PATCH] AbstractFlashMapManager needs to decode parameter names Also includes general alignment with the 4.0.3 versions of AbstractFlashMapManager and FlashMapManagerTests. Issue: SPR-11504 --- .../support/AbstractFlashMapManager.java | 38 +++++++++---------- ...erTests.java => FlashMapManagerTests.java} | 27 +++++++------ 2 files changed, 32 insertions(+), 33 deletions(-) rename spring-webmvc/src/test/java/org/springframework/web/servlet/support/{AbstractFlashMapManagerTests.java => FlashMapManagerTests.java} (94%) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java index 9fe88e8ef74..530bfa3287d 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,12 +20,12 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; - import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; + import org.springframework.util.Assert; import org.springframework.util.CollectionUtils; import org.springframework.util.MultiValueMap; @@ -43,13 +43,14 @@ import org.springframework.web.util.UrlPathHelper; */ public abstract class AbstractFlashMapManager implements FlashMapManager { + private static final Object writeLock = new Object(); + protected final Log logger = LogFactory.getLog(getClass()); private int flashMapTimeout = 180; private UrlPathHelper urlPathHelper = new UrlPathHelper(); - private static final Object writeLock = new Object(); /** * Set the amount of time in seconds after a {@link FlashMap} is saved @@ -82,16 +83,16 @@ public abstract class AbstractFlashMapManager implements FlashMapManager { return this.urlPathHelper; } - public final FlashMap retrieveAndUpdate(HttpServletRequest request, HttpServletResponse response) { + public final FlashMap retrieveAndUpdate(HttpServletRequest request, HttpServletResponse response) { List maps = retrieveFlashMaps(request); if (CollectionUtils.isEmpty(maps)) { return null; } + if (logger.isDebugEnabled()) { logger.debug("Retrieved FlashMap(s): " + maps); } - List mapsToRemove = getExpiredFlashMaps(maps); FlashMap match = getMatchingFlashMap(maps, request); @@ -156,21 +157,20 @@ public abstract class AbstractFlashMapManager implements FlashMapManager { /** * Whether the given FlashMap matches the current request. - * The default implementation uses the target request path and query - * parameters saved in the FlashMap. + * Uses the expected request path and query parameters saved in the FlashMap. */ protected boolean isFlashMapForRequest(FlashMap flashMap, HttpServletRequest request) { - if (flashMap.getTargetRequestPath() != null) { + String expectedPath = flashMap.getTargetRequestPath(); + if (expectedPath != null) { String requestUri = this.urlPathHelper.getOriginatingRequestUri(request); - if (!requestUri.equals(flashMap.getTargetRequestPath()) - && !requestUri.equals(flashMap.getTargetRequestPath() + "/")) { + if (!requestUri.equals(expectedPath) && !requestUri.equals(expectedPath + "/")) { return false; } } MultiValueMap targetParams = flashMap.getTargetRequestParams(); - for (String paramName : targetParams.keySet()) { - for (String targetValue : targetParams.get(paramName)) { - if (!ObjectUtils.containsElement(request.getParameterValues(paramName), targetValue)) { + for (String expectedName : targetParams.keySet()) { + for (String expectedValue : targetParams.get(expectedName)) { + if (!ObjectUtils.containsElement(request.getParameterValues(expectedName), expectedValue)) { return false; } } @@ -185,18 +185,16 @@ public abstract class AbstractFlashMapManager implements FlashMapManager { String path = decodeAndNormalizePath(flashMap.getTargetRequestPath(), request); flashMap.setTargetRequestPath(path); - decodeParameters(flashMap.getTargetRequestParams(), request); if (logger.isDebugEnabled()) { logger.debug("Saving FlashMap=" + flashMap); } - flashMap.startExpirationPeriod(this.flashMapTimeout); synchronized (writeLock) { List allMaps = retrieveFlashMaps(request); - allMaps = (allMaps == null) ? new CopyOnWriteArrayList() : allMaps; + allMaps = (allMaps != null ? allMaps : new CopyOnWriteArrayList()); allMaps.add(flashMap); updateFlashMaps(allMaps, request, response); } @@ -217,7 +215,9 @@ public abstract class AbstractFlashMapManager implements FlashMapManager { private void decodeParameters(MultiValueMap params, HttpServletRequest request) { for (String name : new ArrayList(params.keySet())) { for (String value : new ArrayList(params.remove(name))) { - params.add(name, this.urlPathHelper.decodeRequestString(request, value)); + name = this.urlPathHelper.decodeRequestString(request, name); + value = this.urlPathHelper.decodeRequestString(request, value); + params.add(name, value); } } } @@ -228,7 +228,7 @@ public abstract class AbstractFlashMapManager implements FlashMapManager { * @param request the current request * @param response the current response */ - protected abstract void updateFlashMaps(List flashMaps, HttpServletRequest request, - HttpServletResponse response); + protected abstract void updateFlashMaps( + List flashMaps, HttpServletRequest request, HttpServletResponse response); } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/support/AbstractFlashMapManagerTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/support/FlashMapManagerTests.java similarity index 94% rename from spring-webmvc/src/test/java/org/springframework/web/servlet/support/AbstractFlashMapManagerTests.java rename to spring-webmvc/src/test/java/org/springframework/web/servlet/support/FlashMapManagerTests.java index 98564ae6c1f..f0399ada7d1 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/support/AbstractFlashMapManagerTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/support/FlashMapManagerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,34 +16,30 @@ package org.springframework.web.servlet.support; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertSame; -import static org.junit.Assert.assertTrue; - import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; - import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.junit.Before; import org.junit.Test; + import org.springframework.mock.web.test.MockHttpServletRequest; import org.springframework.mock.web.test.MockHttpServletResponse; +import org.springframework.util.MultiValueMap; import org.springframework.web.servlet.FlashMap; import org.springframework.web.util.WebUtils; +import static org.junit.Assert.*; + /** * Test fixture for testing {@link AbstractFlashMapManager} methods. * * @author Rossen Stoyanchev */ -public class AbstractFlashMapManagerTests { +public class FlashMapManagerTests { private TestFlashMapManager flashMapManager; @@ -264,6 +260,8 @@ public class AbstractFlashMapManagerTests { assertEquals("/once/only", flashMap.getTargetRequestPath()); } + // SPR-9657, SPR-11504 + @Test public void saveOutputFlashMapDecodeParameters() throws Exception { this.request.setCharacterEncoding("UTF-8"); @@ -274,10 +272,12 @@ public class AbstractFlashMapManagerTests { flashMap.addTargetRequestParam("key", "%D0%90%D0%90"); flashMap.addTargetRequestParam("key", "%D0%91%D0%91"); flashMap.addTargetRequestParam("key", "%D0%92%D0%92"); + flashMap.addTargetRequestParam("%3A%2F%3F%23%5B%5D%40", "value"); this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response); - assertEquals(Arrays.asList("\u0410\u0410", "\u0411\u0411", "\u0412\u0412"), - flashMap.getTargetRequestParams().get("key")); + MultiValueMap targetRequestParams = flashMap.getTargetRequestParams(); + assertEquals(Arrays.asList("\u0410\u0410", "\u0411\u0411", "\u0412\u0412"), targetRequestParams.get("key")); + assertEquals(Arrays.asList("value"), targetRequestParams.get(":/?#[]@")); } @@ -303,8 +303,7 @@ public class AbstractFlashMapManagerTests { } @Override - protected void updateFlashMaps(List flashMaps, HttpServletRequest request, - HttpServletResponse response) { + protected void updateFlashMaps(List flashMaps, HttpServletRequest request, HttpServletResponse response) { this.flashMaps = flashMaps; } }