Properly escape Bookmarks and FormField XML

by using XmlStringBuilder. Fixes SMACK-577

Also extend LazyStringBuilder with a cache. And extend XmlStringBuilder
with some more convenience methods.

Move the ELEMENT and NAMESPACE definition from Form to DataForm, where
it belongs.

smack-core/src/main/java/org/jivesoftware/smack/util/LazyStringBuilder.java

@@ -23,12 +23,19 @@ public class LazyStringBuilder implements Appendable, CharSequence {
 
     private final List<CharSequence> list;
 
+    private String cache;
+
+    private void invalidateCache() {
+        cache = null;
+    }
+
     public LazyStringBuilder() {
         list = new ArrayList<CharSequence>(20);
     }
 
     public LazyStringBuilder append(LazyStringBuilder lsb) {
         list.addAll(lsb.list);
+        invalidateCache();
         return this;
     }
 
@@ -36,6 +43,7 @@ public class LazyStringBuilder implements Appendable, CharSequence {
     public LazyStringBuilder append(CharSequence csq) {
         assert csq != null;
         list.add(csq);
+        invalidateCache();
         return this;
     }
 
@@ -43,17 +51,22 @@ public class LazyStringBuilder implements Appendable, CharSequence {
     public LazyStringBuilder append(CharSequence csq, int start, int end) {
         CharSequence subsequence = csq.subSequence(start, end);
         list.add(subsequence);
+        invalidateCache();
         return this;
     }
 
     @Override
     public LazyStringBuilder append(char c) {
         list.add(Character.toString(c));
+        invalidateCache();
         return this;
     }
 
     @Override
     public int length() {
+        if (cache != null) {
+            return cache.length();
+        }
         int length = 0;
         for (CharSequence csq : list) {
             length += csq.length();
@@ -63,6 +76,9 @@ public class LazyStringBuilder implements Appendable, CharSequence {
 
     @Override
     public char charAt(int index) {
+        if (cache != null) {
+            return cache.charAt(index);
+        }
         for (CharSequence csq : list) {
             if (index < csq.length()) {
                 return csq.charAt(index);
@@ -80,10 +96,13 @@ public class LazyStringBuilder implements Appendable, CharSequence {
 
     @Override
     public String toString() {
+        if (cache == null) {
             StringBuilder sb = new StringBuilder(length());
             for (CharSequence csq : list) {
                 sb.append(csq);
             }
-        return sb.toString();
+            cache = sb.toString();
+        }
+        return cache;
     }
 }

smack-core/src/main/java/org/jivesoftware/smack/util/XmlStringBuilder.java

@@ -168,6 +168,25 @@ public class XmlStringBuilder implements Appendable, CharSequence {
         return this;
     }
 
+    public XmlStringBuilder emptyElement(String element) {
+        halfOpenElement(element);
+        return closeEmptyElement();
+    }
+
+    public XmlStringBuilder condEmptyElement(boolean condition, String element) {
+        if (condition) {
+            emptyElement(element);
+        }
+        return this;
+    }
+
+    public XmlStringBuilder condAttribute(boolean condition, String name, String value) {
+        if (condition) {
+            attribute(name, value);
+        }
+        return this;
+    }
+
     @Override
     public XmlStringBuilder append(CharSequence csq) {
         assert csq != null;
@@ -207,4 +226,18 @@ public class XmlStringBuilder implements Appendable, CharSequence {
     public String toString() {
         return sb.toString();
     }
+
+    @Override
+    public boolean equals(Object other) {
+        if (!(other instanceof XmlStringBuilder)) {
+            return false;
+        }
+        XmlStringBuilder otherXmlStringBuilder = (XmlStringBuilder) other;
+        return toString().equals(otherXmlStringBuilder.toString());
+    }
+
+    @Override
+    public int hashCode() {
+        return toString().hashCode();
+    }
 }

smack-extensions/src/main/java/org/jivesoftware/smackx/bookmarks/Bookmarks.java

@@ -16,6 +16,7 @@
  */
 package org.jivesoftware.smackx.bookmarks;
 
+import org.jivesoftware.smack.util.XmlStringBuilder;
 import org.jivesoftware.smackx.iqprivate.packet.PrivateData;
 import org.jivesoftware.smackx.iqprivate.provider.PrivateDataProvider;
 import org.xmlpull.v1.XmlPullParser;
@@ -59,6 +60,9 @@ import java.util.List;
  */
 public class Bookmarks implements PrivateData {
 
+    public static final String NAMESPACE = "storage:bookmarks";
+    public static final String ELEMENT = "storage";
+
     private List<BookmarkedURL> bookmarkedURLS;
     private List<BookmarkedConference> bookmarkedConferences;
 
@@ -145,7 +149,7 @@ public class Bookmarks implements PrivateData {
      * @return the element name.
      */
     public String getElementName() {
-        return "storage";
+        return ELEMENT;
     }
 
     /**
@@ -154,28 +158,26 @@ public class Bookmarks implements PrivateData {
      * @return the namespace.
      */
     public String getNamespace() {
-        return "storage:bookmarks";
+        return NAMESPACE;
     }
 
     /**
-     * Returns the XML reppresentation of the PrivateData.
+     * Returns the XML representation of the PrivateData.
      *
      * @return the private data as XML.
      */
-    public String toXML() {
+    @Override
-        StringBuilder buf = new StringBuilder();
+    public XmlStringBuilder toXML() {
-        buf.append("<storage xmlns=\"storage:bookmarks\">");
+        XmlStringBuilder buf = new XmlStringBuilder();
+        buf.openElement(ELEMENT).xmlnsAttribute(NAMESPACE);
 
         for (BookmarkedURL urlStorage : getBookmarkedURLS()) {
             if(urlStorage.isShared()) {
                 continue;
             }
-            buf.append("<url name=\"").append(urlStorage.getName()).
+            buf.openElement("url").attribute("name", urlStorage.getName()).attribute("url", urlStorage.getURL());
-                    append("\" url=\"").append(urlStorage.getURL()).append("\"");
+            buf.condAttribute(urlStorage.isRss(), "rss", "true");
-            if(urlStorage.isRss()) {
+            buf.closeEmptyElement();
-                buf.append(" rss=\"").append(true).append("\"");
-            }
-            buf.append(" />");
         }
 
         // Add Conference additions
@@ -183,26 +185,20 @@ public class Bookmarks implements PrivateData {
             if(conference.isShared()) {
                 continue;
             }
-            buf.append("<conference ");
+            buf.openElement("conference");
-            buf.append("name=\"").append(conference.getName()).append("\" ");
+            buf.attribute("name", conference.getName());
-            buf.append("autojoin=\"").append(conference.isAutoJoin()).append("\" ");
+            buf.attribute("autojoin", Boolean.toString(conference.isAutoJoin()));
-            buf.append("jid=\"").append(conference.getJid()).append("\" ");
+            buf.attribute("jid", conference.getJid());
-            buf.append(">");
+            buf.rightAngelBracket();
 
-            if (conference.getNickname() != null) {
+            buf.optElement("nick", conference.getNickname());
-                buf.append("<nick>").append(conference.getNickname()).append("</nick>");
+            buf.optElement("password", conference.getPassword());
+
+            buf.closeElement("conference");
         }
 
-
+        buf.closeElement(ELEMENT);
-            if (conference.getPassword() != null) {
+        return buf;
-                buf.append("<password>").append(conference.getPassword()).append("</password>");
-            }
-            buf.append("</conference>");
-        }
-
-
-        buf.append("</storage>");
-        return buf.toString();
     }
 
     /**

smack-extensions/src/main/java/org/jivesoftware/smackx/caps/EntityCapsManager.java

@@ -44,7 +44,6 @@ import org.jivesoftware.smackx.disco.packet.DiscoverInfo;
 import org.jivesoftware.smackx.disco.packet.DiscoverInfo.Feature;
 import org.jivesoftware.smackx.disco.packet.DiscoverInfo.Identity;
 import org.jivesoftware.smackx.disco.packet.DiscoverItems.Item;
-import org.jivesoftware.smackx.xdata.Form;
 import org.jivesoftware.smackx.xdata.FormField;
 import org.jivesoftware.smackx.xdata.packet.DataForm;
 
@@ -528,7 +527,7 @@ public class EntityCapsManager extends Manager {
     protected static boolean verifyPacketExtensions(DiscoverInfo info) {
         List<FormField> foundFormTypes = new LinkedList<FormField>();
         for (PacketExtension pe : info.getExtensions()) {
-            if (pe.getNamespace().equals(Form.NAMESPACE)) {
+            if (pe.getNamespace().equals(DataForm.NAMESPACE)) {
                 DataForm df = (DataForm) pe;
                 for (FormField f : df.getFields()) {
                     if (f.getVariable().equals("FORM_TYPE")) {
@@ -561,7 +560,7 @@ public class EntityCapsManager extends Manager {
         if (md == null)
             return null;
 
-        DataForm extendedInfo = (DataForm) discoverInfo.getExtension(Form.ELEMENT, Form.NAMESPACE);
+        DataForm extendedInfo = (DataForm) discoverInfo.getExtension(DataForm.ELEMENT, DataForm.NAMESPACE);
 
         // 1. Initialize an empty string S ('sb' in this method).
         StringBuilder sb = new StringBuilder(); // Use StringBuilder as we don't

smack-extensions/src/main/java/org/jivesoftware/smackx/iqprivate/packet/PrivateData.java

@@ -45,5 +45,5 @@ public interface PrivateData {
      *
      * @return the private data as XML.
      */
-    public String toXML();
+    public CharSequence toXML();
 }

smack-extensions/src/main/java/org/jivesoftware/smackx/xdata/Form.java

@@ -49,9 +49,6 @@ public class Form {
     public static final String TYPE_CANCEL = "cancel";
     public static final String TYPE_RESULT = "result";
 
-    public static final String NAMESPACE = "jabber:x:data";
-    public static final String ELEMENT = "x";
-
     private DataForm dataForm;
 
     /**

smack-extensions/src/main/java/org/jivesoftware/smackx/xdata/FormField.java

@@ -17,7 +17,6 @@
 
 package org.jivesoftware.smackx.xdata;
 
-import org.jivesoftware.smack.util.StringUtils;
 import org.jivesoftware.smack.util.XmlStringBuilder;
 
 import java.util.ArrayList;
@@ -33,6 +32,8 @@ import java.util.List;
  */
 public class FormField {
 
+    public static final String ELEMENT = "field";
+
     public static final String TYPE_BOOLEAN = "boolean";
     public static final String TYPE_FIXED = "fixed";
     public static final String TYPE_HIDDEN = "hidden";
@@ -263,25 +264,17 @@ public class FormField {
         }
     }
 
-    public String toXML() {
+    public XmlStringBuilder toXML() {
         XmlStringBuilder buf = new XmlStringBuilder();
-        buf.append("<field");
+        buf.halfOpenElement(ELEMENT);
         // Add attributes
-        if (getLabel() != null) {
+        buf.optAttribute("label", getLabel());
-            buf.append(" label=\"").append(getLabel()).append("\"");
+        buf.optAttribute("var", getVariable());
-        }
+        buf.optAttribute("type", getType());
-        buf.attribute("var", getVariable());
+        buf.rightAngelBracket();
-        if (getType() != null) {
-            buf.append(" type=\"").append(getType()).append("\"");
-        }
-        buf.append(">");
         // Add elements
-        if (getDescription() != null) {
+        buf.optElement("desc", getDescription());
-            buf.append("<desc>").append(getDescription()).append("</desc>");
+        buf.condEmptyElement(isRequired(), "required");
-        }
-        if (isRequired()) {
-            buf.append("<required/>");
-        }
         // Loop through all the values and append them to the string buffer
         for (String value : getValues()) {
             buf.element("value", value);
@@ -290,8 +283,8 @@ public class FormField {
         for (Option option : getOptions()) {
             buf.append(option.toXML());
         }
-        buf.append("</field>");
+        buf.closeElement(ELEMENT);
-        return buf.toString();
+        return buf;
     }
 
     @Override
@@ -320,8 +313,10 @@ public class FormField {
      */
     public static class Option {
 
+        public static final String ELEMNT = "option";
+
+        private final String value;
         private String label;
-        private String value;
 
         public Option(String value) {
             this.value = value;
@@ -355,19 +350,18 @@ public class FormField {
             return getLabel();
         }
 
-        public String toXML() {
+        public XmlStringBuilder toXML() {
-            StringBuilder buf = new StringBuilder();
+            XmlStringBuilder xml = new XmlStringBuilder();
-            buf.append("<option");
+            xml.halfOpenElement(ELEMNT);
             // Add attribute
-            if (getLabel() != null) {
+            xml.optAttribute("label", getLabel());
-                buf.append(" label=\"").append(getLabel()).append("\"");
+            xml.rightAngelBracket();
-            }
-            buf.append(">");
-            // Add element
-            buf.append("<value>").append(StringUtils.escapeForXML(getValue())).append("</value>");
 
-            buf.append("</option>");
+            // Add element
-            return buf.toString();
+            xml.element("value", getValue());
+
+            xml.closeElement(ELEMENT);
+            return xml;
         }
 
         @Override

smack-extensions/src/main/java/org/jivesoftware/smackx/xdata/packet/DataForm.java

@@ -18,7 +18,7 @@
 package org.jivesoftware.smackx.xdata.packet;
 
 import org.jivesoftware.smack.packet.PacketExtension;
-import org.jivesoftware.smackx.xdata.Form;
+import org.jivesoftware.smack.util.XmlStringBuilder;
 import org.jivesoftware.smackx.xdata.FormField;
 
 import java.util.ArrayList;
@@ -33,6 +33,9 @@ import java.util.List;
  */
 public class DataForm implements PacketExtension {
 
+    public static final String NAMESPACE = "jabber:x:data";
+    public static final String ELEMENT = "x";
+
     private String type;
     private String title;
     private List<String> instructions = new ArrayList<String>();
@@ -120,11 +123,11 @@ public class DataForm implements PacketExtension {
     }
 
     public String getElementName() {
-        return Form.ELEMENT;
+        return ELEMENT;
     }
 
     public String getNamespace() {
-        return Form.NAMESPACE;
+        return NAMESPACE;
     }
 
     /**
@@ -207,15 +210,15 @@ public class DataForm implements PacketExtension {
         return found;
     }
 
-    public String toXML() {
+    @Override
-        StringBuilder buf = new StringBuilder();
+    public XmlStringBuilder toXML() {
-        buf.append("<").append(getElementName()).append(" xmlns=\"").append(getNamespace()).append(
+        XmlStringBuilder buf = new XmlStringBuilder(this);
-            "\" type=\"" + getType() +"\">");
+        buf.attribute("type", getType());
-        if (getTitle() != null) {
+        buf.rightAngelBracket();
-            buf.append("<title>").append(getTitle()).append("</title>");
+
-        }
+        buf.optElement("title", getTitle());
         for (String instruction : getInstructions()) {
-            buf.append("<instructions>").append(instruction).append("</instructions>");
+            buf.element("instructions", instruction);
         }
         // Append the list of fields returned from a search
         if (getReportedData() != null) {
@@ -229,8 +232,8 @@ public class DataForm implements PacketExtension {
         for (FormField field : getFields()) {
             buf.append(field.toXML());
         }
-        buf.append("</").append(getElementName()).append(">");
+        buf.closeElement(this);
-        return buf.toString();
+        return buf;
     }
 
     /**
@@ -241,6 +244,8 @@ public class DataForm implements PacketExtension {
      * @author Gaston Dombiak
      */
     public static class ReportedData {
+        public static final String ELEMENT = "reported";
+
         private List<FormField> fields = new ArrayList<FormField>();
         
         public ReportedData(List<FormField> fields) {
@@ -256,15 +261,15 @@ public class DataForm implements PacketExtension {
             return Collections.unmodifiableList(new ArrayList<FormField>(fields));
         }
 
-        public String toXML() {
+        public CharSequence toXML() {
-            StringBuilder buf = new StringBuilder();
+            XmlStringBuilder buf = new XmlStringBuilder();
-            buf.append("<reported>");
+            buf.openElement(ELEMENT);
             // Loop through all the form items and append them to the string buffer
             for (FormField field : getFields()) {
                 buf.append(field.toXML());
             }
-            buf.append("</reported>");
+            buf.closeElement(ELEMENT);
-            return buf.toString();
+            return buf;
         }
     }
     
@@ -275,6 +280,8 @@ public class DataForm implements PacketExtension {
      * @author Gaston Dombiak
      */
     public static class Item {
+        public static final String ELEMENT = "item";
+
         private List<FormField> fields = new ArrayList<FormField>();
         
         public Item(List<FormField> fields) {
@@ -290,15 +297,15 @@ public class DataForm implements PacketExtension {
             return Collections.unmodifiableList(new ArrayList<FormField>(fields));
         }
         
-        public String toXML() {
+        public CharSequence toXML() {
-            StringBuilder buf = new StringBuilder();
+            XmlStringBuilder buf = new XmlStringBuilder();
-            buf.append("<item>");
+            buf.openElement(ELEMENT);
             // Loop through all the form items and append them to the string buffer
             for (FormField field : getFields()) {
                 buf.append(field.toXML());
             }
-            buf.append("</item>");
+            buf.closeElement(ELEMENT);
-            return buf.toString();
+            return buf;
         }
     }
 }

smack-extensions/src/test/java/org/jivesoftware/smackx/filetransfer/FileTransferNegotiatorTest.java

@@ -51,6 +51,6 @@ public class FileTransferNegotiatorTest {
         fileNeg.negotiateOutgoingTransfer("me", "streamid", "file", 1024, null, 10);
         Packet packet = connection.getSentPacket();
         String xml = packet.toXML().toString();
-        assertTrue(xml.indexOf("var='stream-method' type=\"list-single\"") != -1);
+        assertTrue(xml.indexOf("var='stream-method' type='list-single'") != -1);
     }
 }