Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[Final] Fixing null zinc repo index bug. #20

Merged
merged 7 commits into from

2 participants

@tonycosentini

I ran into this on a device today. Somehow the index JSON file existed, but was completely empty (not sure how it got into this state). I think it's safe to assume that if GSON can't parse it we should create a new one.

.../com/mindsnacks/zinc/classes/ZincRepoIndexWriter.java
@@ -43,18 +43,33 @@ public ZincRepoIndex getIndex() {
return mRepoIndex;
}
+ public File getIndexFile() {
+ return mIndexFile;

I wish there was a better way to get this in the test without exposing it :thumbsdown:.

@NachoSoto Collaborator

What problem did you run into because this file was empty? Maybe you can assert that that doesn't happen instead of relying on inspecting the implementation detail of the file? :)

ZincRepoIndex indexFromJSON = mGson.fromJson(new FileReader(mIndexFile), ZincRepoIndex.class);

This returns null when the file exists, but is not valid JSON (IE it's empty). Later on down the road during repo creation JavaZinc will throw a NPE because the repo index is null.

@NachoSoto Collaborator

I was asking, how can you reproduce the problem just by using the existing interface?

You can't with the current interface because you need access to the index file object. Currently the constructor takes a file that points to the root and then creates a new file using a private string constant with the JSON filename.

@NachoSoto Collaborator

But what public method could you call to trigger the NPE?

You can't without first creating an empty file object.

@NachoSoto Collaborator

I see. Then let's inject a Gson mock instead to make it return a null index :)

We can't :(

public final class Gson
@NachoSoto Collaborator

Oh crap. Then I think it's better to not write this test so that we don't break the encapsulation? Thanks for trying though :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...java/com/mindsnacks/zinc/ZincRepoIndexWriterTest.java
((17 lines not shown))
+ * Date: 11/14/13
+ * Time: 4:55 PM
+ */
+public class ZincRepoIndexWriterTest extends ZincBaseTest {
+ private ZincRepoIndexWriter mZincRepoIndexWriter;
+
+ @Rule
+ public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+ private void setupZincRepoIndexWriter(File file) {
+ mZincRepoIndexWriter = new ZincRepoIndexWriter(file, createGson());
+ }
+
+ @Test
+ public void indexWriterCreatesFileAndReturnsAnIndexWhenFileDoesNotExist() throws Exception {
+ setupZincRepoIndexWriter(temporaryFolder.newFolder());
@NachoSoto Collaborator

This is repeated in both tests, you can put it in a @Before method :)

Good catch, was originally going to use different file parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...java/com/mindsnacks/zinc/ZincRepoIndexWriterTest.java
((27 lines not shown))
+ @Before
+ public void setupZincRepoIndexWriter() throws Exception {
+ mZincRepoIndexWriter = new ZincRepoIndexWriter(temporaryFolder.newFolder(), createGson());
+ }
+
+ @Test
+ public void indexWriterCreatesFileAndReturnsAnIndexWhenFileDoesNotExist() throws Exception {
+ assertNotNull(mZincRepoIndexWriter.getIndex());
+ }
+
+ @Test
+ public void indexWriterReplacesFIleAndReturnsAnIndexWhenGSONReturnsNullWhenParsingIndexJSON() throws Exception {
+ File indexFile = mZincRepoIndexWriter.getIndexFile();
+
+ assertFalse(indexFile.exists());
+ indexFile.createNewFile();
@NachoSoto Collaborator

I'm not sure what you're testing here. You're manually creating the file in the test?

Yeah, I'm creating an empty file which means the FileNotFoundException won't be thrown in ZincRepoIndexWriter and GSON will just return null.

Oh, but I guess there is no need for these assertions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@NachoSoto
Collaborator

:shipit:

@NachoSoto
Collaborator

@tonycosentini feel free to merge if this change looks ok :)
Thanks for fixing this!

@tonycosentini tonycosentini merged commit 77fba63 into from
@tonycosentini tonycosentini deleted the branch
@tonycosentini

Dunzo. Can we get this on Maven?

@NachoSoto
Collaborator

Yup! Creating a release now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 21 additions and 9 deletions.
  1. +21 −9 src/main/java/com/mindsnacks/zinc/classes/ZincRepoIndexWriter.java
View
30 src/main/java/com/mindsnacks/zinc/classes/ZincRepoIndexWriter.java
@@ -6,6 +6,8 @@
import java.io.*;
+import static com.google.common.base.Preconditions.checkNotNull;
+
/**
* User: NachoSoto
* Date: 9/3/13
@@ -45,16 +47,26 @@ public ZincRepoIndex getIndex() {
private ZincRepoIndex initializeIndex() {
try {
- return mGson.fromJson(new FileReader(mIndexFile), ZincRepoIndex.class);
+ return checkNotNull(readRepoIndexFile());
} catch (FileNotFoundException fnfe) {
- try {
- mIndexFile.getParentFile().mkdirs();
- mIndexFile.createNewFile();
-
- return new ZincRepoIndex();
- } catch (IOException ioe) {
- throw new ZincRuntimeException("Error creating index file", ioe);
- }
+ return createNewIndexFile();
+ } catch (NullPointerException npe) {
+ return createNewIndexFile();
+ }
+ }
+
+ private ZincRepoIndex readRepoIndexFile() throws FileNotFoundException {
+ return mGson.fromJson(new FileReader(mIndexFile), ZincRepoIndex.class);
+ }
+
+ private ZincRepoIndex createNewIndexFile() {
+ try {
+ mIndexFile.getParentFile().mkdirs();
+ mIndexFile.createNewFile();
+
+ return new ZincRepoIndex();
+ } catch (IOException ioe) {
+ throw new ZincRuntimeException("Error creating index file", ioe);
}
}
Something went wrong with that request. Please try again.