Skip to content

Upgrade Apache HttpClient 4.5.13 -> 5.1.3 #35

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Sep 14, 2022
Merged
19 changes: 16 additions & 3 deletions labkey-client-api/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,9 +1,22 @@
# The LabKey Remote API Library for Java - Change Log

## version 2.?.?
*Released*: TBD
## version 3.0.0
*Released*: 14 September 2022
* Migrate internal HTTP handling to use Apache HttpClient 5.1.x
* Switch `StopImpersonatingCommand` to disable redirects (mimicking previous behavior)
* Add `Connection.stopImpersonating()` and deprecate `stopImpersonate()`
* Remove deprecated methods:
* ApiVersionException() (use constructor that takes contentType)
* CommandException() (use constructor that takes contentType)
* Connection.getBaseUrl() (use Connection.getBaseURI())
* CredentialsProvider.configureRequest() (use variant that takes a URI)
* Filter.NON_BLANK (use Filter.NONBLANK)
* Filter.getCaption() (use Filter.getDisplayValue())
* Filter.getName() (use Filter.getUrlKey())
* Filter.isDataValueRequired() (use Filter.isValueRequired())
* Remove SAS macros and wrapper classes
* Add `CreateFolderCommand` and `CreateProjectCommand`
* Add `CreateFolderCommand`
* Add `CreateProjectCommand` (earliest compatible LabKey Server version: 22.3.0)
* Update `LogoutCommand` to use POST

