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.

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

PARQUET-1534: [parquet-cli] Argument error: Illegal character in opaque part at index 2 on Windows #627

Merged
merged 3 commits into from May 27, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -50,6 +50,7 @@
import java.io.InputStreamReader;
import java.net.MalformedURLException;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.security.AccessController;
Expand Down Expand Up @@ -175,12 +176,13 @@ public Path qualifiedPath(String filename) throws IOException {
* @throws IOException if there is an error creating a qualified URI
*/
public URI qualifiedURI(String filename) throws IOException {
URI fileURI = URI.create(filename);
if (RESOURCE_URI_SCHEME.equals(fileURI.getScheme())) {
return fileURI;
} else {
return qualifiedPath(filename).toUri();
}
try {
URI fileURI = new URI(filename);
if (RESOURCE_URI_SCHEME.equals(fileURI.getScheme())) {
return fileURI;
}
} catch (URISyntaxException ignore) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a behavior change that breaks the stated contract of this method: @throws IOException if there is an error creating a qualified URI.

In addition, the tests do not validate this change. What happens to invalid URIs after this change? Are characters automatically escaped by using Path? What about a resource URI with an unescaped character?

It is fine to change the method contract, but the new behavior must be as well understood as the previous behavior and it should be tested. The tests in this PR check, for example, that the file name has not changed and don't exercise the error handling code at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rdblue Thanks for your comment.

This is a behavior change that breaks the stated contract of this method: @throws IOException if there is an error creating a qualified URI.

I don't agree. URI.create and fileURI.getScheme() don't throw IOException and URISyntaxException is not a subclass of IOException.

In addition, the tests do not validate this change. What happens to invalid URIs after this change?

The difference from URI.create and new URI is only whether the exception to throw is the IllegalArgumentException or URISyntaxException. The reason to use new URI is that catching with IllegalArgumentException may catch an unintended exception.

https://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/2d4edd21bb38/src/share/classes/java/net/URI.java#l848

return qualifiedPath(filename).toUri();
}

/**
Expand Down Expand Up @@ -392,5 +394,4 @@ protected Schema getAvroSchema(String source) throws IOException {
"Could not determine file format of %s.", source));
}
}

}
102 changes: 102 additions & 0 deletions parquet-cli/src/test/java/org/apache/parquet/cli/BaseCommandTest.java
@@ -0,0 +1,102 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.parquet.cli;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.Path;
import org.junit.Assert;
import org.junit.Assume;
import org.junit.Before;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.net.URI;
import java.util.List;

public class BaseCommandTest {

private static final String WIN_FILE_PATH = "C:\\Test\\Downloads\\test.parquet";
private static final String LINUX_FILE_PATH = "/var/tmp/test.parquet";

private Logger console = LoggerFactory.getLogger(BaseCommandTest.class);
private TestCommand command;

@Before
public void setUp() {
this.command = new TestCommand(this.console);
}

@Test
public void qualifiedPathOnLinuxTest() throws IOException {
Assume.assumeFalse
(System.getProperty("os.name").toLowerCase().startsWith("win"));
Path path = this.command.qualifiedPath(LINUX_FILE_PATH);
Assert.assertEquals("test.parquet", path.getName());
}

@Test
public void qualifiedPathOnWinTest() throws IOException {
Assume.assumeFalse
gszadovszky marked this conversation as resolved.
Show resolved Hide resolved
(System.getProperty("os.name").toLowerCase().startsWith("linux"));
Path path = this.command.qualifiedPath(WIN_FILE_PATH);
Assert.assertEquals("test.parquet", path.getName());
}

@Test
public void qualifiedURILinuxFileTest() throws IOException {
Assume.assumeFalse
(System.getProperty("os.name").toLowerCase().startsWith("win"));
URI uri = this.command.qualifiedURI(LINUX_FILE_PATH);
Assert.assertEquals("/var/tmp/test.parquet", uri.getPath());
}

@Test
public void qualifiedURIWinFileTest() throws IOException {
Assume.assumeFalse
(System.getProperty("os.name").toLowerCase().startsWith("linux"));
URI uri = this.command.qualifiedURI(WIN_FILE_PATH);
Assert.assertEquals("/C:/Test/Downloads/test.parquet", uri.getPath());
}

@Test
public void qualifiedURIResourceURITest() throws IOException {
URI uri = this.command.qualifiedURI("resource:/a");
Assert.assertEquals("/a", uri.getPath());
}

class TestCommand extends BaseCommand {

public TestCommand(Logger console) {
super(console);
setConf(new Configuration());
}

@Override
public int run() throws IOException {
return 0;
}

@Override
public List<String> getExamples() {
return null;
}
}
}