From e81c3804cd285e406f4ea99b660f65e596e3a5a5 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Wed, 30 Jun 2021 10:33:17 +0200 Subject: Throw instead of returning null for getBytes()/getData() Update callers to handle exception or check path existence before calling methods --- .../server/session/SessionZooKeeperClient.java | 6 ++-- .../config/server/zookeeper/ConfigCurator.java | 9 +++--- .../config/server/zookeeper/ZKApplication.java | 17 ++++------- .../config/server/zookeeper/ZKApplicationFile.java | 18 +++++------ .../server/zookeeper/ZKApplicationPackage.java | 8 +++-- .../config/server/zookeeper/ConfigCuratorTest.java | 35 ++++++++++++++-------- 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 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 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 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", -- cgit v1.2.3