Revert Map constructor for InMemoryReactiveClientRegistrationRepository

This commit reverts f6414e9 and
partial revert of e1b095d.
NOTE: InMemoryReactiveClientRegistrationRepository should not expose a
Map constructor as it would allow the caller to pass in a 'distributed' (remote) Map,
which would result in a blocking I/O operation.

Author: jgrandja

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

@@ -18,13 +18,16 @@
 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.Collectors;
+import java.util.stream.Collector;
+
+import static java.util.stream.Collectors.collectingAndThen;
+import static java.util.stream.Collectors.toConcurrentMap;
 
 /**
  * A {@link ClientRegistrationRepository} that stores {@link ClientRegistration}(s) in-memory.
@@ -37,7 +40,6 @@
  * @see ClientRegistration
  */
 public final class InMemoryClientRegistrationRepository implements ClientRegistrationRepository, Iterable<ClientRegistration> {
-
 	private final Map<String, ClientRegistration> registrations;
 
 	/**
@@ -46,8 +48,7 @@ public final class InMemoryClientRegistrationRepository implements ClientRegistr
 	 * @param registrations the client registration(s)
 	 */
 	public InMemoryClientRegistrationRepository(ClientRegistration... registrations) {
-		Assert.notEmpty(registrations, "registrations cannot be empty");
-		this.registrations = createClientRegistrationIdToClientRegistration(Arrays.asList(registrations));
+		this(Arrays.asList(registrations));
 	}
 
 	/**
@@ -56,7 +57,14 @@ public InMemoryClientRegistrationRepository(ClientRegistration... registrations)
 	 * @param registrations the client registration(s)
 	 */
 	public InMemoryClientRegistrationRepository(List<ClientRegistration> registrations) {
-		this(createClientRegistrationIdToClientRegistration(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));
 	}
 
 	/**
@@ -71,13 +79,6 @@ 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,30 +18,39 @@
 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;
 
 /**
  * A Reactive {@link ClientRegistrationRepository} that stores {@link ClientRegistration}(s) in-memory.
  *
  * @author Rob Winch
- * @author Vedran Pavic
  * @since 5.1
  * @see ClientRegistrationRepository
  * @see ClientRegistration
  */
 public final class InMemoryReactiveClientRegistrationRepository
 		implements ReactiveClientRegistrationRepository, Iterable<ClientRegistration> {
 
-	private final InMemoryClientRegistrationRepository delegate;
+	private final Map<String, ClientRegistration> clientIdToClientRegistration;
 
 	/**
 	 * Constructs an {@code InMemoryReactiveClientRegistrationRepository} using the provided parameters.
 	 *
 	 * @param registrations the client registration(s)
 	 */
 	public InMemoryReactiveClientRegistrationRepository(ClientRegistration... registrations) {
-		this.delegate = new InMemoryClientRegistrationRepository(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);
+		}
 	}
 
 	/**
@@ -50,24 +59,15 @@ public InMemoryReactiveClientRegistrationRepository(ClientRegistration... regist
 	 * @param registrations the client registration(s)
 	 */
 	public InMemoryReactiveClientRegistrationRepository(List<ClientRegistration> registrations) {
-		this.delegate = new InMemoryClientRegistrationRepository(registrations);
+		Assert.notEmpty(registrations, "registrations cannot be null or empty");
+		this.clientIdToClientRegistration = registrations.stream()
+				.collect(Collectors.toConcurrentMap(ClientRegistration::getRegistrationId, Function.identity()));
 	}
 
-	/**
-	 * Constructs an {@code InMemoryReactiveClientRegistrationRepository} using the provided {@code Map}
-	 * of {@link ClientRegistration#getRegistrationId() registration id} to {@link ClientRegistration}.
-	 * <b>NOTE:</b> The supplied {@code Map} must be a non-blocking {@code Map}.
-	 *
-	 * @since 5.2
-	 * @param registrations the {@code Map} of client registration(s)
-	 */
-	public InMemoryReactiveClientRegistrationRepository(
-			Map<String, ClientRegistration> registrations) {
-		this.delegate = new InMemoryClientRegistrationRepository(registrations);
-	}
 
+	@Override
 	public Mono<ClientRegistration> findByRegistrationId(String registrationId) {
-		return Mono.justOrEmpty(this.delegate.findByRegistrationId(registrationId));
+		return Mono.justOrEmpty(this.clientIdToClientRegistration.get(registrationId));
 	}
 
 	/**
@@ -77,6 +77,6 @@ public Mono<ClientRegistration> findByRegistrationId(String registrationId) {
 	 */
 	@Override
 	public Iterator<ClientRegistration> iterator() {
-		return delegate.iterator();
+		return this.clientIdToClientRegistration.values().iterator();
 	}
 }

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

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

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2018 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,21 +16,18 @@
 
 package org.springframework.security.oauth2.client.registration;
 
-import org.junit.Before;
-import org.junit.Test;
-import reactor.test.StepVerifier;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
-import java.util.Arrays;
-import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
 
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import org.junit.Before;
+import org.junit.Test;
+
+import reactor.test.StepVerifier;
 
 /**
  * @author Rob Winch
- * @author Vedran Pavic
  * @since 5.1
  */
 public class InMemoryReactiveClientRegistrationRepositoryTests {
@@ -64,33 +61,13 @@ 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() {
-		assertThatThrownBy(() -> new InMemoryReactiveClientRegistrationRepository(registration, null))
-				.isInstanceOf(IllegalArgumentException.class);
-	}
-
-	@Test
-	public void constructorWhenClientRegistrationMapIsNullThenIllegalArgumentException() {
-		Map<String, ClientRegistration> registrations = null;
-		assertThatThrownBy(() -> new InMemoryReactiveClientRegistrationRepository(registrations))
+		ClientRegistration registration = null;
+		assertThatThrownBy(() -> new InMemoryReactiveClientRegistrationRepository(registration))
 				.isInstanceOf(IllegalArgumentException.class);
 	}
 
-	@Test
-	public void constructorWhenClientRegistrationMapIsEmptyThenRepositoryIsEmpty() {
-		InMemoryReactiveClientRegistrationRepository repository = new InMemoryReactiveClientRegistrationRepository(
-				new HashMap<>());
-		assertThat(repository).isEmpty();
-	}
-
 	@Test
 	public void findByRegistrationIdWhenValidIdThenFound() {
 		StepVerifier.create(this.repository.findByRegistrationId(this.registration.getRegistrationId()))