Skip to content

Commit 1179563

Browse files
garydgregorygarydgregory
authored
DeferredFileOutputStream NPE clean up and new file permission tests (#862)
- Add tests to validate owner permission on POSIX. - Refactor test to check a specific folder for POSIX instead of the default FS. - DeferredFileOutputStream.thresholdReached(): Throws an NPE with a useful message. - DeferredFileOutputStream.thresholdReached(): Better local variable name.
1 parent b0cb5ed commit 1179563

5 files changed

Lines changed: 196 additions & 21 deletions

File tree

‎src/main/java/org/apache/commons/io/output/DeferredFileOutputStream.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -479,14 +479,14 @@ protected void thresholdReached() throws IOException {
479479
tempFile = true;
480480
}
481481
PathUtils.createParentDirectories(outputPath, null, PathUtils.EMPTY_FILE_ATTRIBUTE_ARRAY);
482-
final OutputStream fos = Files.newOutputStream(outputPath);
482+
final OutputStream os = Files.newOutputStream(Objects.requireNonNull(outputPath, "Either output file or prefix must be specified."));
483483
try {
484-
memoryOutputStream.writeTo(fos);
484+
memoryOutputStream.writeTo(os);
485485
} catch (final IOException e) {
486-
fos.close();
486+
os.close();
487487
throw e;
488488
}
489-
currentOutputStream = fos;
489+
currentOutputStream = os;
490490
memoryOutputStream = null;
491491
}
492492

‎src/test/java/org/apache/commons/io/FileUtilsTest.java‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@
8383

8484
import org.apache.commons.io.file.AbstractTempDirTest;
8585
import org.apache.commons.io.file.Counters.PathCounters;
86+
import org.apache.commons.io.file.NioFileSystem;
8687
import org.apache.commons.io.file.PathUtils;
8788
import org.apache.commons.io.file.TempDirectory;
8889
import org.apache.commons.io.file.TempFile;
@@ -96,7 +97,6 @@
9697
import org.junit.jupiter.api.BeforeEach;
9798
import org.junit.jupiter.api.Disabled;
9899
import org.junit.jupiter.api.Test;
99-
import org.junit.jupiter.api.condition.EnabledIf;
100100
import org.junit.jupiter.api.condition.EnabledOnOs;
101101
import org.junit.jupiter.api.condition.OS;
102102
import org.junit.jupiter.api.io.TempDir;
@@ -2843,12 +2843,11 @@ void testReadFileToByteArray_Errors() {
28432843
}
28442844

28452845
@Test
2846-
@EnabledIf("isPosixFilePermissionsSupported")
28472846
void testReadFileToByteArray_IOExceptionOnPosixFileSystem() throws Exception {
2847+
assumeTrue(NioFileSystem.isPosix(tempDirPath));
28482848
final File file = TestUtils.newFile(tempDirFile, "cant-read.txt");
28492849
TestUtils.createFile(file, 100);
28502850
Files.setPosixFilePermissions(file.toPath(), PosixFilePermissions.fromString("---------"));
2851-
28522851
assertThrows(IOException.class, () -> FileUtils.readFileToByteArray(file));
28532852
}
28542853

@@ -2861,12 +2860,11 @@ void testReadFileToString_Errors() {
28612860
}
28622861

28632862
@Test
2864-
@EnabledIf("isPosixFilePermissionsSupported")
28652863
void testReadFileToString_IOExceptionOnPosixFileSystem() throws Exception {
2864+
assumeTrue(NioFileSystem.isPosix(tempDirPath));
28662865
final File file = TestUtils.newFile(tempDirFile, "cant-read.txt");
28672866
TestUtils.createFile(file, 100);
28682867
Files.setPosixFilePermissions(file.toPath(), PosixFilePermissions.fromString("---------"));
2869-
28702868
assertThrows(IOException.class, () -> FileUtils.readFileToString(file));
28712869
}
28722870

@@ -2890,8 +2888,8 @@ void testReadFileToStringWithEncoding() throws Exception {
28902888
}
28912889

28922890
@Test
2893-
@EnabledIf("isPosixFilePermissionsSupported")
28942891
void testReadLines_IOExceptionOnPosixFileSystem() throws Exception {
2892+
assumeTrue(NioFileSystem.isPosix(tempDirPath));
28952893
final File file = TestUtils.newFile(tempDirFile, "cant-read.txt");
28962894
TestUtils.createFile(file, 100);
28972895
Files.setPosixFilePermissions(file.toPath(), PosixFilePermissions.fromString("---------"));

‎src/test/java/org/apache/commons/io/file/AbstractTempDirTest.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222

2323
import java.io.File;
2424
import java.io.IOException;
25-
import java.nio.file.FileSystems;
2625
import java.nio.file.Files;
2726
import java.nio.file.Path;
2827

@@ -97,8 +96,4 @@ public void beforeEachCreateTempDirs() throws IOException {
9796
tempDirFile = tempDirPath.toFile();
9897
}
9998

100-
@SuppressWarnings("resource") // no FileSystem allocation
101-
protected final boolean isPosixFilePermissionsSupported() {
102-
return FileSystems.getDefault().supportedFileAttributeViews().contains("posix");
103-
}
10499
}

