Consistent conversion of Optional array/list arrangements

Issue: SPR-15918
Issue: SPR-15919
Issue: SPR-15676

spring-core/src/main/java/org/springframework/core/convert/support/ObjectToOptionalConverter.java

@@ -16,7 +16,9 @@
 
 package org.springframework.core.convert.support;
 
-import java.util.Collections;
+import java.lang.reflect.Array;
+import java.util.Collection;
+import java.util.LinkedHashSet;
 import java.util.Optional;
 import java.util.Set;
 
@@ -46,7 +48,11 @@ final class ObjectToOptionalConverter implements ConditionalGenericConverter {
 
 	@Override
 	public Set<ConvertiblePair> getConvertibleTypes() {
-		return Collections.singleton(new ConvertiblePair(Object.class, Optional.class));
+		Set<ConvertiblePair> convertibleTypes = new LinkedHashSet<>(4);
+		convertibleTypes.add(new ConvertiblePair(Collection.class, Optional.class));
+		convertibleTypes.add(new ConvertiblePair(Object[].class, Optional.class));
+		convertibleTypes.add(new ConvertiblePair(Object.class, Optional.class));
+		return convertibleTypes;
 	}
 
 	@Override
@@ -69,7 +75,11 @@ final class ObjectToOptionalConverter implements ConditionalGenericConverter {
 		}
 		else if (targetType.getResolvableType().hasGenerics()) {
 			Object target = this.conversionService.convert(source, sourceType, new GenericTypeDescriptor(targetType));
-			return Optional.ofNullable(target);
+			if (target == null || (target.getClass().isArray() && Array.getLength(target) == 0) ||
+						(target instanceof Collection && ((Collection) target).isEmpty())) {
+				return Optional.empty();
+			}
+			return Optional.of(target);
 		}
 		else {
 			return Optional.of(source);

spring-core/src/main/java/org/springframework/util/ObjectUtils.java

@@ -139,12 +139,12 @@ public abstract class ObjectUtils {
 		if (obj instanceof Optional) {
 			return !((Optional) obj).isPresent();
 		}
-		if (obj.getClass().isArray()) {
-			return Array.getLength(obj) == 0;
-		}
 		if (obj instanceof CharSequence) {
 			return ((CharSequence) obj).length() == 0;
 		}
+		if (obj.getClass().isArray()) {
+			return Array.getLength(obj) == 0;
+		}
 		if (obj instanceof Collection) {
 			return ((Collection) obj).isEmpty();
 		}

spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/MultipartControllerTests.java

@@ -19,7 +19,9 @@ package org.springframework.test.web.servlet.samples.standalone;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.util.Collections;
+import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import javax.servlet.Filter;
 import javax.servlet.FilterChain;
 import javax.servlet.ServletException;
@@ -28,6 +30,7 @@ import javax.servlet.http.HttpServletRequestWrapper;
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.Part;
 
+import org.junit.Assert;
 import org.junit.Test;
 
 import org.springframework.http.MediaType;
@@ -49,12 +52,12 @@ import static org.springframework.test.web.servlet.setup.MockMvcBuilders.*;
 
 /**
  * @author Rossen Stoyanchev
- * @since 5.0
+ * @author Juergen Hoeller
  */
 public class MultipartControllerTests {
 
 	@Test
-	public void multipartRequest() throws Exception {
+	public void multipartRequestWithSingleFile() throws Exception {
 		byte[] fileContent = "bar".getBytes(StandardCharsets.UTF_8);
 		MockMultipartFile filePart = new MockMultipartFile("file", "orig", null, fileContent);
 
@@ -62,12 +65,127 @@ public class MultipartControllerTests {
 		MockMultipartFile jsonPart = new MockMultipartFile("json", "json", "application/json", json);
 
 		standaloneSetup(new MultipartController()).build()
-				.perform(multipart("/test-multipartfile").file(filePart).file(jsonPart))
+				.perform(multipart("/multipartfile").file(filePart).file(jsonPart))
 				.andExpect(status().isFound())
 				.andExpect(model().attribute("fileContent", fileContent))
 				.andExpect(model().attribute("jsonContent", Collections.singletonMap("name", "yeeeah")));
 	}
 
+	@Test
+	public void multipartRequestWithFileArray() throws Exception {
+		byte[] fileContent = "bar".getBytes(StandardCharsets.UTF_8);
+		MockMultipartFile filePart1 = new MockMultipartFile("file", "orig", null, fileContent);
+		MockMultipartFile filePart2 = new MockMultipartFile("file", "orig", null, fileContent);
+
+		byte[] json = "{\"name\":\"yeeeah\"}".getBytes(StandardCharsets.UTF_8);
+		MockMultipartFile jsonPart = new MockMultipartFile("json", "json", "application/json", json);
+
+		standaloneSetup(new MultipartController()).build()
+				.perform(multipart("/multipartfilearray").file(filePart1).file(filePart2).file(jsonPart))
+				.andExpect(status().isFound())
+				.andExpect(model().attribute("fileContent", fileContent))
+				.andExpect(model().attribute("jsonContent", Collections.singletonMap("name", "yeeeah")));
+	}
+
+	@Test
+	public void multipartRequestWithFileList() throws Exception {
+		byte[] fileContent = "bar".getBytes(StandardCharsets.UTF_8);
+		MockMultipartFile filePart1 = new MockMultipartFile("file", "orig", null, fileContent);
+		MockMultipartFile filePart2 = new MockMultipartFile("file", "orig", null, fileContent);
+
+		byte[] json = "{\"name\":\"yeeeah\"}".getBytes(StandardCharsets.UTF_8);
+		MockMultipartFile jsonPart = new MockMultipartFile("json", "json", "application/json", json);
+
+		standaloneSetup(new MultipartController()).build()
+				.perform(multipart("/multipartfilelist").file(filePart1).file(filePart2).file(jsonPart))
+				.andExpect(status().isFound())
+				.andExpect(model().attribute("fileContent", fileContent))
+				.andExpect(model().attribute("jsonContent", Collections.singletonMap("name", "yeeeah")));
+	}
+
+	@Test
+	public void multipartRequestWithOptionalFile() throws Exception {
+		byte[] fileContent = "bar".getBytes(StandardCharsets.UTF_8);
+		MockMultipartFile filePart = new MockMultipartFile("file", "orig", null, fileContent);
+
+		byte[] json = "{\"name\":\"yeeeah\"}".getBytes(StandardCharsets.UTF_8);
+		MockMultipartFile jsonPart = new MockMultipartFile("json", "json", "application/json", json);
+
+		standaloneSetup(new MultipartController()).build()
+				.perform(multipart("/optionalfile").file(filePart).file(jsonPart))
+				.andExpect(status().isFound())
+				.andExpect(model().attribute("fileContent", fileContent))
+				.andExpect(model().attribute("jsonContent", Collections.singletonMap("name", "yeeeah")));
+	}
+
+	@Test
+	public void multipartRequestWithOptionalFileNotPresent() throws Exception {
+		byte[] json = "{\"name\":\"yeeeah\"}".getBytes(StandardCharsets.UTF_8);
+		MockMultipartFile jsonPart = new MockMultipartFile("json", "json", "application/json", json);
+
+		standaloneSetup(new MultipartController()).build()
+				.perform(multipart("/optionalfile").file(jsonPart))
+				.andExpect(status().isFound())
+				.andExpect(model().attributeDoesNotExist("fileContent"))
+				.andExpect(model().attribute("jsonContent", Collections.singletonMap("name", "yeeeah")));
+	}
+
+	@Test
+	public void multipartRequestWithOptionalFileArray() throws Exception {
+		byte[] fileContent = "bar".getBytes(StandardCharsets.UTF_8);
+		MockMultipartFile filePart1 = new MockMultipartFile("file", "orig", null, fileContent);
+		MockMultipartFile filePart2 = new MockMultipartFile("file", "orig", null, fileContent);
+
+		byte[] json = "{\"name\":\"yeeeah\"}".getBytes(StandardCharsets.UTF_8);
+		MockMultipartFile jsonPart = new MockMultipartFile("json", "json", "application/json", json);
+
+		standaloneSetup(new MultipartController()).build()
+				.perform(multipart("/optionalfilearray").file(filePart1).file(filePart2).file(jsonPart))
+				.andExpect(status().isFound())
+				.andExpect(model().attribute("fileContent", fileContent))
+				.andExpect(model().attribute("jsonContent", Collections.singletonMap("name", "yeeeah")));
+	}
+
+	@Test
+	public void multipartRequestWithOptionalFileArrayNotPresent() throws Exception {
+		byte[] json = "{\"name\":\"yeeeah\"}".getBytes(StandardCharsets.UTF_8);
+		MockMultipartFile jsonPart = new MockMultipartFile("json", "json", "application/json", json);
+
+		standaloneSetup(new MultipartController()).build()
+				.perform(multipart("/optionalfilearray").file(jsonPart))
+				.andExpect(status().isFound())
+				.andExpect(model().attributeDoesNotExist("fileContent"))
+				.andExpect(model().attribute("jsonContent", Collections.singletonMap("name", "yeeeah")));
+	}
+
+	@Test
+	public void multipartRequestWithOptionalFileList() throws Exception {
+		byte[] fileContent = "bar".getBytes(StandardCharsets.UTF_8);
+		MockMultipartFile filePart1 = new MockMultipartFile("file", "orig", null, fileContent);
+		MockMultipartFile filePart2 = new MockMultipartFile("file", "orig", null, fileContent);
+
+		byte[] json = "{\"name\":\"yeeeah\"}".getBytes(StandardCharsets.UTF_8);
+		MockMultipartFile jsonPart = new MockMultipartFile("json", "json", "application/json", json);
+
+		standaloneSetup(new MultipartController()).build()
+				.perform(multipart("/optionalfilelist").file(filePart1).file(filePart2).file(jsonPart))
+				.andExpect(status().isFound())
+				.andExpect(model().attribute("fileContent", fileContent))
+				.andExpect(model().attribute("jsonContent", Collections.singletonMap("name", "yeeeah")));
+	}
+
+	@Test
+	public void multipartRequestWithOptionalFileListNotPresent() throws Exception {
+		byte[] json = "{\"name\":\"yeeeah\"}".getBytes(StandardCharsets.UTF_8);
+		MockMultipartFile jsonPart = new MockMultipartFile("json", "json", "application/json", json);
+
+		standaloneSetup(new MultipartController()).build()
+				.perform(multipart("/optionalfilelist").file(jsonPart))
+				.andExpect(status().isFound())
+				.andExpect(model().attributeDoesNotExist("fileContent"))
+				.andExpect(model().attribute("jsonContent", Collections.singletonMap("name", "yeeeah")));
+	}
+
 	@Test
 	public void multipartRequestWithServletParts() throws Exception {
 		byte[] fileContent = "bar".getBytes(StandardCharsets.UTF_8);
@@ -78,7 +196,7 @@ public class MultipartControllerTests {
 		jsonPart.getHeaders().setContentType(MediaType.APPLICATION_JSON);
 
 		standaloneSetup(new MultipartController()).build()
-				.perform(multipart("/test-multipartfile").part(filePart).part(jsonPart))
+				.perform(multipart("/multipartfile").part(filePart).part(jsonPart))
 				.andExpect(status().isFound())
 				.andExpect(model().attribute("fileContent", fileContent))
 				.andExpect(model().attribute("jsonContent", Collections.singletonMap("name", "yeeeah")));
@@ -93,34 +211,98 @@ public class MultipartControllerTests {
 		MockMvc mockMvc = standaloneSetup(new MultipartController()).addFilter(filter).build();
 
 		Map<String, String> jsonMap = Collections.singletonMap("name", "yeeeah");
-		mockMvc.perform(multipart("/test-json").file(jsonPart)).andExpect(model().attribute("json", jsonMap));
+		mockMvc.perform(multipart("/json").file(jsonPart)).andExpect(model().attribute("json", jsonMap));
 	}
 
 
 	@Controller
 	private static class MultipartController {
 
-		@RequestMapping(value = "/test-multipartfile", method = RequestMethod.POST)
+		@RequestMapping(value = "/multipartfile", method = RequestMethod.POST)
 		public String processMultipartFile(@RequestParam MultipartFile file,
 				@RequestPart Map<String, String> json, Model model) throws IOException {
 
-			model.addAttribute("jsonContent", json);
 			model.addAttribute("fileContent", file.getBytes());
+			model.addAttribute("jsonContent", json);
 
 			return "redirect:/index";
 		}
 
-		@RequestMapping(value = "/test-part", method = RequestMethod.POST)
-		public String processPart(@RequestParam Part part,
+		@RequestMapping(value = "/multipartfilearray", method = RequestMethod.POST)
+		public String processMultipartFileArray(@RequestParam MultipartFile[] file,
 				@RequestPart Map<String, String> json, Model model) throws IOException {
 
+			byte[] content = file[0].getBytes();
+			Assert.assertArrayEquals(content, file[1].getBytes());
+			model.addAttribute("fileContent", content);
 			model.addAttribute("jsonContent", json);
+
+			return "redirect:/index";
+		}
+
+		@RequestMapping(value = "/multipartfilelist", method = RequestMethod.POST)
+		public String processMultipartFileList(@RequestParam List<MultipartFile> file,
+				@RequestPart Map<String, String> json, Model model) throws IOException {
+
+			byte[] content = file.get(0).getBytes();
+			Assert.assertArrayEquals(content, file.get(1).getBytes());
+			model.addAttribute("fileContent", content);
+			model.addAttribute("jsonContent", json);
+
+			return "redirect:/index";
+		}
+
+		@RequestMapping(value = "/optionalfile", method = RequestMethod.POST)
+		public String processOptionalFile(@RequestParam Optional<MultipartFile> file,
+				@RequestPart Map<String, String> json, Model model) throws IOException {
+
+			if (file.isPresent()) {
+				model.addAttribute("fileContent", file.get().getBytes());
+			}
+			model.addAttribute("jsonContent", json);
+
+			return "redirect:/index";
+		}
+
+		@RequestMapping(value = "/optionalfilearray", method = RequestMethod.POST)
+		public String processOptionalFileArray(@RequestParam Optional<MultipartFile[]> file,
+				@RequestPart Map<String, String> json, Model model) throws IOException {
+
+			if (file.isPresent()) {
+				byte[] content = file.get()[0].getBytes();
+				Assert.assertArrayEquals(content, file.get()[1].getBytes());
+				model.addAttribute("fileContent", content);
+			}
+			model.addAttribute("jsonContent", json);
+
+			return "redirect:/index";
+		}
+
+		@RequestMapping(value = "/optionalfilelist", method = RequestMethod.POST)
+		public String processOptionalFileList(@RequestParam Optional<List<MultipartFile>> file,
+				@RequestPart Map<String, String> json, Model model) throws IOException {
+
+			if (file.isPresent()) {
+				byte[] content = file.get().get(0).getBytes();
+				Assert.assertArrayEquals(content, file.get().get(1).getBytes());
+				model.addAttribute("fileContent", content);
+			}
+			model.addAttribute("jsonContent", json);
+
+			return "redirect:/index";
+		}
+
+		@RequestMapping(value = "/part", method = RequestMethod.POST)
+		public String processPart(@RequestParam Part part,
+				@RequestPart Map<String, String> json, Model model) throws IOException {
+
 			model.addAttribute("fileContent", part.getInputStream());
+			model.addAttribute("jsonContent", json);
 
 			return "redirect:/index";
 		}
 
-		@RequestMapping(value = "/test-json", method = RequestMethod.POST)
+		@RequestMapping(value = "/json", method = RequestMethod.POST)
 		public String processMultipart(@RequestPart Map<String, String> json, Model model) {
 			model.addAttribute("json", json);
 			return "redirect:/index";
@@ -139,4 +321,4 @@ public class MultipartControllerTests {
 		}
 	}
 
-}
+}

spring-web/src/test/java/org/springframework/web/method/ResolvableMethod.java

@@ -54,7 +54,7 @@ import org.springframework.util.ObjectUtils;
 import org.springframework.util.ReflectionUtils;
 import org.springframework.web.bind.annotation.ValueConstants;
 
-import static java.util.stream.Collectors.joining;
+import static java.util.stream.Collectors.*;
 
 /**
  * Convenience class to resolve method parameters from hints.
@@ -238,9 +238,8 @@ public class ResolvableMethod {
 	}
 
 	private static ResolvableType toResolvableType(Class<?> type, Class<?>... generics) {
-		return ObjectUtils.isEmpty(generics) ?
-				ResolvableType.forClass(type) :
-				ResolvableType.forClassWithGenerics(type, generics);
+		return (ObjectUtils.isEmpty(generics) ? ResolvableType.forClass(type) :
+				ResolvableType.forClassWithGenerics(type, generics));
 	}
 
 	private static ResolvableType toResolvableType(Class<?> type, ResolvableType generic, ResolvableType... generics) {

spring-web/src/test/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolverTests.java

@@ -48,16 +48,9 @@ import org.springframework.web.multipart.MultipartException;
 import org.springframework.web.multipart.MultipartFile;
 import org.springframework.web.multipart.support.MissingServletRequestPartException;
 
-import static org.junit.Assert.assertArrayEquals;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
-import static org.mockito.BDDMockito.given;
-import static org.mockito.BDDMockito.mock;
-import static org.springframework.web.method.MvcAnnotationPredicates.requestParam;
-import static org.springframework.web.method.MvcAnnotationPredicates.requestPart;
+import static org.junit.Assert.*;
+import static org.mockito.BDDMockito.*;
+import static org.springframework.web.method.MvcAnnotationPredicates.*;
 
 /**
  * Test fixture with {@link org.springframework.web.method.annotation.RequestParamMethodArgumentResolver}.
@@ -76,13 +69,15 @@ public class RequestParamMethodArgumentResolverTests {
 
 	private ResolvableMethod testMethod = ResolvableMethod.on(getClass()).named("handle").build();
 
+
 	@Before
-	public void setUp() throws Exception {
+	public void setup() throws Exception {
 		resolver = new RequestParamMethodArgumentResolver(null, true);
 		request = new MockHttpServletRequest();
 		webRequest = new ServletWebRequest(request, new MockHttpServletResponse());
 	}
 
+
 	@Test
 	public void supportsParameter() {
 		resolver = new RequestParamMethodArgumentResolver(null, true);
@@ -470,6 +465,88 @@ public class RequestParamMethodArgumentResolverTests {
 		assertEquals(123, ((Optional) result).get());
 	}
 
+	@Test
+	@SuppressWarnings("rawtypes")
+	public void missingOptionalParamValue() throws Exception {
+		ConfigurableWebBindingInitializer initializer = new ConfigurableWebBindingInitializer();
+		initializer.setConversionService(new DefaultConversionService());
+		WebDataBinderFactory binderFactory = new DefaultDataBinderFactory(initializer);
+
+		MethodParameter param = this.testMethod.annotPresent(RequestParam.class).arg(Optional.class, Integer.class);
+		Object result = resolver.resolveArgument(param, null, webRequest, binderFactory);
+		assertEquals(Optional.empty(), result);
+
+		result = resolver.resolveArgument(param, null, webRequest, binderFactory);
+		assertEquals(Optional.class, result.getClass());
+		assertFalse(((Optional) result).isPresent());
+	}
+
+	@Test
+	@SuppressWarnings("rawtypes")
+	public void resolveOptionalParamArray() throws Exception {
+		ConfigurableWebBindingInitializer initializer = new ConfigurableWebBindingInitializer();
+		initializer.setConversionService(new DefaultConversionService());
+		WebDataBinderFactory binderFactory = new DefaultDataBinderFactory(initializer);
+
+		MethodParameter param = this.testMethod.annotPresent(RequestParam.class).arg(Optional.class, Integer[].class);
+		Object result = resolver.resolveArgument(param, null, webRequest, binderFactory);
+		assertEquals(Optional.empty(), result);
+
+		this.request.addParameter("name", "123", "456");
+		result = resolver.resolveArgument(param, null, webRequest, binderFactory);
+		assertEquals(Optional.class, result.getClass());
+		assertArrayEquals(new Integer[] {123, 456}, (Integer[]) ((Optional) result).get());
+	}
+
+	@Test
+	@SuppressWarnings("rawtypes")
+	public void missingOptionalParamArray() throws Exception {
+		ConfigurableWebBindingInitializer initializer = new ConfigurableWebBindingInitializer();
+		initializer.setConversionService(new DefaultConversionService());
+		WebDataBinderFactory binderFactory = new DefaultDataBinderFactory(initializer);
+
+		MethodParameter param = this.testMethod.annotPresent(RequestParam.class).arg(Optional.class, Integer[].class);
+		Object result = resolver.resolveArgument(param, null, webRequest, binderFactory);
+		assertEquals(Optional.empty(), result);
+
+		result = resolver.resolveArgument(param, null, webRequest, binderFactory);
+		assertEquals(Optional.class, result.getClass());
+		assertFalse(((Optional) result).isPresent());
+	}
+
+	@Test
+	@SuppressWarnings("rawtypes")
+	public void resolveOptionalParamList() throws Exception {
+		ConfigurableWebBindingInitializer initializer = new ConfigurableWebBindingInitializer();
+		initializer.setConversionService(new DefaultConversionService());
+		WebDataBinderFactory binderFactory = new DefaultDataBinderFactory(initializer);
+
+		MethodParameter param = this.testMethod.annotPresent(RequestParam.class).arg(Optional.class, List.class);
+		Object result = resolver.resolveArgument(param, null, webRequest, binderFactory);
+		assertEquals(Optional.empty(), result);
+
+		this.request.addParameter("name", "123", "456");
+		result = resolver.resolveArgument(param, null, webRequest, binderFactory);
+		assertEquals(Optional.class, result.getClass());
+		assertEquals(Arrays.asList("123", "456"), ((Optional) result).get());
+	}
+
+	@Test
+	@SuppressWarnings("rawtypes")
+	public void missingOptionalParamList() throws Exception {
+		ConfigurableWebBindingInitializer initializer = new ConfigurableWebBindingInitializer();
+		initializer.setConversionService(new DefaultConversionService());
+		WebDataBinderFactory binderFactory = new DefaultDataBinderFactory(initializer);
+
+		MethodParameter param = this.testMethod.annotPresent(RequestParam.class).arg(Optional.class, List.class);
+		Object result = resolver.resolveArgument(param, null, webRequest, binderFactory);
+		assertEquals(Optional.empty(), result);
+
+		result = resolver.resolveArgument(param, null, webRequest, binderFactory);
+		assertEquals(Optional.class, result.getClass());
+		assertFalse(((Optional) result).isPresent());
+	}
+
 	@Test
 	public void resolveOptionalMultipartFile() throws Exception {
 		ConfigurableWebBindingInitializer initializer = new ConfigurableWebBindingInitializer();
@@ -536,6 +613,8 @@ public class RequestParamMethodArgumentResolverTests {
 			@RequestParam("name") String paramRequired,
 			@RequestParam(name = "name", required = false) String paramNotRequired,
 			@RequestParam("name") Optional<Integer> paramOptional,
+			@RequestParam("name") Optional<Integer[]> paramOptionalArray,
+			@RequestParam("name") Optional<List> paramOptionalList,
 			@RequestParam("mfile") Optional<MultipartFile> multipartFileOptional) {
 	}