[AA] Bugzilla Bug 60628: Change after code review by @janmaterne

manual/Tasks/get.html

@@ -183,12 +183,12 @@ plain text' authentication is used. This is only secure over an HTTPS link.
   </tr>
   <tr>
     <td valign="top">name</td>
-    <td valign="top">The name or key of this header.</td>
+    <td valign="top">The name or key of this header. Cannot be null or empty. Leading and trailing spaces are removed</td>
     <td align="center" valign="top">Yes</td>
   </tr>
   <tr>
     <td valign="top">value</td>
-    <td valign="top">The value to assign to the.</td>
+    <td valign="top">The value to assign to the header. Cannot be null or empty. Leading and trailing spaces are removed</td>
     <td align="center" valign="top">Yes</td>
   </tr>
 </table>
@@ -261,10 +261,9 @@ the <a href="input.html">input task</a> to query for a password.</p>
 <p>With custom HTTP headers</p>
 <pre>
 &lt;get src=&quot;http://ant.apache.org/index.html&quot; dest=&quot;downloads&quot;&gt;
-  &lt;header name=&quot;header1&quot; value==&quot;headerValue1&quot; /&gt;
-  &lt;header name=&quot;header2&quot; value==&quot;headerValue2&quot; /&gt;
-  &lt;header name=&quot;header3&quot; value==&quot;headerValue3&quot; /&gt;
-
+  &lt;header name=&quot;header1&quot; value=&quot;headerValue1&quot; /&gt;
+  &lt;header name=&quot;header2&quot; value=&quot;headerValue2&quot; /&gt;
+  &lt;header name=&quot;header3&quot; value=&quot;headerValue3&quot; /&gt;
 &lt;/get&gt;
 </pre>
 

src/etc/testcases/taskdefs/get.xml

@@ -98,21 +98,21 @@
     </fail>
   </target>
 
-  <target name="testTwoHeaders">
+  <target name="testTwoHeadersAreAddedOK">
     <get src="http://www.apache.org/" dest="get.tmp">
       <header name="header1" value="header1Value"/>
       <header name="header2" value="header2Value"/>
     </get>
   </target>
 
-  <target name="testEmptyHeaders">
+  <target name="testEmptyHeadersAreNeverAdded">
     <get src="http://www.apache.org/" dest="get.tmp">
       <header name="" value="headerValue"/>
       <header name="header2" value=""/>
     </get>
   </target>
 
-  <target name="testDuplicateHeaderNames">
+  <target name="testThatWhenMoreThanOneHeaderHaveSameNameOnlyLastOneIsAdded">
     <get src="http://www.apache.org/" dest="get.tmp">
       <header name="header1" value="headerValue1"/>
       <header name="header1" value="headerValue2"/>
@@ -120,7 +120,7 @@
     </get>
   </target>
 
-  <target name="testHeaderSpacesTrimmed">
+  <target name="testHeaderSpaceTrimmed">
     <get src="http://www.apache.org/" dest="get.tmp">
       <header name="  header1     " value="  headerValue1  "/>
     </get>

src/main/org/apache/tools/ant/taskdefs/Get.java

@@ -45,6 +45,8 @@ import org.apache.tools.ant.types.resources.URLProvider;
 import org.apache.tools.ant.types.resources.URLResource;
 import org.apache.tools.ant.util.FileNameMapper;
 import org.apache.tools.ant.util.FileUtils;
+import org.apache.tools.ant.util.StringUtils;
+
 import java.util.LinkedHashMap;
 import java.util.Map;
 