‎src/test/java/org/apache/commons/io/file/NioFileSystem.java‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* https://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
18+
package org.apache.commons.io.file;
19+
20+
import java.nio.file.FileSystem;
21+
import java.nio.file.Path;
22+
import java.util.Set;
23+
24+
/**
25+
* Helps tests use {@link FileSystem}s.
26+
*/
27+
public final class NioFileSystem {
28+
// macOS:
29+
// jshell> FileSystems.getDefault().supportedFileAttributeViews()
30+
// $1 ==> [owner, basic, posix, user, unix]
31+
32+
/**
33+
* The {@value} name from {@link FileSystem#supportedFileAttributeViews()}.
34+
*/
35+
public static final String BASIC = "basic";
36+
37+
/**
38+
* The {@value} name from {@link FileSystem#supportedFileAttributeViews()}.
39+
*/
40+
public static final String DOS = "dos";
41+
42+
/**
43+
* The {@value} name from {@link FileSystem#supportedFileAttributeViews()}.
44+
*/
45+
public static final String OWNER = "owner";
46+
47+
/**
48+
* The {@value} name from {@link FileSystem#supportedFileAttributeViews()}.
49+
*/
50+
public static final String POSIX = "posix";
51+
52+
/**
53+
* The {@value} name from {@link FileSystem#supportedFileAttributeViews()}.
54+
*/
55+
public static final String UNIX = "unix";
56+
57+
/**
58+
* The {@value} name from {@link FileSystem#supportedFileAttributeViews()}.
59+
*/
60+
public static final String USER = "user";
61+
62+
/**
63+
* Tests whether the given view names contains the {@link #DOS} name.
64+
*
65+
* @param views The names to test.
66+
* @return whether the given view names contains the {@link #DOS} name.
67+
*/
68+
public static boolean isDos(final Set<String> views) {
69+
return views.contains(DOS);
70+
}
71+
72+
/**
73+
* Tests whether the given FileSystem contains the {@link #POSIX} file attribute view name.
74+
*
75+
* @param fileSystem The FileSystem to test.
76+
* @return whether the given FileSystem contains the {@link #POSIX} file attribute view name.
77+
*/
78+
public static boolean isPosix(final FileSystem fileSystem) {
79+
return supportsFileAttributeView(fileSystem, POSIX);
80+
}
81+
82+
/**
83+
* Tests whether the given Path contains the {@link #POSIX} file attribute view name.
84+
*
85+
* @param path The Path to test.
86+
* @return whether the given FileSystem contains the {@link #POSIX} file attribute view name.
87+
*/
88+
public static boolean isPosix(final Path path) {
89+
return supportsFileAttributeView(path.getFileSystem(), POSIX);
90+
}
91+
92+
/**
93+
* Tests whether the given view names contains the {@link #POSIX} name.
94+
*
95+
* @param views The names to test.
96+
* @return whether the given view names contains the {@link #POSIX} name.
97+
*/
98+
public static boolean isPosix(final Set<String> views) {
99+
return views.contains(POSIX);
100+
}
101+
102+
/**
103+
* Tests whether the given view names contains the {@link #UNIX} name.
104+
*
105+
* @param views The names to test.
106+
* @return whether the given view names contains the {@link #UNIX} name.
107+
*/
108+
public static boolean isUnix(final Set<String> views) {
109+
return views.contains(UNIX);
110+
}
111+
112+
/**
113+
* Tests whether the given FileSystem contains the {@code view} file attribute view name.
114+
*
115+
* @param fileSystem The FileSystem to test.
116+
* @param view The view name to test.
117+
* @return whether the given FileSystem contains the {@code view} file attribute view name.
118+
*/
119+
public static boolean supportsFileAttributeView(final FileSystem fileSystem, final String view) {
120+
return fileSystem.supportedFileAttributeViews().contains(view);
121+
}
122+
123+
/**
124+
* No instances needed.
125+
*/
126+
private NioFileSystem() {
127+
// empty
128+
}
129+
}

‎src/test/java/org/apache/commons/io/output/DeferredFileOutputStreamTest.java‎

Lines changed: 59 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,17 +24,22 @@
2424
import static org.junit.jupiter.api.Assertions.assertNull;
2525
import static org.junit.jupiter.api.Assertions.assertThrows;
2626
import static org.junit.jupiter.api.Assertions.assertTrue;
27+
import static org.junit.jupiter.api.Assumptions.assumeTrue;
2728

