review changes

Author: pjfanning

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/XMLUtils.java

@@ -179,16 +179,10 @@ public static SAXTransformerFactory newSecureSAXTransformerFactory()
    */
   private static void setOptionalSecureTransformerAttributes(
           TransformerFactory transformerFactory) {
-    if (CAN_SET_TRANSFORMER_ACCESS_EXTERNAL_DTD.get()) {
-      if (!bestEffortSetAttribute(transformerFactory, XMLConstants.ACCESS_EXTERNAL_DTD, "")) {
-        CAN_SET_TRANSFORMER_ACCESS_EXTERNAL_DTD.set(false);
-      }
-    }
-    if (CAN_SET_TRANSFORMER_ACCESS_EXTERNAL_STYLESHEET.get()) {
-      if (!bestEffortSetAttribute(transformerFactory, XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "")) {
-        CAN_SET_TRANSFORMER_ACCESS_EXTERNAL_STYLESHEET.set(false);
-      }
-    }
+    bestEffortSetAttribute(transformerFactory, CAN_SET_TRANSFORMER_ACCESS_EXTERNAL_DTD,
+            XMLConstants.ACCESS_EXTERNAL_DTD, "");
+    bestEffortSetAttribute(transformerFactory, CAN_SET_TRANSFORMER_ACCESS_EXTERNAL_STYLESHEET,
+            XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
   }
 
   /**
@@ -197,18 +191,19 @@ private static void setOptionalSecureTransformerAttributes(
    * logs the issue at debug level.
    *
    * @param transformerFactory to update
+   * @param flag that indicates whether to do the update and the flag can be set to false if an update fails
    * @param name of the attribute to set
    * @param value to set on the attribute
-   * @return whether the attribute was successfully set
    */
-  static boolean bestEffortSetAttribute(TransformerFactory transformerFactory,
-                                        String name, Object value) {
-    try {
-      transformerFactory.setAttribute(name, value);
-      return true;
-    } catch (Throwable t) {
-      LOG.debug("Issue setting TransformerFactory attribute {}: {}", name, t.toString());
+  static void bestEffortSetAttribute(TransformerFactory transformerFactory, AtomicBoolean flag,
+                                     String name, Object value) {
+    if (flag.get()) {
+      try {
+        transformerFactory.setAttribute(name, value);
+      } catch (Throwable t) {
+        flag.set(false);
+        LOG.debug("Issue setting TransformerFactory attribute {}: {}", name, t.toString());
+      }
     }
-    return false;
   }
 }

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestXMLUtils.java

@@ -20,6 +20,7 @@
 import java.io.InputStream;
 import java.io.StringReader;
 import java.io.StringWriter;
+import java.util.concurrent.atomic.AtomicBoolean;
 import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.SAXParser;
@@ -134,10 +135,16 @@ public void testExternalDtdWithSecureSAXTransformerFactory() throws Exception {
   @Test
   public void testBestEffortSetAttribute() throws Exception {
     TransformerFactory factory = TransformerFactory.newInstance();
-    Assert.assertFalse("unexpected attribute results in return of false",
-            XMLUtils.bestEffortSetAttribute(factory, "unsupportedAttribute false", "abc"));
-    Assert.assertTrue("expected attribute results in return of false",
-            XMLUtils.bestEffortSetAttribute(factory, XMLConstants.ACCESS_EXTERNAL_DTD, ""));
+    AtomicBoolean flag1 = new AtomicBoolean(true);
+    XMLUtils.bestEffortSetAttribute(factory, flag1, "unsupportedAttribute false", "abc");
+    Assert.assertFalse("unexpected attribute results in return of false?", flag1.get());
+    AtomicBoolean flag2 = new AtomicBoolean(true);
+    XMLUtils.bestEffortSetAttribute(factory, flag2, XMLConstants.ACCESS_EXTERNAL_DTD, "");
+    Assert.assertTrue("expected attribute results in return of true?", flag2.get());
+    AtomicBoolean flag3 = new AtomicBoolean(false);
+    XMLUtils.bestEffortSetAttribute(factory, flag3, XMLConstants.ACCESS_EXTERNAL_DTD, "");
+    Assert.assertFalse("expected attribute results in return of false if input flag is false?",
+            flag3.get());
   }
 
   private static InputStream getResourceStream(final String filename) {