ok2c commented on code in PR #852:
URL:
https://github.com/apache/httpcomponents-client/pull/852#discussion_r3607848560
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java:
##########
@@ -412,12 +414,78 @@ public void cancelled() {
}
SimpleHttpRequest prepareRequest(final HttpRequest request,
- final AsyncEntityProducer entityProducer)
{
- // To be revised when implementing QUERY support
- if (entityProducer != null) {
- return null;
+ final AsyncEntityProducer entityProducer)
throws IOException {
+ if (entityProducer == null) {
+ return SimpleRequestBuilder.copy(request).build();
+ }
+ // QUERY content must be read in full in order to determine the cache
key
+ if (Method.QUERY.isSame(request.getMethod())
+ && entityProducer.isRepeatable()
+ && entityProducer.getContentEncoding() == null) {
+ final byte[] content = drainEntityProducer(entityProducer);
+ if (content != null) {
+ return SimpleRequestBuilder.copy(request)
+ .setBody(content,
ContentType.parseLenient(entityProducer.getContentType()))
+ .build();
+ }
+ }
+ return null;
+ }
+
+ /**
+ * Drains the given repeatable entity producer into a byte array without
an I/O reactor.
+ * Returns {@code null} if the producer fails to make progress, in which
case it is reset
+ * by {@link AsyncEntityProducer#releaseResources()} and can still be used
to execute
+ * the request directly.
+ */
+ private static byte[] drainEntityProducer(final AsyncEntityProducer
entityProducer) throws IOException {
Review Comment:
@hunghhdev When draining the entity producer we should enforce a total cap
on body length in order to avoid some "security professional" reporting it as a
potential DoS vulnerability. I imagine requests with content body larger than
25K should be considered non-cacheable
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -221,12 +222,21 @@ private static ClassicHttpResponse convert(final
SimpleHttpResponse cacheRespons
return response;
}
- SimpleHttpRequest prepareRequest(final ClassicHttpRequest request) {
- // To be revised when implementing QUERY support
- if (request.getEntity() != null) {
- return null;
+ SimpleHttpRequest prepareRequest(final ClassicHttpRequest request) throws
IOException {
+ final HttpEntity entity = request.getEntity();
+ if (entity == null) {
+ return SimpleRequestBuilder.copy(request).build();
+ }
+ // QUERY content must be read in full in order to determine the cache
key
+ if (Method.QUERY.isSame(request.getMethod()) &&
entity.getContentEncoding() == null) {
+ final byte[] content = EntityUtils.toByteArray(entity);
Review Comment:
@hunghhdev Likewise, we should avoid buffering of content body larger than
25K. That also makes it necessary for the content entity to be repeatable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
