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.

Sign up for GitHub

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

Jump to bottom

Opens gzip compressed content #513

Merged

dr0i merged 8 commits into master from 511-enableGzipInHttpOpener

Jan 2, 2024

Member

dr0i commented Dec 18, 2023

follows redirects
fixes misconception of "Content-Encoding"

Resolves #511.


          Opens gzip compressed content (#511)

6dd2889

- follows redirects
- fixes misconception of "Content-Encoding"

dr0i requested a review from blackwinter

December 18, 2023 16:57

dr0i assigned blackwinter

blackwinter added 7 commits

December 20, 2023 15:08


          Update HttpOpener Flux command description. (#513)

c55cbe0


          Update HttpOpener Content-Encoding header constant name and JavaDoc. (

8a18b2a

#513)

Also, group constants.


          Update HttpOpener Accept-Encoding header setter. (#513)

401babb


          Extract HttpOpener response charset from Content-Type header. (#513)

2ef2d3a

The `Accept-Charset` request header value is generally not suitable as a charset name.

Should be applied to compressed content as well.


          Clean up HttpOpener test. (#513)

0aafe05


          Group HttpOpener body setter together with URL setter. (#513)

0bddba1


          Refactor HttpOpener connection handling. (#513)

8621e7c

Apply connect timeout to all requests.

blackwinter approved these changes

View reviewed changes

Member

blackwinter left a comment •

edited

This pull request does a lot, and in a single commit to boot.

Handling compressed content: Generally 👍, ~~but maybe a generic "decompressor" module would be more versatile?~~
Following redirects: HttpUrlConnection does this by default, why replace it with a custom implementation?
Fixing "encoding" vs. "charset": Good catch, 👍

I've tried to address most of the issues I have with this pull request in individual commits. Please review.

I'm approving the amended pull request for now, despite my hesitation towards the custom follow logic.

Please note that there are backwards incompatible changes here, so it would probably necessitate a major release.

metafacture-io/src/main/java/org/metafacture/io/HttpOpener.java Outdated Show resolved Hide resolved

metafacture-io/src/main/java/org/metafacture/io/HttpOpener.java

                   private static final Pattern HEADER_FIELD_SEPARATOR_PATTERN = Pattern.compile(HEADER_FIELD_SEPARATOR);
                   private static final Pattern HEADER_VALUE_SEPARATOR_PATTERN = Pattern.compile(HEADER_VALUE_SEPARATOR);
+                  private static final int ALLOWED_REDIRECTIONS = 3;

Member

blackwinter Dec 20, 2023

Should this value be configurable?

Member Author

dr0i Jan 2, 2024

I would say: let's wait and implement if need arises. Would you be ok with this?

metafacture-io/src/main/java/org/metafacture/io/HttpOpener.java

                   private static final Pattern HEADER_FIELD_SEPARATOR_PATTERN = Pattern.compile(HEADER_FIELD_SEPARATOR);
                   private static final Pattern HEADER_VALUE_SEPARATOR_PATTERN = Pattern.compile(HEADER_VALUE_SEPARATOR);
+                  private static final int ALLOWED_REDIRECTIONS = 3;
+                  private static final int CONNECTION_TIMEOUT = 11000;

Member

blackwinter Dec 20, 2023 •

edited

How did you arrive at this value? 11 seconds seems kind of arbitrary.

Should this value be configurable?

Member Author

dr0i Jan 2, 2024 •

edited

It's kind of arbitrary. Setting these values at least easily prevent possible infinite loops.
Re configurable: I would say: let's wait and implement if need arises. Would you be ok with this?

metafacture-io/src/main/java/org/metafacture/io/HttpOpener.java Outdated Show resolved Hide resolved

metafacture-io/src/main/java/org/metafacture/io/HttpOpener.java Outdated Show resolved Hide resolved

metafacture-io/src/test/java/org/metafacture/io/HttpOpenerTest.java Outdated Show resolved Hide resolved

metafacture-io/src/main/java/org/metafacture/io/HttpOpener.java Show resolved Hide resolved

metafacture-io/src/main/java/org/metafacture/io/HttpOpener.java Outdated Show resolved Hide resolved

metafacture-io/src/main/java/org/metafacture/io/HttpOpener.java Outdated Show resolved Hide resolved

metafacture-io/src/test/java/org/metafacture/io/HttpOpenerTest.java Outdated Show resolved Hide resolved

blackwinter assigned dr0i and unassigned blackwinter

blackwinter mentioned this pull request

Close resources. #514

Merged

dr0i mentioned this pull request

Release Metafacture core 6.0.0 #515

Closed

12 tasks

Member Author

dr0i commented Jan 2, 2024

Unfortunately HttpURLConnection only follows redirects if they use the same protocol. As this was the initial issue (unable to open http://schema.org/) a custom implementation (or other library) was needed.

Thx for the many improvements/rewritings. Also agreed on multiple commits vs one big commit.
re "major release" : opened #515.

dr0i merged commit db1a32b into master

1 check passed

dr0i deleted the 511-enableGzipInHttpOpener branch

January 2, 2024 12:22

Member

blackwinter commented Jan 11, 2024

As this was the initial issue (unable to open http://schema.org/) a custom implementation (or other library) was needed.

Unfortunately, this motivation wasn't made explicit. Both this pull request and the referenced issue are primarily concerned with compression. Wouldn't it have been possible to adjust the URL being opened client-side?

Unfortunately HttpURLConnection only follows redirects if they use the same protocol.

Is this custom implementation affected by the same concerns then? ("serious security consequences")

And what about the objections against URLDecoder.decode in the provided workaround? (which seems to have been the basis for your implementation)

Member Author

dr0i commented Jan 11, 2024

Sorry to not have been explicit in the issue. I will remember to do better.

"serious security consequences": java network developers decided to treat protocol switches as a possible threat. We don't.

Re objections: you mean https://stackoverflow.com/questions/1884230/httpurlconnection-doesnt-follow-redirect-from-http-to-https/26046079#comment105077439_26046079 ? Is the location header really needed? There is no test for it, so maybe not. If it brings trouble we also could wait for a report here and then fix it.

Member

blackwinter commented Jan 11, 2024

java network developers decided to treat protocol switches as a possible threat. We don't.

Was this discussed anywhere?

Is the location header really needed?

Of course it's needed, that's where the redirect URL comes from. The objection was against decoding that URL.

There is no test for it, so maybe not.

That's because you only added a test for the compression handling, not for the redirect logic. I missed it in my review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment