Repository: komoot/photon
Pull request #646 (2022-02-27 16:22:10)

Clean up commandline parameters
Introduce a JCommander converter function to be used by all command line parameters that expect a comma-separated list of strings. The converter ensures that the result is always a non-null array of strings with all strings trimmed and no string is empty.


Patch:
diff --git a/src/main/java/de/komoot/photon/App.java b/src/main/java/de/komoot/photon/App.java
index 321391c..4388c48 100644
--- a/src/main/java/de/komoot/photon/App.java
+++ b/src/main/java/de/komoot/photon/App.java
@@ -23,30 +23,7 @@ import static spark.Spark.*;
 public class App {
 
     public static void main(String[] rawArgs) throws Exception {
-        // parse command line arguments
-        CommandLineArgs args = new CommandLineArgs();
-        final JCommander jCommander = new JCommander(args);
-        try {
-            jCommander.parse(rawArgs);
-            if (args.isCorsAnyOrigin() && args.getCorsOrigin() != null) { // these are mutually exclusive
-                throw new ParameterException("Use only one cors configuration type");
-            }
-        } catch (ParameterException e) {
-            log.warn("could not start photon: " + e.getMessage());
-            jCommander.usage();
-            return;
-        }
-
-        // show help
-        if (args.isUsage()) {
-            jCommander.usage();
-            return;
-        }
-
-        if (args.getJsonDump() != null) {
-            startJsonDump(args);
-            return;
-        }
+        CommandLineArgs args = parseCommandLine(rawArgs);
 
         boolean shutdownES = false;
         final Server esServer = new Server(args.getDataDirectory()).start(args.getCluster(), args.getTransportAddresses());
@@ -83,6 +60,32 @@ public class App {
     }
 
 
+    private static CommandLineArgs parseCommandLine(String[] rawArgs) {
+        CommandLineArgs args = new CommandLineArgs();
+        final JCommander jCommander = new JCommander(args);
+        try {
+            jCommander.parse(rawArgs);
+
+            // Cors arguments are mutually exclusive.
+            if (args.isCorsAnyOrigin() && args.getCorsOrigin() != null) {
+                throw new ParameterException("Use only one cors configuration type");
+            }
+        } catch (ParameterException e) {
+            log.warn("could not start photon: " + e.getMessage());
+            jCommander.usage();
+            System.exit(1);
+        }
+
+        // show help
+        if (args.isUsage()) {
+            jCommander.usage();
+            System.exit(1);
+        }
+
+        return args;
+    }
+
+
     /**
      * take nominatim data and dump it to json
      *
@@ -94,7 +97,7 @@ public class App {
             final JsonDumper jsonDumper = new JsonDumper(filename, args.getLanguages(), args.getExtraTags());
             NominatimConnector nominatimConnector = new NominatimConnector(args.getHost(), args.getPort(), args.getDatabase(), args.getUser(), args.getPassword());
             nominatimConnector.setImporter(jsonDumper);
-            nominatimConnector.readEntireDatabase(args.getCountryCodes().split(","));
+            nominatimConnector.readEntireDatabase(args.getCountryCodes());
             log.info("json dump was created: " + filename);
         } catch (FileNotFoundException e) {
             log.error("cannot create dump", e);
@@ -112,7 +115,7 @@ public class App {
     private static void startNominatimImport(CommandLineArgs args, Server esServer, Client esNodeClient) {
         DatabaseProperties dbProperties;
         try {
-            dbProperties = esServer.recreateIndex(args.getLanguagesOrDefault()); // clear out previous data
+            dbProperties = esServer.recreateIndex(args.getLanguages()); // clear out previous data
         } catch (IOException e) {
             throw new RuntimeException("cannot setup index, elastic search config files not readable", e);
         }
@@ -121,7 +124,7 @@ public class App {
         de.komoot.photon.elasticsearch.Importer importer = new de.komoot.photon.elasticsearch.Importer(esNodeClient, dbProperties.getLanguages(), args.getExtraTags());
         NominatimConnector nominatimConnector = new NominatimConnector(args.getHost(), args.getPort(), args.getDatabase(), args.getUser(), args.getPassword());
         nominatimConnector.setImporter(importer);
-        nominatimConnector.readEntireDatabase(args.getCountryCodes().split(","));
+        nominatimConnector.readEntireDatabase(args.getCountryCodes());
 
         log.info("imported data from nominatim to photon with languages: " + String.join(",", dbProperties.getLanguages()));
     }
@@ -153,8 +156,8 @@ public class App {
         // Get database properties and ensure that the version is compatible.
         DatabaseProperties dbProperties = new DatabaseProperties();
         dbProperties.loadFromDatabase(esNodeClient);
-        if (!args.getLanguages().isEmpty()) {
-            dbProperties.restrictLanguages(args.getLanguages().split(","));
+        if (args.getLanguages().length > 0) {
+            dbProperties.restrictLanguages(args.getLanguages());
         }
 
         port(args.getListenPort());
diff --git a/src/main/java/de/komoot/photon/CommandLineArgs.java b/src/main/java/de/komoot/photon/CommandLineArgs.java
index f2d6591..8c0c764 100644
--- a/src/main/java/de/komoot/photon/CommandLineArgs.java
+++ b/src/main/java/de/komoot/photon/CommandLineArgs.java
@@ -5,17 +5,20 @@ package de.komoot.photon;
  */
 
 import com.beust.jcommander.Parameter;
+import de.komoot.photon.utils.StringArrayConverter;
 import lombok.Data;
 
 import java.io.File;
 
+
 @Data
 public class CommandLineArgs {
+
     @Parameter(names = "-cluster", description = "name of elasticsearch cluster to put the server into (default is 'photon')")
     private String cluster = "photon";
 
-    @Parameter(names = "-transport-addresses", description = "the comma separated addresses of external elasticsearch nodes where the client can connect to (default is an empty string which forces an internal node to start)")
-    private String transportAddresses = "";
+    @Parameter(names = "-transport-addresses", description = "the comma separated addresses of external elasticsearch nodes where the client can connect to (default is an empty string which forces an internal node to start)", converter = StringArrayConverter.class)
+    private String[] transportAddresses = new String[]{};
 
     @Parameter(names = "-nominatim-import", description = "import nominatim database into photon (this will delete previous index)")
     private boolean nominatimImport = false;
@@ -23,17 +26,17 @@ public class CommandLineArgs {
     @Parameter(names = "-nominatim-update", description = "fetch updates from nominatim database into photon and exit (this updates the index only without offering an API)")
     private boolean nominatimUpdate = false;
 
-    @Parameter(names = "-languages", description = "languages nominatim importer should import and use at run-time, comma separated (default is 'en,fr,de,it')")
-    private String languages = "";
+    @Parameter(names = "-languages", description = "languages nominatim importer should import and use at run-time, comma separated (default is 'en,fr,de,it')", converter = StringArrayConverter.class)
+    private String[] languages = new String[]{"en", "de", "fr", "it"};
 
     @Parameter(names = "-default-language", description = "language to return results in when no explicit language is choosen by the user")
     private String defaultLanguage = "default";
 
-    @Parameter(names = "-country-codes", description = "country codes filter that nominatim importer should import, comma separated. If empty full planet is done")
-    private String countryCodes = "";
+    @Parameter(names = "-country-codes", description = "country codes filter that nominatim importer should import, comma separated. If empty full planet is done", converter = StringArrayConverter.class)
+    private String[] countryCodes = new String[]{};
 
-    @Parameter(names = "-extra-tags", description = "additional tags to save for each place")
-    private String extraTags = "";
+    @Parameter(names = "-extra-tags", description = "comma-separated list of additional tags to save for each place", converter = StringArrayConverter.class)
+    private String[] extraTags = new String[]{};
 
     @Parameter(names = "-synonym-file", description = "file with synonym and classification terms")
     private String synonymFile = null;
@@ -76,9 +79,5 @@ public class CommandLineArgs {
 
     @Parameter(names = "-h", description = "show help / usage")
     private boolean usage = false;
-
-    public String[] getLanguagesOrDefault() {
-        return getLanguages().isEmpty() ? new String[]{"en", "de", "fr", "it"}: getLanguages().split(",");
-    }
 }
 
diff --git a/src/main/java/de/komoot/photon/JsonDumper.java b/src/main/java/de/komoot/photon/JsonDumper.java
index 05e8070..9ab056e 100644
--- a/src/main/java/de/komoot/photon/JsonDumper.java
+++ b/src/main/java/de/komoot/photon/JsonDumper.java
@@ -13,14 +13,14 @@ import java.io.PrintWriter;
  */
 @Slf4j
 public class JsonDumper implements Importer {
-    private PrintWriter writer = null;
+    private PrintWriter writer;
     private final String[] languages;
     private final String[] extraTags;
 
-    public JsonDumper(String filename, String languages, String extraTags) throws FileNotFoundException {
+    public JsonDumper(String filename, String[] languages, String[] extraTags) throws FileNotFoundException {
         this.writer = new PrintWriter(filename);
-        this.languages = languages.split(",");
-        this.extraTags = extraTags.split(",");
+        this.languages = languages;
+        this.extraTags = extraTags;
     }
 
     @Override
diff --git a/src/main/java/de/komoot/photon/elasticsearch/Importer.java b/src/main/java/de/komoot/photon/elasticsearch/Importer.java
index 2236eb4..cdc04e8 100644
--- a/src/main/java/de/komoot/photon/elasticsearch/Importer.java
+++ b/src/main/java/de/komoot/photon/elasticsearch/Importer.java
@@ -23,11 +23,11 @@ public class Importer implements de.komoot.photon.Importer {
     private final String[] languages;
     private final String[] extraTags;
 
-    public Importer(Client esClient, String[] languages, String extraTags) {
+    public Importer(Client esClient, String[] languages, String[] extraTags) {
         this.esClient = esClient;
         this.bulkRequest = esClient.prepareBulk();
         this.languages = languages;
-        this.extraTags = extraTags.split(",");
+        this.extraTags = extraTags;
     }
 
     @Override
diff --git a/src/main/java/de/komoot/photon/elasticsearch/Server.java b/src/main/java/de/komoot/photon/elasticsearch/Server.java
index d281f4a..72e5987 100644
--- a/src/main/java/de/komoot/photon/elasticsearch/Server.java
+++ b/src/main/java/de/komoot/photon/elasticsearch/Server.java
@@ -59,16 +59,15 @@ public class Server {
         }
     }
 
-    public Server start(String clusterName, String transportAddresses) {
+    public Server start(String clusterName, String[] transportAddresses) {
         Settings.Builder sBuilder = Settings.builder();
         sBuilder.put("path.home", this.esDirectory.toString());
         sBuilder.put("network.host", "127.0.0.1"); // http://stackoverflow.com/a/15509589/1245622
         sBuilder.put("cluster.name", clusterName);
 
-        if (transportAddresses != null && !transportAddresses.isEmpty()) {
+        if (transportAddresses.length > 0) {
             TransportClient trClient = new PreBuiltTransportClient(sBuilder.build());
-            List<String> addresses = Arrays.asList(transportAddresses.split(","));
-            for (String tAddr : addresses) {
+            for (String tAddr : transportAddresses) {
                 int index = tAddr.indexOf(":");
                 if (index >= 0) {
                     int port = Integer.parseInt(tAddr.substring(index + 1));
@@ -81,7 +80,7 @@ public class Server {
 
             esClient = trClient;
 
-            log.info("started elastic search client connected to " + addresses);
+            log.info("Started elastic search client connected to " + transportAddresses);
 
         } else {
 
diff --git a/src/main/java/de/komoot/photon/elasticsearch/Updater.java b/src/main/java/de/komoot/photon/elasticsearch/Updater.java
index 4a4f155..01dd63e 100644
--- a/src/main/java/de/komoot/photon/elasticsearch/Updater.java
+++ b/src/main/java/de/komoot/photon/elasticsearch/Updater.java
@@ -21,11 +21,11 @@ public class Updater implements de.komoot.photon.Updater {
     private final String[] languages;
     private final String[] extraTags;
 
-    public Updater(Client esClient, String[] languages, String extraTags) {
+    public Updater(Client esClient, String[] languages, String[] extraTags) {
         this.esClient = esClient;
         this.bulkRequest = esClient.prepareBulk();
         this.languages = languages;
-        this.extraTags = extraTags.split(",");
+        this.extraTags = extraTags;
     }
 
     public void finish() {
diff --git a/src/main/java/de/komoot/photon/utils/StringArrayConverter.java b/src/main/java/de/komoot/photon/utils/StringArrayConverter.java
new file mode 100644
index 0000000..377d739
--- /dev/null
+++ b/src/main/java/de/komoot/photon/utils/StringArrayConverter.java
@@ -0,0 +1,22 @@
+package de.komoot.photon.utils;
+
+import com.beust.jcommander.IStringConverter;
+
+import java.util.Arrays;
+
+/**
+ * Converter function for JCommand that creates a String array from a comma separated list.
+ */
+public class StringArrayConverter implements IStringConverter<String[]> {
+    @Override
+    public String[] convert(String value) {
+        if (value == null) {
+            return new String[]{};
+        }
+
+        return Arrays.stream(value.split(","))
+                .map(s -> s.trim())
+                .filter(s -> !s.isEmpty())
+                .toArray(String[]::new);
+    }
+}


Test patch:
diff --git a/src/test/java/de/komoot/photon/ESBaseTester.java b/src/test/java/de/komoot/photon/ESBaseTester.java
index 4e2867c..87baf7b 100644
--- a/src/test/java/de/komoot/photon/ESBaseTester.java
+++ b/src/test/java/de/komoot/photon/ESBaseTester.java
@@ -56,29 +56,29 @@ public class ESBaseTester {
      * @throws IOException
      */
     public void setUpES(String... languages) throws IOException {
-        server = new Server(new File("./target/es_photon_test").getAbsolutePath()).setMaxShards(1).start(TEST_CLUSTER_NAME, "");
+        server = new Server(new File("./target/es_photon_test").getAbsolutePath()).setMaxShards(1).start(TEST_CLUSTER_NAME, new String[]{});
         deleteIndex(); // just in case of an abnormal abort previously
         server.recreateIndex(languages);
         refresh();
     }
 
     protected Importer makeImporter() {
-        return new Importer(getClient(), new String[]{"en"}, "");
+        return new Importer(getClient(), new String[]{"en"}, new String[]{});
     }
 
-    protected Importer makeImporterWithExtra(String extraTags) {
+    protected Importer makeImporterWithExtra(String... extraTags) {
         return new Importer(getClient(), new String[]{"en"}, extraTags);
     }
 
     protected Importer makeImporterWithLanguages(String... languages) {
-        return new Importer(getClient(), languages, "");
+        return new Importer(getClient(), languages, new String[]{});
     }
 
     protected Updater makeUpdater() {
-        return new Updater(getClient(), new String[]{"en"}, "");
+        return new Updater(getClient(), new String[]{"en"}, new String[]{});
     }
 
-    protected Updater makeUpdaterWithExtra(String extraTags) {
+    protected Updater makeUpdaterWithExtra(String... extraTags) {
         return new Updater(getClient(), new String[]{"en"}, extraTags);
     }
 
diff --git a/src/test/java/de/komoot/photon/elasticsearch/ImporterTest.java b/src/test/java/de/komoot/photon/elasticsearch/ImporterTest.java
index 162d56d..11712a6 100644
--- a/src/test/java/de/komoot/photon/elasticsearch/ImporterTest.java
+++ b/src/test/java/de/komoot/photon/elasticsearch/ImporterTest.java
@@ -45,7 +45,7 @@ public class ImporterTest extends ESBaseTester {
 
     @Test
     public void testSelectedExtraTagsCanBeIncluded() {
-        Importer instance = makeImporterWithExtra("maxspeed,website");
+        Importer instance = makeImporterWithExtra("maxspeed", "website");
 
         Map<String, String> extratags = new HashMap<>();
         extratags.put("website", "foo");
diff --git a/src/test/java/de/komoot/photon/utils/ConvertToJsonTest.java b/src/test/java/de/komoot/photon/utils/ConvertToJsonTest.java
index 32298ec..357d192 100644
--- a/src/test/java/de/komoot/photon/utils/ConvertToJsonTest.java
+++ b/src/test/java/de/komoot/photon/utils/ConvertToJsonTest.java
@@ -21,7 +21,7 @@ public class ConvertToJsonTest extends ESBaseTester {
 
     private SearchResponse databaseFromDoc(PhotonDoc doc) throws IOException {
         setUpES();
-        Importer instance = makeImporterWithExtra("maxspeed,website");
+        Importer instance = makeImporterWithExtra("maxspeed","website");
         instance.add(doc);
         instance.finish();
         refresh();
diff --git a/src/test/java/de/komoot/photon/utils/StringArrayConverterTest.java b/src/test/java/de/komoot/photon/utils/StringArrayConverterTest.java
new file mode 100644
index 0000000..813dacb
--- /dev/null
+++ b/src/test/java/de/komoot/photon/utils/StringArrayConverterTest.java
@@ -0,0 +1,33 @@
+package de.komoot.photon.utils;
+
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import java.util.stream.Stream;
+
+import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.params.provider.Arguments.arguments;
+
+class StringArrayConverterTest {
+
+    @ParameterizedTest
+    @MethodSource("convertArgumentProvider")
+    void testConvert(String arg, String[] expected) {
+        assertArrayEquals(expected, new StringArrayConverter().convert(arg));
+    }
+
+    static Stream<Arguments> convertArgumentProvider() {
+        return Stream.of(
+                arguments("foo", new String[]{"foo"}),
+                arguments("with space", new String[]{"with space"}),
+                arguments(" 123", new String[]{"123"}),
+                arguments("", new String[]{}),
+                arguments(",", new String[]{}),
+                arguments("1,2,3", new String[]{"1", "2", "3"}),
+                arguments("1, 2, 3", new String[]{"1", "2", "3"}),
+                arguments("a,,b", new String[]{"a", "b"}),
+                arguments("a, ,b", new String[]{"a", "b"})
+        );
+    }
+}
\ No newline at end of file