-
Notifications
You must be signed in to change notification settings - Fork 0
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
Changes from all commits
331ba51
f745b71
d18229f
a620284
c464d13
70d84cb
1f464dc
69f74c9
07cadc3
9392fe6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -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() | ||
|
@@ -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()); | ||
|
@@ -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); | ||
} | ||
|
@@ -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(); | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, ourConnection.executeRequest()
returnsCloseableHttpResponse
(from HttpClient) which implementsClassicHttpResponse
(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.