added factory tests

plugins/ingest/src/main/java/org/elasticsearch/ingest/processor/ConfigurationUtils.java

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.ingest.processor;
 
+import java.util.List;
 import java.util.Map;
 
 public final class ConfigurationUtils {
@@ -26,6 +27,26 @@ public final class ConfigurationUtils {
     private ConfigurationUtils() {
     }
 
+    /**
+     * Returns and removes the specified optional property from the specified configuration map.
+     *
+     * If the property value isn't of type string a {@link IllegalArgumentException} is thrown.
+     */
+    public static String readOptionalStringProperty(Map<String, Object> configuration, String propertyName) {
+        Object value = configuration.remove(propertyName);
+        return readString(propertyName, value);
+    }
+
+    /**
+     * Returns and removes the specified property from the specified configuration map.
+     *
+     * If the property value isn't of type string an {@link IllegalArgumentException} is thrown.
+     * If the property is missing an {@link IllegalArgumentException} is thrown
+     */
+    public static String readStringProperty(Map<String, Object> configuration, String propertyName) {
+        return readStringProperty(configuration, propertyName, null);
+    }
+
     /**
      * Returns and removes the specified property from the specified configuration map.
      *
@@ -39,13 +60,36 @@ public final class ConfigurationUtils {
         } else if (value == null) {
             throw new IllegalArgumentException("required property [" + propertyName + "] is missing");
         }
-
+        return readString(propertyName, value);
-        if (value instanceof String) {
-            return (String) value;
-        } else {
-            throw new IllegalArgumentException("property [" + propertyName + "] isn't a string, but of type [" + value.getClass() + "]");
-        }
     }
 
+    private static String readString(String propertyName, Object value) {
+        if (value == null) {
+            return null;
+        }
+        if (value instanceof String) {
+            return (String) value;
+        }
+        throw new IllegalArgumentException("property [" + propertyName + "] isn't a string, but of type [" + value.getClass().getName() + "]");
+    }
 
+    /**
+     * Returns and removes the specified property of type list from the specified configuration map.
+     *
+     * If the property value isn't of type list an {@link IllegalArgumentException} is thrown.
+     * If the property is missing an {@link IllegalArgumentException} is thrown
+     */
+    public static List<String> readStringList(Map<String, Object> configuration, String propertyName) {
+        Object value = configuration.remove(propertyName);
+        if (value == null) {
+            throw new IllegalArgumentException("required property [" + propertyName + "] is missing");
+        }
+        if (value instanceof List) {
+            @SuppressWarnings("unchecked")
+            List<String> stringList = (List<String>) value;
+            return stringList;
+        } else {
+            throw new IllegalArgumentException("property [" + propertyName + "] isn't a list, but of type [" + value.getClass().getName() + "]");
+        }
+    }
 }

plugins/ingest/src/main/java/org/elasticsearch/ingest/processor/date/DateProcessor.java

@@ -20,9 +20,11 @@
 package org.elasticsearch.ingest.processor.date;
 
 import org.elasticsearch.ingest.Data;
+import org.elasticsearch.ingest.processor.ConfigurationUtils;
 import org.elasticsearch.ingest.processor.Processor;
 import org.joda.time.DateTime;
 import org.joda.time.DateTimeZone;
+import org.joda.time.format.ISODateTimeFormat;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -38,6 +40,7 @@ public final class DateProcessor implements Processor {
     private final Locale locale;
     private final String matchField;
     private final String targetField;
+    private final List<String> matchFormats;
     private final List<DateParser> dateParsers;
 
     DateProcessor(DateTimeZone timezone, Locale locale, String matchField, List<String> matchFormats, String targetField) {
@@ -45,6 +48,7 @@ public final class DateProcessor implements Processor {
         this.locale = locale;
         this.matchField = matchField;
         this.targetField = targetField;
+        this.matchFormats = matchFormats;
         this.dateParsers = new ArrayList<>();
         for (String matchFormat : matchFormats) {
              dateParsers.add(DateParserFactory.createDateParser(matchFormat, timezone, locale));
@@ -62,8 +66,7 @@ public final class DateProcessor implements Processor {
             try {
                 dateTime = dateParser.parseDateTime(value);
             } catch(Exception e) {
-                //TODO is there a better way other than catching exception?
+                //try the next parser and keep track of the last exception
-                //try the next parser
                 lastException = e;
             }
         }
@@ -72,26 +75,41 @@ public final class DateProcessor implements Processor {
             throw new IllegalArgumentException("unable to parse date [" + value + "]", lastException);
         }
 
-        String dateAsISO8601 = dateTime.toString();
+        data.addField(targetField, ISODateTimeFormat.dateTime().print(dateTime));
-        data.addField(targetField, dateAsISO8601);
     }
 
-    public static class Factory implements Processor.Factory {
+    DateTimeZone getTimezone() {
+        return timezone;
+    }
+
+    Locale getLocale() {
+        return locale;
+    }
+
+    String getMatchField() {
+        return matchField;
+    }
+
+    String getTargetField() {
+        return targetField;
+    }
+
+    List<String> getMatchFormats() {
+        return matchFormats;
+    }
+
+    public static class Factory implements Processor.Factory<DateProcessor> {
 
         @SuppressWarnings("unchecked")
-        public Processor create(Map<String, Object> config) {
+        public DateProcessor create(Map<String, Object> config) {
-            String timezoneString = (String) config.get("timezone");
+            String matchField = ConfigurationUtils.readStringProperty(config, "match_field");
-            DateTimeZone timezone = (timezoneString == null) ? DateTimeZone.UTC : DateTimeZone.forID(timezoneString);
+            String targetField = ConfigurationUtils.readStringProperty(config, "target_field", DEFAULT_TARGET_FIELD);
-            String localeString = (String) config.get("locale");
+            String timezoneString = ConfigurationUtils.readOptionalStringProperty(config, "timezone");
+            DateTimeZone timezone = timezoneString == null ? DateTimeZone.UTC : DateTimeZone.forID(timezoneString);
+            String localeString = ConfigurationUtils.readOptionalStringProperty(config, "locale");
             Locale locale = localeString == null ? Locale.ENGLISH : Locale.forLanguageTag(localeString);
-            String matchField = (String) config.get("match_field");
+            List<String> matchFormats = ConfigurationUtils.readStringList(config, "match_formats");
-            List<String> matchFormats = (List<String>) config.get("match_formats");
-            String targetField = (String) config.get("target_field");
-            if (targetField == null) {
-                targetField = DEFAULT_TARGET_FIELD;
-            }
             return new DateProcessor(timezone, locale, matchField, matchFormats, targetField);
         }
     }
-
 }

plugins/ingest/src/main/java/org/elasticsearch/ingest/processor/geoip/GeoIpProcessor.java

@@ -164,7 +164,7 @@ public final class GeoIpProcessor implements Processor {
         private final DatabaseReaderService databaseReaderService = new DatabaseReaderService();
 
         public GeoIpProcessor create(Map<String, Object> config) throws IOException {
-            String ipField = readStringProperty(config, "ip_field", null);
+            String ipField = readStringProperty(config, "ip_field");
             String targetField = readStringProperty(config, "target_field", "geoip");
             String databaseFile = readStringProperty(config, "database_file", "GeoLite2-City.mmdb");
 

plugins/ingest/src/main/java/org/elasticsearch/ingest/processor/grok/GrokProcessor.java

@@ -68,8 +68,8 @@ public final class GrokProcessor implements Processor {
         private Path grokConfigDirectory;
 
         public GrokProcessor create(Map<String, Object> config) throws IOException {
-            String matchField = ConfigurationUtils.readStringProperty(config, "field", null);
+            String matchField = ConfigurationUtils.readStringProperty(config, "field");
-            String matchPattern = ConfigurationUtils.readStringProperty(config, "pattern", null);
+            String matchPattern = ConfigurationUtils.readStringProperty(config, "pattern");
             Map<String, String> patternBank = new HashMap<>();
             Path patternsDirectory = grokConfigDirectory.resolve("patterns");
             try (DirectoryStream<Path> stream = Files.newDirectoryStream(patternsDirectory)) {

plugins/ingest/src/main/java/org/elasticsearch/ingest/processor/simple/SimpleProcessor.java

@@ -55,10 +55,10 @@ public final class SimpleProcessor implements Processor {
     public static class Factory implements Processor.Factory<SimpleProcessor> {
 
         public SimpleProcessor create(Map<String, Object> config) {
-            String path = ConfigurationUtils.readStringProperty(config, "path", null);
+            String path = ConfigurationUtils.readStringProperty(config, "path");
-            String expectedValue = ConfigurationUtils.readStringProperty(config, "expected_value", null);
+            String expectedValue = ConfigurationUtils.readStringProperty(config, "expected_value");
             String addField = ConfigurationUtils.readStringProperty(config, "add_field", null);
-            String addFieldValue = ConfigurationUtils.readStringProperty(config, "add_field_value", null);
+            String addFieldValue = ConfigurationUtils.readStringProperty(config, "add_field_value");
             return new SimpleProcessor(path, expectedValue, addField, addFieldValue);
         }
 

plugins/ingest/src/test/java/org/elasticsearch/ingest/processor/date/DateProcessorFactoryTests.java

@@ -0,0 +1,142 @@
+/*
+ * Licensed to ElasticSearch and Shay Banon under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. ElasticSearch licenses this
+ * file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.ingest.processor.date;
+
+import org.elasticsearch.test.ESTestCase;
+import org.joda.time.DateTimeZone;
+
+import java.util.*;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+
+public class DateProcessorFactoryTests extends ESTestCase {
+
+    public void testBuildDefaults() throws Exception {
+        DateProcessor.Factory factory = new DateProcessor.Factory();
+        Map<String, Object> config = new HashMap<>();
+        String sourceField = randomAsciiOfLengthBetween(1, 10);
+        config.put("match_field", sourceField);
+        config.put("match_formats", Collections.singletonList("dd/MM/yyyyy"));
+
+        DateProcessor processor = factory.create(config);
+        assertThat(processor.getMatchField(), equalTo(sourceField));
+        assertThat(processor.getTargetField(), equalTo(DateProcessor.DEFAULT_TARGET_FIELD));
+        assertThat(processor.getMatchFormats(), equalTo(Collections.singletonList("dd/MM/yyyyy")));
+        assertThat(processor.getLocale(), equalTo(Locale.ENGLISH));
+        assertThat(processor.getTimezone(), equalTo(DateTimeZone.UTC));
+    }
+
+    public void testMatchFieldIsMandatory() throws Exception {
+        DateProcessor.Factory factory = new DateProcessor.Factory();
+        Map<String, Object> config = new HashMap<>();
+        String targetField = randomAsciiOfLengthBetween(1, 10);
+        config.put("target_field", targetField);
+        config.put("match_formats", Collections.singletonList("dd/MM/yyyyy"));
+
+        try {
+            factory.create(config);
+            fail("processor creation should have failed");
+        } catch(IllegalArgumentException e) {
+            assertThat(e.getMessage(), containsString("required property [match_field] is missing"));
+        }
+    }
+
+    public void testMatchFormatsIsMandatory() throws Exception {
+        DateProcessor.Factory factory = new DateProcessor.Factory();
+        Map<String, Object> config = new HashMap<>();
+        String sourceField = randomAsciiOfLengthBetween(1, 10);
+        String targetField = randomAsciiOfLengthBetween(1, 10);
+        config.put("match_field", sourceField);
+        config.put("target_field", targetField);
+
+        try {
+            factory.create(config);
+            fail("processor creation should have failed");
+        } catch(IllegalArgumentException e) {
+            assertThat(e.getMessage(), containsString("required property [match_formats] is missing"));
+        }
+    }
+
+    public void testParseLocale() throws Exception {
+        DateProcessor.Factory factory = new DateProcessor.Factory();
+        Map<String, Object> config = new HashMap<>();
+        String sourceField = randomAsciiOfLengthBetween(1, 10);
+        config.put("match_field", sourceField);
+        config.put("match_formats", Collections.singletonList("dd/MM/yyyyy"));
+        Locale locale = randomLocale(random());
+        config.put("locale", locale.toLanguageTag());
+
+        DateProcessor processor = factory.create(config);
+        assertThat(processor.getLocale(), equalTo(locale));
+    }
+
+    public void testParseTimezone() throws Exception {
+        DateProcessor.Factory factory = new DateProcessor.Factory();
+        Map<String, Object> config = new HashMap<>();
+        String sourceField = randomAsciiOfLengthBetween(1, 10);
+        config.put("match_field", sourceField);
+        config.put("match_formats", Collections.singletonList("dd/MM/yyyyy"));
+        DateTimeZone timeZone = DateTimeZone.forTimeZone(randomTimeZone(random()));
+        config.put("timezone", timeZone.getID());
+
+        DateProcessor processor = factory.create(config);
+        assertThat(processor.getTimezone(), equalTo(timeZone));
+    }
+
+    public void testParseMatchFormats() throws Exception {
+        DateProcessor.Factory factory = new DateProcessor.Factory();
+        Map<String, Object> config = new HashMap<>();
+        String sourceField = randomAsciiOfLengthBetween(1, 10);
+        config.put("match_field", sourceField);
+        config.put("match_formats", Arrays.asList("dd/MM/yyyy", "dd-MM-yyyy"));
+
+        DateProcessor processor = factory.create(config);
+        assertThat(processor.getMatchFormats(), equalTo(Arrays.asList("dd/MM/yyyy", "dd-MM-yyyy")));
+    }
+
+    public void testParseMatchFormatsFailure() throws Exception {
+        DateProcessor.Factory factory = new DateProcessor.Factory();
+        Map<String, Object> config = new HashMap<>();
+        String sourceField = randomAsciiOfLengthBetween(1, 10);
+        config.put("match_field", sourceField);
+        config.put("match_formats", "dd/MM/yyyy");
+
+        try {
+            factory.create(config);
+            fail("processor creation should have failed");
+        } catch(IllegalArgumentException e) {
+            assertThat(e.getMessage(), containsString("property [match_formats] isn't a list, but of type [java.lang.String]"));
+        }
+    }
+
+    public void testParseTargetField() throws Exception {
+        DateProcessor.Factory factory = new DateProcessor.Factory();
+        Map<String, Object> config = new HashMap<>();
+        String sourceField = randomAsciiOfLengthBetween(1, 10);
+        String targetField = randomAsciiOfLengthBetween(1, 10);
+        config.put("match_field", sourceField);
+        config.put("target_field", targetField);
+        config.put("match_formats", Arrays.asList("dd/MM/yyyy", "dd-MM-yyyy"));
+
+        DateProcessor processor = factory.create(config);
+        assertThat(processor.getTargetField(), equalTo(targetField));
+    }
+}

plugins/ingest/src/test/java/org/elasticsearch/ingest/processor/geoip/GeoIpProcessorFactoryTests.java

@@ -94,6 +94,4 @@ public class GeoIpProcessorFactoryTests extends ESTestCase {
             assertThat(e.getMessage(), startsWith("database file [does-not-exist.mmdb] doesn't exist in"));
         }
     }
-
-
 }