Merge pull request #789 from marklogic/feature/23425-bug

MLE-23425 Better error when ManageConfig is null

Author: rjrudin

ml-app-deployer/src/main/java/com/marklogic/mgmt/ManageClient.java

@@ -18,7 +18,6 @@
 import org.springframework.web.client.RestTemplate;
 
 import java.io.IOException;
-import java.io.StringReader;
 import java.io.StringWriter;
 import java.net.URI;
 import java.util.ArrayList;
@@ -37,41 +36,39 @@ public class ManageClient extends LoggingObject {
 	private PayloadParser payloadParser;
 
 	public ManageClient(ManageConfig config) {
-		setManageConfig(config);
+		this.manageConfig = config;
+		if (logger.isInfoEnabled()) {
+			logger.info("Initializing ManageClient with manage config of: {}", config);
+		}
 	}
 
 	/**
-	 * Uses the given ManageConfig instance to construct a Spring RestTemplate for communicating with the Manage API.
-	 * In addition, if adminUsername on the ManageConfig instance differs from username, then a separate RestTemplate is
-	 * constructed for making calls to the Manage API that need user with the manage-admin and security roles, which is
-	 * often an admin user.
+	 * Deprecated in 6.0.1 with the intention of removing in 7.0.0 so that the underlying ManageConfig can be declared
+	 * as final.
 	 *
-	 * @param config
+	 * @deprecated
 	 */
+	@Deprecated(since = "6.0.1", forRemoval = true)
 	public void setManageConfig(ManageConfig config) {
 		this.manageConfig = config;
-		if (logger.isInfoEnabled()) {
-			logger.info("Initializing ManageClient with manage config of: " + config);
-		}
 	}
 
 	/**
-	 * Use this when you want to provide your own RestTemplate as opposed to using the one that's constructed via a
-	 * ManageConfig instance.
-	 *
-	 * @param restTemplate
+	 * Deprecated in 6.0.1 as it will not work without a ManageConfig instance being set, which is then unlikely to
+	 * be consistent with the given RestTemplate.
+	 * @deprecated
 	 */
+	@Deprecated(since = "6.0.1", forRemoval = true)
 	public ManageClient(RestTemplate restTemplate) {
 		this(restTemplate, restTemplate);
 	}
 
 	/**
-	 * Use this when you want to provide your own RestTemplate as opposed to using the one that's constructed via a
-	 * ManageConfig instance.
-	 *
-	 * @param restTemplate
-	 * @param adminRestTemplate
+	 * Deprecated in 6.0.1 as it will not work without a ManageConfig instance being set, which is then unlikely to
+	 * be consistent with the given RestTemplate.
+	 * @deprecated
 	 */
+	@Deprecated(since = "6.0.1", forRemoval = true)
 	public ManageClient(RestTemplate restTemplate, RestTemplate adminRestTemplate) {
 		this.restTemplate = restTemplate;
 		this.securityUserRestTemplate = adminRestTemplate;
@@ -257,8 +254,10 @@ public HttpEntity<String> buildXmlEntity(String xml) {
 
 	protected void logRequest(String path, String contentType, String method) {
 		if (logger.isInfoEnabled()) {
-			String username = String.format("as user '%s' ", manageConfig.getUsername());
-			logger.info("Sending {} {} request {}to path: {}", contentType, method, username, buildUri(path));
+			String username = manageConfig != null && StringUtils.hasText(manageConfig.getUsername()) ?
+				String.format("as user '%s' ", manageConfig.getUsername()) : "";
+			URI uri = buildUri(path);
+			logger.info("Sending {} {} request {}to path: {}", contentType, method, username, uri);
 		}
 	}
 
@@ -268,7 +267,8 @@ protected void logSecurityUserRequest(String path, String contentType, String me
 			if (!"".equals(username)) {
 				username = String.format("as user '%s' (who should have the 'manage-admin' and 'security' roles) ", username);
 			}
-			logger.info("Sending {} {} request {}to path: {}", contentType, method, username, buildUri(path));
+			URI uri = buildUri(path);
+			logger.info("Sending {} {} request {}to path: {}", contentType, method, username, uri);
 		}
 	}
 
@@ -307,6 +307,7 @@ private void initializeSecurityUserRestTemplate() {
 	}
 
 	public URI buildUri(String path) {
+		Objects.requireNonNull(manageConfig, "A ManageConfig instance must be provided");
 		return manageConfig.buildUri(path);
 	}
 
@@ -321,6 +322,7 @@ public ManageConfig getManageConfig() {
 		return manageConfig;
 	}
 
+	@Deprecated(since = "6.0.1", forRemoval = true)
 	public void setRestTemplate(RestTemplate restTemplate) {
 		this.restTemplate = restTemplate;
 	}
@@ -332,6 +334,7 @@ public RestTemplate getSecurityUserRestTemplate() {
 		return securityUserRestTemplate;
 	}
 
+	@Deprecated(since = "6.0.1", forRemoval = true)
 	public void setSecurityUserRestTemplate(RestTemplate restTemplate) {
 		this.securityUserRestTemplate = restTemplate;
 	}

ml-app-deployer/src/test/java/com/marklogic/mgmt/ManageClientTest.java

@@ -3,13 +3,16 @@
  */
 package com.marklogic.mgmt;
 
-import static org.junit.jupiter.api.Assertions.*;
+import com.marklogic.mgmt.resource.databases.DatabaseManager;
 import org.junit.jupiter.api.Test;
 
-public class ManageClientTest  {
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+class ManageClientTest {
 
 	@Test
-	public void determineUsernameForSecurityUserRequest() {
+	void determineUsernameForSecurityUserRequest() {
 		ManageConfig config = new ManageConfig("localhost", 8002, "someone", "someword");
 		config.setSecurityUsername("admin");
 		config.setSecurityPassword("admin");
@@ -20,4 +23,15 @@ public void determineUsernameForSecurityUserRequest() {
 		config.setSecurityUsername(null);
 		assertEquals("someone", client.determineUsernameForSecurityUserRequest());
 	}
+
+	@Test
+	void nullManageConfig() {
+		ManageClient client = new ManageClient(null);
+		NullPointerException npe = assertThrows(NullPointerException.class, () -> new DatabaseManager(client).getAsXml());
+		assertEquals("A ManageConfig instance must be provided", npe.getMessage(),
+			"It's possible to pass in null as the ManageConfig since there's still a setManageConfig method, but that's been " +
+				"deprecated so that it can be removed in 7.0.0. The goal is to have ManageConfig be final once " +
+				"it's set, and ideally hidden as well so that the ManageClient is effectively immutable. " +
+				"In the meantime, we expect a nice error message if the ManageConfig is null.");
+	}
 }