Re: Handle BOM Entity in gadget spec (issue217102)

[Ziv Horesh]

Attached is the patch with new test. The codereview service keep on giving
me 500 error.
I will put it there when it will be more stable.

-Ziv

On Wed, Feb 24, 2010 at 9:53 AM, Ziv Horesh <zhor...@gmail.com> wrote:

> 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, <johnfa...@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
>>
>
>

### 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 = "&#xFEFF;";
+    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("&#xFEFF;" + 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 {