Handle BOM Entity in gadget spec

Some services put BOM entity (actual string '' not just the bytes) as first item on XML stream.
For example try: http://www.google.com/ig/modules/china/ijoke.xml

I am not sure how correct is that behavior, but most browsers support it by ignoring it (FireFox, IE, Chrome).

Attached change handle that special case by ignoring bom entity when parsing gadget spec.

[johnfargo, 2010-02-23 20:35:22]

This code looks fine. Have you tried w/ the actual BOM bytes as well? We may as
well be sure to handle both.

Also... has anyone seen this particular behavior before re: BOMs (escaped BOM
chars)? It's not clear to me if this is standard behavior. I don't see any
particular problem supporting it in any case however.

[zhoresh, 2010-02-24 18:07:20]

The actual BOM char is being trimmed by HttpResponse.getResponseAsString
function.
We can also trim the BOM string there.
Since we are not sure if it is standard, I would vote to handle it only in
he cases we really want to, i.e. trim it just before XML parsing in
SpecFactory.

I will attach next a patch with a new testcase


On Tue, Feb 23, 2010 at 12:35 PM, <johnfargo@gmail.com> wrote:

> This code looks fine. Have you tried w/ the actual BOM bytes as well? We
> may as well be sure to handle both.
>
> Also... has anyone seen this particular behavior before re: BOMs
> (escaped BOM chars)? It's not clear to me if this is standard behavior.
> I don't see any particular problem supporting it in any case however.
>
>
> http://codereview.appspot.com/217102/show
>

[zhoresh, 2010-02-24 18:15:57]

### Eclipse Workspace Patch 1.0
#P shindig-project
Index:
java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java
===================================================================
---
java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java	(revision
915473)
+++
java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java	(working
copy)
@@ -18,6 +18,10 @@
  */
 package org.apache.shindig.gadgets;
 
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+
 import org.apache.shindig.common.cache.Cache;
 import org.apache.shindig.common.cache.CacheProvider;
 import org.apache.shindig.common.uri.Uri;
@@ -27,11 +31,6 @@
 import org.apache.shindig.gadgets.servlet.HtmlAccelServlet;
 import org.apache.shindig.gadgets.spec.GadgetSpec;
 import org.apache.shindig.gadgets.spec.SpecParserException;
-
-import com.google.inject.Inject;
-import com.google.inject.Singleton;
-import com.google.inject.name.Named;
-
 import org.w3c.dom.Element;
 
 import java.util.concurrent.ExecutorService;
@@ -89,6 +88,11 @@
 
   @Override
   protected GadgetSpec parse(String content, Query query) throws XmlException,
GadgetException {
+    // Allow BOM entity as first item on stream and ignore it:
+    final String BOM_ENTITY = "";
+    if (content.substring(0, BOM_ENTITY.length()).equalsIgnoreCase(BOM_ENTITY))
{
+      content = content.substring(BOM_ENTITY.length());
+    }
     Element element = XmlUtil.parse(content);
     return new GadgetSpec(query.getSpecUri(), element, content);
   }
Index:
java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactoryTest.java
===================================================================
---
java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactoryTest.java	(revision
915473)
+++
java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactoryTest.java	(working
copy)
@@ -34,7 +34,6 @@
 import org.apache.shindig.gadgets.http.RequestPipeline;
 import org.apache.shindig.gadgets.spec.GadgetSpec;
 import org.apache.shindig.gadgets.spec.SpecParserException;
-
 import org.easymock.EasyMock;
 import org.junit.Test;
 
@@ -134,6 +133,30 @@
     assertEquals(LOCAL_CONTENT,
spec.getView(GadgetSpec.DEFAULT_VIEW).getContent());
   }
 
+  @Test
+  public void specFetchedWithBom() throws Exception {
+    HttpRequest request = createIgnoreCacheRequest();
+    HttpResponse response = new HttpResponse("" + LOCAL_SPEC_XML);
+    expect(pipeline.execute(request)).andReturn(response);
+    replay(pipeline);
+
+    GadgetSpec spec = specFactory.getGadgetSpec(createContext(SPEC_URL, true));
+
+    assertEquals(LOCAL_CONTENT,
spec.getView(GadgetSpec.DEFAULT_VIEW).getContent());
+  }
+
+  @Test
+  public void specFetchedWithBomChar() throws Exception {
+    HttpRequest request = createIgnoreCacheRequest();
+    HttpResponse response = new HttpResponse("\uFEFF" + LOCAL_SPEC_XML);
+    expect(pipeline.execute(request)).andReturn(response);
+    replay(pipeline);
+
+    GadgetSpec spec = specFactory.getGadgetSpec(createContext(SPEC_URL, true));
+
+    assertEquals(LOCAL_CONTENT,
spec.getView(GadgetSpec.DEFAULT_VIEW).getContent());
+  }
+
   // TODO: Move these tests into AbstractSpecFactoryTest
   @Test
   public void specRefetchedAsync() throws Exception {

[zhoresh, 2010-02-24 23:49:37]

Add test for BOM char

[johnfargo, 2010-03-10 23:00:41]

LGTM, committed.