Proposal #1: return simple DOM with error text when relevant (issue199073)

[johnfargo]

Reviewers: shindig.remailer_gmail.com,

Description:
This patch returns a bare-bones DOM that includes error info from a
DOMException generated by parsing invalid text. The purpose is to echo
the error text back to a gadget developer rather than obscure it without
any context.
The implementation isn't perfect in that it would arguably be better for
MutableContent.getDocument() to throw a suitable Exception on parse
failure. However, doing so changes the MutableContent API to throw a
checked Exception, which could force several downstream API changes. An
alternative would be to add a MutableContent.checkDocument() API that
does throw an exception, whose use would be isolated to HtmlRenderer.
This forces DOM parsing before any rewriters are run, however, which has
non-trivial performance impact when String-based rewriters are run.

This implementation's main semantic downside is that the status code
returned for an erroring-render is 200 rather than 404.

Input welcome.

Please review this at http://codereview.appspot.com/199073/show

Affected files:
   
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java


Index:  
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java
===================================================================
---  
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java	 
(revision 905859)
+++  
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java	 
(working copy)
@@ -21,7 +21,6 @@
 import org.apache.shindig.common.cache.CacheProvider;
 import org.apache.shindig.common.util.HashUtil;
 import org.apache.shindig.gadgets.GadgetException;
-import org.apache.shindig.gadgets.GadgetException.Code;
 import org.apache.shindig.gadgets.parse.nekohtml.NekoSimplifiedHtmlParser;

 import com.google.common.collect.BiMap;
@@ -31,8 +30,10 @@
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import org.w3c.dom.Attr;
+import org.w3c.dom.DOMImplementation;
 import org.w3c.dom.Document;
 import org.w3c.dom.DocumentFragment;
+import org.w3c.dom.DOMException;
 import org.w3c.dom.Node;
 import org.w3c.dom.NodeList;

@@ -50,6 +51,7 @@
   private Cache<String, Document> documentCache;
   private Cache<String, DocumentFragment> fragmentCache;
   private Provider<HtmlSerializer> serializerProvider = new  
DefaultSerializerProvider();
+  private DOMImplementation domImpl;

   /**
    * Allowed tag names for OpenSocial Data and template blocks.
@@ -73,6 +75,11 @@
   public void setSerializerProvider(Provider<HtmlSerializer> serProvider) {
     this.serializerProvider = serProvider;
   }
+
+  @Inject
+  public void setDomImplementation(DOMImplementation domImpl) {
+    this.domImpl = domImpl;
+  }

   /**
    * @param content
@@ -99,9 +106,9 @@
         document = parseDomImpl(source);
       } catch (GadgetException e) {
         throw e;
-      } catch (Exception e) {
+      } catch (DOMException e) {
         // DOMException is a RuntimeException
-        throw new GadgetException(Code.HTML_PARSE_ERROR, e);
+        return errorDom(e);
       }

       HtmlSerialization.attach(document, serializerProvider.get(), source);
@@ -240,9 +247,10 @@
     DocumentFragment fragment = null;
     try {
       fragment = parseFragmentImpl(source);
-    } catch (Exception e) {
+    } catch (DOMException e) {
       // DOMException is a RuntimeException
-      throw new GadgetException(Code.HTML_PARSE_ERROR, e);
+      appendParseException(result, e);
+      return;
     }

     reprocessScriptForOpenSocial(fragment);
@@ -261,6 +269,27 @@
     }
   }

+  protected Document errorDom(DOMException e) {
+    // Create a bare-bones DOM whose body is just error text.
+    // We do this to echo information to the developer that originally
+    // supplied the data, since doing so is more useful than simply
+    // returning a black-box HTML error code stemming from an NPE or other  
condition downstream.
+    // The method is protected to allow overriding of this behavior.
+    Document doc = domImpl.createDocument(null, null, null);
+    Node html = doc.createElement("html");
+    html.appendChild(doc.createElement("head"));
+    Node body = doc.createElement("body");
+    appendParseException(body, e);
+    html.appendChild(body);
+    doc.appendChild(html);
+    return doc;
+  }
+
+  private void appendParseException(Node node, DOMException e) {
+    node.appendChild(node.getOwnerDocument().createTextNode(
+        GadgetException.Code.HTML_PARSE_ERROR.toString() + ": " +  
e.toString()));
+  }
+
   protected boolean shouldCache() {
     return documentCache != null && documentCache.getCapacity() != 0;
   }