Polish "Fallback to ping if Solr URL references core"

See gh-16477

spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/solr/SolrHealthIndicator.java

@@ -16,20 +16,15 @@
 
 package org.springframework.boot.actuate.solr;
 
-import java.io.IOException;
-
 import org.apache.solr.client.solrj.SolrClient;
-import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.impl.HttpSolrClient.RemoteSolrException;
-import org.apache.solr.client.solrj.impl.HttpSolrClient;
 import org.apache.solr.client.solrj.request.CoreAdminRequest;
-import org.apache.solr.client.solrj.response.CoreAdminResponse;
 import org.apache.solr.common.params.CoreAdminParams;
 
 import org.springframework.boot.actuate.health.AbstractHealthIndicator;
 import org.springframework.boot.actuate.health.Health;
 import org.springframework.boot.actuate.health.HealthIndicator;
 import org.springframework.boot.actuate.health.Status;
-import org.springframework.http.HttpStatus;
 
 /**
  * {@link HealthIndicator} for Apache Solr.
@@ -37,78 +32,104 @@ import org.springframework.http.HttpStatus;
  * @author Andy Wilkinson
  * @author Stephane Nicoll
  * @author Markus Schuch
+ * @author Phillip Webb
  * @since 2.0.0
  */
 public class SolrHealthIndicator extends AbstractHealthIndicator {
 
+	private static final int HTTP_NOT_FOUND_STATUS = 404;
+
 	private final SolrClient solrClient;
 
-	private PathType detectedPathType = PathType.UNKNOWN;
+	private volatile StatusCheck statusCheck;
 
 	public SolrHealthIndicator(SolrClient solrClient) {
 		super("Solr health check failed");
 		this.solrClient = solrClient;
 	}
 
+	@Override
 	protected void doHealthCheck(Health.Builder builder) throws Exception {
-		int statusCode;
+		int statusCode = initializeStatusCheck();
+		Status status = (statusCode != 0) ? Status.DOWN : Status.UP;
+		builder.status(status).withDetail("status", statusCode).withDetail("detectedPathType",
+				this.statusCheck.getPathType());
+	}
 
-		if (this.detectedPathType == PathType.ROOT) {
+	private int initializeStatusCheck() throws Exception {
-			statusCode = doCoreAdminCheck();
+		StatusCheck statusCheck = this.statusCheck;
+		if (statusCheck != null) {
+			// Already initilized
+			return statusCheck.getStatus(this.solrClient);
 		}
-		else if (this.detectedPathType == PathType.PARTICULAR_CORE) {
-			statusCode = doPingCheck();
-		}
-		else {
-			// We do not know yet, which is the promising
-			// health check strategy, so we start trying with
-			// a CoreAdmin check, which is the common case
 		try {
-				statusCode = doCoreAdminCheck();
+			return initializeStatusCheck(new RootStatusCheck());
-				// When the CoreAdmin request returns with a
-				// valid response, we can assume that this
-				// SolrClient is configured with a baseUrl
-				// pointing to the root path of the Solr instance
-				this.detectedPathType = PathType.ROOT;
 		}
-			catch (HttpSolrClient.RemoteSolrException ex) {
+		catch (RemoteSolrException ex) {
-				// CoreAdmin requests to not work with
+			// 404 is thrown when SolrClient has a baseUrl pointing to a particular core.
-				// SolrClients configured with a baseUrl pointing to
+			if (ex.code() == HTTP_NOT_FOUND_STATUS) {
-				// a particular core and a 404 response indicates
+				return initializeStatusCheck(new ParticularCoreStatusCheck());
-				// that this might be the case.
-				if (ex.code() == HttpStatus.NOT_FOUND.value()) {
-					statusCode = doPingCheck();
-					// When the SolrPing returns with a valid
-					// response, we can assume that the baseUrl
-					// of this SolrClient points to a particular core
-					this.detectedPathType = PathType.PARTICULAR_CORE;
 			}
-				else {
-					// Rethrow every other response code leaving us
-					// in the dark about the type of the baseUrl
 			throw ex;
 		}
 	}
-		}
+
-		Status status = (statusCode != 0) ? Status.DOWN : Status.UP;
+	private int initializeStatusCheck(StatusCheck statusCheck) throws Exception {
-		builder.status(status).withDetail("status", statusCode).withDetail("detectedPathType",
+		int result = statusCheck.getStatus(this.solrClient);
-				this.detectedPathType.toString());
+		this.statusCheck = statusCheck;
+		return result;
 	}
 
-	private int doCoreAdminCheck() throws IOException, SolrServerException {
+	/**
+	 * Strategy used to perform the status check.
+	 */
+	private abstract static class StatusCheck {
+
+		private final String pathType;
+
+		StatusCheck(String pathType) {
+			this.pathType = pathType;
+		}
+
+		abstract int getStatus(SolrClient client) throws Exception;
+
+		String getPathType() {
+			return this.pathType;
+		}
+
+	}
+
+	/**
+	 * {@link StatusCheck} used when {@code baseUrl} points to the root context.
+	 */
+	private static class RootStatusCheck extends StatusCheck {
+
+		RootStatusCheck() {
+			super("root");
+		}
+
+		@Override
+		public int getStatus(SolrClient client) throws Exception {
 			CoreAdminRequest request = new CoreAdminRequest();
 			request.setAction(CoreAdminParams.CoreAdminAction.STATUS);
-		CoreAdminResponse response = request.process(this.solrClient);
+			return request.process(client).getStatus();
-		return response.getStatus();
 		}
 
-	private int doPingCheck() throws IOException, SolrServerException {
-		return this.solrClient.ping().getStatus();
 	}
 
-	enum PathType {
+	/**
+	 * {@link StatusCheck} used when {@code baseUrl} points to the particular core.
+	 */
+	private static class ParticularCoreStatusCheck extends StatusCheck {
 
-		ROOT, PARTICULAR_CORE, UNKNOWN
+		ParticularCoreStatusCheck() {
+			super("particular core");
+		}
+
+		@Override
+		public int getStatus(SolrClient client) throws Exception {
+			return client.ping().getStatus();
+		}
 
 	}
 

spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/solr/SolrHealthIndicatorTests.java

@@ -19,7 +19,7 @@ package org.springframework.boot.actuate.solr;
 import java.io.IOException;
 
 import org.apache.solr.client.solrj.SolrClient;
-import org.apache.solr.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.client.solrj.impl.HttpSolrClient.RemoteSolrException;
 import org.apache.solr.client.solrj.request.CoreAdminRequest;
 import org.apache.solr.client.solrj.response.SolrPingResponse;
 import org.apache.solr.common.util.NamedList;
@@ -43,6 +43,8 @@ import static org.mockito.Mockito.verifyNoMoreInteractions;
  * Tests for {@link SolrHealthIndicator}
  *
  * @author Andy Wilkinson
+ * @author Markus Schuch
+ * @author Phillip Webb
  */
 public class SolrHealthIndicatorTests {
 
@@ -56,40 +58,69 @@ public class SolrHealthIndicatorTests {
 	}
 
 	@Test
-	public void solrIsUpWithBaseUrlPointsToRoot() throws Exception {
+	public void healthWhenSolrStatusUpAndBaseUrlPointsToRootReturnsUp() throws Exception {
 		SolrClient solrClient = mock(SolrClient.class);
 		given(solrClient.request(any(CoreAdminRequest.class), isNull())).willReturn(mockResponse(0));
 		SolrHealthIndicator healthIndicator = new SolrHealthIndicator(solrClient);
-		Health health = healthIndicator.health();
+		assertHealth(healthIndicator, Status.UP, 0, "root");
-		assertThat(health.getStatus()).isEqualTo(Status.UP);
-		assertThat(health.getDetails().get("status")).isEqualTo(0);
-		assertThat(health.getDetails().get("detectedPathType")).isEqualTo(SolrHealthIndicator.PathType.ROOT.toString());
 		verify(solrClient, times(1)).request(any(CoreAdminRequest.class), isNull());
 		verifyNoMoreInteractions(solrClient);
 	}
 
 	@Test
-	public void solrIsUpWithBaseUrlPointsToParticularCore() throws Exception {
+	public void healthWhenSolrStatusDownAndBaseUrlPointsToRootReturnsDown() throws Exception {
+		SolrClient solrClient = mock(SolrClient.class);
+		given(solrClient.request(any(CoreAdminRequest.class), isNull())).willReturn(mockResponse(400));
+		SolrHealthIndicator healthIndicator = new SolrHealthIndicator(solrClient);
+		assertHealth(healthIndicator, Status.DOWN, 400, "root");
+		verify(solrClient, times(1)).request(any(CoreAdminRequest.class), isNull());
+		verifyNoMoreInteractions(solrClient);
+	}
+
+	@Test
+	public void healthWhenSolrStatusUpAndBaseUrlPointsToParticularCoreReturnsUp() throws Exception {
 		SolrClient solrClient = mock(SolrClient.class);
 		given(solrClient.request(any(CoreAdminRequest.class), isNull()))
-				.willThrow(new HttpSolrClient.RemoteSolrException("mock", 404, "", null));
+				.willThrow(new RemoteSolrException("mock", 404, "", null));
 		given(solrClient.ping()).willReturn(mockPingResponse(0));
 		SolrHealthIndicator healthIndicator = new SolrHealthIndicator(solrClient);
-		Health health = healthIndicator.health();
+		assertHealth(healthIndicator, Status.UP, 0, "particular core");
-		assertThat(health.getStatus()).isEqualTo(Status.UP);
-		assertThat(health.getDetails().get("status")).isEqualTo(0);
-		assertThat(health.getDetails().get("detectedPathType"))
-				.isEqualTo(SolrHealthIndicator.PathType.PARTICULAR_CORE.toString());
 		verify(solrClient, times(1)).request(any(CoreAdminRequest.class), isNull());
 		verify(solrClient, times(1)).ping();
 		verifyNoMoreInteractions(solrClient);
 	}
 
 	@Test
-	public void pathTypeIsRememberedForConsecutiveChecks() throws Exception {
+	public void healthWhenSolrStatusDownAndBaseUrlPointsToParticularCoreReturnsDown() throws Exception {
 		SolrClient solrClient = mock(SolrClient.class);
 		given(solrClient.request(any(CoreAdminRequest.class), isNull()))
-				.willThrow(new HttpSolrClient.RemoteSolrException("mock", 404, "", null));
+				.willThrow(new RemoteSolrException("mock", 404, "", null));
+		given(solrClient.ping()).willReturn(mockPingResponse(400));
+		SolrHealthIndicator healthIndicator = new SolrHealthIndicator(solrClient);
+		assertHealth(healthIndicator, Status.DOWN, 400, "particular core");
+		verify(solrClient, times(1)).request(any(CoreAdminRequest.class), isNull());
+		verify(solrClient, times(1)).ping();
+		verifyNoMoreInteractions(solrClient);
+	}
+
+	@Test
+	public void healthWhenSolrConnectionFailsReturnsDown() throws Exception {
+		SolrClient solrClient = mock(SolrClient.class);
+		given(solrClient.request(any(CoreAdminRequest.class), isNull()))
+				.willThrow(new IOException("Connection failed"));
+		SolrHealthIndicator healthIndicator = new SolrHealthIndicator(solrClient);
+		Health health = healthIndicator.health();
+		assertThat(health.getStatus()).isEqualTo(Status.DOWN);
+		assertThat((String) health.getDetails().get("error")).contains("Connection failed");
+		verify(solrClient, times(1)).request(any(CoreAdminRequest.class), isNull());
+		verifyNoMoreInteractions(solrClient);
+	}
+
+	@Test
+	public void healthWhenMakingMultipleCallsRemembersStatusStrategy() throws Exception {
+		SolrClient solrClient = mock(SolrClient.class);
+		given(solrClient.request(any(CoreAdminRequest.class), isNull()))
+				.willThrow(new RemoteSolrException("mock", 404, "", null));
 		given(solrClient.ping()).willReturn(mockPingResponse(0));
 		SolrHealthIndicator healthIndicator = new SolrHealthIndicator(solrClient);
 		healthIndicator.health();
@@ -101,47 +132,12 @@ public class SolrHealthIndicatorTests {
 		verifyNoMoreInteractions(solrClient);
 	}
 
-	@Test
+	private void assertHealth(SolrHealthIndicator healthIndicator, Status expectedStatus, int expectedStatusCode,
-	public void solrIsUpAndRequestFailedWithBaseUrlPointsToRoot() throws Exception {
+			String expectedPathType) {
-		SolrClient solrClient = mock(SolrClient.class);
-		given(solrClient.request(any(CoreAdminRequest.class), isNull())).willReturn(mockResponse(400));
-		SolrHealthIndicator healthIndicator = new SolrHealthIndicator(solrClient);
 		Health health = healthIndicator.health();
-		assertThat(health.getStatus()).isEqualTo(Status.DOWN);
+		assertThat(health.getStatus()).isEqualTo(expectedStatus);
-		assertThat(health.getDetails().get("status")).isEqualTo(400);
+		assertThat(health.getDetails().get("status")).isEqualTo(expectedStatusCode);
-		assertThat(health.getDetails().get("detectedPathType")).isEqualTo(SolrHealthIndicator.PathType.ROOT.toString());
+		assertThat(health.getDetails().get("detectedPathType")).isEqualTo(expectedPathType);
-		verify(solrClient, times(1)).request(any(CoreAdminRequest.class), isNull());
-		verifyNoMoreInteractions(solrClient);
-	}
-
-	@Test
-	public void solrIsUpAndRequestFailedWithBaseUrlPointsToParticularCore() throws Exception {
-		SolrClient solrClient = mock(SolrClient.class);
-		given(solrClient.request(any(CoreAdminRequest.class), isNull()))
-				.willThrow(new HttpSolrClient.RemoteSolrException("mock", 404, "", null));
-		given(solrClient.ping()).willReturn(mockPingResponse(400));
-		SolrHealthIndicator healthIndicator = new SolrHealthIndicator(solrClient);
-		Health health = healthIndicator.health();
-		assertThat(health.getStatus()).isEqualTo(Status.DOWN);
-		assertThat(health.getDetails().get("status")).isEqualTo(400);
-		assertThat(health.getDetails().get("detectedPathType"))
-				.isEqualTo(SolrHealthIndicator.PathType.PARTICULAR_CORE.toString());
-		verify(solrClient, times(1)).request(any(CoreAdminRequest.class), isNull());
-		verify(solrClient, times(1)).ping();
-		verifyNoMoreInteractions(solrClient);
-	}
-
-	@Test
-	public void solrIsDown() throws Exception {
-		SolrClient solrClient = mock(SolrClient.class);
-		given(solrClient.request(any(CoreAdminRequest.class), isNull()))
-				.willThrow(new IOException("Connection failed"));
-		SolrHealthIndicator healthIndicator = new SolrHealthIndicator(solrClient);
-		Health health = healthIndicator.health();
-		assertThat(health.getStatus()).isEqualTo(Status.DOWN);
-		assertThat((String) health.getDetails().get("error")).contains("Connection failed");
-		verify(solrClient, times(1)).request(any(CoreAdminRequest.class), isNull());
-		verifyNoMoreInteractions(solrClient);
 	}
 
 	private NamedList<Object> mockResponse(int status) {