false positive: Suggests to prefer Files.newOutputStream when FileOutputStream is needed

[McPringle]

In my project I have a method which requires a FileOutputStream:

private @NotNull Path downloadDatabaseFile() {
    try {
        final Path temporaryDatabaseFile = Files.createTempFile("jfs-", ".db");
        LOGGER.info("Start downloading database from {} ...", dbUrl);
        final URI uri = URI.create(dbUrl);
        final URL url = uri.toURL();
        try (
                ReadableByteChannel rbc = Channels.newChannel(url.openStream());
                FileOutputStream fos = new FileOutputStream(temporaryDatabaseFile.toFile())
        ) {
            fos.getChannel().transferFrom(rbc, 0, Long.MAX_VALUE);
        }
        LOGGER.info("Successfully downloaded database to temporary file {}", temporaryDatabaseFile);
        return temporaryDatabaseFile;
    } catch (final IOException e) {
        throw new SessionImportException(String.format(
                "Error downloading database file from '%s': %s",
                dbUrl, e.getMessage()), e);
    }
}

The code in question is the second variable in the try-with-resources statement:

FileOutputStream fos = new FileOutputStream(temporaryDatabaseFile.toFile())

The output of the modernizer plugin during the build:

[INFO] --- modernizer:3.0.0:modernizer (modernizer) @ apus ---
[ERROR] /home/mcpringle/dev/apus/src/main/java/swiss/fihlon/apus/plugin/event/jfs/JavaForumStuttgartPlugin.java:112: Prefer java.nio.file.Files.newOutputStream(java.nio.file.Path)

In this case, the FileOutputStream is mandatory and can't be replaced by a OutputStream. That breaks the following logic in this method (use of transferFrom).

Link to source file in repo: https://github.com/McPringle/apus/blob/e5b2ab3124250ca7bcf61e40e34987aa0fe34e27/src/main/java/swiss/fihlon/apus/plugin/event/jfs/JavaForumStuttgartPlugin.java#L114

Steps to reproduce:

1. clone the repo
   git clone https://github.com/McPringle/apus.git
2. change into the new directory
   cd apus
3. run the verify phase
   ./mvnw verify
4. see the error

Attention: In the latest commit I added the @SuppressModernizer in line 105. You have to remove it first, to reproduce the error message!

[gaul]

Unfortunately Files.newInputStream returns an InputStream and not a FileInputStream. Users cannot cast from the former to the latter since it is actually a sun.nio.ch.ChannelInputStream. I agree that there are legitimate uses of FileInputStream to get channels. I'm not sure there is some narrower violation modernizer could use like FileInputStream.read since many of these will get wrapped in BufferedInputStream or be assigned to the InputStream superclass.

CC @mkarg who introduced this violation. Any suggestions if we can improve this situation or force users to suppress violations?

[mkarg]

@McPringle Hello Marcus, I know that code line (in fact I used Apus for a beta test befor committing the NIO fixes for Modernizer) and already wanted to propose to get rid of the Channel recently, but forgot to ask you on our last telco on Friday! Let's discuss the solution out-of-bands. The solution should be as-easy-as using InputStream.transferTo(OutputStream) which internally does the pretty same as your explicit code (I knew it because I implemented it in OpenJDK).

@gaul Nobody should be forced to keep FileInputStream, as I explained above, so there is nothing to change here, actually. In fact, getting rid of FileInputStream is the intention of NIO2.

[McPringle]

Thank you for your explanation, @mkarg. Maybe the message could be made more clear about that. Or should the modernizer plugin mention the problem/solution in the line containing fos.getChannel().transferFrom instead (or additional)?

[mkarg]

The use of FileOutputStream in general is not necessary since Java 7, and the intention of the message is to replace it completely. I do not see that a special message for fos.getChannel().fransferFrom provides any additional benefit. But maybe I am missing something?

[mkarg]

@gaul Proposing to close this issue, as the question is answered. In the exception case where a violation is to be ignored, the code location can be annotated.