Separate from expectations from response creation

This commit separates the creation of a response so that it is executed
after the synchronized block inside which requests need to be matched
and validated (for order and count).

This allows a ResponseCreator to be slow or block if it has to.

Issue: SPR-16319

Author: rstoyanchev

spring-test/src/main/java/org/springframework/test/web/client/AbstractRequestExpectationManager.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 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.
@@ -73,19 +73,29 @@ public ResponseActions expectRequest(ExpectedCount count, RequestMatcher matcher
 		return expectation;
 	}
 
+	@SuppressWarnings("deprecation")
 	@Override
 	public ClientHttpResponse validateRequest(ClientHttpRequest request) throws IOException {
+		RequestExpectation expectation = null;
 		synchronized (this.requests) {
 			if (this.requests.isEmpty()) {
 				afterExpectationsDeclared();
 			}
 			try {
-				return validateRequestInternal(request);
+				// Try this first for backwards compatibility
+				ClientHttpResponse response = validateRequestInternal(request);
+				if (response != null) {
+					return response;
+				}
+				else {
+					expectation = matchRequest(request);
+				}
 			}
 			finally {
 				this.requests.add(request);
 			}
 		}
+		return expectation.createResponse(request);
 	}
 
 	/**
@@ -98,9 +108,34 @@ protected void afterExpectationsDeclared() {
 	/**
 	 * Subclasses must implement the actual validation of the request
 	 * matching to declared expectations.
+	 * @deprecated as of 5.0.3 sub-classes should implement
+	 * {@link #matchRequest(ClientHttpRequest)} instead and return only the matched
+	 * expectation, leaving the call to create the response as a separate step
+	 * (to be invoked by this class).
 	 */
