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

[jo...@gmail.com]

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;
    }