@@ -495,28 +497,14 @@ public class Get extends Task {
      */
     public void addConfiguredHeader(Header header) {
         if (header != null) {
-            String key = trimToNull(header.getName());
-            String value = trimToNull(header.getValue());
+            String key = StringUtils.trimToNull(header.getName());
+            String value = StringUtils.trimToNull(header.getValue());
             if (key != null && value != null) {
                 this.headers.put(key, value);
             }
         }
     }
 
-    private String trimToNull(String inputString) {
-
-        if (inputString == null) {
-            return null;
-        }
-
-        inputString = inputString.trim();
-        if ("".equals(inputString)) {
-            return null;
-        }
-        return inputString;
-    }
-
-
     /**
      * Define the mapper to map source to destination files.
      * @return a mapper to be configured.
@@ -761,14 +749,14 @@ public class Get extends Task {
                 connection.setRequestProperty("Accept-Encoding", GZIP_CONTENT_ENCODING);
             }
 
-            if (!headers.isEmpty()) {
-                for (final Map.Entry<String, String> header : headers.entrySet()) {
-                    //we do not log the header value as it may contain sensitive data like passwords
-                    log(String.format("Adding header '%s' ", header.getKey()));
-                    connection.setRequestProperty(header.getKey(), header.getValue());
-                }
+
+            for (final Map.Entry<String, String> header : headers.entrySet()) {
+                //we do not log the header value as it may contain sensitive data like passwords
+                log(String.format("Adding header '%s' ", header.getKey()));
+                connection.setRequestProperty(header.getKey(), header.getValue());
             }
 
+
             if (connection instanceof HttpURLConnection) {
                 ((HttpURLConnection) connection)
                         .setInstanceFollowRedirects(false);

src/main/org/apache/tools/ant/util/StringUtils.java

@@ -306,4 +306,25 @@ public final class StringUtils {
     private static Collector<CharSequence,?,String> joining(CharSequence separator) {
         return separator == null ? Collectors.joining() : Collectors.joining(separator);
     }
+
+
+    /**
+     * @param inputString String to trim
+     * @return null if the input string is null or empty or contain only empty spaces.
+     * It returns the input string without leading and trailing spaces otherwise.
+     *
+     */
+    public static String trimToNull(String inputString) {
+
+        if (inputString == null) {
+            return null;
+        }
+
+       String tmpString = inputString.trim();
+        if ("".equals(tmpString)) {
+            return null;
+        }
+        return tmpString;
+    }
+
 }

src/tests/junit/org/apache/tools/ant/taskdefs/GetTest.java

@@ -124,8 +124,8 @@ public class GetTest {
     }
 
     @Test
-    public void testTwoHeaders() {
-        buildRule.executeTarget("testTwoHeaders");
+    public void testTwoHeadersAreAddedOK() {
+        buildRule.executeTarget("testTwoHeadersAreAddedOK");
         String log = buildRule.getLog();
         AntAssert.assertContains("Adding header 'header1'", log);
         AntAssert.assertContains("Adding header 'header2'", log);
@@ -133,13 +133,13 @@ public class GetTest {
 
     @Test
     public void testEmptyHeadersAreNeverAdded() {
-        buildRule.executeTarget("testEmptyHeaders");
+        buildRule.executeTarget("testEmptyHeadersAreNeverAdded");
         AntAssert.assertNotContains("Adding header", buildRule.getLog());
     }
 
     @Test
     public void testThatWhenMoreThanOneHeaderHaveSameNameOnlyLastOneIsAdded() {
-        buildRule.executeTarget("testDuplicateHeaderNames");
+        buildRule.executeTarget("testThatWhenMoreThanOneHeaderHaveSameNameOnlyLastOneIsAdded");
         String log = buildRule.getLog();
         AntAssert.assertContains("Adding header 'header1'", log);
 
@@ -150,7 +150,7 @@ public class GetTest {
 
     @Test
     public void testHeaderSpaceTrimmed() {
-        buildRule.executeTarget("testHeaderSpacesTrimmed");
+        buildRule.executeTarget("testHeaderSpaceTrimmed");
         AntAssert.assertContains("Adding header 'header1'", buildRule.getLog());
     }
 

src/tests/junit/org/apache/tools/ant/util/StringUtilsTest.java

@@ -17,16 +17,14 @@
  */
 package org.apache.tools.ant.util;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Vector;
 
 import org.junit.Test;
 
+import static org.junit.Assert.*;
+
 /**
  * Test for StringUtils
  */
@@ -195,5 +193,24 @@ public class StringUtilsTest {
     public void testJoinNullSeparator() {
         assertEquals("abc", StringUtils.join(Arrays.asList("a", "b", "c"), null));
     }
-    
+
+    @Test
+    public void testTrimToNullWithNullInput(){
+        assertNull(StringUtils.trimToNull(null));
+    }
+
+    @Test
+    public void testTrimToNullWithEmptyInput(){
+        assertNull(StringUtils.trimToNull(""));
+    }
+
+    @Test
+    public void testTrimToNullWithBlankSpaceInput(){
+        assertNull(StringUtils.trimToNull("   "));
+    }
+
+    @Test
+    public void testTrimToNullWithInputPaddedWithSpace(){
+        assertEquals("aaBcDeF",StringUtils.trimToNull(" aaBcDeF  "));
+    }
 }