SPR-7000 - AnnotationMethodHandlerAdapter gives priority to media type order over quality when selecting a method

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerAdapter.java

@@ -74,7 +74,6 @@ import org.springframework.ui.Model;
 import org.springframework.util.AntPathMatcher;
 import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
-import org.springframework.util.CollectionUtils;
 import org.springframework.util.ObjectUtils;
 import org.springframework.util.PathMatcher;
 import org.springframework.util.StringUtils;
@@ -883,7 +882,7 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator
 			if (acceptedMediaTypes.isEmpty()) {
 				acceptedMediaTypes = Collections.singletonList(MediaType.ALL);
 			}
-			MediaType.sortBySpecificity(acceptedMediaTypes);
+			MediaType.sortByQualityValue(acceptedMediaTypes);
 			Class<?> returnValueType = returnValue.getClass();
 			List<MediaType> allSupportedMediaTypes = new ArrayList<MediaType>();
 			if (getMessageConverters() != null) {
@@ -1017,6 +1016,8 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator
 
 		private int compareAcceptHeaders(RequestMappingInfo info1, RequestMappingInfo info2) {
 			List<MediaType> requestAccepts = request.getHeaders().getAccept();
+			MediaType.sortByQualityValue(requestAccepts);
+			
 			List<MediaType> info1Accepts = getAcceptHeaderValue(info1);
 			List<MediaType> info2Accepts = getAcceptHeaderValue(info2);
 

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerExceptionResolver.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2009 the original author or authors.
+ * Copyright 2002-2010 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,11 +16,11 @@
 
 package org.springframework.web.servlet.mvc.annotation;
 
+import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.Reader;
 import java.io.Writer;
-import java.io.IOException;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.security.Principal;
@@ -32,9 +32,9 @@ import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import javax.servlet.ServletException;
 import javax.servlet.ServletRequest;
 import javax.servlet.ServletResponse;
-import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpSession;
@@ -42,13 +42,23 @@ import javax.servlet.http.HttpSession;
 import org.springframework.core.GenericTypeResolver;
 import org.springframework.core.MethodParameter;
 import org.springframework.core.annotation.AnnotationUtils;
+import org.springframework.http.HttpInputMessage;
+import org.springframework.http.HttpOutputMessage;
+import org.springframework.http.MediaType;
+import org.springframework.http.converter.ByteArrayHttpMessageConverter;
+import org.springframework.http.converter.FormHttpMessageConverter;
+import org.springframework.http.converter.HttpMessageConverter;
+import org.springframework.http.converter.StringHttpMessageConverter;
+import org.springframework.http.converter.xml.SourceHttpMessageConverter;
+import org.springframework.http.server.ServletServerHttpRequest;
+import org.springframework.http.server.ServletServerHttpResponse;
 import org.springframework.ui.Model;
 import org.springframework.util.ClassUtils;
 import org.springframework.util.ObjectUtils;
 import org.springframework.util.ReflectionUtils;
 import org.springframework.web.bind.annotation.ExceptionHandler;
-import org.springframework.web.bind.annotation.ResponseStatus;
 import org.springframework.web.bind.annotation.ResponseBody;
+import org.springframework.web.bind.annotation.ResponseStatus;
 import org.springframework.web.bind.support.WebArgumentResolver;
 import org.springframework.web.context.request.NativeWebRequest;
 import org.springframework.web.context.request.ServletWebRequest;
@@ -57,17 +67,6 @@ import org.springframework.web.servlet.ModelAndView;
 import org.springframework.web.servlet.View;
 import org.springframework.web.servlet.handler.AbstractHandlerExceptionResolver;
 import org.springframework.web.servlet.support.RequestContextUtils;
-import org.springframework.web.HttpMediaTypeNotAcceptableException;
-import org.springframework.http.converter.HttpMessageConverter;
-import org.springframework.http.converter.ByteArrayHttpMessageConverter;
-import org.springframework.http.converter.StringHttpMessageConverter;
-import org.springframework.http.converter.FormHttpMessageConverter;
-import org.springframework.http.converter.xml.SourceHttpMessageConverter;
-import org.springframework.http.HttpInputMessage;
-import org.springframework.http.MediaType;
-import org.springframework.http.HttpOutputMessage;
-import org.springframework.http.server.ServletServerHttpRequest;
-import org.springframework.http.server.ServletServerHttpResponse;
 
 /**
  * Implementation of the {@link org.springframework.web.servlet.HandlerExceptionResolver} interface that handles
@@ -396,7 +395,7 @@ public class AnnotationMethodHandlerExceptionResolver extends AbstractHandlerExc
 		if (acceptedMediaTypes.isEmpty()) {
 			acceptedMediaTypes = Collections.singletonList(MediaType.ALL);
 		}
-		MediaType.sortBySpecificity(acceptedMediaTypes);
+		MediaType.sortByQualityValue(acceptedMediaTypes);
 		HttpOutputMessage outputMessage = new ServletServerHttpResponse(webRequest.getResponse());
 		Class<?> returnValueType = returnValue.getClass();
 		if (messageConverters != null) {

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

@@ -23,8 +23,6 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
-import java.util.SortedMap;
-import java.util.TreeMap;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import javax.activation.FileTypeMap;
@@ -292,7 +290,7 @@ public class ContentNegotiatingViewResolver extends WebApplicationObjectSupport
 			String acceptHeader = request.getHeader(ACCEPT_HEADER);
 			if (StringUtils.hasText(acceptHeader)) {
 				List<MediaType> mediaTypes = MediaType.parseMediaTypes(acceptHeader);
-				MediaType.sortBySpecificity(mediaTypes);
+				MediaType.sortByQualityValue(mediaTypes);
 				if (logger.isDebugEnabled()) {
 					logger.debug("Requested media types are " + mediaTypes + " (based on Accept header)");
 				}

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestMappingInfoComparatorTests.java

@@ -120,8 +120,15 @@ public class RequestMappingInfoComparatorTests {
 
 		assertTrue(comparator.compare(html, xml) == 0);
 		assertTrue(comparator.compare(xml, html) == 0);
-	}
 
+		// See SPR-7000
+		request = new MockHttpServletRequest();
+		request.addHeader("Accept", "text/html;q=0.9,application/xml");
+		comparator = new AnnotationMethodHandlerAdapter.RequestMappingInfoComparator(new MockComparator(), request);
+
+		assertTrue(comparator.compare(html, xml) > 0);
+		assertTrue(comparator.compare(xml, html) < 0);
+	}
 
 	private static class MockComparator implements Comparator<String> {
 

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java

@@ -1289,6 +1289,12 @@ public class ServletAnnotationControllerTests {
 		response = new MockHttpServletResponse();
 		servlet.service(request, response);
 		assertEquals("xml", response.getContentAsString());
+
+		request = new MockHttpServletRequest("GET", "/something");
+		request.addHeader("Accept", "text/html;q=0.9, application/xml");
+		response = new MockHttpServletResponse();
+		servlet.service(request, response);
+		assertEquals("xml", response.getContentAsString());
 	}
 
 	@Test
@@ -2633,5 +2639,4 @@ public class ServletAnnotationControllerTests {
 
 	}
 
-
 }

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

@@ -82,7 +82,7 @@ public class ContentNegotiatingViewResolverTests {
 	@Test
 	public void getMediaTypeAcceptHeader() {
 		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/test");
-		request.addHeader("Accept", "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8");
+		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", 4, result.size());
 		assertEquals("Invalid content type", new MediaType("text", "html"), result.get(0));

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

@@ -648,6 +648,35 @@ public class MediaType implements Comparable<MediaType> {
 		}
 	}
 
+	/**
+	 * Sorts the given list of {@link MediaType} objects by quality value.
+	 *
+	 * <p>Given two media types:
+	 * <ol>
+	 *   <li>if the two media types have different {@linkplain #getQualityValue() quality value}, then the media type
+	 *   with the highest quality value is ordered before the other.</li>
+	 *   <li>if either media type has a {@linkplain #isWildcardType() wildcard type}, then the media type without the
+	 *   wildcard is ordered before the other.</li>
+	 *   <li>if the two media types have different {@linkplain #getType() types}, then they are considered equal and
+	 *   remain their current order.</li>
+	 *   <li>if either media type has a {@linkplain #isWildcardSubtype() wildcard subtype}, then the media type without
+	 *   the wildcard is sorted before the other.</li>
+	 *   <li>if the two media types have different {@linkplain #getSubtype() subtypes}, then they are considered equal
+	 *   and remain their current order.</li>
+	 *   <li>if the two media types have a different amount of {@linkplain #getParameter(String) parameters}, then the
+	 *   media type with the most parameters is ordered before the other.</li>
+	 * </ol>
+	 *
+	 * @param mediaTypes the list of media types to be sorted
+	 * @see #getQualityValue()
+	 */
+	public static void sortByQualityValue(List<MediaType> mediaTypes) {
+		Assert.notNull(mediaTypes, "'mediaTypes' must not be null");
+		if (mediaTypes.size() > 1) {
+			Collections.sort(mediaTypes, QUALITY_VALUE_COMPARATOR);
+		}
+	}
+
 	static final Comparator<MediaType> SPECIFICITY_COMPARATOR = new Comparator<MediaType>() {
 
 		public int compare(MediaType mediaType1, MediaType mediaType2) {
@@ -686,4 +715,39 @@ public class MediaType implements Comparable<MediaType> {
 		}
 	};
 
+	static final Comparator<MediaType> QUALITY_VALUE_COMPARATOR = new Comparator<MediaType>() {
+
+		public int compare(MediaType mediaType1, MediaType mediaType2) {
+			double quality1 = mediaType1.getQualityValue();
+			double quality2 = mediaType2.getQualityValue();
+			int qualityComparison = Double.compare(quality2, quality1);
+			if (qualityComparison != 0) {
+				return qualityComparison;  // audio/*;q=0.7 < audio/*;q=0.3
+			}
+			else if (mediaType1.isWildcardType() && !mediaType2.isWildcardType()) { // */* < audio/*
+				return 1;
+			}
+			else if (mediaType2.isWildcardType() && !mediaType1.isWildcardType()) { // audio/* > */*
+				return -1;
+			}
+			else if (!mediaType1.getType().equals(mediaType2.getType())) { // audio/basic == text/html
+				return 0;
+			}
+			else { // mediaType1.getType().equals(mediaType2.getType())
+				if (mediaType1.isWildcardSubtype() && !mediaType2.isWildcardSubtype()) { // audio/* < audio/basic
+					return 1;
+				}
+				else if (mediaType2.isWildcardSubtype() && !mediaType1.isWildcardSubtype()) { // audio/basic > audio/*
+					return -1;
+				}
+				else if (!mediaType1.getSubtype().equals(mediaType2.getSubtype())) { // audio/basic == audio/wave
+					return 0;
+				} else {
+					int paramsSize1 = mediaType1.parameters.size();
+					int paramsSize2 = mediaType2.parameters.size();
+					return (paramsSize2 < paramsSize1 ? -1 : (paramsSize2 == paramsSize1 ? 0 : 1)); // audio/basic;level=1 < audio/basic
+				}
+			}
+		}
+	};
 }

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

@@ -373,4 +373,108 @@ public class MediaTypeTests {
 
 	}
 
+	@Test
+	public void qualityComparator() throws Exception {
+		MediaType audioBasic = new MediaType("audio", "basic");
+		MediaType audioWave = new MediaType("audio", "wave");
+		MediaType audio = new MediaType("audio");
+		MediaType audio03 = new MediaType("audio", "*", 0.3);
+		MediaType audio07 = new MediaType("audio", "*", 0.7);
+		MediaType audioBasicLevel = new MediaType("audio", "basic", Collections.singletonMap("level", "1"));
+		MediaType textHtml = new MediaType("text", "html");
+		MediaType all = MediaType.ALL;
+
+		Comparator<MediaType> comp = MediaType.QUALITY_VALUE_COMPARATOR;
+
+		// equal
+		assertEquals("Invalid comparison result", 0, comp.compare(audioBasic,audioBasic));
+		assertEquals("Invalid comparison result", 0, comp.compare(audio, audio));
+		assertEquals("Invalid comparison result", 0, comp.compare(audio07, audio07));
+		assertEquals("Invalid comparison result", 0, comp.compare(audio03, audio03));
+		assertEquals("Invalid comparison result", 0, comp.compare(audioBasicLevel, audioBasicLevel));
+
+		// specific to unspecific
+		assertTrue("Invalid comparison result", comp.compare(audioBasic, audio) < 0);
+		assertTrue("Invalid comparison result", comp.compare(audioBasic, all) < 0);
+		assertTrue("Invalid comparison result", comp.compare(audio, all) < 0);
+
+		// unspecific to specific
+		assertTrue("Invalid comparison result", comp.compare(audio, audioBasic) > 0);
+		assertTrue("Invalid comparison result", comp.compare(all, audioBasic) > 0);
+		assertTrue("Invalid comparison result", comp.compare(all, audio) > 0);
+
+		// qualifiers
+		assertTrue("Invalid comparison result", comp.compare(audio, audio07) < 0);
+		assertTrue("Invalid comparison result", comp.compare(audio07, audio) > 0);
+		assertTrue("Invalid comparison result", comp.compare(audio07, audio03) < 0);
+		assertTrue("Invalid comparison result", comp.compare(audio03, audio07) > 0);
+		assertTrue("Invalid comparison result", comp.compare(audio03, all) > 0);
+		assertTrue("Invalid comparison result", comp.compare(all, audio03) < 0);
+
+		// other parameters
+		assertTrue("Invalid comparison result", comp.compare(audioBasic, audioBasicLevel) > 0);
+		assertTrue("Invalid comparison result", comp.compare(audioBasicLevel, audioBasic) < 0);
+
+		// different types
+		assertEquals("Invalid comparison result", 0, comp.compare(audioBasic, textHtml));
+		assertEquals("Invalid comparison result", 0, comp.compare(textHtml, audioBasic));
+
+		// different subtypes
+		assertEquals("Invalid comparison result", 0, comp.compare(audioBasic, audioWave));
+		assertEquals("Invalid comparison result", 0, comp.compare(audioWave, audioBasic));
+	}
+
+	@Test
+	public void sortByQualityRelated() {
+		MediaType audioBasic = new MediaType("audio", "basic");
+		MediaType audio = new MediaType("audio");
+		MediaType audio03 = new MediaType("audio", "*", 0.3);
+		MediaType audio07 = new MediaType("audio", "*", 0.7);
+		MediaType audioBasicLevel = new MediaType("audio", "basic", Collections.singletonMap("level", "1"));
+		MediaType all = MediaType.ALL;
+
+		List<MediaType> expected = new ArrayList<MediaType>();
+		expected.add(audioBasicLevel);
+		expected.add(audioBasic);
+		expected.add(audio);
+		expected.add(all);
+		expected.add(audio07);
+		expected.add(audio03);
+
+		List<MediaType> result = new ArrayList<MediaType>(expected);
+		Random rnd = new Random();
+		// shuffle & sort 10 times
+		for (int i = 0; i < 10; i++) {
+			Collections.shuffle(result, rnd);
+			MediaType.sortByQualityValue(result);
+
+			for (int j = 0; j < result.size(); j++) {
+				assertSame("Invalid media type at " + j, expected.get(j), result.get(j));
+			}
+		}
+	}
+	
+	@Test
+	public void sortByQualityUnrelated() {
+		MediaType audioBasic = new MediaType("audio", "basic");
+		MediaType audioWave = new MediaType("audio", "wave");
+		MediaType textHtml = new MediaType("text", "html");
+
+		List<MediaType> expected = new ArrayList<MediaType>();
+		expected.add(textHtml);
+		expected.add(audioBasic);
+		expected.add(audioWave);
+
+		List<MediaType> result = new ArrayList<MediaType>(expected);
+		MediaType.sortBySpecificity(result);
+
+		for (int i = 0; i < result.size(); i++) {
+			assertSame("Invalid media type at " + i, expected.get(i), result.get(i));
+		}
+
+	}
+
+
+
+
 }