From 747d85c18a5237de95ea7f9eccb2367066f40a2e Mon Sep 17 00:00:00 2001 From: Dariusz Luksza Date: Wed, 10 Sep 2014 10:04:02 +0200 Subject: [PATCH] Make SecureStore an abstract class This modification will make the implementation of a site program to switch the secure store easier. The new SwitchSecureStore program will be added by a follow-up change. In switch-secure-store we need to migrate all values stored in the old SecureStore to new one. Because we don't store (and also can't reliably figure out) the value type the only possible way is to first try read the value as String, then when it is null try to get it as List. But this assumption will fail with jgit Config implementation which is used in DefaultSecureStore. JGit Config during getString() method call will call getRawStringList() and return its first element. Therefore migration will be incorrect for all stored List values. The solution here is to always use getList() and setList() in switch-secure-store and repeat jgit behavior in SecureStore. Making the get() method to return getList()[0] and set() to call setList() with single element. To provide default implementations of get() and set() method in SecureStore we need to make it as an abstract class. Also those methods are marked as final to prevent implementors from overriding them. Also changes JarScanner.findImplementationsOf to findSubClassesOf(). This method was introduced for SecureStore (and it was only used by it), so now when SecureStore becomes abstract class its implementation (and name) also should be updated. Change-Id: Id13bc6ec170c31b43b5a1cd696ba746006e1f0b2 Signed-off-by: Dariusz Luksza --- .../com/google/gerrit/pgm/init/BaseInit.java | 2 +- .../gerrit/pgm/init/UpgradeFrom2_0_xTest.java | 12 +-------- .../gerrit/server/plugins/JarScanner.java | 26 ++++++++++++------- .../securestore/DefaultSecureStore.java | 17 +----------- .../server/securestore/SecureStore.java | 24 ++++++++++++----- 5 files changed, 36 insertions(+), 45 deletions(-) diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/BaseInit.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/BaseInit.java index 689a55602f..c46f8e3f75 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/BaseInit.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/BaseInit.java @@ -293,7 +293,7 @@ public class BaseInit extends SiteProgram { } JarScanner scanner = new JarScanner(secureStoreLib); List secureStores = - scanner.findImplementationsOf(SecureStore.class); + scanner.findSubClassesOf(SecureStore.class); if (secureStores.isEmpty()) { throw new InvalidSecureStoreException(String.format( "Cannot find class implementing %s interface in %s", diff --git a/gerrit-pgm/src/test/java/com/google/gerrit/pgm/init/UpgradeFrom2_0_xTest.java b/gerrit-pgm/src/test/java/com/google/gerrit/pgm/init/UpgradeFrom2_0_xTest.java index c73a85fc5c..720d1082cd 100644 --- a/gerrit-pgm/src/test/java/com/google/gerrit/pgm/init/UpgradeFrom2_0_xTest.java +++ b/gerrit-pgm/src/test/java/com/google/gerrit/pgm/init/UpgradeFrom2_0_xTest.java @@ -116,24 +116,14 @@ public class UpgradeFrom2_0_xTest extends InitTestCase { u.run(); } - private static class InMemorySecureStore implements SecureStore { + private static class InMemorySecureStore extends SecureStore { private final Config cfg = new Config(); - @Override - public String get(String section, String subsection, String name) { - return cfg.getString(section, subsection, name); - } - @Override public String[] getList(String section, String subsection, String name) { return cfg.getStringList(section, subsection, name); } - @Override - public void set(String section, String subsection, String name, String value) { - cfg.setString(section, subsection, name, value); - } - @Override public void setList(String section, String subsection, String name, List values) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/JarScanner.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/JarScanner.java index df4ead8f80..a8600fe683 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/JarScanner.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/JarScanner.java @@ -43,7 +43,7 @@ import java.io.File; import java.io.IOException; import java.io.InputStream; import java.lang.annotation.Annotation; -import java.util.Arrays; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.Enumeration; @@ -134,11 +134,14 @@ public class JarScanner implements PluginContentScanner { return result.build(); } - public List findImplementationsOf(Class requestedInterface) - throws IOException { - List result = Lists.newArrayList(); - String name = requestedInterface.getName().replace('.', '/'); + public List findSubClassesOf(Class superClass) throws IOException { + return findSubClassesOf(superClass.getName()); + } + private List findSubClassesOf(String superClass) throws IOException { + String name = superClass.replace('.', '/'); + + List classes = new ArrayList<>(); Enumeration e = jarFile.entries(); while (e.hasMoreElements()) { JarEntry entry = e.nextElement(); @@ -155,13 +158,15 @@ public class JarScanner implements PluginContentScanner { continue; } - if (def.isConcrete() && def.interfaces != null - && Iterables.contains(Arrays.asList(def.interfaces), name)) { - result.add(def.className); + if (name.equals(def.superName)) { + classes.addAll(findSubClassesOf(def.className)); + if (def.isConcrete()) { + classes.add(def.className); + } } } - return result; + return classes; } private static boolean skip(JarEntry entry) { @@ -192,6 +197,7 @@ public class JarScanner implements PluginContentScanner { public static class ClassData extends ClassVisitor { int access; String className; + String superName; String annotationName; String annotationValue; String[] interfaces; @@ -212,7 +218,7 @@ public class JarScanner implements PluginContentScanner { String superName, String[] interfaces) { this.className = Type.getObjectType(name).getClassName(); this.access = access; - this.interfaces = interfaces; + this.superName = superName; } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/securestore/DefaultSecureStore.java b/gerrit-server/src/main/java/com/google/gerrit/server/securestore/DefaultSecureStore.java index 130ed0fdce..b852217555 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/securestore/DefaultSecureStore.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/securestore/DefaultSecureStore.java @@ -30,7 +30,7 @@ import java.util.ArrayList; import java.util.List; @Singleton -public class DefaultSecureStore implements SecureStore { +public class DefaultSecureStore extends SecureStore { private final FileBasedConfig sec; @Inject @@ -44,26 +44,11 @@ public class DefaultSecureStore implements SecureStore { } } - @Override - public String get(String section, String subsection, String name) { - return sec.getString(section, subsection, name); - } - @Override public String[] getList(String section, String subsection, String name) { return sec.getStringList(section, subsection, name); } - @Override - public void set(String section, String subsection, String name, String value) { - if (value != null) { - sec.setString(section, subsection, name, value); - } else { - sec.unset(section, subsection, name); - } - save(); - } - @Override public void setList(String section, String subsection, String name, List values) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/securestore/SecureStore.java b/gerrit-server/src/main/java/com/google/gerrit/server/securestore/SecureStore.java index b1f07aec7c..f3eeaf6f50 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/securestore/SecureStore.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/securestore/SecureStore.java @@ -14,9 +14,11 @@ package com.google.gerrit.server.securestore; +import com.google.common.collect.Lists; + import java.util.List; -public interface SecureStore { +public abstract class SecureStore { public static class EntryKey { public final String name; public final String section; @@ -29,15 +31,23 @@ public interface SecureStore { } } - String get(String section, String subsection, String name); + public final String get(String section, String subsection, String name) { + String[] values = getList(section, subsection, name); + if (values != null && values.length > 0) { + return values[0]; + } + return null; + } - String[] getList(String section, String subsection, String name); + public abstract String[] getList(String section, String subsection, String name); - void set(String section, String subsection, String name, String value); + public final void set(String section, String subsection, String name, String value) { + setList(section, subsection, name, Lists.newArrayList(value)); + } - void setList(String section, String subsection, String name, List values); + public abstract void setList(String section, String subsection, String name, List values); - void unset(String section, String subsection, String name); + public abstract void unset(String section, String subsection, String name); - Iterable list(); + public abstract Iterable list(); }