Skip to content
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

Deprecate Volley's use of Apache HTTP. #75

Merged
merged 3 commits into from
Sep 12, 2017
Merged

Deprecate Volley's use of Apache HTTP. #75

merged 3 commits into from
Sep 12, 2017

Conversation

jpd236
Copy link
Collaborator

@jpd236 jpd236 commented Aug 21, 2017

This change makes it feasible for Volley clients to remove their dependency on the legacy Apache HTTP library; if HurlStack is used, or if their existing HttpStack implementation is refactored atop the new BaseHttpStack (which should be trivial), then Volley will never depend on classes in org.apache at runtime. However, legacy clients built atop HttpStack (including HttpClientStack) should continue to work without modifications for now.

Fixes #2

jpd236 and others added 2 commits August 18, 2017 15:45
This change makes it feasible for Volley clients to remove their
dependency on the legacy Apache HTTP library; if HurlStack is used, or
if their existing HttpStack implementation is refactored atop the new
BaseHttpStack (which should be trivial), then Volley will never depend
on classes in org.apache at runtime. However, legacy clients built
atop HttpStack (including HttpClientStack) should continue to work
without modifications for now.

Fixes #2
-Block large (>MAX_INT) responses in AdaptedHttpStack
-Fix copying headers in BaseHttpStack
Copy link
Contributor

@joebowbeer joebowbeer left a comment

Choose a reason for hiding this comment

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

I scanned the changes and nothing popped out.

@jpd236 jpd236 requested a review from jjoslin September 11, 2017 19:19
Copy link
Collaborator

@jjoslin jjoslin left a comment

Choose a reason for hiding this comment

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

Minor comments, okay to submit as-is.

@@ -48,11 +47,11 @@ public NetworkResponse(int statusCode, byte[] data, Map<String, String> headers,
}

public NetworkResponse(byte[] data) {
this(HttpStatus.SC_OK, data, Collections.<String, String>emptyMap(), false, 0);
this(HttpURLConnection.HTTP_OK, data, Collections.<String, String>emptyMap(), false, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to consider a static import for the codes to cut down on the verbosity (here and elsewhere)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I always thought this was against the style guide, but it doesn't look like it. Well, I guess I personally prefer having the verbosity since I feel like imports are generally not noticed when looking at a file and the source of these constants is kind of relevant, though I wouldn't push back if a static import were used elsewhere. Going to just leave this as is to avoid churn.

headers.add(new BasicHeader(entry.getKey(), value));
}
}
apacheResponse.setHeaders(headers.toArray(new Header[0]));
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you allocate the correct size up front, headers.toArray(new Header[headers.size()]), then you can avoid an unnecessary allocation of an array.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

return DateUtils.parseDate(dateStr).getTime();
} catch (DateParseException e) {
return newRfc1123Formatter().parse(dateStr).getTime();
} catch (ParseException e) {
// Date in invalid format, fallback to 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I realize this is out of scope for this review but it seems like this exception should be logged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed; added log.

@jpd236 jpd236 merged commit b33a53f into master Sep 12, 2017
@jpd236 jpd236 deleted the apache-http branch September 12, 2017 00:45
Copy link
Collaborator

@jjoslin jjoslin left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants