Throw instead of returning null for getBytes()/getData()

Update callers to handle exception or check path existence before
calling methods

6 files changed, 47 insertions, 46 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java
index c3d6bba0ac2..dfe4f12195d 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java
@@ -154,10 +154,8 @@ public class SessionZooKeeperClient {
     }
 
     public Optional<ApplicationId> readApplicationId() {
-        String idString = configCurator.getData(applicationIdPath());
-        return (idString == null)
-                ? Optional.empty()
-                : Optional.of(ApplicationId.fromSerializedForm(idString));
+        if ( ! configCurator.exists(applicationIdPath())) return Optional.empty();
+        return Optional.of(ApplicationId.fromSerializedForm(configCurator.getData(applicationIdPath())));
     }
 
     void writeApplicationPackageReference(Optional<FileReference> applicationPackageReference) {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ConfigCurator.java b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ConfigCurator.java
index 24553704862..5bfa06a29dd 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ConfigCurator.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ConfigCurator.java
@@ -1,4 +1,4 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.zookeeper;
 
 import com.google.inject.Inject;
@@ -93,12 +93,12 @@ public class ConfigCurator {
         }
     }
 
-    /** Returns the data at a path and node. Replaces / by # in node names. Returns null if the path doesn't exist. */
+    /** Returns the data at a path and node. Replaces / by # in node names. */
     public String getData(String path, String node) {
         return getData(createFullPath(path, node));
     }
 
