Skip to content
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

Introduce robust URL to java.nio.file.Path conversion #691

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+93 −49
Draft

Introduce robust URL to java.nio.file.Path conversion #691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 46 additions & 30 deletions ...es/org.eclipse.equinox.common.tests/src/org/eclipse/equinox/common/tests/URIUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.junit.Assume.assumeTrue;

import java.io.File;
import java.io.InputStream;
import java.net.MalformedURLException;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
import java.nio.file.Path;

import org.eclipse.core.runtime.FileLocator;
import org.eclipse.core.runtime.IPath;
Expand Down Expand Up @@ -97,9 +99,7 @@ public void testToFile() {
*/
@Test
public void testToFileUNC() throws URISyntaxException {
if (!WINDOWS) {
return;
}
assumeTrue(WINDOWS);
// UNC paths
URI path = new URI("file://HOST/some/path");
File result = URIUtil.toFile(path);
Expand Down Expand Up @@ -187,6 +187,38 @@ public void testURLtoURI() throws MalformedURLException, URISyntaxException {
assertEquals("3.2", new URI("file:////SERVER/some/path"), URIUtil.toURI(new URL("file:////SERVER/some/path")));
}

@Test
public void testURLtoPath() throws MalformedURLException {

assertThrows(IllegalArgumentException.class, () -> URIUtil.toFilePath(new URL("http://foo.bar")));

assertEquals(Path.of("C:/foo/bar.txt"), URIUtil.toFilePath(new URL("file:/C:/foo/bar.txt")));
assertEquals(Path.of("C:/foo/bar.txt"), URIUtil.toFilePath(new URL("file:///C:/foo/bar.txt")));

// Not escaped special characters like spaces, % or #
assertEquals(Path.of("C:/foo/a b.txt"), URIUtil.toFilePath(new URL("file:/C:/foo/a b.txt")));
assertEquals(Path.of("C:/foo/a#b c.txt"), URIUtil.toFilePath(new URL("file:/C:/foo/a#b c.txt")));
assertEquals(Path.of("C:/foo/a%b.txt"), URIUtil.toFilePath(new URL("file:/C:/foo/a%b.txt")));
assertEquals(Path.of("C:/foo/bar#my-query"), URIUtil.toFilePath(new URL("file:/C:/foo/bar#my-query")));

// UNC paths (with not escaped spaces and %)
assertEquals(Path.of("////SERVER/some/path"), URIUtil.toFilePath(new URL("file://SERVER/some/path")));
assertEquals(Path.of("////SERVER/some/path"), URIUtil.toFilePath(new URL("file:////SERVER/some/path")));
assertEquals(Path.of("////SERVER/a b/path"), URIUtil.toFilePath(new URL("file://SERVER/a b/path")));
assertEquals(Path.of("////SERVER/a#b c/path"), URIUtil.toFilePath(new URL("file:////SERVER/a#b c/path")));
assertEquals(Path.of("////SERVER/some/a%b.txt"), URIUtil.toFilePath(new URL("file://SERVER/some/a%b.txt")));
assertEquals(Path.of("////SERVER/some/a%b.txt"), URIUtil.toFilePath(new URL("file:////SERVER/some/a%b.txt")));


// TODO: split the following? Make this a Windows only test case and create a
// separate UNIX test-case?
assumeTrue(WINDOWS);
assertEquals(Path.of("C:/foo/bar.txt"), URIUtil.toFilePath(new URL("file:c:/foo/bar.txt")));
assertEquals(Path.of("C:/foo/bar.txt"), URIUtil.toFilePath(new URL("file:/c:/foo/bar.txt")));

assertEquals(Path.of("foo/bar.txt"), URIUtil.toFilePath(new URL("file:foo/bar.txt")));
}

/**
* Tests for {@link URIUtil#toURL(java.net.URI)}.
*/
Expand All @@ -210,9 +242,7 @@ public void testURItoURL() throws MalformedURLException, URISyntaxException {
*/
@Test
public void testWindowsPathsFromURI() throws MalformedURLException, URISyntaxException {
if (!WINDOWS) {
return;
}
assumeTrue(WINDOWS);
assertEquals("1.1", new URI("file:/c:/foo/bar.txt"), URIUtil.toURI(new URL("file:c:/foo/bar.txt")));
assertEquals("1.2", new URI("file:/c:/foo/bar.txt"), URIUtil.toURI(new URL("file:/c:/foo/bar.txt")));
}
Expand All @@ -223,9 +253,7 @@ public void testWindowsPathsFromURI() throws MalformedURLException, URISyntaxExc
*/
@Test
public void testWindowsPathsFromString() throws URISyntaxException {
if (!WINDOWS) {
return;
}
assumeTrue(WINDOWS);
assertEquals("1.1", new URI("file:/c:/foo/bar.txt"), URIUtil.fromString("file:c:/foo/bar.txt"));
assertEquals("1.2", new URI("file:/c:/foo/bar.txt"), URIUtil.fromString("file:/c:/foo/bar.txt"));
}
Expand All @@ -239,22 +267,13 @@ public void testFileWithSpaces() throws MalformedURLException, URISyntaxExceptio
File fileWithSpaces = new File("/c:/with spaces/goo");
URI correctURI = fileWithSpaces.toURI();
URL fileURL = fileWithSpaces.toURL();
URI fileURI = null;
try {
fileURI = fileURL.toURI();
fail();
} catch (URISyntaxException e) {
fileURI = URIUtil.toURI(fileURL);
}
assertEquals("1.1", correctURI, fileURI);

try {
fileURI = new URI(fileURL.toString());
fail();
} catch (URISyntaxException e) {
fileURI = URIUtil.fromString(fileURL.toString());
}
assertEquals("1.2", correctURI, fileURI);
assertThrows(URISyntaxException.class, () -> fileURL.toURI());
assertEquals("1.1", correctURI, URIUtil.toURI(fileURL));

String fileURLString = fileURL.toString();
assertThrows(URISyntaxException.class, () -> new URI(fileURLString));
assertEquals("1.2", correctURI, URIUtil.fromString(fileURLString));
}

/**
Expand Down Expand Up @@ -365,9 +384,8 @@ public void testBug286339() throws URISyntaxException {

@Test
public void testAppendWindows() throws URISyntaxException {
if (!WINDOWS) {
return;
}
assumeTrue(WINDOWS);

URI base = new URI("file:/C:/a%20b");
URI result = URIUtil.append(base, "file.txt");
assertEquals("1.0", "file:/C:/a%20b/file.txt", result.toString());
Expand Down Expand Up @@ -440,9 +458,7 @@ public void testSameURI() throws URISyntaxException {

@Test
public void testSameURIWindows() throws URISyntaxException {
if (!WINDOWS) {
return;
}
assumeTrue(WINDOWS);
// device and case variants
assertTrue("1.0", URIUtil.sameURI(new URI("file:C:/a"), new URI("file:c:/a")));
assertTrue("1.1", URIUtil.sameURI(new URI("file:/C:/a"), new URI("file:/c:/a")));
Expand Down
2 changes: 1 addition & 1 deletion bundles/org.eclipse.equinox.common/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: %pluginName
Bundle-SymbolicName: org.eclipse.equinox.common; singleton:=true
Bundle-Version: 3.19.200.qualifier
Bundle-Version: 3.20.0.qualifier
Bundle-Localization: plugin
Export-Package: org.eclipse.core.internal.boot;x-friends:="org.eclipse.core.resources,org.eclipse.pde.build",
org.eclipse.core.internal.runtime;common=split;mandatory:=common;
Expand Down
9 changes: 4 additions & 5 deletions bundles/org.eclipse.equinox.common/src/org/eclipse/core/internal/runtime/DataArea.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,11 @@ protected synchronized void assertLocationInitialized() throws IllegalStateExcep
// This will try to init url either from the specified location value or from
// service default
URL url = service.getURL();
if (url == null)
if (url == null) {
throw new IllegalStateException(CommonMessages.meta_instanceDataUnspecified);
// TODO assume the URL is a file:
// Use the new File technique to ensure that the resultant string is
// in the right format (e.g., leading / removed from /c:/foo etc)
location = IPath.fromOSString(new File(url.getFile()).toString());
}
// Assume the URL is a file:
location = IPath.fromPath(URIUtil.toFilePath(url));
initializeLocation();
} catch (CoreException e) {
throw new IllegalStateException(e.getMessage());
Expand Down
22 changes: 11 additions & 11 deletions ...pse.equinox.common/src/org/eclipse/core/internal/runtime/PlatformURLConfigConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
import java.io.*;
import java.net.URL;
import java.net.UnknownServiceException;
import java.nio.file.Files;
import org.eclipse.core.internal.boot.PlatformURLConnection;
import org.eclipse.core.internal.boot.PlatformURLHandler;
import org.eclipse.core.runtime.URIUtil;
import org.eclipse.osgi.service.datalocation.Location;
import org.eclipse.osgi.util.NLS;

Expand Down Expand Up @@ -51,23 +53,21 @@ protected URL resolve() throws IOException {
Location parentConfig = localConfig.getParentLocation();
// assume we will find the file locally
URL localURL = new URL(localConfig.getURL(), path);
if (!FILE_PROTOCOL.equals(localURL.getProtocol()) || parentConfig == null)
if (!FILE_PROTOCOL.equals(localURL.getProtocol()) || parentConfig == null) {
// we only support cascaded file: URLs
return localURL;
File localFile = new File(localURL.getPath());
if (localFile.exists())
}
if (Files.exists(URIUtil.toFilePath(localURL))) {
// file exists in local configuration
return localURL;
}
// try to find in the parent configuration
URL parentURL = new URL(parentConfig.getURL(), path);
if (FILE_PROTOCOL.equals(parentURL.getProtocol())) {
// we only support cascaded file: URLs
File parentFile = new File(parentURL.getPath());
if (parentFile.exists()) {
// parent has the location
parentConfiguration = true;
return parentURL;
}
// we only support cascaded file: URLs
if (FILE_PROTOCOL.equals(parentURL.getProtocol()) && Files.exists(URIUtil.toFilePath(parentURL))) {
// parent has the location
parentConfiguration = true;
return parentURL;
}
return localURL;
}
Expand Down
33 changes: 31 additions & 2 deletions bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/URIUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import java.io.File;
import java.net.*;
import java.nio.file.Path;

/**
* A utility class for manipulating URIs. This class works around some of the
Expand Down Expand Up @@ -312,11 +313,12 @@ public static URI toURI(URL url) throws URISyntaxException {
// URL behaves differently across platforms so for file: URLs we parse from
// string form
if (SCHEME_FILE.equals(url.getProtocol())) {
String pathString = url.toExternalForm().substring(5);
String pathString = url.toExternalForm().substring(SCHEME_FILE.length() + 1);
// ensure there is a leading slash to handle common malformed URLs such as
// file:c:/tmp
if (pathString.indexOf('/') != 0)
if (!pathString.startsWith("/")) { //$NON-NLS-1$
pathString = '/' + pathString;
}
return toURI(SCHEME_FILE, null, pathString, null);
}
try {
Expand All @@ -328,6 +330,33 @@ public static URI toURI(URL url) throws URISyntaxException {
}
}

/**
* Returns the {@code Path} specified by the given {@code file} {@link URL}.
*
* @throws IllegalArgumentException if the specified URL does not use the
* {@code file} protocol
*
* @since 3.20
*/
public static Path toFilePath(URL url) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That the name contains File and Path seems to be a duplication at first, but since any kind of URL can have a path I think it should be made clear that it's a file-system path (although nio-Path can also handle non-classical file-systems).
On the other hand toFileSystemPath() seemed to be a bit verbose to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only food for thought, this suggestion isn't common convention. Could use Path fromFileUrl(URL url).
Cannot say I like that, but it seems to make it clear.

if (!SCHEME_FILE.equals(url.getProtocol())) {
throw new IllegalArgumentException("Not a 'file' url: " + url); //$NON-NLS-1$
}
Comment on lines +342 to +344
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively this method could return an empty optional if the URL is not a file-URL.
But since callers that are able to handle other protocols can simple check the precondition before calling this method, I think it's fine to keep it simple for those cases where a file-URL is used with certainty.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional seem much nicer, calles can throw exception if the want anyways with Optional.orElseThrow(...)

Then we can easily handel other error conditions here as well for example if invalid characters are in the path.

try {
return Path.of(url.toURI());
} catch (URISyntaxException | IllegalArgumentException e) {
try {
return Path.of(toURI(url));
} catch (URISyntaxException e1) {
String pathString = url.toExternalForm().substring(SCHEME_FILE.length() + 1);
if (pathString.length() > 1 && pathString.charAt(0) == '/' && pathString.charAt(1) != '/') {
pathString = pathString.substring(1);
}
return Path.of(pathString);
}
}
}

/**
* An UNC-safe factory the the URI-ctor
*
Expand Down
Loading