You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "thachlp (via GitHub)" <gi...@apache.org> on 2024/05/23 04:26:21 UTC
[PR] IO-831 Support https & http by URIOrigin [commons-io]
thachlp opened a new pull request, #630:
URL: https://github.com/apache/commons-io/pull/630
#### Summary
This PR addresses issue IO-831, support `http` and `https`
#### Changes
Handle in case uri are `http` or `https`
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IO-831 Support https & http by URIOrigin [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #630:
URL: https://github.com/apache/commons-io/pull/630#discussion_r1611790377
##########
src/test/java/org/apache/commons/io/build/AbstractOriginTest.java:
##########
@@ -130,4 +133,12 @@ public void testGetWriter() throws IOException {
public void testSize() throws IOException {
assertEquals(Files.size(Paths.get(FILE_NAME_RO)), getOriginRo().getByteArray().length);
}
+
+ @Test
+ public void testURI() throws Exception {
Review Comment:
This test is in the wrong class, it does not do anything generic that would apply to all of its subclasses. Use `URIOriginTest`
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [IO-831] Add getInputStream() for 'https' & 'http' in URIOrigin [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #630:
URL: https://github.com/apache/commons-io/pull/630#issuecomment-2143975946
https://issues.apache.org/jira/browse/IO-831
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IO-831 Support https & http by URIOrigin [commons-io]
Posted by "thachlp (via GitHub)" <gi...@apache.org>.
thachlp commented on PR #630:
URL: https://github.com/apache/commons-io/pull/630#issuecomment-2143385605
> @thachlp Thank you for your PR. The PR does not test all the new code, specifically HTTP (as opposed to HTTPS). Refactor duplicate method calls. You should check if a file system is installed already that handles HTTP[S] before overriding NIO Path handling; see `org.apache.commons.io.file.spi.FileSystemProviders`. TY!
Hi @garydgregory, thanks for your review.
Base on the test `testGetFileSystemProvider_all` on `FileSystemProvidersTest`, there is no scheme `http` and `https`. Do I miss something?
Please help review again 🙇
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IO-831 Support https & http by URIOrigin [commons-io]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #630:
URL: https://github.com/apache/commons-io/pull/630#issuecomment-2143393271
## [Codecov](https://app.codecov.io/gh/apache/commons-io/pull/630?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `66.66667%` with `1 lines` in your changes are missing coverage. Please review.
> Project coverage is 86.50%. Comparing base [(`858f5cf`)](https://app.codecov.io/gh/apache/commons-io/commit/858f5cf238259e50923482d11fd17b315233fa23?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`3527f1a`)](https://app.codecov.io/gh/apache/commons-io/commit/3527f1a5dbe3a36eb08ed06df36da5e959f50c0c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 188 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/commons-io/pull/630?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...va/org/apache/commons/io/build/AbstractOrigin.java](https://app.codecov.io/gh/apache/commons-io/pull/630?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fapache%2Fcommons%2Fio%2Fbuild%2FAbstractOrigin.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvaW8vYnVpbGQvQWJzdHJhY3RPcmlnaW4uamF2YQ==) | 66.66% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/commons-io/pull/630?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #630 +/- ##
============================================
+ Coverage 86.01% 86.50% +0.49%
+ Complexity 3499 3484 -15
============================================
Files 231 231
Lines 8250 8143 -107
Branches 960 927 -33
============================================
- Hits 7096 7044 -52
+ Misses 864 811 -53
+ Partials 290 288 -2
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/commons-io/pull/630?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IO-831 Support https & http by URIOrigin [commons-io]
Posted by "thachlp (via GitHub)" <gi...@apache.org>.
thachlp commented on code in PR #630:
URL: https://github.com/apache/commons-io/pull/630#discussion_r1623191676
##########
src/test/java/org/apache/commons/io/build/AbstractOriginTest.java:
##########
@@ -130,4 +133,12 @@ public void testGetWriter() throws IOException {
public void testSize() throws IOException {
assertEquals(Files.size(Paths.get(FILE_NAME_RO)), getOriginRo().getByteArray().length);
}
+
+ @Test
+ public void testURI() throws Exception {
Review Comment:
Yes, I updated.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [IO-831] Add getInputStream() for 'https' & 'http' in URIOrigin [commons-io]
Posted by "thachlp (via GitHub)" <gi...@apache.org>.
thachlp commented on PR #630:
URL: https://github.com/apache/commons-io/pull/630#issuecomment-2144179515
> @thachlp Thank you for your updates. Merged!
Thanks @garydgregory 🚀
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IO-831 Support https & http by URIOrigin [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #630:
URL: https://github.com/apache/commons-io/pull/630#discussion_r1623207046
##########
src/test/java/org/apache/commons/io/build/URIOriginTest.java:
##########
@@ -35,4 +39,21 @@ public void beforeEach() {
setOriginRw(new URIOrigin(Paths.get(FILE_NAME_RW).toUri()));
}
+ @Test
+ void testGetInputStreamHttpURI() throws Exception {
+ final String uri = "https://example.com";
+ final AbstractOrigin.URIOrigin origin = new AbstractOrigin.URIOrigin(new URI(uri));
+ try (final InputStream in = origin.getInputStream()) {
+ assertNotEquals(-1, in.read());
+ }
+ }
+
+ @Test
+ void testGetInputStreamHttps() throws Exception {
+ final String uri = "https://example.com";
+ final AbstractOrigin.URIOrigin origin = new AbstractOrigin.URIOrigin(new URI(uri));
+ try (final InputStream in = origin.getInputStream()) {
+ assertNotEquals(-1, in.read());
+ }
+ }
Review Comment:
Add tests for 'http' just like you have 'https'. Use JUnit parameterized tests to reduce code duplication.
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -402,6 +402,13 @@ public Path getPath() {
return Paths.get(get());
}
+ @Override
+ public InputStream getInputStream(final OpenOption... options) throws IOException {
+ if ("http".equalsIgnoreCase(get().getScheme()) || "https".equalsIgnoreCase(get().getScheme())) {
Review Comment:
This method should only call `get()` and `getScheme()` once.
Don't override the file systems for these two schemes. You should check if a file system is installed already that handles HTTP[S] before overriding NIO Path handling; use `org.apache.commons.io.file.spi.FileSystemProviders`.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [IO-831] Support getInputStream() for https & http in URIOrigin [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #630:
URL: https://github.com/apache/commons-io/pull/630
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [IO-831] Add getInputStream() for 'https' & 'http' in URIOrigin [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #630:
URL: https://github.com/apache/commons-io/pull/630#issuecomment-2143976329
@thachlp
Thank you for your updates. Merged!
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IO-831 Support https & http by URIOrigin [commons-io]
Posted by "thachlp (via GitHub)" <gi...@apache.org>.
thachlp commented on code in PR #630:
URL: https://github.com/apache/commons-io/pull/630#discussion_r1623321060
##########
src/test/java/org/apache/commons/io/build/URIOriginTest.java:
##########
@@ -35,4 +39,21 @@ public void beforeEach() {
setOriginRw(new URIOrigin(Paths.get(FILE_NAME_RW).toUri()));
}
+ @Test
+ void testGetInputStreamHttpURI() throws Exception {
+ final String uri = "https://example.com";
+ final AbstractOrigin.URIOrigin origin = new AbstractOrigin.URIOrigin(new URI(uri));
+ try (final InputStream in = origin.getInputStream()) {
+ assertNotEquals(-1, in.read());
+ }
+ }
+
+ @Test
+ void testGetInputStreamHttps() throws Exception {
+ final String uri = "https://example.com";
+ final AbstractOrigin.URIOrigin origin = new AbstractOrigin.URIOrigin(new URI(uri));
+ try (final InputStream in = origin.getInputStream()) {
+ assertNotEquals(-1, in.read());
+ }
+ }
Review Comment:
Yes, updated
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IO-831 Support https & http by URIOrigin [commons-io]
Posted by "thachlp (via GitHub)" <gi...@apache.org>.
thachlp commented on code in PR #630:
URL: https://github.com/apache/commons-io/pull/630#discussion_r1623321114
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -402,6 +402,13 @@ public Path getPath() {
return Paths.get(get());
}
+ @Override
+ public InputStream getInputStream(final OpenOption... options) throws IOException {
+ if ("http".equalsIgnoreCase(get().getScheme()) || "https".equalsIgnoreCase(get().getScheme())) {
Review Comment:
Hope that I understand you correctly, please help review it 🙇
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org