2829
import java.io.File;
2930
import java.io.IOException;
3031
import java.io.InputStream;
32+
import java.nio.charset.StandardCharsets;
3133
import java.nio.file.Files;
3234
import java.nio.file.NoSuchFileException;
3335
import java.nio.file.Path;
36+
import java.nio.file.attribute.PosixFilePermissions;
37+
import java.util.concurrent.atomic.AtomicReference;
3438
import java.util.stream.IntStream;
3539

3640
import org.apache.commons.io.IOUtils;
3741
import org.apache.commons.io.file.AbstractTempDirTest;
42+
import org.apache.commons.io.file.NioFileSystem;
3843
import org.apache.commons.io.file.PathUtils;
3944
import org.junit.jupiter.api.Test;
4045
import org.junit.jupiter.api.io.TempDir;
@@ -205,6 +210,57 @@ void testBelowThresholdGetInputStream(final int initialBufferSize) throws IOExce
205210
}
206211
}
207212

213+
/**
214+
* Tests that the temporary file is created with owner-only permissions on POSIX file systems.
215+
*/
216+
@Test
217+
void testPersistedFileIsOwnerOnlyOnPosix() throws IOException {
218+
assumeTrue(NioFileSystem.isPosix(tempDirPath));
219+
// Mirror production, where the supplier only resolves a not-yet-existing path.
220+
final Path target = tempDirPath.resolve("posixPerms.bin");
221+
final AtomicReference<Path> pathRef = new AtomicReference<>();
222+
try (DeferredFileOutputStream dos = DeferredFileOutputStream.builder()
223+
// @formatter:off
224+
.setPath(target)
225+
.setPrefix(getClass().getSimpleName())
226+
.setDeleteTempFileOnClose(false)
227+
.get()
228+
// @formatter:on
229+
) {
230+
dos.write(testBytes);
231+
pathRef.set(dos.getPath());
232+
}
233+
final Path pathTemp = pathRef.get();
234+
assertTrue(Files.exists(pathTemp));
235+
assertTrue(Files.isRegularFile(pathTemp));
236+
assertEquals("rw-------", PosixFilePermissions.toString(Files.getPosixFilePermissions(pathTemp)));
237+
}
238+
239+
/**
240+
* Tests that persisting over an already existing path truncates and overwrites it.
241+
*/
242+
@Test
243+
void testPersistedFileOverwritesExisting() throws IOException {
244+
final Path target = tempDirPath.resolve("overwrite.bin");
245+
Files.write(target, "stale-and-longer".getBytes(StandardCharsets.UTF_8));
246+
final AtomicReference<Path> pathRef = new AtomicReference<>();
247+
try (DeferredFileOutputStream dos = DeferredFileOutputStream.builder()
248+
// @formatter:off
249+
.setPath(target)
250+
.setPrefix(getClass().getSimpleName())
251+
.setDeleteTempFileOnClose(false)
252+
.get()
253+
// @formatter:on
254+
) {
255+
dos.write(testBytes);
256+
pathRef.set(dos.getPath());
257+
}
258+
final Path pathTemp = pathRef.get();
259+
assertTrue(Files.exists(pathTemp));
260+
assertTrue(Files.isRegularFile(pathTemp));
261+
assertArrayEquals(testBytes, Files.readAllBytes(pathTemp));
262+
}
263+
208264
/**
209265
* Tests that writing beyond the threshold throws a {@link NoSuchFileException} when the directory supplied to
210266
* {@link DeferredFileOutputStream.Builder#setDirectory(Path)} does not exist.
@@ -516,8 +572,7 @@ void testWriteToLarge(final int initialBufferSize) throws IOException {
516572
assertThrows(IOException.class, () -> dfos.writeTo(baos));
517573
dfos.close();
518574
dfos.writeTo(baos);
519-
final byte[] copiedBytes = baos.toByteArray();
520-
assertArrayEquals(testBytes, copiedBytes);
575+
assertArrayEquals(testBytes, baos.toByteArray());
521576
verifyResultFile(testFile);
522577
}
523578
}
@@ -541,8 +596,7 @@ void testWriteToLargeCtor(final int initialBufferSize) throws IOException {
541596
assertEquals(testBytes.length, dfos.getByteCount());
542597
dfos.close();
543598
dfos.writeTo(baos);
544-
final byte[] copiedBytes = baos.toByteArray();
545-
assertArrayEquals(testBytes, copiedBytes);
599+
assertArrayEquals(testBytes, baos.toByteArray());
546600
verifyResultFile(testFile);
547601
}
548602
}
@@ -567,8 +621,7 @@ void testWriteToSmall(final int initialBufferSize) throws IOException {
567621
assertEquals(testBytes.length, dfos.getByteCount());
568622
dfos.close();
569623
dfos.writeTo(baos);
570-
final byte[] copiedBytes = baos.toByteArray();
571-
assertArrayEquals(testBytes, copiedBytes);
624+
assertArrayEquals(testBytes, baos.toByteArray());
572625
}
573626
}
574627

0 commit comments

Comments
 (0)