Fix content negotiation issue with sort by q-value

Before this fix the q-value of media types in the Accept header were
ignored when using the new RequestMappingHandlerAdapter in combination
with @ResponseBody and HttpMessageConverters.

Issue: SPR-9160
Backport-Issue: SPR-9168
Backport-Commit: 982cb2f258

build-spring-framework/resources/changelog.txt

@@ -18,6 +18,8 @@ Changes in version 3.1.2 (2012-06-??)
 * fix issue with parsing invalid Content-Type or Accept headers
 * add Jackson 2 HttpMessageConverter and View types
 * translate IOException from Jackson to HttpMessageNotReadableException
+* fix content negotiation issue when sorting selected media types by quality value
+
 
 Changes in version 3.1.1 (2012-02-16)
 -------------------------------------

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java

@@ -21,7 +21,6 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.LinkedHashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 
 import javax.servlet.http.HttpServletRequest;
@@ -117,7 +116,7 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe
 		}
 
 		List<MediaType> mediaTypes = new ArrayList<MediaType>(compatibleMediaTypes);
-		MediaType.sortBySpecificity(mediaTypes);
+		MediaType.sortBySpecificityAndQuality(mediaTypes);
 
 		MediaType selectedMediaType = null;
 		for (MediaType mediaType : mediaTypes) {
@@ -131,6 +130,8 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe
 			}
 		}
 
+		selectedMediaType = selectedMediaType.removeQualityValue();
+
 		if (selectedMediaType != null) {
 			for (HttpMessageConverter<?> messageConverter : messageConverters) {
 				if (messageConverter.canWrite(returnValueClass, selectedMediaType)) {
@@ -188,14 +189,12 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe
 	}
 
 	/**
-	 * Returns the more specific media type using the q-value of the first media type for both.
+	 * Return the more specific of the acceptable and the producible media types
+	 * with the q-value of the former.
 	 */
-	private MediaType getMostSpecificMediaType(MediaType type1, MediaType type2) {
-		double quality = type1.getQualityValue();
-		Map<String, String> params = Collections.singletonMap("q", String.valueOf(quality));
-		MediaType t1 = new MediaType(type1, params);
-		MediaType t2 = new MediaType(type2, params);
-		return MediaType.SPECIFICITY_COMPARATOR.compare(t1, t2) <= 0 ? type1 : type2;
+	private MediaType getMostSpecificMediaType(MediaType acceptType, MediaType produceType) {
+		produceType = produceType.copyQualityValue(acceptType);
+		return MediaType.SPECIFICITY_COMPARATOR.compare(acceptType, produceType) < 0 ? acceptType : produceType;
 	}
 
 }

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/view/ContentNegotiatingViewResolver.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 the original author or authors.
+ * Copyright 2002-2012 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.
@@ -101,6 +101,7 @@ import org.springframework.web.util.WebUtils;
  *
  * @author Arjen Poutsma
  * @author Juergen Hoeller
+ * @author Rossen Stoyanchev
  * @since 3.0
  * @see ViewResolver
  * @see InternalResourceViewResolver
@@ -117,6 +118,9 @@ public class ContentNegotiatingViewResolver extends WebApplicationObjectSupport
 
 	private static final UrlPathHelper urlPathHelper = new UrlPathHelper();
 
+	static {
+		urlPathHelper.setUrlDecode(false);
+	}
 
 	private int order = Ordered.HIGHEST_PRECEDENCE;
 
@@ -351,13 +355,13 @@ public class ContentNegotiatingViewResolver extends WebApplicationObjectSupport
 							}
 						}
 					}
