Fix #329: Empty RAF creation on read #330

gab1one · 2018-06-19T10:28:09Z

When trying to read from a non-existing file, the FileHandle creates a RandomAccessFile, which results in an empty file appearing on disk. This PR fixes this by throwing an IOException when trying to read from a non-existent file.

imagejan · 2018-06-19T10:38:10Z

src/test/java/org/scijava/io/handle/FileHandleTest.java

+			handle.read(); // this will fail as there is no underlying file!
+			fail("Read successfully from non-existing file!");
+		}
+		catch (final Exception exc) {


How about catching only IOException instead of Exception here?

Good point, I just pushed an updated version that does just that.

ctrueden

This is awesome! Splitting the RAF logic along read/write lines is exactly the right change to finally fix this bugbear. I have only a few minor comments+nitpicks, and then we should be good to merge.

ctrueden · 2018-06-19T16:17:23Z

src/test/java/org/scijava/io/handle/FileHandleTest.java

+		assertFalse(nonExistentFile.exists());
+
+		final FileLocation loc = new FileLocation(uri);
+		final DataHandle<?> handle = dhs.create(loc);


We should use try-with-resources here, no?

try (final DataHandle<?> handle = dhs.create(loc)) { ... }

So that it definitely gets closed at the end, regardless of the outcome of the test

ctrueden · 2018-06-19T16:22:48Z

src/main/java/org/scijava/io/handle/FileHandle.java

 	}

 	@Override
 	public long length() throws IOException {
-		return exists() ? raf().length() : -1;
+		return exists() ? writeRaf().length() : -1;


Conceptually, this should be readRaf() since querying the length is a "read" operation. (I realize that in practice, it won't matter due to the exists() check beforehand, but the code looks strange/wrong with writeRaf() here.)

ctrueden · 2018-06-19T16:23:09Z

src/main/java/org/scijava/io/handle/FileHandle.java

+	 */
+	private RandomAccessFile readRaf() throws IOException {
+		if (raf == null) {
+			if (!exists()) {


I would prefer this exists() check live in the initRAF() method. It can be done by making the method initRAF(final boolean create) and then writing if (!create && !exists()) throw .... Then, here in readRAF(), simply put if (raf == null) initRAF(false);—and correspondingly, initRAF(true) in the writeRAF() method. Then the two methods will be symmetrical, and IMHO easier to understand.

Note also the case of RAF—it is an acronym, so should be all capitals, not camel case Raf. Maybe we want to call them reader() and writer() instead of readRaf() and writeRaf()? I feel like readRaf() and writeRaf() are somehow too long/unwieldy.

Great suggestion, this makes the code much cleaner :)

gab1one · 2018-06-20T12:08:33Z

Thanks for the input @ctrueden I integrated your comments and it should be good to go now.

ctrueden

Just a handful of little things now!

ctrueden · 2018-06-20T14:48:25Z

src/main/java/org/scijava/io/handle/FileHandle.java

@@ -101,199 +101,199 @@ public Date lastModified() {

 	@Override
 	public long offset() throws IOException {
-		return raf().getFilePointer();
+		return exists() ? writer().getFilePointer() : 0;


This should be reader().

ctrueden · 2018-06-20T14:49:23Z

src/main/java/org/scijava/io/handle/FileHandle.java

@@ -62,7 +62,7 @@

 	/** Gets the random access file object backing this FileHandle. */
 	public RandomAccessFile getRandomAccessFile() throws IOException {
-		return raf();
+		return writer();


We should amend the javadoc of this method to indicate that calling this method will create the file if it does not already exist.

ctrueden · 2018-06-20T14:50:47Z

src/main/java/org/scijava/io/handle/FileHandle.java

+	 * independently of the underlying file existing on disk. This allows us to
+	 * create a new file for writing.
+	 *
+	 * @return the internal {@link RandomAccessFile} or creates if if needed.


Should be the internal {@link RandomAccessFile}, creating it if needed.

ctrueden · 2018-06-20T14:51:14Z

src/main/java/org/scijava/io/handle/FileHandle.java

+	 * if the underlying file exists on disk. This prevents accidental creation of
+	 * an empty RAF file when calling read operations on a non-existent file.
+	 *
+	 * @return the internal {@link RandomAccessFile} or creates if if needed.


Should be the internal {@link RandomAccessFile}, creating it if needed.

ctrueden · 2018-06-20T15:04:38Z

src/main/java/org/scijava/io/handle/FileHandle.java

+	}
+
+	/**
+	 * Initializes the {@link RandomAccessFile},


, ➡️ .

ctrueden · 2018-06-20T15:05:29Z

src/main/java/org/scijava/io/handle/FileHandle.java

+	 * @param create whether to create the {@link RandomAccessFile} if the
+	 *          underlying file does not exist yet.
+	 * @throws IOException if the {@link RandomAccessFile} could not be created,
+	 *           or the backing file does not exists and the {@code create}


exists ➡️ exist

ctrueden · 2018-06-20T15:05:38Z

src/main/java/org/scijava/io/handle/FileHandle.java

+	 *          underlying file does not exist yet.
+	 * @throws IOException if the {@link RandomAccessFile} could not be created,
+	 *           or the backing file does not exists and the {@code create}
+	 *           parameter was set to {@code true}.


true ➡️ false

When trying to read from a non-existing file, the FileHandle created a RandomAccessFile, which resulted in an empty file appearing on disk.

gab1one · 2018-06-21T15:03:37Z

Just a handful of little things now!
Also included :)

ctrueden · 2018-06-21T18:07:51Z

Thanks @gab1one !!

gab1one requested a review from ctrueden June 19, 2018 10:28

imagejan reviewed Jun 19, 2018

View reviewed changes

gab1one force-pushed the fix-raf-creation branch from e868c41 to ceb430f Compare June 19, 2018 13:08

ctrueden requested changes Jun 19, 2018

View reviewed changes

Add test that reproduces #329

b5ad01a

gab1one force-pushed the fix-raf-creation branch from ceb430f to 83fd2a1 Compare June 20, 2018 12:06

ctrueden requested changes Jun 20, 2018

View reviewed changes

Fix #329: Don't create empty RAF files on reading

15b70a1

When trying to read from a non-existing file, the FileHandle created a RandomAccessFile, which resulted in an empty file appearing on disk.

gab1one force-pushed the fix-raf-creation branch from 83fd2a1 to 15b70a1 Compare June 20, 2018 16:19

ctrueden merged commit 560ad96 into master Jun 21, 2018

ctrueden deleted the fix-raf-creation branch June 21, 2018 18:07

Fix #329: Empty RAF creation on read #330

Fix #329: Empty RAF creation on read #330

Uh oh!

Conversation

gab1one commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctrueden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gab1one commented Jun 20, 2018

Uh oh!

ctrueden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gab1one commented Jun 21, 2018

Uh oh!

ctrueden commented Jun 21, 2018

Uh oh!

Uh oh!