4.8 of 5: Xml Fixes for Merger Integration

* SimpleXmlResourceValue records all attributes except name. (I'd like to toss description, just as soon as I know I can -- bloats the serialized form.)
* Added handling for <item> to SimpleXmlResourceValue: this can be used as placeholders, or (apparently) mostly unvalidated value definitions.
* drawable can be used to put in colored backgrounds in a values.xml
* Strings, Plurals, need to read sub tags such as xliff as string
* <eat-comment/> and <skip/> need to be handled: we ignore them as all comments are eaten right now; and skip appears to be for localization diffs
* xml parser must be resilient to random characters in resources defined in values.
* Handles the <resource> not <resources> coding mistake gracefully.
* Supports a <public> declaration. This feature is more wildly used than the nonexistant documentation suggests.

--
MOS_MIGRATED_REVID=121490019

1 files changed, 145 insertions, 24 deletions

diff --git a/src/tools/android/java/com/google/devtools/build/android/XmlResourceValues.java b/src/tools/android/java/com/google/devtools/build/android/XmlResourceValues.java
index 1ed13c1057..978a4d0a69 100644
--- a/src/tools/android/java/com/google/devtools/build/android/XmlResourceValues.java
+++ b/src/tools/android/java/com/google/devtools/build/android/XmlResourceValues.java
@@ -32,13 +32,19 @@ import java.io.OutputStream;
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.logging.Logger;
 
+import javax.annotation.Nullable;
 import javax.xml.namespace.QName;
 import javax.xml.stream.XMLEventReader;
 import javax.xml.stream.XMLStreamException;
 import javax.xml.stream.events.Attribute;
+import javax.xml.stream.events.Characters;
+import javax.xml.stream.events.EndElement;
 import javax.xml.stream.events.StartElement;
 import javax.xml.stream.events.XMLEvent;
 
@@ -50,13 +56,16 @@ import javax.xml.stream.events.XMLEvent;
  * declared inside the <resources> tag.
  */
 public class XmlResourceValues {
+  private static final Logger logger = Logger.getLogger(XmlResourceValues.class.getCanonicalName());
 
+  private static final QName TAG_EAT_COMMENT = QName.valueOf("eat-comment");
   private static final QName TAG_PLURALS = QName.valueOf("plurals");
   private static final QName ATTR_QUANTITY = QName.valueOf("quantity");
   private static final QName TAG_ATTR = QName.valueOf("attr");
   private static final QName TAG_DECLARE_STYLEABLE = QName.valueOf("declare-styleable");
   private static final QName TAG_ITEM = QName.valueOf("item");
   private static final QName TAG_STYLE = QName.valueOf("style");
+  private static final QName TAG_SKIP = QName.valueOf("skip");
   private static final QName TAG_RESOURCES = QName.valueOf("resources");
   private static final QName ATTR_FORMAT = QName.valueOf("format");
   private static final QName ATTR_NAME = QName.valueOf("name");
@@ -66,13 +75,18 @@ public class XmlResourceValues {
 
   static XmlResourceValue parsePlurals(XMLEventReader eventReader) throws XMLStreamException {
     ImmutableMap.Builder<String, String> values = ImmutableMap.builder();
-    for (XMLEvent element = eventReader.nextTag();
+    for (XMLEvent element = nextTag(eventReader);
         !isEndTag(element, TAG_PLURALS);
-        element = eventReader.nextTag()) {
+        element = nextTag(eventReader)) {
       if (isItem(element)) {
+        if (!element.isStartElement()) {
+          throw new XMLStreamException(
+              String.format("Expected start element %s", element), element.getLocation());
+        }
+        String contents = readContentsAsString(eventReader, element.asStartElement().getName());
         values.put(
             getElementAttributeByName(element.asStartElement(), ATTR_QUANTITY),
-            eventReader.getElementText());
+            contents == null ? "" : contents);
       }
     }
     return PluralXmlResourceValue.of(values.build());
@@ -81,14 +95,16 @@ public class XmlResourceValues {
   static XmlResourceValue parseStyle(XMLEventReader eventReader, StartElement start)
       throws XMLStreamException {
     Map<String, String> values = new HashMap<>();
-    for (XMLEvent element = eventReader.nextTag();
+    for (XMLEvent element = nextTag(eventReader);
         !isEndTag(element, TAG_STYLE);
-        element = eventReader.nextTag()) {
+        element = nextTag(eventReader)) {
       if (isItem(element)) {
         values.put(getElementName(element.asStartElement()), eventReader.getElementText());
       }
     }
-    return StyleXmlResourceValue.of(parseReferenceFromElementAttribute(start, ATTR_PARENT), values);
+    String parent = parseReferenceFromElementAttribute(start, ATTR_PARENT, false);
+    // Parents can be very lazily declared as just: <resource name>
+    return StyleXmlResourceValue.of(parent, values);
   }
 
   static void parseDeclareStyleable(
@@ -100,15 +116,22 @@ public class XmlResourceValues {
       StartElement start)
       throws XMLStreamException {
     List<String> members = new ArrayList<>();
-    for (XMLEvent element = eventReader.nextTag();
+    for (XMLEvent element = nextTag(eventReader);
         !isEndTag(element, TAG_DECLARE_STYLEABLE);
-        element = eventReader.nextTag()) {
+        element = nextTag(eventReader)) {
       if (isStartTag(element, TAG_ATTR)) {
         StartElement attr = element.asStartElement();
-        members.add(getElementName(attr));
-        overwritingConsumer.consume(
-            fqnFactory.create(ResourceType.ATTR, getElementName(attr)),
-            DataResourceXml.of(path, parseAttr(eventReader, start)));
+        String attrName = getElementName(attr);
+        members.add(attrName);
+        // If there is format and the next tag is a starting tag, treat it as an attr definition.
+        // Without those, it will be an attr reference.
+        if (XmlResourceValues.getElementAttributeByName(attr, ATTR_FORMAT) != null
+            || (XmlResourceValues.peekNextTag(eventReader) != null
+                && XmlResourceValues.peekNextTag(eventReader).isStartElement())) {
+          overwritingConsumer.consume(
+              fqnFactory.create(ResourceType.ATTR, attrName),
+              DataResourceXml.of(path, parseAttr(eventReader, attr)));
+        }
       }
     }
     nonOverwritingConsumer.consume(
@@ -118,8 +141,10 @@ public class XmlResourceValues {
 
   static XmlResourceValue parseAttr(XMLEventReader eventReader, StartElement start)
       throws XMLStreamException {
-    return AttrXmlResourceValue.from(
-        start, getElementAttributeByName(start, ATTR_FORMAT), eventReader);
+    XmlResourceValue value =
+        AttrXmlResourceValue.from(
+            start, getElementAttributeByName(start, ATTR_FORMAT), eventReader);
+    return value;
   }
 
   static XmlResourceValue parseId() {
@@ -127,14 +152,72 @@ public class XmlResourceValues {
   }
 
   static XmlResourceValue parseSimple(
-      XMLEventReader eventReader, ResourceType resourceType, QName startTag)
+      XMLEventReader eventReader, ResourceType resourceType, StartElement start)
+      throws XMLStreamException {
+    // Using a map to deduplicate xmlns declarations on the attributes.
+    Map<String, String> attributeMap = new LinkedHashMap<>();
+    @SuppressWarnings({
+      "cast",
+      "unchecked"
+    }) // The interface returns Iterator, force casting based on documentation.
+    Iterator<Attribute> attributes = (Iterator<Attribute>) start.getAttributes();
+    while (attributes.hasNext()) {
+      Attribute attribute = attributes.next();
+      QName name = attribute.getName();
+      // Name used as the resource key, so skip it here.
+      if (ATTR_NAME.equals(name)) {
+        continue;
+      }
+      String value = escapeXmlValues(attribute.getValue()).replace("\"", "&quot;");
+      if (!name.getNamespaceURI().isEmpty()) {
+        // Declare the xmlns here, so that the written xml will be semantically correct,
+        // if a bit verbose. This allows the resource keys to be written into a generic <resources>
+        // tag.
+        attributeMap.put("xmlns:" + name.getPrefix(), name.getNamespaceURI());
+        attributeMap.put(name.getPrefix() + ":" + attribute.getName().getLocalPart(), value);
+      } else {
+        attributeMap.put(attribute.getName().getLocalPart(), value);
+      }
+    }
+    String contents;
+    // Check and see if the element is unary. If it is, the contents is null
+    if (isEndTag(eventReader.peek(), start.getName())) {
+      contents = null;
+    } else {
+      contents = readContentsAsString(eventReader, start.getName());
+    }
+    return SimpleXmlResourceValue.of(
+        start.getName().equals(TAG_ITEM)
+            ? SimpleXmlResourceValue.Type.ITEM
+            : SimpleXmlResourceValue.Type.from(resourceType),
+        ImmutableMap.copyOf(attributeMap),
+        contents);
+  }
+
+  // TODO(corysmith): Replace this with real escaping system, preferably a performant high level xml
+  //writing library. See AndroidDataWritingVisitor TODO.
+  private static String escapeXmlValues(String data) {
+    return data.replace("&", "&amp;").replace("<", "&lt;").replace(">", "&gt;");
+  }
+
+  /**
+   * Reads the xml events as a string until finding a closing tag.
+   *
+   * @param eventReader The current xml stream.
+   * @param startTag The name of the tag to close on.
+   * @return A xml escaped string representation of the xml stream
+   */
+  @Nullable
+  public static String readContentsAsString(XMLEventReader eventReader, QName startTag)
       throws XMLStreamException {
     StringBuilder contents = new StringBuilder();
     while (!isEndTag(eventReader.peek(), startTag)) {
       XMLEvent xmlEvent = eventReader.nextEvent();
       if (xmlEvent.isCharacters()) {
-        contents.append(xmlEvent.asCharacters().getData());
+        contents.append(escapeXmlValues(xmlEvent.asCharacters().getData()));
       } else if (xmlEvent.isStartElement()) {
+        // TODO(corysmith): Replace this with a proper representation of the contents that can be
+        // serialized and reconstructed appropriately without modification.
         QName name = xmlEvent.asStartElement().getName();
         contents.append("<");
         if (!name.getNamespaceURI().isEmpty()) {
@@ -163,24 +246,32 @@ public class XmlResourceValues {
         contents.append(">");
       }
     }
-    Preconditions.checkArgument(eventReader.nextEvent().asEndElement().getName().equals(startTag));
-    return SimpleXmlResourceValue.of(
-        SimpleXmlResourceValue.Type.from(resourceType), contents.toString());
+    // Verify the end element.
+    EndElement endElement = eventReader.nextEvent().asEndElement();
+    Preconditions.checkArgument(endElement.getName().equals(startTag));
+    return contents.toString();
   }
 
   /* XML helper methods follow. */
   // TODO(corysmith): Move these to a wrapper class for XMLEventReader.
-  private static String parseReferenceFromElementAttribute(StartElement element, QName name)
-      throws XMLStreamException {
+  private static String parseReferenceFromElementAttribute(
+      StartElement element, QName name, boolean requiresPrefix) throws XMLStreamException {
     String value = getElementAttributeByName(element, name);
+    if (value == null) {
+      return null;
+    }
     if (value.startsWith("?") || value.startsWith("@")) {
       return value.substring(1);
     }
+    if (!requiresPrefix) {
+      return value;
+    }
     throw new XMLStreamException(
         String.format("Invalid resource reference from %s in %s", name, element),
         element.getLocation());
   }
 
+  @Nullable
   public static String getElementAttributeByName(StartElement element, QName name) {
     Attribute attribute = element.getAttributeByName(name);
     return attribute == null ? null : attribute.getValue();
@@ -226,13 +317,34 @@ public class XmlResourceValues {
     return false;
   }
 
+  public static XMLEvent nextTag(XMLEventReader eventReader) throws XMLStreamException {
+    while (eventReader.hasNext()
+        && !(eventReader.peek().isEndElement() || eventReader.peek().isStartElement())) {
+      XMLEvent nextEvent = eventReader.nextEvent();
+      if (nextEvent.isCharacters() && !nextEvent.asCharacters().isIgnorableWhiteSpace()) {
+        Characters characters = nextEvent.asCharacters();
+        // TODO(corysmith): Turn into a warning with the Path is available to add to it.
+        // This case is when unexpected characters are thrown into the xml. Best case, it's a
+        // incorrect comment type...
+        logger.fine(
+            String.format(
+                "Invalid characters [%s] found at %s",
+                characters.getData(),
+                characters.getLocation().getLineNumber()));
+      }
+    }
+    return eventReader.nextEvent();
+  }
+
   public static XMLEvent peekNextTag(XMLEventReader eventReader) throws XMLStreamException {
-    while (!(eventReader.peek().isEndElement() || eventReader.peek().isStartElement())) {
+    while (eventReader.hasNext()
+        && !(eventReader.peek().isEndElement() || eventReader.peek().isStartElement())) {
       eventReader.nextEvent();
     }
     return eventReader.peek();
   }
 
+  @Nullable
   static StartElement findNextStart(XMLEventReader eventReader) throws XMLStreamException {
     while (eventReader.hasNext()) {
       XMLEvent event = eventReader.nextEvent();
@@ -245,14 +357,15 @@ public class XmlResourceValues {
 
   static boolean moveToResources(XMLEventReader eventReader) throws XMLStreamException {
     while (eventReader.hasNext()) {
-      if (findNextStart(eventReader).getName().equals(TAG_RESOURCES)) {
+      StartElement next = findNextStart(eventReader);
+      if (next != null && next.getName().equals(TAG_RESOURCES)) {
         return true;
       }
     }
     return false;
   }
 
-  public static SerializeFormat.DataValue.Builder newProtoDataBuilder(Path source) {
+  public static SerializeFormat.DataValue.Builder newSerializableDataValueBuilder(Path source) {
     SerializeFormat.DataValue.Builder builder = SerializeFormat.DataValue.newBuilder();
     return builder.setSource(builder.getSourceBuilder().setFilename(source.toString()));
   }
@@ -264,4 +377,12 @@ public class XmlResourceValues {
     return CodedOutputStream.computeUInt32SizeNoTag(value.getSerializedSize())
         + value.getSerializedSize();
   }
+
+  public static boolean isEatComment(StartElement start) {
+    return isTag(start, TAG_EAT_COMMENT);
+  }
+
+  public static boolean isSkip(StartElement start) {
+    return isTag(start, TAG_SKIP);
+  }
 }