-					List<MediaType> mediaTypes = new ArrayList<MediaType>(compatibleMediaTypes);
-					MediaType.sortByQualityValue(mediaTypes);
+					List<MediaType> selectedMediaTypes = new ArrayList<MediaType>(compatibleMediaTypes);
+					MediaType.sortBySpecificityAndQuality(selectedMediaTypes);
 					if (logger.isDebugEnabled()) {
-						logger.debug("Requested media types are " + mediaTypes + " based on Accept header types " +
+						logger.debug("Requested media types are " + selectedMediaTypes + " based on Accept header types " +
 								"and producible media types " + producibleMediaTypes + ")");
 					}
-					return mediaTypes;
+					return selectedMediaTypes;
 				}
 				catch (IllegalArgumentException ex) {
 					if (logger.isDebugEnabled()) {
@@ -392,14 +396,12 @@ public class ContentNegotiatingViewResolver extends WebApplicationObjectSupport
 	}
 
 	/**
-	 * Returns the more specific media type using the q-value of the first media type for both.
+	 * Return the more specific of the acceptable and the producible media types
+	 * with the q-value of the former.
 	 */
-	private MediaType getMostSpecificMediaType(MediaType type1, MediaType type2) {
-		double quality = type1.getQualityValue();
-		Map<String, String> params = Collections.singletonMap("q", String.valueOf(quality));
-		MediaType t1 = new MediaType(type1, params);
-		MediaType t2 = new MediaType(type2, params);
-		return MediaType.SPECIFICITY_COMPARATOR.compare(t1, t2) <= 0 ? type1 : type2;
+	private MediaType getMostSpecificMediaType(MediaType acceptType, MediaType produceType) {
+		produceType = produceType.copyQualityValue(acceptType);
+		return MediaType.SPECIFICITY_COMPARATOR.compare(acceptType, produceType) < 0 ? acceptType : produceType;
 	}
 
 	/**

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestResponseBodyMethodProcessorTests.java

@@ -48,21 +48,21 @@ import org.springframework.http.MediaType;
 import org.springframework.http.converter.ByteArrayHttpMessageConverter;
 import org.springframework.http.converter.HttpMessageConverter;
 import org.springframework.http.converter.StringHttpMessageConverter;
+import org.springframework.http.converter.json.MappingJackson2HttpMessageConverter;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean;
 import org.springframework.web.HttpMediaTypeNotAcceptableException;
 import org.springframework.web.HttpMediaTypeNotSupportedException;
+import org.springframework.web.bind.MethodArgumentNotValidException;
 import org.springframework.web.bind.WebDataBinder;
 import org.springframework.web.bind.annotation.RequestBody;
 import org.springframework.web.bind.annotation.ResponseBody;
 import org.springframework.web.bind.support.WebDataBinderFactory;
 import org.springframework.web.context.request.NativeWebRequest;
 import org.springframework.web.context.request.ServletWebRequest;
-import org.springframework.web.bind.MethodArgumentNotValidException;
 import org.springframework.web.method.support.ModelAndViewContainer;
 import org.springframework.web.servlet.HandlerMapping;
-import org.springframework.web.servlet.mvc.method.annotation.RequestResponseBodyMethodProcessor;
 
 /**
  * Test fixture with {@link RequestResponseBodyMethodProcessor} and mock {@link HttpMessageConverter}.
@@ -264,8 +264,24 @@ public class RequestResponseBodyMethodProcessorTests {
 		fail("Expected exception");
 	}
 
+	// SPR-9160
+
+	@Test
+	public void handleReturnValueSortByQuality() throws Exception {
+		this.servletRequest.addHeader("Accept", "text/plain; q=0.5, application/json");
+
+		List<HttpMessageConverter<?>> converters = new ArrayList<HttpMessageConverter<?>>();
+		converters.add(new MappingJackson2HttpMessageConverter());
+		converters.add(new StringHttpMessageConverter());
+		RequestResponseBodyMethodProcessor handler = new RequestResponseBodyMethodProcessor(converters);
+
+		handler.writeWithMessageConverters("Foo", returnTypeStringProduces, webRequest);
+
+		assertEquals("application/json;charset=UTF-8", servletResponse.getHeader("Content-Type"));
+	}
+
 	@Test
-	public void handleStringReturnValue() throws Exception {
+	public void handleReturnValueString() throws Exception {
 		List<HttpMessageConverter<?>>converters = new ArrayList<HttpMessageConverter<?>>();
 		converters.add(new ByteArrayHttpMessageConverter());
 		converters.add(new StringHttpMessageConverter());

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/view/ContentNegotiatingViewResolverTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 the original author or authors.
+ * Copyright 2002-2012 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,6 +16,16 @@
 
 package org.springframework.web.servlet.view;
 
+import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.replay;
+import static org.easymock.EasyMock.verify;
+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.Collections;
@@ -28,7 +38,6 @@ import java.util.Set;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
-
 import org.springframework.http.MediaType;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
@@ -40,9 +49,6 @@ import org.springframework.web.servlet.HandlerMapping;
 import org.springframework.web.servlet.View;
 import org.springframework.web.servlet.ViewResolver;
 
-import static org.easymock.EasyMock.*;
-import static org.junit.Assert.*;
-
 /**
  * @author Arjen Poutsma
  */
@@ -110,6 +116,15 @@ public class ContentNegotiatingViewResolverTests {
 		assertTrue("Context path should be excluded", viewResolver.getMediaTypes(request).isEmpty());
 	}
 
+	// SPR-9390
+
+	@Test
+	public void getMediaTypeFilenameWithEncodedURI() {
+		request.setRequestURI("/quo%20vadis%3f.html");
+		List<MediaType> result = viewResolver.getMediaTypes(request);
+		assertEquals("Invalid content type", Collections.singletonList(new MediaType("text", "html")), result);
+	}
+
 	@Test
 	public void getMediaTypeParameter() {
 		viewResolver.setFavorParameter(true);
@@ -139,7 +154,6 @@ public class ContentNegotiatingViewResolverTests {
 		request.setAttribute(HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE, producibleTypes);
 		request.addHeader("Accept", "text/html,application/xml;q=0.9,application/xhtml+xml,*/*;q=0.8");
 		List<MediaType> result = viewResolver.getMediaTypes(request);
-		assertEquals("Invalid amount of media types", 1, result.size());
 		assertEquals("Invalid content type", new MediaType("application", "xhtml+xml"), result.get(0));
 	}
 
@@ -294,6 +308,35 @@ public class ContentNegotiatingViewResolverTests {
 		verify(viewResolverMock1, viewResolverMock2, viewMock1, viewMock2);
 	}
 
+	// SPR-9160
+
+	@Test
+	public void resolveViewNameAcceptHeaderSortByQuality() throws Exception {
+		request.addHeader("Accept", "text/plain;q=0.5, application/json");
+
+		ViewResolver htmlViewResolver = createMock(ViewResolver.class);
+		ViewResolver jsonViewResolver = createMock(ViewResolver.class);
+		viewResolver.setViewResolvers(Arrays.asList(htmlViewResolver, jsonViewResolver));
+
+		View htmlView = createMock("text_html", View.class);
+		View jsonViewMock = createMock("application_json", View.class);
+
+		String viewName = "view";
+		Locale locale = Locale.ENGLISH;
+
+		expect(htmlViewResolver.resolveViewName(viewName, locale)).andReturn(htmlView);
+		expect(jsonViewResolver.resolveViewName(viewName, locale)).andReturn(jsonViewMock);
+		expect(htmlView.getContentType()).andReturn("text/html").anyTimes();
+		expect(jsonViewMock.getContentType()).andReturn("application/json").anyTimes();
+		replay(htmlViewResolver, jsonViewResolver, htmlView, jsonViewMock);
+
+		viewResolver.setFavorPathExtension(false);
+		View result = viewResolver.resolveViewName(viewName, locale);
+		assertSame("Invalid view", jsonViewMock, result);
+
+		verify(htmlViewResolver, jsonViewResolver, htmlView, jsonViewMock);
+	}
+
 	@Test
 	public void resolveViewNameAcceptHeaderDefaultView() throws Exception {
 		request.addHeader("Accept", "application/json");

org.springframework.web/src/main/java/org/springframework/http/MediaType.java

@@ -33,6 +33,7 @@ import org.springframework.util.Assert;
 import org.springframework.util.CollectionUtils;
 import org.springframework.util.LinkedCaseInsensitiveMap;
 import org.springframework.util.StringUtils;
+import org.springframework.util.comparator.CompoundComparator;
 
 /**
  * Represents an Internet Media Type, as defined in the HTTP specification.
@@ -43,6 +44,7 @@ import org.springframework.util.StringUtils;
  *
  * @author Arjen Poutsma
  * @author Juergen Hoeller
+ * @author Rossen Stoyanchev
  * @since 3.0
  * @see <a href="http://tools.ietf.org/html/rfc2616#section-3.7">HTTP 1.1, section 3.7</a>
  */
@@ -529,6 +531,32 @@ public class MediaType implements Comparable<MediaType> {
 		return false;
 	}
 
+	/**
+	 * Return a replica of this instance with the quality value of the given MediaType.
+	 * @return the same instance if the given MediaType doesn't have a quality value, or a new one otherwise
+	 */
+	public MediaType copyQualityValue(MediaType mediaType) {
+		if (!mediaType.parameters.containsKey(PARAM_QUALITY_FACTOR)) {
+			return this;
+		}
+		Map<String, String> params = new LinkedHashMap<String, String>(this.parameters);
+		params.put(PARAM_QUALITY_FACTOR, mediaType.parameters.get(PARAM_QUALITY_FACTOR));
+		return new MediaType(this, params);
+	}
+
+	/**
+	 * Return a replica of this instance with its quality value removed.
+	 * @return the same instance if the media type doesn't contain a quality value, or a new one otherwise
+	 */
+	public MediaType removeQualityValue() {
+		if (!this.parameters.containsKey(PARAM_QUALITY_FACTOR)) {
+			return this;
+		}
+		Map<String, String> params = new LinkedHashMap<String, String>(this.parameters);
+		params.remove(PARAM_QUALITY_FACTOR);
+		return new MediaType(this, params);
+	}
+
 	/**
 	 * Compares this {@code MediaType} to another alphabetically.
 	 * @param other media type to compare to
@@ -772,6 +800,22 @@ public class MediaType implements Comparable<MediaType> {
 		}
 	}
 
+	/**
+	 * Sorts the given list of {@code MediaType} objects by specificity as the
+	 * primary criteria and quality value the secondary.
+	 * @see MediaType#sortBySpecificity(List)
+	 * @see MediaType#sortByQualityValue(List)
+	 */
+	public static void sortBySpecificityAndQuality(List<MediaType> mediaTypes) {
+		Assert.notNull(mediaTypes, "'mediaTypes' must not be null");
+		if (mediaTypes.size() > 1) {
+			Comparator<?>[] comparators = new Comparator[2];
+			comparators[0] = MediaType.SPECIFICITY_COMPARATOR;
+			comparators[1] = MediaType.QUALITY_VALUE_COMPARATOR;
+			Collections.sort(mediaTypes, new CompoundComparator<MediaType>(comparators));
+		}
+	}
+
 
 	/**
 	 * Comparator used by {@link #sortBySpecificity(List)}.

org.springframework.web/src/test/java/org/springframework/http/MediaTypeTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 the original author or authors.
+ * Copyright 2002-2012 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.