Refactor Java remote API Command hierarchy (#48)

Author: labkey-adam

CHANGELOG.md

@@ -1,16 +1,42 @@
 # The LabKey Remote API Library for Java - Change Log
 
-## version 4.4.0-SNAPSHOT
+## version 5.1.0-SNAPSHOT
 *Released*: TBD
 
+## version 5.0.0
+*Released*: 24 January 2023
+* Refactor the `Command` class hierarchy:
+  * Add `GetCommand`, new abstract base class for all commands that use get
+  * Make `Command` and `PostCommand` abstract
+  * Add `SimpleGetCommand`, new concrete class used (instead of `Command`) to invoke GET API actions without creating a subclass
+  * Add `SimplePostCommand`, new concrete class used (instead of `PostCommand`) to invoke POST API actions without creating a subclass
+* Refactor parameter handling for consistency:
+  * `createParameterMap()` is now used by subclasses to create and populate a mutable map of URL parameters
+  * `getParameters()` now returns an immutable copy of the current URL parameter map for external use, typically logging or testing
+  * Commands no longer stash and reuse the parameter map; `createParameterMap()` always creates a new map.
+  * `setParameters()` is now available only on `SimpleGetCommand` and `SimplePostCommand`. Custom `GetCommand` and `PostCommand`
+    subclasses that need to specify parameters are expected to override `createParameterMap()`.
+  * `setJsonObject()` is now available only on `SimplePostCommand`. Custom `PostCommand` subclasses that need to post JSON are
+    expected to override `getJsonObject()`.
+* Stop passing command subclasses when constructing every `CommandResponse`. The two response classes that need this now implement
+  it without burdening all other commands. 
+* Introduce `HasRequiredVersion` interface and use it when instantiating `CommandResponse` subclasses that need required version
+* Remove all `Command` copy constructors. Same rationale as the earlier removal of `copy` methods.
+* Switch `SelectRowsCommand` and `NAbRunsCommand` to post their parameters as JSON
+* Fix NAbReplicate to handle `"NaN"` values
+* Remove `CommandException` from `getHttpRequest()` throws list
+* Adjust the `Demo.java` and `Test.java` tests to match current sample data and `Command` hierarchy changes
+
 ## version 4.3.1
 *Released*: 15 January 2023
 * Fix regression introduced in 4.3.0: `SelectRowsCommand` should create a fresh parameter map for every invocation of `getParameters()`
 
 ## version 4.3.0
 *Released*: 11 January 2023
-* [Issue 47030](https://www.labkey.org/home/Developer/issues/issues-details.view?issueId=47030): Switch `SelectRowsCommand` and `NAbRunsCommand` to always use POST
-* Add support for `includeTotalCount`, `includeMetadata`, and `ignoreFilter` flags to `BaseQueryCommand` and reconcile duplicate parameter handling code vs. SelectRowsCommand
+* [Issue 47030](https://www.labkey.org/home/Developer/issues/issues-details.view?issueId=47030): Switch `SelectRowsCommand`
+  and `NAbRunsCommand` to always use POST
+* Add support for `includeTotalCount`, `includeMetadata`, and `ignoreFilter` flags to `BaseQueryCommand` and reconcile 
+  duplicate parameter handling code vs. SelectRowsCommand
 * Add support for `includeTitle` and `includeViewDataUrl` flags to `GetQueriesCommand`
 
 ## version 4.2.0

build.gradle

@@ -48,7 +48,7 @@ repositories {
 
 group "org.labkey.api"
 
-version "4.4.0-SNAPSHOT"
+version "5.1.0-SNAPSHOT"
 
 dependencies {
     api "org.json:json:${jsonObjectVersion}"

src/org/labkey/remoteapi/BasicAuthCredentialsProvider.java

@@ -39,7 +39,7 @@ public BasicAuthCredentialsProvider(String email, String password)
     public void configureRequest(URI baseURI, HttpUriRequest request, HttpClientContext httpClientContext)
     {
         BasicCredentialsProvider provider = new BasicCredentialsProvider();
-        AuthScope scope = new AuthScope(baseURI.getHost(), -1);
+        AuthScope scope = new AuthScope(baseURI.getHost(), baseURI.getPort());
         Credentials credentials = new UsernamePasswordCredentials(_email, _password.toCharArray());
         provider.setCredentials(scope, credentials);
         httpClientContext.setCredentialsProvider(provider);

src/org/labkey/remoteapi/Command.java

@@ -17,7 +17,6 @@
 
 import org.apache.commons.logging.LogFactory;
 import org.apache.hc.client5.http.auth.AuthenticationException;
-import org.apache.hc.client5.http.classic.methods.HttpGet;
 import org.apache.hc.client5.http.classic.methods.HttpUriRequest;
 import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
 import org.apache.hc.core5.http.Header;
@@ -37,30 +36,26 @@
 import java.net.URISyntaxException;
 import java.nio.charset.StandardCharsets;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Scanner;
 
 /**
- * Base class for all API commands. Typically, a developer will not
- * use this class directly, but will instead create one of the
- * classes that extend this class. For example, to select data,
- * create an instance of {@link org.labkey.remoteapi.query.SelectRowsCommand},
- * which provides helpful methods for setting options and obtaining
- * specific result types.
+ * Abstract base class for all API commands. Developers interact with concrete classes that
+ * extend this class. For example, to select data, create an instance of
+ * {@link org.labkey.remoteapi.query.SelectRowsCommand}, which provides helpful methods for
+ * setting options and obtaining specific result types.
  * <p>
- * However, if future versions of the LabKey Server expose new HTTP APIs
- * that are not yet supported with a specialized class in this library,
- * the developer may still invoke these APIs by creating an instance of the
- * {@link Command} class directly, providing the controller and action name for
- * the new API. Parameters may then be specified by calling the {@link #setParameters(Map)}
- * method, passing a populated parameter <code>Map&lt;String, Object&gt;</code>
+ * If a developer wishes to invoke actions that are not yet supported with a specialized class
+ * in this library, the developer may invoke these APIs by creating an instance of the
+ * {@link SimpleGetCommand} or {@link SimplePostCommand} classes, providing the controller and
+ * action name for the API, and setting parameters or JSON to send.
  * <p>
- * Note that this class is not thread-safe. Do not share instances of this class
- * or its descendants between threads, unless the descendant declares explicitly that
- * it is thread-safe.
+ * Note that this class is not thread-safe. Do not share instances of this class or its
+ * descendants between threads, unless the descendant declares explicitly that it is thread-safe.
  */
-public class Command<ResponseType extends CommandResponse>
+public abstract class Command<ResponseType extends CommandResponse, RequestType extends HttpUriRequest> implements HasRequiredVersion
 {
     /**
      * A constant for the official JSON content type ("application/json")
@@ -77,7 +72,6 @@ public enum CommonParameters
 
     private final String _controllerName;
     private final String _actionName;
-    private Map<String, Object> _parameters = null;
     private Integer _timeout = null;
     private double _requiredVersion = 8.3;
 
@@ -96,20 +90,6 @@ public Command(String controllerName, String actionName)
         _actionName = actionName;
     }
 
-    /**
-     * Constructs a new Command from an existing command
-     * @param source A source Command
-     */
-    public Command(Command<ResponseType> source)
-    {
-        _actionName = source.getActionName();
-        _controllerName = source.getControllerName();
-        if (null != source.getParameters())
-            _parameters = new HashMap<>(source.getParameters());
-        _timeout = source._timeout;
-        _requiredVersion = source.getRequiredVersion();
-    }
-
     /**
      * Returns the controller name specified when constructing this object.
      * @return The controller name.
@@ -129,26 +109,23 @@ public String getActionName()
     }
 
     /**
-     * Returns the current parameter map, or null if a map has not yet been set.
-     * Derived classes will typically override this method to provide a parameter
-     * map based on data members set by specialized setter methods.
-     * @return The current parameter map.
+     * Makes an immutable copy of the parameter map used for building the URL available to callers. Typically used for
+     * logging and testing.
+     * @return An immutable map of parameters used when building the URL.
      */
-    public Map<String, Object> getParameters()
+    public final Map<String, Object> getParameters()
     {
-        if (null == _parameters)
-            _parameters = new HashMap<>();
-
-        return _parameters;
+        return Collections.unmodifiableMap(createParameterMap());
     }
 
     /**
-     * Sets the current parameter map.
-     * @param parameters The parameter map to use
+     * Returns a new, mutable parameter map. Derived classes will typically override this
+     * method to put values passed to specialized setter methods into the map.
+     * @return The parameter map to use when building the URL.
      */
-    public void setParameters(Map<String, Object> parameters)
+    protected Map<String, Object> createParameterMap()
     {
-        _parameters = parameters;
+        return new HashMap<>();
     }
 
     /**
@@ -408,7 +385,6 @@ private void throwError(Response r, boolean throwByDefault) throws IOException,
         r._responseText = responseText;
     }
 
-
     /**
      * Creates an instance of the response class, initialized with
      * the response text, the HTTP status code, and parsed JSONObject.
@@ -423,7 +399,7 @@ private void throwError(Response r, boolean throwByDefault) throws IOException,
      */
     protected ResponseType createResponse(String text, int status, String contentType, JSONObject json)
     {
-        return (ResponseType)new CommandResponse(text, status, contentType, json, this);
+        return (ResponseType)new CommandResponse(text, status, contentType, json);
     }
 
     /**
@@ -438,12 +414,10 @@ protected ResponseType createResponse(String text, int status, String contentTyp
      * This and the base URL from the connection will be used to construct the
      * first part of the URL.
      * @return The constructed HttpUriRequest instance.
-     * @throws CommandException Thrown if there is a problem encoding or parsing the URL
      * @throws URISyntaxException Thrown if there is a problem parsing the base URL in the connection.
      */
 
-    // TODO: For next major release, remove CommandException from throws list
-    protected HttpUriRequest getHttpRequest(Connection connection, String folderPath) throws CommandException, URISyntaxException
+    protected RequestType getHttpRequest(Connection connection, String folderPath) throws URISyntaxException
     {
         //construct a URI from connection base URI, folder path, and current parameters
         URI uri = createURI(connection, folderPath);
@@ -452,15 +426,11 @@ protected HttpUriRequest getHttpRequest(Connection connection, String folderPath
     }
 
     /**
-     * Creates the appropriate HttpUriRequest instance. Override to create something
-     * other than <code>HttpGet</code>.
+     * Creates the appropriate HttpUriRequest instance. Override to create <code>HttpGet</code>, <code>HttpPost</code>, etc.
      * @param uri the uri to convert
      * @return The HttpUriRequest instance.
      */
-    protected HttpUriRequest createRequest(URI uri)
-    {
-        return new HttpGet(uri);
-    }
+    protected abstract RequestType createRequest(URI uri);
 
     /**
      * Returns a full URI for this Command, including base URI, folder path, and query string.
@@ -492,26 +462,21 @@ private URI createURI(Connection connection, String folderPath) throws URISyntax
 
         //add the controller name
         String controller = getControllerName();
-        //for now, strip leading /. TODO: For next major release, throw
-        if (controller.charAt(0) == '/')
-            controller = controller.substring(1);
-        //for now, strip trailing /. TODO: For next major release, throw
-        if (controller.charAt(controller.length() - 1) == '/')
-            controller = controller.substring(0, controller.length() - 1);
+        if (controller.contains("/"))
+            throw new IllegalArgumentException("Controller name should not contain a slash (/)");
         path.append(controller);
 
         //add the action name + ".api"
         String actionName = getActionName();
-        //for now, strip leading /. TODO: For next major release, throw
-        if (actionName.charAt(0) == '/')
-            actionName = actionName.substring(1);
+        if (actionName.contains("/"))
+            throw new IllegalArgumentException("Action name should not contain a slash (/)");
         path.append("-").append(actionName);
         if (!actionName.endsWith(".api"))
             path.append(".api");
 
         URIBuilder builder = new URIBuilder(uri).setPath(path.toString());
 
-        Map<String, Object> params = getParameters();
+        Map<String, Object> params = createParameterMap();
 
         //add the required version if it's > 0
         if (getRequiredVersion() > 0)
@@ -566,6 +531,7 @@ protected String getParamValueAsString(Object param, String name)
      * Returns the required version number of this API call.
      * @return The required version number
      */
+    @Override
     public double getRequiredVersion()
     {
         return _requiredVersion;

src/org/labkey/remoteapi/CommandResponse.java

@@ -36,26 +36,24 @@ public class CommandResponse
     private final String _text;
     private final int _statusCode;
     private final String _contentType;
-    private final double _requiredVersion;
 
     private Map<String, Object> _data;
 
     /**
      * Constructs a new CommandResponse, initialized with the provided
      * response text and status code.
-     * @param text The response text
-     * @param statusCode The HTTP status code
+     *
+     * @param text        The response text
+     * @param statusCode  The HTTP status code
      * @param contentType The response content type
-     * @param json The parsed JSONObject (or null if JSON was not returned).
-     * @param sourceCommand A copy of the command that created this response
+     * @param json        The parsed JSONObject (or null if JSON was not returned)
      */
-    public CommandResponse(String text, int statusCode, String contentType, JSONObject json, Command<?> sourceCommand)
+    public CommandResponse(String text, int statusCode, String contentType, JSONObject json)
     {
         _text = text;
         _statusCode = statusCode;
         _contentType = contentType;
         _data = null != json ? json.toMap() : null;
-        _requiredVersion = sourceCommand != null ? sourceCommand.getRequiredVersion() : 0.0;
     }
 
     /**
@@ -94,16 +92,6 @@ public String getContentType()
         return _contentType;
     }
 
-    /**
-     * Returns the API version number required by the source command.
-     * Some APIs may return data in a different format depending on the required version
-     * @return the required API version number
-     */
-    public double getRequiredVersion()
-    {
-        return _requiredVersion;
-    }
-
     /**
      * Attempts to parse the response text and return a property Map.
      * <p>

src/org/labkey/remoteapi/Connection.java

@@ -234,9 +234,7 @@ private HttpClientBuilder clientBuilder()
 
         HttpClientBuilder builder = HttpClientBuilder.create()
             .setConnectionManager(connectionManager)
-            .setDefaultRequestConfig(RequestConfig.custom().setResponseTimeout(getTimeout(), TimeUnit.MILLISECONDS).build())
-            .setDefaultCookieStore(_httpClientContext.getCookieStore())
-            .setDefaultCredentialsProvider(_httpClientContext.getCredentialsProvider());
+            .setDefaultRequestConfig(RequestConfig.custom().setResponseTimeout(getTimeout(), TimeUnit.MILLISECONDS).build());
 
         if (_proxyHost != null && _proxyPort != null)
             builder.setProxy(new HttpHost(_proxyHost, _proxyPort));

src/org/labkey/remoteapi/GetCommand.java

@@ -0,0 +1,25 @@
+package org.labkey.remoteapi;
+
+import org.apache.hc.client5.http.classic.methods.HttpGet;
+
+import java.net.URI;
+
+/** Base class for all commands that use get **/
+public abstract class GetCommand<ResponseType extends CommandResponse> extends Command<ResponseType, HttpGet>
+{
+    protected GetCommand(String controllerName, String actionName)
+    {
+        super(controllerName, actionName);
+    }
+
+    /**
+     * Creates the {@link HttpGet} instance used for the request. Override to modify the HttpGet object before use.
+     * @param uri the request uri
+     * @return The HttpUriRequest instance.
+     */
+    @Override
+    protected HttpGet createRequest(URI uri)
+    {
+        return new HttpGet(uri);
+    }
+}

src/org/labkey/remoteapi/HasRequiredVersion.java

@@ -0,0 +1,6 @@
+package org.labkey.remoteapi;
+
+public interface HasRequiredVersion
+{
+    double getRequiredVersion();
+}