EntityUtils clean ups and internal refactoring by garydgregory · Pull Request #161 · apache/httpcomponents-core

garydgregory · 2019-11-15T16:56:06Z

EntityUtils clean ups and internal refactoring. If this is OK, I'll do something similar for 4.5.x if needed.

- Javadoc minor edits.

michael-o · 2019-11-15T18:57:23Z

I will have a look at this tomorrow.

ok2c · 2019-11-16T09:36:44Z

httpcore5/src/main/java/org/apache/hc/core5/http/io/entity/EntityUtils.java

    }

+    private static int getCheckedContentLength(final HttpEntity entity) {
+        final int contentLength = (int) Args.checkContentLength(entity);


@garydgregory I am not a very big fan of shared code at all cost. I would very much prefer to keep Args.checkContentLength and similar argument checks at the very beginning of each method, and not hidden somewhere deep inside a static method. Otherwise looks good to me.

I agree, they should be on public methods.

@ok2c I moved the checks to the start of methods.

@michael-o I did not add public methods to keep the API surface minimal.

@garydgregory What I actually mean was that Args#checkContentLength should be at the beginning of the method. getCheckedContentLength method could be wherever it needs to be. Please decouple Args#checkContentLength and getCheckedContentLength. They are unrelated.

@ok2c Sorry for the misunderstanding, I just committed changes for this request.

michael-o · 2019-11-16T16:46:28Z

httpcore5/src/main/java/org/apache/hc/core5/http/io/entity/EntityUtils.java

    }

+    private static int getCheckedContentLength(final HttpEntity entity) {
+        final int contentLength = (int) Args.checkContentLength(entity);


I agree, they should be on public methods.

michael-o · 2019-11-16T16:47:37Z

httpcore5/src/main/java/org/apache/hc/core5/http/io/entity/EntityUtils.java

-            }
-            final ByteArrayBuffer buffer = new ByteArrayBuffer(i);
-            final byte[] tmp = new byte[4096];
+            final ByteArrayBuffer buffer = new ByteArrayBuffer(getCheckedContentLength(entity));


What if Content-Length is 500_000_000? Will you really create a buffer with that size?

@michael-o
That's what the currently release code does; please see https://github.com/apache/httpcomponents-core/blob/rel/v4.4.12/httpcore/src/main/java/org/apache/http/util/EntityUtils.java#L135

May we address this separately please?

Yes, we should. It requires at least warning/documentation.

@michael-o Anyone in their mind should never be using EntityUtils#toString in the first place. I do not think there is anything to be done here other than saying that one ought not use this method in production.

@ok2c That would be perfectly fine. Nothing can be improved here codewise.

Would it then be appropriate to have a method that takes a max result size argument?

As an overload, likely. How do you intend the fail if content length is greater or chunked transfer has reached that limit?

@michael-o You get what you asked for, a String of length N. The call site can decide what to do if that length is less than the content length or some other length specifier.

@garydgregory I propose to commit this change-set and raise another PR for the new method.

michael-o · 2019-11-16T16:48:23Z

httpcore5/src/main/java/org/apache/hc/core5/http/io/entity/EntityUtils.java

 */
 public final class EntityUtils {

+    private static final Charset DEFAULT_CHARSET = StandardCharsets.ISO_8859_1;


I wonder why are still have this. RFC 7230 and friends to not define any default encoding any more.

michael-o · 2019-11-16T16:48:38Z

httpcore5/src/main/java/org/apache/hc/core5/http/io/entity/EntityUtils.java

+            final Charset charset) throws IOException {
+        final Charset actualCharset = charset == null ? DEFAULT_CHARSET : charset;
+        final CharArrayBuffer buf = new CharArrayBuffer(
+                contentLength > 0 ? (int) contentLength : DEFAULT_CHAR_BUFFER_SIZE);


Same here for the length...

per Oleg's PR review.

ok2c · 2019-11-18T08:13:49Z

httpcore5/src/main/java/org/apache/hc/core5/http/io/entity/EntityUtils.java

    public static byte[] toByteArray(final HttpEntity entity) throws IOException {
        Args.notNull(entity, "Entity");
+        int contentLength = (int) Args.checkContentLength(entity);
+        contentLength = toContentLength(contentLength);


@garydgregory Do you mind folding these two one in order to make contentLength final? final int contentLength = toContentLength((int) Args.checkContentLength(entity)) and then everything is going to be fine.

@ok2c Done!

* Refactor and reuse magic numbers into constants. * Refactor some common code into private methods.

* EntityUtils clean ups and internal refactoring (#161) * - Refactor and reuse magic numbers into constants. - Javadoc minor edits. * Refactor some common code into private methods. * Move calls to getCheckedContentLength(entity) to the start of methods per Oleg's PR review. * Refactor per Oleg's review of PR #161. * In-line result of Args.checkContentLength(). * Add and use max result length parameter version of APIs in EntityUtils. All previous methods now delegate to the new methods and use a default length constant.

garydgregory added 2 commits November 15, 2019 11:01

- Refactor and reuse magic numbers into constants.

12988a8

- Javadoc minor edits.

Refactor some common code into private methods.

3ee42c3

ok2c reviewed Nov 16, 2019

View reviewed changes

michael-o reviewed Nov 16, 2019

View reviewed changes

garydgregory added 2 commits November 17, 2019 08:43

Move calls to getCheckedContentLength(entity) to the start of methods

f499b73

per Oleg's PR review.

Refactor per Oleg's review of PR apache#161.

b014f95

ok2c approved these changes Nov 18, 2019

View reviewed changes

In-line result of Args.checkContentLength().

5fe4ed3

garydgregory merged commit a50d790 into apache:master Nov 18, 2019

asfgit pushed a commit that referenced this pull request Nov 19, 2019

EntityUtils clean ups and internal refactoring (#161)

f45003f

* Refactor and reuse magic numbers into constants. * Refactor some common code into private methods.

garydgregory deleted the EnityUtils_clean_ups branch November 19, 2019 14:22

Comments

Conversation

garydgregory commented Nov 15, 2019

Uh oh!

michael-o commented Nov 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

garydgregory Nov 18, 2019 •

edited

Loading