-    /** Returns the data at a path. Returns null if the path doesn't exist. */
+    /** Returns the data at a path */
     public String getData(String path) {
         byte[] data = getBytes(path);
         return (data == null) ? null : Utf8.toString(data);
@@ -111,8 +111,9 @@ public class ConfigCurator {
      * @return a byte array with data.
      */
     public byte[] getBytes(String path) {
+        if ( ! exists(path)) throw new IllegalArgumentException("Cannot read data from path " + path + ", it does not exist");
+
         try {
-            if ( ! exists(path)) return null; // TODO: Ugh
             return curator.framework().getData().forPath(path);
         }
         catch (Exception e) {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ZKApplication.java b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ZKApplication.java
index 5f180bdaee1..084f26bd368 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ZKApplication.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ZKApplication.java
@@ -1,4 +1,4 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.zookeeper;
 
 import com.yahoo.io.reader.NamedReader;
@@ -9,7 +9,6 @@ import java.io.StringReader;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-import java.util.Optional;
 
 /**
  * Responsible for providing data from an application subtree in zookeeper.
@@ -75,18 +74,12 @@ public class ZKApplication {
      * @return a Reader that can be used to get the data
      */
     Reader getDataReader(String path, String node) {
-        String data = getData(path, node);
-        if (data == null) {
-            throw new IllegalArgumentException("No node for " + getFullPath(path) + "/" + node + " exists");
-        }
-        return reader(data);
-    }
-
-    Optional<Reader> getOptionalDataReader(String path, String node) {
-        return Optional.ofNullable(getData(path, node)).map(data -> reader(data));
+        return reader(getData(path, node));
     }
 
     public String getData(String path, String node) {
+        if ( ! exists(path, node)) throw new IllegalArgumentException("No node for " + getFullPath(path) + "/" + node + " exists");
+
         try {
             return zk.getData(getFullPath(path), node);
         } catch (Exception e) {
@@ -96,6 +89,8 @@ public class ZKApplication {
     }
 
     public String getData(String path) {
+        if ( ! exists(path)) throw new IllegalArgumentException("No node for " + getFullPath(path) + " exists");
+
         try {
             return zk.getData(getFullPath(path));
         } catch (RuntimeException e) {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ZKApplicationFile.java b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ZKApplicationFile.java
index 2a933ddf3e5..674c0f72c40 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ZKApplicationFile.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ZKApplicationFile.java
@@ -1,4 +1,4 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.zookeeper;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
@@ -61,21 +61,17 @@ class ZKApplicationFile extends ApplicationFile {
     @Override
     public Reader createReader() throws FileNotFoundException {
         String zkPath = getZKPath(path);
-        String data = zkApp.getData(zkPath);
-        if (data == null) {
-            throw new FileNotFoundException("No such path: " + path);
-        }
-        return new StringReader(data);
+        if ( ! zkApp.exists(zkPath)) throw new FileNotFoundException("No such path: " + path);
+
+        return new StringReader(zkApp.getData(zkPath));
     }
 
     @Override
     public InputStream createInputStream() throws FileNotFoundException {
         String zkPath = getZKPath(path);
-        byte[] data = zkApp.getBytes(zkPath);
-        if (data == null) {
-            throw new FileNotFoundException("No such path: " + path);
-        }
-        return new ByteArrayInputStream(data);
+        if ( ! zkApp.exists(zkPath)) throw new FileNotFoundException("No such path: " + path);
+
+        return new ByteArrayInputStream(zkApp.getBytes(zkPath));
     }
 
     @Override
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ZKApplicationPackage.java b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ZKApplicationPackage.java
index fa562c4813b..bcb19a8f25a 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ZKApplicationPackage.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ZKApplicationPackage.java
@@ -31,7 +31,6 @@ import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Optional;
 
 /**
@@ -257,9 +256,12 @@ public class ZKApplicationPackage implements ApplicationPackage {
 
     @Override
     public File getFileReference(Path pathRelativeToAppDir) {
-        String fileName = zkApplication.getData(ConfigCurator.USERAPP_ZK_SUBPATH + "/" + pathRelativeToAppDir.getRelative());
+        String path = ConfigCurator.USERAPP_ZK_SUBPATH + "/" + pathRelativeToAppDir.getRelative();
+
         // File does not exist: Manufacture a non-existing file
-        return new File(Objects.requireNonNullElseGet(fileName, pathRelativeToAppDir::getRelative));
+        if ( ! zkApplication.exists(path)) return new File(pathRelativeToAppDir.getRelative());
+
+        return new File(zkApplication.getData(path));
     }
 
     @Override
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/zookeeper/ConfigCuratorTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/zookeeper/ConfigCuratorTest.java
index 4fab1f88226..9a8aec72564 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/zookeeper/ConfigCuratorTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/zookeeper/ConfigCuratorTest.java
@@ -1,9 +1,11 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.zookeeper;
 
 import com.yahoo.text.Utf8;
 import com.yahoo.vespa.curator.mock.MockCurator;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 import static org.junit.Assert.*;
 
@@ -146,7 +148,7 @@ public class ConfigCuratorTest {
     }
 
     private ConfigCurator deployApp() {
-        ConfigCurator zkIf = getFacade();
+        ConfigCurator zkIf = create();
         initAndClearZK(zkIf);
         zkIf.putData(ConfigCurator.DEFCONFIGS_ZK_SUBPATH, defKey1, payload1);
         // zkIf.putData(ConfigCurator.USERCONFIGS_ZK_SUBPATH, cfgKey1, payload3);
@@ -159,9 +161,12 @@ public class ConfigCuratorTest {
         return zkIf;
     }
 
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
+
     @Test
     public void testZKInterface() {
-        ConfigCurator zkIf = getFacade();
+        ConfigCurator zkIf = create();
         zkIf.putData("", "test", "foo");
         zkIf.putData("/test", "me", "bar");
         zkIf.putData("", "test;me;now,then", "baz");
@@ -171,16 +176,20 @@ public class ConfigCuratorTest {
     }
 
     @Test
-    public void testWatcher() {
-        ConfigCurator zkIf = getFacade();
+    public void testNonExistingPath() {
+        ConfigCurator configCurator = create();
 
-        String data = zkIf.getData("/nothere");
-        assertNull(data);
-        zkIf.putData("", "/nothere", "foo");
-        assertEquals(zkIf.getData("/nothere"), "foo");
+        expectedException.expect(IllegalArgumentException.class);
+        expectedException.expectMessage("Cannot read data from path /non-existing, it does not exist");
+        configCurator.getData("/non-existing");
+    }
+
+    @Test
+    public void testWatcher() {
+        ConfigCurator zkIf = create();
 
         zkIf.putData("", "test", "foo");
-        data = zkIf.getData("/test");
+        String data = zkIf.getData("/test");
         assertEquals(data, "foo");
         zkIf.putData("", "/test", "bar");
         data = zkIf.getData("/test");
@@ -190,7 +199,7 @@ public class ConfigCuratorTest {
         zkIf.putData("", "test2", "foo2");
     }
 
-    private ConfigCurator getFacade() {
+    private ConfigCurator create() {
         return ConfigCurator.create(new MockCurator());
     }
 
@@ -202,14 +211,14 @@ public class ConfigCuratorTest {
 
     @Test
     public void testEmptyData() {
-        ConfigCurator zkIf = getFacade();
+        ConfigCurator zkIf = create();
         zkIf.createNode("/empty", "data");
         assertEquals("", zkIf.getData("/empty", "data"));
     }
 
     @Test
     public void testRecursiveDelete() {
-        ConfigCurator configCurator = getFacade();
+        ConfigCurator configCurator = create();
         configCurator.putData("/foo", Utf8.toBytes("sadsdfsdfsdfsdf"));
         configCurator.putData("/foo/bar", Utf8.toBytes("dsfsdffds"));
         configCurator.putData("/foo/baz",