Make InMemory*ClientRegistrationRepository Consistent

The previous builders with the list argument were inconsistent with their
respective builders of var args.

Author: dperezcabrera

oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/InMemoryClientRegistrationRepository.java

@@ -18,16 +18,13 @@
 import org.springframework.util.Assert;
 
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
-import java.util.concurrent.ConcurrentMap;
 import java.util.function.Function;
-import java.util.stream.Collector;
-
-import static java.util.stream.Collectors.collectingAndThen;
-import static java.util.stream.Collectors.toConcurrentMap;
+import java.util.stream.Collectors;
 
 /**
  * A {@link ClientRegistrationRepository} that stores {@link ClientRegistration}(s) in-memory.
@@ -40,6 +37,7 @@
  * @see ClientRegistration
  */
 public final class InMemoryClientRegistrationRepository implements ClientRegistrationRepository, Iterable<ClientRegistration> {
+
 	private final Map<String, ClientRegistration> registrations;
 
 	/**
@@ -48,7 +46,8 @@ public final class InMemoryClientRegistrationRepository implements ClientRegistr
 	 * @param registrations the client registration(s)
 	 */
 	public InMemoryClientRegistrationRepository(ClientRegistration... registrations) {
-		this(Arrays.asList(registrations));
+		Assert.notEmpty(registrations, "registrations cannot be empty");
+		this.registrations = createClientRegistrationIdToClientRegistration(Arrays.asList(registrations));
 	}
 
 	/**
@@ -57,14 +56,7 @@ public InMemoryClientRegistrationRepository(ClientRegistration... registrations)
 	 * @param registrations the client registration(s)
 	 */
 	public InMemoryClientRegistrationRepository(List<ClientRegistration> registrations) {
-		this(createRegistrationsMap(registrations));
-	}
-
-	private static Map<String, ClientRegistration> createRegistrationsMap(List<ClientRegistration> registrations) {
-		Assert.notEmpty(registrations, "registrations cannot be empty");
-		Collector<ClientRegistration, ?, ConcurrentMap<String, ClientRegistration>> collector =
-				toConcurrentMap(ClientRegistration::getRegistrationId, Function.identity());
-		return registrations.stream().collect(collectingAndThen(collector, Collections::unmodifiableMap));
+		this(createClientRegistrationIdToClientRegistration(registrations));
 	}
 
 	/**
@@ -79,6 +71,13 @@ public InMemoryClientRegistrationRepository(Map<String, ClientRegistration> regi
 		this.registrations = registrations;
 	}
 
+	private static Map<String, ClientRegistration> createClientRegistrationIdToClientRegistration(Collection<ClientRegistration> registrations) {
+		Assert.notNull(registrations, "registrations cannot be null");
+		return Collections.unmodifiableMap(registrations.stream()
+				.peek(registration -> Assert.notNull(registration, "registrations cannot contain null values"))
+				.collect(Collectors.toMap(ClientRegistration::getRegistrationId, Function.identity())));
+	}
+
 	@Override
 	public ClientRegistration findByRegistrationId(String registrationId) {
 		Assert.hasText(registrationId, "registrationId cannot be empty");

oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/InMemoryReactiveClientRegistrationRepository.java

@@ -18,11 +18,6 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
-import java.util.function.Function;
-import java.util.stream.Collectors;
-
-import org.springframework.util.Assert;
-import org.springframework.util.ConcurrentReferenceHashMap;
 
 import reactor.core.publisher.Mono;
 
@@ -38,20 +33,15 @@
 public final class InMemoryReactiveClientRegistrationRepository
 		implements ReactiveClientRegistrationRepository, Iterable<ClientRegistration> {
 
-	private final Map<String, ClientRegistration> clientIdToClientRegistration;
+	private final InMemoryClientRegistrationRepository delegate;
 
 	/**
 	 * Constructs an {@code InMemoryReactiveClientRegistrationRepository} using the provided parameters.
 	 *
 	 * @param registrations the client registration(s)
 	 */
 	public InMemoryReactiveClientRegistrationRepository(ClientRegistration... registrations) {
-		Assert.notEmpty(registrations, "registrations cannot be empty");
-		this.clientIdToClientRegistration = new ConcurrentReferenceHashMap<>();
-		for (ClientRegistration registration : registrations) {
-			Assert.notNull(registration, "registrations cannot contain null values");
-			this.clientIdToClientRegistration.put(registration.getRegistrationId(), registration);
-		}
+		this.delegate = new InMemoryClientRegistrationRepository(registrations);
 	}
 
 	/**
@@ -60,9 +50,7 @@ public InMemoryReactiveClientRegistrationRepository(ClientRegistration... regist
 	 * @param registrations the client registration(s)
 	 */
 	public InMemoryReactiveClientRegistrationRepository(List<ClientRegistration> registrations) {
-		Assert.notEmpty(registrations, "registrations cannot be null or empty");
-		this.clientIdToClientRegistration = registrations.stream()
-				.collect(Collectors.toConcurrentMap(ClientRegistration::getRegistrationId, Function.identity()));
+		this.delegate = new InMemoryClientRegistrationRepository(registrations);
 	}
 
 	/**
@@ -73,14 +61,13 @@ public InMemoryReactiveClientRegistrationRepository(List<ClientRegistration> reg
 	 * @since 5.2
 	 * @param registrations the {@code Map} of client registration(s)
 	 */
-	public InMemoryReactiveClientRegistrationRepository(Map<String, ClientRegistration> registrations) {
-		Assert.notNull(registrations, "registrations cannot be null");
-		this.clientIdToClientRegistration = registrations;
+	public InMemoryReactiveClientRegistrationRepository(
+			Map<String, ClientRegistration> registrations) {
+		this.delegate = new InMemoryClientRegistrationRepository(registrations);
 	}
 
-	@Override
 	public Mono<ClientRegistration> findByRegistrationId(String registrationId) {
-		return Mono.justOrEmpty(this.clientIdToClientRegistration.get(registrationId));
+		return Mono.justOrEmpty(this.delegate.findByRegistrationId(registrationId));
 	}
 
 	/**
@@ -90,6 +77,6 @@ public Mono<ClientRegistration> findByRegistrationId(String registrationId) {
 	 */
 	@Override
 	public Iterator<ClientRegistration> iterator() {
-		return this.clientIdToClientRegistration.values().iterator();
+		return delegate.iterator();
 	}
 }

oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/InMemoryClientRegistrationRepositoryTests.java

@@ -19,7 +19,6 @@
 import org.junit.Test;
 
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -39,14 +38,14 @@ public class InMemoryClientRegistrationRepositoryTests {
 	private InMemoryClientRegistrationRepository clients = new InMemoryClientRegistrationRepository(this.registration);
 
 	@Test(expected = IllegalArgumentException.class)
-	public void constructorListClientRegistrationWhenNullThenIllegalArgumentException() {
-		List<ClientRegistration> registrations = null;
-		new InMemoryClientRegistrationRepository(registrations);
+	public void constructorVarArgsListClientRegistrationWhenNullThenIllegalArgumentException() {
+		ClientRegistration nullRegistration = null;
+		new InMemoryClientRegistrationRepository(nullRegistration);
 	}
 
 	@Test(expected = IllegalArgumentException.class)
-	public void constructorListClientRegistrationWhenEmptyThenIllegalArgumentException() {
-		List<ClientRegistration> registrations = Collections.emptyList();
+	public void constructorListClientRegistrationWhenNullThenIllegalArgumentException() {
+		List<ClientRegistration> registrations = null;
 		new InMemoryClientRegistrationRepository(registrations);
 	}
 

oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/InMemoryReactiveClientRegistrationRepositoryTests.java

@@ -20,6 +20,7 @@
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import java.util.HashMap;
+import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
 
@@ -64,10 +65,16 @@ public void constructorWhenClientRegistrationListThenIllegalArgumentException()
 				.isInstanceOf(IllegalArgumentException.class);
 	}
 
+	@Test
+	public void constructorWhenClientRegistrationListHasNullThenIllegalArgumentException() {
+		List<ClientRegistration> registrations = Arrays.asList(null, registration);
+		assertThatThrownBy(() -> new InMemoryReactiveClientRegistrationRepository(registrations))
+				.isInstanceOf(IllegalArgumentException.class);
+	}
+
 	@Test
 	public void constructorWhenClientRegistrationIsNullThenIllegalArgumentException() {
-		ClientRegistration registration = null;
-		assertThatThrownBy(() -> new InMemoryReactiveClientRegistrationRepository(registration))
+		assertThatThrownBy(() -> new InMemoryReactiveClientRegistrationRepository(registration, null))
 				.isInstanceOf(IllegalArgumentException.class);
 	}