From 80701082cd872a2526823dc26bc7f9271e52bd48 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 24 Nov 2020 17:30:02 +0000 Subject: [PATCH] Set favorPathExtension to false by default Applies a change that was intended in #23915 but wasn't. Closes gh-26119 --- .../ContentNegotiationManagerFactoryBean.java | 2 +- ...entNegotiationManagerFactoryBeanTests.java | 5 +++-- .../web/servlet/config/MvcNamespaceTests.java | 14 ++++++++---- .../ContentNegotiationConfigurerTests.java | 21 +++++++++++++----- ...MvcConfigurationSupportExtensionTests.java | 22 ++++++------------- ...nnotationControllerHandlerMethodTests.java | 14 ++++++++++-- .../ContentNegotiatingViewResolverTests.java | 18 ++++++++++++--- ...mvc-config-content-negotiation-manager.xml | 1 + 8 files changed, 64 insertions(+), 33 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java index f0339ce18d..c23db8e97c 100644 --- a/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java +++ b/spring-web/src/main/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBean.java @@ -108,7 +108,7 @@ public class ContentNegotiationManagerFactoryBean private String parameterName = "format"; - private boolean favorPathExtension = true; + private boolean favorPathExtension = false; private Map mediaTypes = new HashMap<>(); diff --git a/spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java b/spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java index ec85f08c99..24628334e7 100644 --- a/spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java +++ b/spring-web/src/test/java/org/springframework/web/accept/ContentNegotiationManagerFactoryBeanTests.java @@ -72,8 +72,8 @@ class ContentNegotiationManagerFactoryBeanTests { this.servletRequest.setRequestURI("/flower.gif"); assertThat(manager.resolveMediaTypes(this.webRequest)) - .as("Should be able to resolve file extensions by default") - .isEqualTo(Collections.singletonList(MediaType.IMAGE_GIF)); + .as("Should not resolve file extensions by default") + .containsExactly(MediaType.ALL); this.servletRequest.setRequestURI("/flower.foobarbaz"); @@ -226,6 +226,7 @@ class ContentNegotiationManagerFactoryBeanTests { @Test void ignoreAcceptHeader() throws Exception { this.factoryBean.setIgnoreAcceptHeader(true); + this.factoryBean.setFavorParameter(true); this.factoryBean.afterPropertiesSet(); ContentNegotiationManager manager = this.factoryBean.getObject(); diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/config/MvcNamespaceTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/config/MvcNamespaceTests.java index 078abe5857..1a1c4fcff3 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/config/MvcNamespaceTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/config/MvcNamespaceTests.java @@ -206,7 +206,9 @@ public class MvcNamespaceTests { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/foo.json"); NativeWebRequest webRequest = new ServletWebRequest(request); ContentNegotiationManager manager = mapping.getContentNegotiationManager(); - assertThat(manager.resolveMediaTypes(webRequest)).isEqualTo(Collections.singletonList(MediaType.APPLICATION_JSON)); + assertThat(manager.resolveMediaTypes(webRequest)) + .as("Should not resolve file extensions by default") + .containsExactly(MediaType.ALL); RequestMappingHandlerAdapter adapter = appContext.getBean(RequestMappingHandlerAdapter.class); assertThat(adapter).isNotNull(); @@ -743,13 +745,17 @@ public class MvcNamespaceTests { RequestMappingHandlerMapping mapping = appContext.getBean(RequestMappingHandlerMapping.class); ContentNegotiationManager manager = mapping.getContentNegotiationManager(); - MockHttpServletRequest request = new MockHttpServletRequest("GET", "/foo.xml"); + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/foo"); + request.setParameter("format", "xml"); NativeWebRequest webRequest = new ServletWebRequest(request); - assertThat(manager.resolveMediaTypes(webRequest)).isEqualTo(Collections.singletonList(MediaType.valueOf("application/rss+xml"))); + assertThat(manager.resolveMediaTypes(webRequest)) + .containsExactly(MediaType.valueOf("application/rss+xml")); ViewResolverComposite compositeResolver = this.appContext.getBean(ViewResolverComposite.class); assertThat(compositeResolver).isNotNull(); - assertThat(compositeResolver.getViewResolvers().size()).as("Actual: " + compositeResolver.getViewResolvers()).isEqualTo(1); + assertThat(compositeResolver.getViewResolvers().size()) + .as("Actual: " + compositeResolver.getViewResolvers()) + .isEqualTo(1); ViewResolver resolver = compositeResolver.getViewResolvers().get(0); assertThat(resolver.getClass()).isEqualTo(ContentNegotiatingViewResolver.class); diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/ContentNegotiationConfigurerTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/ContentNegotiationConfigurerTests.java index ea897d49fc..80c5f4195e 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/ContentNegotiationConfigurerTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/ContentNegotiationConfigurerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -59,26 +59,34 @@ public class ContentNegotiationConfigurerTests { this.servletRequest.setRequestURI("/flower.gif"); - assertThat(manager.resolveMediaTypes(this.webRequest).get(0)).as("Should be able to resolve file extensions by default").isEqualTo(MediaType.IMAGE_GIF); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .as("Should not resolve file extensions by default") + .containsExactly(MediaType.ALL); this.servletRequest.setRequestURI("/flower?format=gif"); this.servletRequest.addParameter("format", "gif"); - assertThat(manager.resolveMediaTypes(this.webRequest)).as("Should not resolve request parameters by default").isEqualTo(ContentNegotiationStrategy.MEDIA_TYPE_ALL_LIST); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .as("Should not resolve request parameters by default") + .isEqualTo(ContentNegotiationStrategy.MEDIA_TYPE_ALL_LIST); this.servletRequest.setRequestURI("/flower"); this.servletRequest.addHeader("Accept", MediaType.IMAGE_GIF_VALUE); - assertThat(manager.resolveMediaTypes(this.webRequest).get(0)).as("Should resolve Accept header by default").isEqualTo(MediaType.IMAGE_GIF); + assertThat(manager.resolveMediaTypes(this.webRequest)) + .as("Should resolve Accept header by default") + .containsExactly(MediaType.IMAGE_GIF); } @Test public void addMediaTypes() throws Exception { + this.configurer.favorParameter(true); this.configurer.mediaTypes(Collections.singletonMap("json", MediaType.APPLICATION_JSON)); ContentNegotiationManager manager = this.configurer.buildContentNegotiationManager(); - this.servletRequest.setRequestURI("/flower.json"); - assertThat(manager.resolveMediaTypes(this.webRequest).get(0)).isEqualTo(MediaType.APPLICATION_JSON); + this.servletRequest.setRequestURI("/flower"); + this.servletRequest.addParameter("format", "json"); + assertThat(manager.resolveMediaTypes(this.webRequest)).containsExactly(MediaType.APPLICATION_JSON); } @Test @@ -97,6 +105,7 @@ public class ContentNegotiationConfigurerTests { @Test public void ignoreAcceptHeader() throws Exception { this.configurer.ignoreAcceptHeader(true); + this.configurer.favorParameter(true); ContentNegotiationManager manager = this.configurer.buildContentNegotiationManager(); this.servletRequest.setRequestURI("/flower"); diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/WebMvcConfigurationSupportExtensionTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/WebMvcConfigurationSupportExtensionTests.java index 8b69c9c887..929cf231ea 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/WebMvcConfigurationSupportExtensionTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/config/annotation/WebMvcConfigurationSupportExtensionTests.java @@ -33,7 +33,6 @@ import org.springframework.core.convert.converter.Converter; import org.springframework.core.io.FileSystemResourceLoader; import org.springframework.format.FormatterRegistry; import org.springframework.http.HttpStatus; -import org.springframework.http.MediaType; import org.springframework.http.converter.HttpMessageConverter; import org.springframework.http.converter.StringHttpMessageConverter; import org.springframework.http.converter.json.MappingJackson2HttpMessageConverter; @@ -91,7 +90,6 @@ import static com.fasterxml.jackson.databind.DeserializationFeature.FAIL_ON_UNKN import static com.fasterxml.jackson.databind.MapperFeature.DEFAULT_VIEW_INCLUSION; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; -import static org.springframework.http.MediaType.APPLICATION_ATOM_XML; import static org.springframework.http.MediaType.APPLICATION_JSON; import static org.springframework.http.MediaType.APPLICATION_XML; @@ -268,34 +266,28 @@ public class WebMvcConfigurationSupportExtensionTests { @Test @SuppressWarnings("deprecation") public void contentNegotiation() throws Exception { - MockHttpServletRequest request = new MockHttpServletRequest("GET", "/foo.json"); + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/foo"); NativeWebRequest webRequest = new ServletWebRequest(request); RequestMappingHandlerMapping mapping = this.config.requestMappingHandlerMapping( this.config.mvcContentNegotiationManager(), this.config.mvcConversionService(), this.config.mvcResourceUrlProvider()); + + request.setParameter("f", "json"); ContentNegotiationManager manager = mapping.getContentNegotiationManager(); assertThat(manager.resolveMediaTypes(webRequest)).isEqualTo(Collections.singletonList(APPLICATION_JSON)); - request.setRequestURI("/foo.xml"); + request.setParameter("f", "xml"); assertThat(manager.resolveMediaTypes(webRequest)).isEqualTo(Collections.singletonList(APPLICATION_XML)); - request.setRequestURI("/foo.rss"); - assertThat(manager.resolveMediaTypes(webRequest)).isEqualTo(Collections.singletonList(MediaType.valueOf("application/rss+xml"))); - - request.setRequestURI("/foo.atom"); - assertThat(manager.resolveMediaTypes(webRequest)).isEqualTo(Collections.singletonList(APPLICATION_ATOM_XML)); - - request.setRequestURI("/foo"); - request.setParameter("f", "json"); - assertThat(manager.resolveMediaTypes(webRequest)).isEqualTo(Collections.singletonList(APPLICATION_JSON)); - - request.setRequestURI("/resources/foo.gif"); SimpleUrlHandlerMapping handlerMapping = (SimpleUrlHandlerMapping) this.config.resourceHandlerMapping( this.config.mvcContentNegotiationManager(), this.config.mvcConversionService(), this.config.mvcResourceUrlProvider()); handlerMapping.setApplicationContext(this.context); + + request = new MockHttpServletRequest("GET", "/resources/foo.gif"); HandlerExecutionChain chain = handlerMapping.getHandler(request); + assertThat(chain).isNotNull(); ResourceHttpRequestHandler handler = (ResourceHttpRequestHandler) chain.getHandler(); assertThat(handler).isNotNull(); diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java index cadaa20ddf..0f962a958b 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java @@ -1742,6 +1742,7 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl } ContentNegotiationManagerFactoryBean factoryBean = new ContentNegotiationManagerFactoryBean(); + factoryBean.setFavorPathExtension(true); factoryBean.afterPropertiesSet(); RootBeanDefinition adapterDef = new RootBeanDefinition(RequestMappingHandlerAdapter.class); @@ -1773,6 +1774,7 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl void responseBodyAsHtmlWithSuffixPresent(boolean usePathPatterns) throws Exception { initDispatcherServlet(TextRestController.class, usePathPatterns, wac -> { ContentNegotiationManagerFactoryBean factoryBean = new ContentNegotiationManagerFactoryBean(); + factoryBean.setFavorPathExtension(true); factoryBean.afterPropertiesSet(); RootBeanDefinition adapterDef = new RootBeanDefinition(RequestMappingHandlerAdapter.class); adapterDef.getPropertyValues().add("contentNegotiationManager", factoryBean.getObject()); @@ -1833,14 +1835,22 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl void responseBodyAsTextWithCssExtension(boolean usePathPatterns) throws Exception { initDispatcherServlet(TextRestController.class, usePathPatterns, wac -> { ContentNegotiationManagerFactoryBean factoryBean = new ContentNegotiationManagerFactoryBean(); + factoryBean.setFavorParameter(true); + factoryBean.addMediaType("css", MediaType.parseMediaType("text/css")); factoryBean.afterPropertiesSet(); + + RootBeanDefinition mappingDef = new RootBeanDefinition(RequestMappingHandlerMapping.class); + mappingDef.getPropertyValues().add("contentNegotiationManager", factoryBean.getObject()); + wac.registerBeanDefinition("handlerMapping", mappingDef); + RootBeanDefinition adapterDef = new RootBeanDefinition(RequestMappingHandlerAdapter.class); adapterDef.getPropertyValues().add("contentNegotiationManager", factoryBean.getObject()); wac.registerBeanDefinition("handlerAdapter", adapterDef); }); byte[] content = "body".getBytes(StandardCharsets.ISO_8859_1); - MockHttpServletRequest request = new MockHttpServletRequest("GET", "/a4.css"); + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/a4"); + request.addParameter("format", "css"); request.setContent(content); MockHttpServletResponse response = new MockHttpServletResponse(); @@ -3826,7 +3836,7 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl return body; } - @RequestMapping(path = "/a4.css", method = RequestMethod.GET) + @RequestMapping(path = "/a4", method = RequestMethod.GET) public String a4(@RequestBody String body) { return body; } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/view/ContentNegotiatingViewResolverTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/view/ContentNegotiatingViewResolverTests.java index e3b8295dfb..deef4171d2 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/view/ContentNegotiatingViewResolverTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/view/ContentNegotiatingViewResolverTests.java @@ -34,6 +34,7 @@ import org.springframework.web.accept.FixedContentNegotiationStrategy; import org.springframework.web.accept.HeaderContentNegotiationStrategy; import org.springframework.web.accept.MappingMediaTypeFileExtensionResolver; import org.springframework.web.accept.ParameterContentNegotiationStrategy; +import org.springframework.web.accept.PathExtensionContentNegotiationStrategy; import org.springframework.web.context.request.RequestContextHolder; import org.springframework.web.context.request.ServletRequestAttributes; import org.springframework.web.context.support.StaticWebApplicationContext; @@ -85,9 +86,16 @@ public class ContentNegotiatingViewResolverTests { @Test public void resolveViewNameWithPathExtension() throws Exception { - request.setRequestURI("/test.xls"); + request.setRequestURI("/test"); + request.setParameter("format", "xls"); + + String mediaType = "application/vnd.ms-excel"; + ContentNegotiationManager manager = new ContentNegotiationManager( + new ParameterContentNegotiationStrategy( + Collections.singletonMap("xls", MediaType.parseMediaType(mediaType)))); ViewResolver viewResolverMock = mock(ViewResolver.class); + viewResolver.setContentNegotiationManager(manager); viewResolver.setViewResolvers(Collections.singletonList(viewResolverMock)); viewResolver.afterPropertiesSet(); @@ -98,7 +106,7 @@ public class ContentNegotiatingViewResolverTests { given(viewResolverMock.resolveViewName(viewName, locale)).willReturn(null); given(viewResolverMock.resolveViewName(viewName + ".xls", locale)).willReturn(viewMock); - given(viewMock.getContentType()).willReturn("application/vnd.ms-excel"); + given(viewMock.getContentType()).willReturn(mediaType); View result = viewResolver.resolveViewName(viewName, locale); assertThat(result).as("Invalid view").isSameAs(viewMock); @@ -307,8 +315,12 @@ public class ContentNegotiatingViewResolverTests { public void resolveViewNameFilename() throws Exception { request.setRequestURI("/test.html"); + ContentNegotiationManager manager = + new ContentNegotiationManager(new PathExtensionContentNegotiationStrategy()); + ViewResolver viewResolverMock1 = mock(ViewResolver.class, "viewResolver1"); ViewResolver viewResolverMock2 = mock(ViewResolver.class, "viewResolver2"); + viewResolver.setContentNegotiationManager(manager); viewResolver.setViewResolvers(Arrays.asList(viewResolverMock1, viewResolverMock2)); viewResolver.afterPropertiesSet(); @@ -336,7 +348,7 @@ public class ContentNegotiatingViewResolverTests { request.setRequestURI("/test.json"); Map mapping = Collections.singletonMap("json", MediaType.APPLICATION_JSON); - org.springframework.web.accept.PathExtensionContentNegotiationStrategy pathStrategy = new org.springframework.web.accept.PathExtensionContentNegotiationStrategy(mapping); + PathExtensionContentNegotiationStrategy pathStrategy = new PathExtensionContentNegotiationStrategy(mapping); viewResolver.setContentNegotiationManager(new ContentNegotiationManager(pathStrategy)); ViewResolver viewResolverMock1 = mock(ViewResolver.class); diff --git a/spring-webmvc/src/test/resources/org/springframework/web/servlet/config/mvc-config-content-negotiation-manager.xml b/spring-webmvc/src/test/resources/org/springframework/web/servlet/config/mvc-config-content-negotiation-manager.xml index 3d2fc2d5eb..62fe63a5c7 100644 --- a/spring-webmvc/src/test/resources/org/springframework/web/servlet/config/mvc-config-content-negotiation-manager.xml +++ b/spring-webmvc/src/test/resources/org/springframework/web/servlet/config/mvc-config-content-negotiation-manager.xml @@ -14,6 +14,7 @@ + xml=application/rss+xml