-	protected abstract ClientHttpResponse validateRequestInternal(ClientHttpRequest request)
-			throws IOException;
+	@Deprecated
+	@Nullable
+	protected ClientHttpResponse validateRequestInternal(ClientHttpRequest request)
+			throws IOException {
+
+		return null;
+	}
+
+	/**
+	 * As of 5.0.3 subclasses should implement this method instead of
+	 * {@link #validateRequestInternal(ClientHttpRequest)} in order to match the
+	 * request to an expectation, leaving the call to create the response as a separate step
+	 * (to be invoked by this class).
+	 * @param request the current request
+	 * @return the matched expectation with its request count updated via
+	 * {@link RequestExpectation#incrementAndValidate()}.
+	 * @since 5.0.3
+	 */
+	protected RequestExpectation matchRequest(ClientHttpRequest request) throws IOException {
+		throw new java.lang.UnsupportedOperationException(
+				"It looks like neither the deprecated \"validateRequestInternal\"" +
+						"nor its replacement (this method) are implemented.");
+	}
 
 	@Override
 	public void verify() {
@@ -162,10 +197,15 @@ protected static class RequestExpectationGroup {
 
 		private final Set<RequestExpectation> expectations = new LinkedHashSet<>();
 
+
 		public Set<RequestExpectation> getExpectations() {
 			return this.expectations;
 		}
 
+		public void addAllExpectations(Collection<RequestExpectation> expectations) {
+			this.expectations.addAll(expectations);
+		}
+
 
 		/**
 		 * Return a matching expectation, or {@code null} if none match.
@@ -186,10 +226,15 @@ public RequestExpectation findExpectation(ClientHttpRequest request) throws IOEx
 
 		/**
 		 * Invoke this for an expectation that has been matched.
-		 * <p>The given expectation will either be stored if it has a remaining
-		 * count or it will be removed otherwise.
+		 * <p>The count of the given expectation is incremented, then it is
+		 * either stored if remainingCount > 0 or removed otherwise.
 		 */
 		public void update(RequestExpectation expectation) {
+			expectation.incrementAndValidate();
+			updateInternal(expectation);
+		}
+
+		private void updateInternal(RequestExpectation expectation) {
 			if (expectation.hasRemainingCount()) {
 				this.expectations.add(expectation);
 			}
@@ -199,11 +244,12 @@ public void update(RequestExpectation expectation) {
 		}
 
 		/**
-		 * Collection variant of {@link #update(RequestExpectation)} that can
-		 * be used to insert expectations.
+		 * Add expectations to this group.
+		 * @deprecated as of 5.0.3 please use {@link #addAllExpectations(Collection)} instead.
 		 */
+		@Deprecated
 		public void updateAll(Collection<RequestExpectation> expectations) {
-			expectations.forEach(this::update);
+			expectations.forEach(this::updateInternal);
 		}
 
 		public void reset() {

spring-test/src/main/java/org/springframework/test/web/client/DefaultRequestExpectation.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 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.
@@ -88,11 +88,16 @@ public void match(ClientHttpRequest request) throws IOException {
 		}
 	}
 
+	/**
+	 * Note that as of 5.0.3, the creation of the response, which may block
+	 * intentionally, is separated from request count tracking, and this
+	 * method no longer increments the count transparently. Instead
+	 * {@link #incrementAndValidate()} must be invoked independently.
+	 */
 	@Override
 	public ClientHttpResponse createResponse(@Nullable ClientHttpRequest request) throws IOException {
 		ResponseCreator responseCreator = getResponseCreator();
 		Assert.state(responseCreator != null, "createResponse() called before ResponseCreator was set");
-		getRequestCount().incrementAndValidate();
 		return responseCreator.createResponse(request);
 	}
 
@@ -101,6 +106,11 @@ public boolean hasRemainingCount() {
 		return getRequestCount().hasRemainingCount();
 	}
 
+	@Override
+	public void incrementAndValidate() {
+		getRequestCount().incrementAndValidate();
+	}
+
 	@Override
 	public boolean isSatisfied() {
 		return getRequestCount().isSatisfied();

spring-test/src/main/java/org/springframework/test/web/client/RequestExpectation.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 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.
@@ -34,6 +34,12 @@ public interface RequestExpectation extends ResponseActions, RequestMatcher, Res
 	 */
 	boolean hasRemainingCount();
 
+	/**
+	 * Increase the matched request count and check we haven't passed the max count.
+	 * @since 5.0.3
+	 */
+	void incrementAndValidate();
+
 	/**
 	 * Whether the requirements for this request expectation have been met.
 	 */

spring-test/src/main/java/org/springframework/test/web/client/SimpleRequestExpectationManager.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 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.
@@ -20,7 +20,6 @@
 import java.util.Iterator;
 
 import org.springframework.http.client.ClientHttpRequest;
-import org.springframework.http.client.ClientHttpResponse;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 
@@ -53,7 +52,7 @@ protected void afterExpectationsDeclared() {
 	}
 
 	@Override
-	public ClientHttpResponse validateRequestInternal(ClientHttpRequest request) throws IOException {
+	protected RequestExpectation matchRequest(ClientHttpRequest request) throws IOException {
 		RequestExpectation expectation = this.repeatExpectations.findExpectation(request);
 		if (expectation == null) {
 			if (this.expectationIterator == null || !this.expectationIterator.hasNext()) {
@@ -62,9 +61,8 @@ public ClientHttpResponse validateRequestInternal(ClientHttpRequest request) thr
 			expectation = this.expectationIterator.next();
 			expectation.match(request);
 		}
-		ClientHttpResponse response = expectation.createResponse(request);
 		this.repeatExpectations.update(expectation);
-		return response;
+		return expectation;
 	}
 
 	@Override

spring-test/src/main/java/org/springframework/test/web/client/UnorderedRequestExpectationManager.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 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.
@@ -19,7 +19,6 @@
 import java.io.IOException;
 
 import org.springframework.http.client.ClientHttpRequest;
-import org.springframework.http.client.ClientHttpResponse;
 
 /**
  * {@code RequestExpectationManager} that matches requests to expectations
@@ -35,18 +34,17 @@ public class UnorderedRequestExpectationManager extends AbstractRequestExpectati
 
 	@Override
 	protected void afterExpectationsDeclared() {
-		this.remainingExpectations.updateAll(getExpectations());
+		this.remainingExpectations.addAllExpectations(getExpectations());
 	}
 
 	@Override
-	public ClientHttpResponse validateRequestInternal(ClientHttpRequest request) throws IOException {
+	public RequestExpectation matchRequest(ClientHttpRequest request) throws IOException {
 		RequestExpectation expectation = this.remainingExpectations.findExpectation(request);
 		if (expectation == null) {
 			throw createUnexpectedRequestError(request);
 		}
-		ClientHttpResponse response = expectation.createResponse(request);
 		this.remainingExpectations.update(expectation);
-		return response;
+		return expectation;
 	}
 
 	@Override

spring-test/src/test/java/org/springframework/test/web/client/DefaultRequestExpectationTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 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.
@@ -62,26 +62,26 @@ public void matchWithFailedExpectation() throws Exception {
 	}
 
 	@Test
-	public void hasRemainingCount() throws Exception {
+	public void hasRemainingCount() {
 		RequestExpectation expectation = new DefaultRequestExpectation(twice(), requestTo("/foo"));
 		expectation.andRespond(withSuccess());
 
-		expectation.createResponse(createRequest(GET, "/foo"));
+		expectation.incrementAndValidate();
 		assertTrue(expectation.hasRemainingCount());
 
-		expectation.createResponse(createRequest(GET, "/foo"));
+		expectation.incrementAndValidate();
 		assertFalse(expectation.hasRemainingCount());
 	}
 
 	@Test
-	public void isSatisfied() throws Exception {
+	public void isSatisfied() {
 		RequestExpectation expectation = new DefaultRequestExpectation(twice(), requestTo("/foo"));
 		expectation.andRespond(withSuccess());
 
-		expectation.createResponse(createRequest(GET, "/foo"));
+		expectation.incrementAndValidate();
 		assertFalse(expectation.isSatisfied());
 
-		expectation.createResponse(createRequest(GET, "/foo"));
+		expectation.incrementAndValidate();
 		assertTrue(expectation.isSatisfied());
 	}