Consistent conversion of Optional array/list arrangements

Issue: SPR-15918 Issue: SPR-15919 Issue: SPR-15676 (cherry picked from commit 15c82af)
spring-projects · Sep 26, 2017 · 53a9697 · 53a9697
1 parent d11bd64
commit 53a9697
Show file tree

Hide file tree

Showing 3 changed files with 103 additions and 10 deletions.
diff --git a/...ore/src/main/java/org/springframework/core/convert/support/ObjectToOptionalConverter.java b/...ore/src/main/java/org/springframework/core/convert/support/ObjectToOptionalConverter.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 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,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;
 
@@ -47,7 +49,11 @@ public ObjectToOptionalConverter(ConversionService conversionService) {
 
 	@Override
 	public Set<ConvertiblePair> getConvertibleTypes() {
-		return Collections.singleton(new ConvertiblePair(Object.class, Optional.class));
+		Set<ConvertiblePair> convertibleTypes = new LinkedHashSet<ConvertiblePair>(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
@@ -70,7 +76,11 @@ else if (source instanceof Optional) {
 		}
 		else if (targetType.getResolvableType() != null) {
 			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);
@@ -82,7 +92,7 @@ else if (targetType.getResolvableType() != null) {
 	private static class GenericTypeDescriptor extends TypeDescriptor {
 
 		public GenericTypeDescriptor(TypeDescriptor typeDescriptor) {
-			super(typeDescriptor.getResolvableType().getGeneric(0), null, typeDescriptor.getAnnotations());
+			super(typeDescriptor.getResolvableType().getGeneric(), null, typeDescriptor.getAnnotations());
 		}
 	}
 

diff --git a/spring-core/src/main/java/org/springframework/util/ObjectUtils.java b/spring-core/src/main/java/org/springframework/util/ObjectUtils.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 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.
@@ -131,12 +131,12 @@ public static boolean isEmpty(Object obj) {
 			return true;
 		}
 
-		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();
 		}

diff --git a/...va/org/springframework/web/method/annotation/RequestParamMethodArgumentResolverTests.java b/...va/org/springframework/web/method/annotation/RequestParamMethodArgumentResolverTests.java
@@ -84,6 +84,8 @@ public class RequestParamMethodArgumentResolverTests {
 	private MethodParameter paramRequired;
 	private MethodParameter paramNotRequired;
 	private MethodParameter paramOptional;
+	private MethodParameter paramOptionalArray;
+	private MethodParameter paramOptionalList;
 	private MethodParameter multipartFileOptional;
 
 	private NativeWebRequest webRequest;
@@ -119,7 +121,9 @@ public void setUp() throws Exception {
 		paramRequired = new SynthesizingMethodParameter(method, 15);
 		paramNotRequired = new SynthesizingMethodParameter(method, 16);
 		paramOptional = new SynthesizingMethodParameter(method, 17);
-		multipartFileOptional = new SynthesizingMethodParameter(method, 18);
+		paramOptionalArray = new SynthesizingMethodParameter(method, 18);
+		paramOptionalList = new SynthesizingMethodParameter(method, 19);
+		multipartFileOptional = new SynthesizingMethodParameter(method, 20);
 
 		request = new MockHttpServletRequest();
 		webRequest = new ServletWebRequest(request, new MockHttpServletResponse());
@@ -437,6 +441,83 @@ public void resolveOptionalParamValue() throws Exception {
 		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);
+
+		Object result = resolver.resolveArgument(paramOptional, null, webRequest, binderFactory);
+		assertEquals(Optional.empty(), result);
+
+		result = resolver.resolveArgument(paramOptional, 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);
+
+		Object result = resolver.resolveArgument(paramOptionalArray, null, webRequest, binderFactory);
+		assertEquals(Optional.empty(), result);
+
+		this.request.addParameter("name", "123", "456");
+		result = resolver.resolveArgument(paramOptionalArray, 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);
+
+		Object result = resolver.resolveArgument(paramOptionalArray, null, webRequest, binderFactory);
+		assertEquals(Optional.empty(), result);
+
+		result = resolver.resolveArgument(paramOptionalArray, 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);
+
+		Object result = resolver.resolveArgument(paramOptionalList, null, webRequest, binderFactory);
+		assertEquals(Optional.empty(), result);
+
+		this.request.addParameter("name", "123", "456");
+		result = resolver.resolveArgument(paramOptionalList, 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);
+
+		Object result = resolver.resolveArgument(paramOptionalList, null, webRequest, binderFactory);
+		assertEquals(Optional.empty(), result);
+
+		result = resolver.resolveArgument(paramOptionalList, null, webRequest, binderFactory);
+		assertEquals(Optional.class, result.getClass());
+		assertFalse(((Optional) result).isPresent());
+	}
+
 	@Test
 	public void resolveOptionalMultipartFile() throws Exception {
 		ConfigurableWebBindingInitializer initializer = new ConfigurableWebBindingInitializer();
@@ -493,6 +574,8 @@ public void handle(
 			@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) {
 	}