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

Add File.java to forbidden APIs #8666

Merged
merged 1 commit into from Dec 2, 2014
Merged

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Nov 26, 2014

This commit cuts over all of core (not quite all tests) to java.nio.Path
It also adds the file class to the core forbidden APIs to prevent its usage

@@ -114,3 +114,7 @@ org.apache.lucene.search.TimeLimitingCollector#getGlobalCounter()

@defaultMessage Don't interrupt threads use FutureUtils#cancel(Future<T>) instead
java.util.concurrent.Future#cancel(boolean)

@defaultMessage Use java.nio.file.Path / java.nio.file.Files instead of java.io.File API
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add all the java.io.File signatures (even with ones commented out if we can't yet fix them).

Full list here: http://svn.apache.org/repos/asf/lucene/dev/trunk/lucene/tools/forbiddenApis/lucene.txt

@s1monw
Copy link
Contributor Author

s1monw commented Nov 27, 2014

@rmuir I addressed your comments and added the missing lines to the signature files. I also moved away from ZipFile. If you have time I'd love to have another review...

try {
listener.onResponse(buildResponse(request, currentSnapshots, null));
} catch (IOException ex) {
listener.onFailure(ex);
Copy link
Member

Choose a reason for hiding this comment

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

This is masking the IOException? Should we rethrow after wrapping with ElasticsearchException?

@rjernst
Copy link
Member

rjernst commented Dec 1, 2014

LGTM.

try {
listener.onResponse(buildResponse(request, currentSnapshots, null));
} catch (IOException ex) {
throw new ElasticsearchException("failed to build response", ex);
Copy link
Member

Choose a reason for hiding this comment

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

This try-catch throws an exception while the one modified below calls listener.onFailure(ex). Should they match? I suspect this should call listener.onFailure(ex).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I will fix the interface of masterOperation instead to allow for exceptions

@dakrone
Copy link
Member

dakrone commented Dec 2, 2014

LGTM, left a lot of little comments.

Path[] paths = new Path[files.length];
for (int i = 0; i < files.length; i++) {
paths[i] = files[i].toPath();
public static Path append(Path base, Path path, int strip) {
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we should have a unit test for this utility method?

@rmuir
Copy link
Contributor

rmuir commented Dec 2, 2014

looks good, i am fine with the change! The suggestions i listed are pre-existing conditions i think, we can do them as a followup.


Properties pluginProps = new Properties();
InputStream is = null;
try {
Copy link
Member

Choose a reason for hiding this comment

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

May be a try with resources here?

try (InputStream is = Files.newInputStream(pluginPropFile)) {
    pluginProps.load(is);
    description = pluginProps.getProperty("description", PluginInfo.DESCRIPTION_NOT_AVAILABLE);
    version = pluginProps.getProperty("version", PluginInfo.VERSION_NOT_AVAILABLE);
} catch (Exception e) {
    // Can not load properties for this site plugin. Ignoring.
    logger.debug("can not load {} file.", e, esPluginPropertiesFile);
}

@dadoonet
Copy link
Member

dadoonet commented Dec 2, 2014

Left some comments. Most of them are not really related to this change but while you are at it :)

@s1monw
Copy link
Contributor Author

s1monw commented Dec 2, 2014

thanks guys I pushed fixes for the comments. I think it's ready...

@dadoonet
Copy link
Member

dadoonet commented Dec 2, 2014

LGTM. Do you plan to push this in 1.x?

This commit cuts over all of core (not quite all tests) to java.nio.Path
It also adds the file class to the core forbidden APIs to prevent its usage.

This commit also resolves elastic#8254 since we now consistently useing the NIO Path
API. The Changes in this commit allow for more information if IO operations fail
since the NIO API throws exceptions instead of boolean return values. The build-in
methods used in this commit are also more resillient to encodeing errors like
unmappable characters and throw exceptions if those chars are present in a file.

Closes elastic#8254
Closes elastic#8666
@s1monw
Copy link
Contributor Author

s1monw commented Dec 2, 2014

no this won't go into 1.x

@s1monw s1monw merged commit a6510f9 into elastic:master Dec 2, 2014
@s1monw s1monw deleted the remove_file branch December 2, 2014 20:52
tlrx added a commit to tlrx/elasticsearch that referenced this pull request Dec 8, 2014
The method Path.endsWith(String s) doesn't work exactly the same way as String.endsWith() (see http://docs.oracle.com/javase/7/docs/api/java/nio/file/Path.html#endsWith(java.nio.file.Path)). This blocks the loading of plugins.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants