On Wed, 23 Apr 2025 17:34:33 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Please help review this PR which disables the unspecified but long-standing >> feature where an `FtpURLConnection` is opened as a fallback for non-local >> file URLs. >> >> Before this change, if a file URL has a non-local host component, say >> `file://remotehost/folder/data.txt`, then the implementation would attempt >> opening an FTP connection to `remotehost`. After this change, such URLs will >> be rejected with a `MalformedURLException`, unless the FTP fallback feature >> is explicitly re-enabled via a system property. >> >> This change was initially discussed here: >> https://mail.openjdk.org/pipermail/net-dev/2025-March/025988.html >> >> See the above discussion and CSR draft JDK-8354678 for the motivation for >> this change. I plan to update the CSR pending an initial round of review of >> this PR. >> >> This PR: >> >> * Changes file URL `Handler::openConnection` implementation for unix/windows >> to throw `MalformedURLException`, unless the FTP fallback feature is >> explicitly enabled by configuration. >> * Introduces a new system property `sun.net.www.protocol.file.ftp-enabled` >> which when set to `true` re-enables the feature. >> * Updates the existing test `NonLocalFtpFallback` to enable the feature via >> said system property. >> * Adds a new test `NonLocalFtpFallbackDisabled` verifying that a >> `MalformedURLException` is thrown by default for a non-local URL host >> component. >> >> I have added a Release Note as a subtask in the JBS issue, this also needs a >> review. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Fix repeated "should should" > > java/net/URLConnection/UNCTest.java > My wild guess would be that any current CI system doesn't have this > particular UNC share/file available, so instead an (unconnected) > FtpURLConnection would be returned. If that's the case, then I don't think > this test actually excercises the code path intended. That could only happen > if the UNC path exists and a FileURLConnection was returned. As a side note: If Handler::openConnection was updated to use `java.nio.file.Files::exists` instead of `java.io.File:exists`, then we could rewrite such tests that now rely on the UNC environment to instead mock the UNC path into existance using a custom default filesystem. Not in scope for this PR though. ------------- PR Comment: https://git.openjdk.org/jdk/pull/24657#issuecomment-2825303793
