Fix bug with config file parsing (#2)

priteshkp · web-flow · commit 30dae1f0e13b · 2017-01-25T12:44:46.000-08:00
* Fix bug with config file parsing

The Java config file parser implements a very basic key-value line parse.  This change fixes the logic to better handle whitespace and key/value separation.

* One more unit test validation

* Add missing file
diff --git a/bmc-common/src/main/java/com/oracle/bmc/ConfigFileReader.java b/bmc-common/src/main/java/com/oracle/bmc/ConfigFileReader.java
@@ -1,5 +1,5 @@
 /**
- * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016-2017 Oracle and/or its affiliates. All rights reserved.
  */
 package com.oracle.bmc;
 
@@ -30,6 +30,7 @@ public final class ConfigFileReader {
      * Default location of the config file.
      */
     public static final String DEFAULT_FILE_PATH = "~/.oraclebmc/config";
+    private static final String DEFAULT_PROFILE_NAME = "DEFAULT";
 
     /**
      * Creates a new ConfigFile instance using the configuration at the default location,
@@ -146,7 +147,7 @@ public String get(String key) {
                     && (accumulator.configurationsByProfile.get(profile).containsKey(key))) {
                 return accumulator.configurationsByProfile.get(profile).get(key);
             }
-            return accumulator.configurationsByProfile.get("DEFAULT").get(key);
+            return accumulator.configurationsByProfile.get(DEFAULT_PROFILE_NAME).get(key);
         }
     }
 
@@ -159,7 +160,6 @@ private static final class ConfigAccumulator {
         private void accept(String line) {
             final String trimmedLine = line.trim();
 
-            // no blank lines
             if (trimmedLine.isEmpty()) {
                 return;
             }
@@ -169,30 +169,39 @@ private void accept(String line) {
                 return;
             }
 
-            if (trimmedLine.charAt(0) == '[') {
+            if (trimmedLine.charAt(0) == '['
+                    && trimmedLine.charAt(trimmedLine.length() - 1) == ']') {
                 currentProfile = trimmedLine.substring(1, trimmedLine.length() - 1).trim();
-                if (currentProfile.equals("DEFAULT")) {
+                if (currentProfile.isEmpty()) {
+                    throw new IllegalStateException("Cannot have empty profile name: " + line);
+                }
+                if (currentProfile.equals(DEFAULT_PROFILE_NAME)) {
                     foundDefaultProfile = true;
                 }
-            }
+                if (!configurationsByProfile.containsKey(currentProfile)) {
+                    configurationsByProfile.put(currentProfile, new HashMap<String, String>());
+                }
 
-            if (currentProfile == null) {
-                throw new IllegalStateException(
-                        "Config parse error, attempted to read configuration without specifying a profile: "
-                                + trimmedLine);
+                return;
             }
 
-            final String[] keyValue = trimmedLine.split("=");
-            if (keyValue.length != 2) {
-                return;
+            final int splitIndex = trimmedLine.indexOf('=');
+            if (splitIndex == -1) {
+                throw new IllegalStateException("Found line with no key-value pair: " + line);
             }
 
-            final String key = keyValue[0];
-            final String value = keyValue[1];
+            final String key = trimmedLine.substring(0, splitIndex).trim();
+            final String value = trimmedLine.substring(splitIndex + 1).trim();
+            if (key.isEmpty()) {
+                throw new IllegalStateException("Found line with no key: " + line);
+            }
 
-            if (!configurationsByProfile.containsKey(currentProfile)) {
-                configurationsByProfile.put(currentProfile, new HashMap<String, String>());
+            if (currentProfile == null) {
+                throw new IllegalStateException(
+                        "Config parse error, attempted to read configuration without specifying a profile: "
+                                + line);
             }
+
             configurationsByProfile.get(currentProfile).put(key, value);
         }
     }
diff --git a/bmc-common/src/test/java/com/oracle/bmc/ConfigFileReaderTest.java b/bmc-common/src/test/java/com/oracle/bmc/ConfigFileReaderTest.java
@@ -1,5 +1,5 @@
 /**
- * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016-2017 Oracle and/or its affiliates. All rights reserved.
  */
 package com.oracle.bmc;
 
@@ -24,29 +24,52 @@ public void noDefaultProfile() throws IOException {
         ConfigFileReader.parse("src/test/resources/unit_test_no_default_config");
     }
 
+    @Test(expected = IllegalStateException.class)
+    public void noLeadingProfile() throws IOException {
+        ConfigFileReader.parse("src/test/resources/unit_test_no_leading_profile_config");
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void lineWithNoValue() throws IOException {
+        ConfigFileReader.parse("src/test/resources/unit_test_no_value_config");
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void lineWithEmptyKey() throws IOException {
+        ConfigFileReader.parse("src/test/resources/unit_test_empty_key_config");
+    }
+
     @Test(expected = IllegalArgumentException.class)
     public void noProfile() throws IOException {
         ConfigFileReader.parse("src/test/resources/unit_test_config", "foobar");
     }
 
+    @Test(expected = IllegalStateException.class)
+    public void emptyProfileName() throws IOException {
+        ConfigFileReader.parse("src/test/resources/unit_test_empty_profile_name_config");
+    }
+
     @Test
     public void defaultProfile() throws IOException {
         ConfigFile configFile = ConfigFileReader.parse("src/test/resources/unit_test_config");
-        assertEquals("default_tenancy", configFile.get("tenancy"));
-        assertEquals("default_user", configFile.get("user"));
-        assertEquals("default_fingerprint", configFile.get("fingerprint"));
-        assertEquals("default_key_file", configFile.get("key_file"));
-        assertNull(configFile.get("pass_phrase"));
+        assertEquals("value", configFile.get("key"));
+        assertEquals("value2", configFile.get("key2"));
+        assertEquals("value3", configFile.get("key3"));
+        assertEquals("value4", configFile.get("key4"));
+        assertEquals("=val=ue=", configFile.get("key5"));
+        assertEquals("value6", configFile.get("[key6"));
     }
 
     @Test
     public void overrideProfile() throws IOException {
-        ConfigFile configFile =
-                ConfigFileReader.parse("src/test/resources/unit_test_config", "OVERRIDE");
-        assertEquals("default_tenancy", configFile.get("tenancy"));
-        assertEquals("default_user", configFile.get("user"));
-        assertEquals("default_fingerprint", configFile.get("fingerprint"));
-        assertEquals("override_key_file", configFile.get("key_file"));
-        assertEquals("override_pass_phrase", configFile.get("pass_phrase"));
+        ConfigFile profile1 =
+                ConfigFileReader.parse("src/test/resources/unit_test_config", "PROFILE1");
+        assertEquals("new value", profile1.get("key"));
+        assertEquals("value2", profile1.get("key2"));
+
+        ConfigFile profile2 =
+                ConfigFileReader.parse("src/test/resources/unit_test_config", "PROFILE2");
+        assertEquals("value=foobar", profile2.get("key"));
+        assertEquals("nota#comment", profile2.get("key2"));
     }
 }
diff --git a/bmc-common/src/test/java/com/oracle/bmc/waiter/ExponentialBackoffDelayStrategyTest.java b/bmc-common/src/test/java/com/oracle/bmc/waiter/ExponentialBackoffDelayStrategyTest.java
@@ -1,5 +1,5 @@
 /**
- * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016-2017 Oracle and/or its affiliates. All rights reserved.
  */
 package com.oracle.bmc.waiter;
 
@@ -30,12 +30,12 @@ public void getDelays() {
         context.incrementAttempts();
         assertEquals(30000L, strategy.nextDelay(context));
     }
-    
+
     @Test
     public void getDelays_overflow() {
         ExponentialBackoffDelayStrategy strategy = new ExponentialBackoffDelayStrategy(30000L);
         WaitContext context = new WaitContext(System.currentTimeMillis());
-        for (int attempt=0; attempt < 100; attempt++) {
+        for (int attempt = 0; attempt < 100; attempt++) {
             context.incrementAttempts();
             assertTrue(strategy.nextDelay(context) > 0);
             assertTrue(strategy.nextDelay(context) <= 30000L);
diff --git a/bmc-common/src/test/resources/unit_test_config b/bmc-common/src/test/resources/unit_test_config
@@ -1,9 +1,18 @@
 [DEFAULT]
-tenancy=default_tenancy
-user=default_user
-fingerprint=default_fingerprint
-key_file=default_key_file
-
-[OVERRIDE]
-key_file=override_key_file
-pass_phrase=override_pass_phrase
+key=value
+ 
+key2= value2
+   # ignore line
+ key3 =value3
+# ignore line
+ key4 = value4     
+key5 ==val=ue=
+[key6=value6
+ 
+[ PROFILE1 ]
+key=new value
+ 
+[PROFILE2 ]
+key = value=foobar
+key2=nota#comment
+  
diff --git a/bmc-common/src/test/resources/unit_test_empty_key_config b/bmc-common/src/test/resources/unit_test_empty_key_config
@@ -0,0 +1,2 @@
+[DEFAULT]
+ =foobar
diff --git a/bmc-common/src/test/resources/unit_test_empty_profile_name_config b/bmc-common/src/test/resources/unit_test_empty_profile_name_config
@@ -0,0 +1,2 @@
+[   ]
+key=value
diff --git a/bmc-common/src/test/resources/unit_test_no_leading_profile_config b/bmc-common/src/test/resources/unit_test_no_leading_profile_config
@@ -0,0 +1 @@
+foo=bar
diff --git a/bmc-common/src/test/resources/unit_test_no_value_config b/bmc-common/src/test/resources/unit_test_no_value_config
@@ -0,0 +1,2 @@
+[DEFAULT]
+foobar