## version 2.0.0
Expand Down
7 changes: 3 additions & 4 deletions labkey-client-api/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -57,20 +57,19 @@ repositories {

group "org.labkey.api"

version "2.1.0-SNAPSHOT"
version "3.1.0-SNAPSHOT"

dependencies {
implementation "org.apache.httpcomponents:httpmime:${httpmimeVersion}"
api ("com.googlecode.json-simple:json-simple:${jsonSimpleVersion}")
{
// exclude this because it gets in the way of our own JSON object implementations from server/api
exclude group: "org.json", module:"json"
}
api "org.apache.httpcomponents.client5:httpclient5:${httpclient5Version}"
api "org.apache.httpcomponents.core5:httpcore5:${httpcore5Version}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking that these are actually api dependencies (that is, we expose some objects from these jar files in our interfaces). I assume so since they were before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, unfortunately our wrappers expose a few native HttpClient/HttpCore classes. For example, our Connection.executeRequest() returns CloseableHttpResponse (from HttpClient) which implements ClassicHttpResponse (from HttpCore). I've made a few internal methods private in this PR to reduce the leakage, but we can't eliminate it without more work.

implementation "net.sf.opencsv:opencsv:${opencsvVersion}"
implementation "commons-logging:commons-logging:${commonsLoggingVersion}"
api "org.apache.httpcomponents:httpclient:${httpclientVersion}"
implementation "commons-codec:commons-codec:${commonsCodecVersion}"
api "org.apache.httpcomponents:httpcore:${httpcoreVersion}"
}

configurations.all
Expand Down
6 changes: 3 additions & 3 deletions labkey-client-api/gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ commonsCodecVersion=1.15
commonsLoggingVersion=1.2

hamcrestVersion=1.3
httpclientVersion=4.5.13
httpcoreVersion=4.4.14
httpmimeVersion=4.5.13

httpclient5Version=5.1.3
httpcore5Version=5.1.4

jsonSimpleVersion=1.1.1

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@
*/
package org.labkey.remoteapi;

import org.apache.http.auth.AuthenticationException;
import org.apache.http.client.methods.HttpUriRequest;
import org.apache.http.client.protocol.HttpClientContext;
import org.apache.hc.client5.http.auth.AuthenticationException;
import org.apache.hc.client5.http.classic.methods.HttpUriRequest;
import org.apache.hc.client5.http.protocol.HttpClientContext;

import java.net.URI;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,6 @@
*/
public class ApiVersionException extends CommandException
{
/**
* @deprecated Use {@link #ApiVersionException(String, int, Map, String, String)}
*/
ApiVersionException(String message, int statusCode, Map<String, Object> properties, String responseText)
{
this(message, statusCode, properties, responseText, null);
}

ApiVersionException(String message, int statusCode, Map<String, Object> properties, String responseText, String contentType)
{
super(message, statusCode, properties, responseText, contentType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@
*/
package org.labkey.remoteapi;

import org.apache.http.auth.AuthScope;
import org.apache.http.auth.AuthenticationException;
import org.apache.http.auth.Credentials;
import org.apache.http.auth.UsernamePasswordCredentials;
import org.apache.http.client.methods.HttpUriRequest;
import org.apache.http.client.protocol.HttpClientContext;
import org.apache.http.impl.auth.BasicScheme;
import org.apache.http.impl.client.BasicCredentialsProvider;
import org.apache.hc.client5.http.auth.AuthScope;
import org.apache.hc.client5.http.auth.AuthenticationException;
import org.apache.hc.client5.http.auth.Credentials;
import org.apache.hc.client5.http.auth.UsernamePasswordCredentials;
import org.apache.hc.client5.http.classic.methods.HttpUriRequest;
import org.apache.hc.client5.http.impl.auth.BasicCredentialsProvider;
import org.apache.hc.client5.http.impl.auth.BasicScheme;
import org.apache.hc.client5.http.protocol.HttpClientContext;

import java.net.URI;

Expand All @@ -43,12 +43,10 @@ public BasicAuthCredentialsProvider(String email, String password)
@Override
public void configureRequest(URI baseURI, HttpUriRequest request, HttpClientContext httpClientContext) throws AuthenticationException
{
AuthScope scope = new AuthScope(baseURI.getHost(), AuthScope.ANY_PORT, AuthScope.ANY_REALM);
BasicCredentialsProvider provider = new BasicCredentialsProvider();
Credentials credentials = new UsernamePasswordCredentials(_email, _password);
AuthScope scope = new AuthScope(baseURI.getHost(), -1);
Credentials credentials = new UsernamePasswordCredentials(_email, _password.toCharArray());
provider.setCredentials(scope, credentials);

httpClientContext.setCredentialsProvider(provider);
request.addHeader(new BasicScheme().authenticate(credentials, request, httpClientContext));
}
}
93 changes: 40 additions & 53 deletions labkey-client-api/src/org/labkey/remoteapi/Command.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,13 @@
*/
package org.labkey.remoteapi;

import org.apache.commons.codec.EncoderException;
import org.apache.commons.codec.net.URLCodec;
import org.apache.commons.logging.LogFactory;
import org.apache.http.Header;
import org.apache.http.auth.AuthenticationException;
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpUriRequest;
import org.apache.http.client.utils.URIBuilder;
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;
import org.apache.hc.core5.net.URIBuilder;
import org.json.simple.JSONObject;
import org.json.simple.JSONValue;

Expand Down Expand Up @@ -256,12 +254,12 @@ private Response(CloseableHttpResponse httpResponse, String contentType, Long co

public String getStatusText()
{
return _httpResponse.getStatusLine().getReasonPhrase();
return _httpResponse.getReasonPhrase();
}

public int getStatusCode()
{
return _httpResponse.getStatusLine().getStatusCode();
return _httpResponse.getCode();
}

public String getContentType()
Expand Down Expand Up @@ -328,7 +326,7 @@ protected Response _execute(Connection connection, String folderPath) throws Com
{
//construct and initialize the HttpUriRequest
request = getHttpRequest(connection, folderPath);
LogFactory.getLog(Command.class).info("Requesting URL: " + request.getURI().toString());
LogFactory.getLog(Command.class).info("Requesting URL: " + request.getRequestUri());

//execute the request
httpResponse = connection.executeRequest(request, getTimeout());
Expand Down Expand Up @@ -448,19 +446,13 @@ protected HttpUriRequest getHttpRequest(Connection connection, String folderPath
// TODO (Dave 11/11/14 -- as far as I can tell the default of the AuthSchemes is to automatically handle challenges
// method.setDoAuthentication(true);

// NOTE: Fairly sure that the "unescaped" comment below is obsolete
// TODO: Combine getActionUrl() and addParameters() into a single method
//construct a URI from the results of the getActionUrl method
//note that this method returns an unescaped URL, so pass
//false for the second parameter to escape it
URI uri = getActionUrl(connection, folderPath);

//the query string values will need to be escaped as they are
//put into the query string, and we don't want to re-escape the
//entire thing, as the %, & and = characters will be escaped again
//so add this separately as a raw value
//(indicating that it has already been escaped)
String queryString = getQueryString();
if (null != queryString)
uri = new URIBuilder(uri).setQuery(queryString).build();
uri = addParameters(uri);

return createRequest(uri);
}
Expand All @@ -487,7 +479,7 @@ protected HttpUriRequest createRequest(URI uri)
* @return The URL
* @throws URISyntaxException if the uri constructed from the parameters is malformed
*/
protected URI getActionUrl(Connection connection, String folderPath) throws URISyntaxException
private URI getActionUrl(Connection connection, String folderPath) throws URISyntaxException
{
URI uri = connection.getBaseURI();

Expand Down Expand Up @@ -520,60 +512,55 @@ protected URI getActionUrl(Connection connection, String folderPath) throws URIS
}

/**
* Returns the query string portion of the URL for this command.
* Adds all parameters to the passed URI
* <p>
* The parameters in the query string should be encoded to avoid parsing errors on the server.
* @return The query string
* @throws CommandException Thrown if there is a problem encoding the query string parameter names or values
* @return The URI with parameters added
* @throws CommandException Thrown if there is a problem building the URI
*/
protected String getQueryString() throws CommandException
protected URI addParameters(URI uri) throws CommandException, URISyntaxException
{
Map<String, Object> params = getParameters();

//add the required version if it's > 0
if (getRequiredVersion() > 0)
params.put(CommonParameters.apiVersion.name(), getRequiredVersion());

StringBuilder qstring = new StringBuilder();
URLCodec urlCodec = new URLCodec();
try
if (params.isEmpty())
return uri;

URIBuilder builder = new URIBuilder(uri);

for (String name : params.keySet())
{
for (String name : params.keySet())
Object value = params.get(name);
if (value instanceof Collection<?> col)
{
Object value = params.get(name);
if (value instanceof Collection)
for (Object o : col)
{
for (Object o : ((Collection) value))
{
appendParameter(qstring, urlCodec, name, o);
}
}
else
{
appendParameter(qstring, urlCodec, name, value);
addParameter(builder, name, o);
}
}
else
{
addParameter(builder, name, value);
}
}

try
{
return builder.build();
}
catch(EncoderException e)
catch (URISyntaxException e)
{
throw new CommandException(e.getMessage());
}
Comment on lines +553 to 556
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setParameters is declared as throwing URISyntaxException, why not just let this bubble up instead of wrapping in a CommandException?
Also, I would argue that Command._execute shouldn't wrap it in a CommandException either. Wrapping in an IllegalArgumentException seems more appropriate to me since the most likely cause of such an exception is somebody specifying bad query parameters or some such thing.
Although, I suppose, throwing unchecked exceptions from an API is probably bad practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just following the existing pattern, which I agree is inconsistent. As I think I noted somewhere, I'm planning to merge the create URL and add parameters steps into a single method at some point. Will look at reconciling exception handling at that point. But I think this set of PRs is close enough and I need to move on...


return qstring.length() > 0 ? qstring.toString() : null;
}

private void appendParameter(StringBuilder qstring, URLCodec urlCodec, String name, Object value)
throws EncoderException
private void addParameter(URIBuilder builder, String name, Object value)
{
String strValue = null == value ? null : getParamValueAsString(value, name);
if(null != strValue)
{
if (qstring.length() > 0)
qstring.append('&');
qstring.append(urlCodec.encode(name));
qstring.append('=');
qstring.append(urlCodec.encode(strValue));
}
if (null != strValue)
builder.addParameter(name, strValue);
}

/**
Expand Down
14 changes: 1 addition & 13 deletions labkey-client-api/src/org/labkey/remoteapi/CommandException.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/
package org.labkey.remoteapi;

import org.apache.http.impl.EnglishReasonPhraseCatalog;
import org.apache.hc.core5.http.impl.EnglishReasonPhraseCatalog;

import java.util.Locale;
import java.util.Map;
Expand Down Expand Up @@ -51,18 +51,6 @@ public CommandException(String message)
this(message, 0, null, null, null);
}

/**
* @param message The message text (should not be null).
* @param statusCode The HTTP status code.
* @param properties The exception property map (may be null)
* @param responseText The full response text (may be null)
* @deprecated Use {@link #CommandException(String, int, Map, String, String)}
*/
public CommandException(String message, int statusCode, Map<String, Object> properties, String responseText)
{
this(message, statusCode, properties, responseText, null);
}

/**
* Constructs a new CommandException given a message, HTTP status code,
* exception property map, responseText, and contentType.
Expand Down
Loading