NullPointerException in ParserUtils.parseConfig if config is missing …

…<final> element #318 (#319) * NullPointerException in ParserUtils.parseConfig if config is missing <final> element #318 * Use -> Using * Added test resource folder
tony-framework · Jun 6, 2019 · 6fef164 · 6fef164
1 parent da95ebc
commit 6fef164
Show file tree

Hide file tree

Showing 5 changed files with 74 additions and 8 deletions.
diff --git a/.gitignore b/.gitignore
@@ -14,7 +14,6 @@
 *.vcxproj.user
 *.zip
 */classes
-*tony-final.xml
 .DS_Store
 .classpath
 .gradle/

diff --git a/tony-core/src/main/java/com/linkedin/tony/util/ParserUtils.java b/tony-core/src/main/java/com/linkedin/tony/util/ParserUtils.java
@@ -209,10 +209,19 @@ public static List<JobConfig> parseConfig(FileSystem fs, Path jobFolderPath) {
         if (property.getNodeType() == Node.ELEMENT_NODE) {
           Element p = (Element) property;
           JobConfig jobConf = new JobConfig();
-          jobConf.setName(p.getElementsByTagName("name").item(0).getTextContent());
-          jobConf.setValue(p.getElementsByTagName("value").item(0).getTextContent());
-          jobConf.setFinal(p.getElementsByTagName("final").item(0).getTextContent().equals("true"));
-          jobConf.setSource(p.getElementsByTagName("source").item(0).getTextContent());
+          String name = getNodeElementText(p, "name");
+          String value = getNodeElementText(p, "value");
+          if (name == null || value == null) {
+            LOG.warn("Found config with null name or value. Name = " + name + ", value = " + value);
+            continue;
+          }
+          jobConf.setName(name);
+          jobConf.setValue(value);
+          String finalText = getNodeElementText(p, "final");
+          if (finalText != null && finalText.equalsIgnoreCase("true")) {
+            jobConf.setFinal(true);
+          }
+          jobConf.setSource(getNodeElementText(p, "source"));
           configs.add(jobConf);
         }
       }
@@ -230,6 +239,16 @@ public static List<JobConfig> parseConfig(FileSystem fs, Path jobFolderPath) {
     return configs;
   }
 
+  private static String getNodeElementText(Element node, String tagName) {
+    if (node != null) {
+      NodeList nodeList = node.getElementsByTagName(tagName);
+      if (nodeList.getLength() > 0) {
+        return nodeList.item(0).getTextContent();
+      }
+    }
+    return null;
+  }
+
   /**
    * Parses the newest (by start time) jhist file in {@code jobFolderPath}, and returns a list of {@link Event}s
    * @param fs FileSystem object.

diff --git a/tony-core/src/test/java/com/linkedin/tony/util/TestParserUtils.java b/tony-core/src/test/java/com/linkedin/tony/util/TestParserUtils.java
@@ -20,10 +20,18 @@
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
-import static org.mockito.Mockito.*;
-import static org.testng.Assert.*;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.fail;
+
 
 public class TestParserUtils {
+  private static final ClassLoader CLASSLOADER = TestParserUtils.class.getClassLoader();
+
   private static FileSystem fs = null;
   private YarnConfiguration yarnConf = new YarnConfiguration();
 
@@ -112,6 +120,13 @@ public void testParseConfigSuccess() {
     assertEquals(actual.get(0).getSource(), expected.get(0).getSource());
   }
 
+  @Test
+  public void testConfigMissingElements() {
+    Path jobFolder = new Path(CLASSLOADER.getResource("application_123_456").getFile());
+    List<JobConfig> actualConfigs = ParserUtils.parseConfig(fs, jobFolder);
+    assertEquals(actualConfigs.size(), 3);
+  }
+
   @Test
   public void testParseConfigFailIOException() throws IOException {
     Path jobFolder = new Path(Constants.TONY_CORE_SRC + "test/resources/typicalHistFolder/job1");

diff --git a/tony-core/src/test/resources/application_123_456/tony-final.xml b/tony-core/src/test/resources/application_123_456/tony-final.xml
@@ -0,0 +1,33 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<configuration>
+  <!-- some configs are missing subelements -->
+  <property>
+    <!-- missing everything -->
+  </property>
+  <property>
+    <name>fookey</name>
+    <value>fooval</value>
+    <!-- missing final -->
+    <source>source</source>
+  </property>
+  <property>
+    <name>barkey</name>
+    <value>barval</value>
+    <!-- missing source and final -->
+  </property>
+  <property>
+    <name>key_only</name>
+    <!-- missing value, source, and final -->
+  </property>
+  <property>
+    <value>value_only</value>
+    <!-- missing name, source, and final -->
+  </property>
+  <property>
+    <!-- has all fields -->
+    <name>bazkey</name>
+    <value>bazval</value>
+    <final>true</final>
+    <source>bazsource</source>
+  </property>
+</configuration>
diff --git a/tony-portal/app/utils/ConfigUtils.java b/tony-portal/app/utils/ConfigUtils.java
@@ -21,7 +21,7 @@ public static String fetchConfigIfExists(Config conf, String key, String default
     try {
       value = conf.getString(key);
     } catch (ConfigException.Missing e) {
-      LOG.error("Failed to fetch value for `" + key + "`. Use `" + defaultVal + "` instead.", e);
+      LOG.warn("Failed to fetch value for `" + key + "`. Using `" + defaultVal + "` instead.", e);
     }
     return value;
   }