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

Merged
merged 7 commits into from Nov 15, 2013

Projects

None yet

2 participants

@tonycosentini
Contributor

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.

@tonycosentini tonycosentini and 1 other commented on an outdated diff Nov 15, 2013
.../com/mindsnacks/zinc/classes/ZincRepoIndexWriter.java
@@ -43,18 +43,33 @@ public ZincRepoIndex getIndex() {
return mRepoIndex;
}
+ public File getIndexFile() {
+ return mIndexFile;
@tonycosentini
tonycosentini Nov 15, 2013 Contributor

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

@NachoSoto
NachoSoto Nov 15, 2013 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? :)

@tonycosentini
tonycosentini Nov 15, 2013 Contributor

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
NachoSoto Nov 15, 2013 Collaborator

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

@tonycosentini
tonycosentini Nov 15, 2013 Contributor

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
NachoSoto Nov 15, 2013 Collaborator

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

@tonycosentini
tonycosentini Nov 15, 2013 Contributor

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

@NachoSoto
NachoSoto Nov 15, 2013 Collaborator

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

@tonycosentini
tonycosentini Nov 15, 2013 Contributor

We can't :(

public final class Gson
@NachoSoto
NachoSoto Nov 15, 2013 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 :)

@NachoSoto NachoSoto and 1 other commented on an outdated diff Nov 15, 2013
...java/com/mindsnacks/zinc/ZincRepoIndexWriterTest.java
+ * 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
NachoSoto Nov 15, 2013 Collaborator

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

@tonycosentini
tonycosentini Nov 15, 2013 Contributor

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

@NachoSoto NachoSoto and 1 other commented on an outdated diff Nov 15, 2013
...java/com/mindsnacks/zinc/ZincRepoIndexWriterTest.java
+ @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
NachoSoto Nov 15, 2013 Collaborator

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

@tonycosentini
tonycosentini Nov 15, 2013 Contributor

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

@tonycosentini
tonycosentini Nov 15, 2013 Contributor

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

@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 master Nov 15, 2013

1 check passed

default The Travis CI build passed
Details
@tonycosentini tonycosentini deleted the fixing_null_zinc_repo_index_bug branch Nov 15, 2013
@tonycosentini
Contributor

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