AAPT2: Better error messages for ManifestFixer

AAPT2 will now print the XML hierarchy where it found an unexpected
element.

Test: make aapt2_tests
Change-Id: Iac7918b2f344fab874f0a3e7aa9c6936ecde8913

diff --git a/tools/aapt2/xml/XmlActionExecutor.cpp b/tools/aapt2/xml/XmlActionExecutor.cpp
index cc664a5..602a902 100644 (file)
--- a/tools/aapt2/xml/XmlActionExecutor.cpp
+++ b/tools/aapt2/xml/XmlActionExecutor.cpp
@@ -16,6 +16,8 @@
 
 #include "xml/XmlActionExecutor.h"
 
+using ::android::StringPiece;
+
 namespace aapt {
 namespace xml {
 
@@ -46,8 +48,8 @@ static void PrintElementToDiagMessage(const Element* el, DiagMessage* msg) {
   *msg << el->name << ">";
 }
 
-bool XmlNodeAction::Execute(XmlActionExecutorPolicy policy, SourcePathDiagnostics* diag,
-                            Element* el) const {
+bool XmlNodeAction::Execute(XmlActionExecutorPolicy policy, std::vector<StringPiece>* bread_crumb,
+                            SourcePathDiagnostics* diag, Element* el) const {
   bool error = false;
   for (const ActionFuncWithDiag& action : actions_) {
     error |= !action(el, diag);
@@ -57,15 +59,21 @@ bool XmlNodeAction::Execute(XmlActionExecutorPolicy policy, SourcePathDiagnostic
     if (child_el->namespace_uri.empty()) {
       std::map<std::string, XmlNodeAction>::const_iterator iter = map_.find(child_el->name);
       if (iter != map_.end()) {
-        error |= !iter->second.Execute(policy, diag, child_el);
+        // Use the iterator's copy of the element name, because the element may be modified.
+        bread_crumb->push_back(iter->first);
+        error |= !iter->second.Execute(policy, bread_crumb, diag, child_el);
+        bread_crumb->pop_back();
         continue;
       }
 
       if (policy == XmlActionExecutorPolicy::kWhitelist) {
         DiagMessage error_msg(child_el->line_number);
-        error_msg << "unknown element ";
+        error_msg << "unexpected element ";
         PrintElementToDiagMessage(child_el, &error_msg);
-        error_msg << " found";
+        error_msg << " found in ";
+        for (const StringPiece& element : *bread_crumb) {
+          error_msg << "<" << element << ">";
+        }
         diag->Error(error_msg);
         error = true;
       }
@@ -90,14 +98,15 @@ bool XmlActionExecutor::Execute(XmlActionExecutorPolicy policy, IDiagnostics* di
   if (el->namespace_uri.empty()) {
     std::map<std::string, XmlNodeAction>::const_iterator iter = map_.find(el->name);
     if (iter != map_.end()) {
-      return iter->second.Execute(policy, &source_diag, el);
+      std::vector<StringPiece> bread_crumb;
+      bread_crumb.push_back(iter->first);
+      return iter->second.Execute(policy, &bread_crumb, &source_diag, el);
     }
 
     if (policy == XmlActionExecutorPolicy::kWhitelist) {
       DiagMessage error_msg(el->line_number);
-      error_msg << "unknown element ";
+      error_msg << "unexpected root element ";
       PrintElementToDiagMessage(el, &error_msg);
-      error_msg << " found";
       source_diag.Error(error_msg);
       return false;
     }

diff --git a/tools/aapt2/xml/XmlActionExecutor.h b/tools/aapt2/xml/XmlActionExecutor.h
index 1d70045..df70100 100644 (file)
--- a/tools/aapt2/xml/XmlActionExecutor.h
+++ b/tools/aapt2/xml/XmlActionExecutor.h
@@ -40,56 +40,46 @@ enum class XmlActionExecutorPolicy {
   kWhitelist,
 };
 
-/**
- * Contains the actions to perform at this XML node. This is a recursive data
- * structure that
- * holds XmlNodeActions for child XML nodes.
- */
+// Contains the actions to perform at this XML node. This is a recursive data structure that
+// holds XmlNodeActions for child XML nodes.
 class XmlNodeAction {
  public:
   using ActionFuncWithDiag = std::function<bool(Element*, SourcePathDiagnostics*)>;
   using ActionFunc = std::function<bool(Element*)>;
 
-  /**
-   * Find or create a child XmlNodeAction that will be performed for the child
-   * element with the name `name`.
-   */
-  XmlNodeAction& operator[](const std::string& name) { return map_[name]; }
+  // Find or create a child XmlNodeAction that will be performed for the child element with the
+  // name `name`.
+  XmlNodeAction& operator[](const std::string& name) {
+    return map_[name];
+  }
 
-  /**
-   * Add an action to be performed at this XmlNodeAction.
-   */
+  // Add an action to be performed at this XmlNodeAction.
   void Action(ActionFunc f);
   void Action(ActionFuncWithDiag);
 
  private:
   friend class XmlActionExecutor;
 
-  bool Execute(XmlActionExecutorPolicy policy, SourcePathDiagnostics* diag, Element* el) const;
+  bool Execute(XmlActionExecutorPolicy policy, std::vector<::android::StringPiece>* bread_crumb,
+               SourcePathDiagnostics* diag, Element* el) const;
 
   std::map<std::string, XmlNodeAction> map_;
   std::vector<ActionFuncWithDiag> actions_;
 };
 
-/**
- * Allows the definition of actions to execute at specific XML elements defined
- * by their
- * hierarchy.
- */
+// Allows the definition of actions to execute at specific XML elements defined by their hierarchy.
 class XmlActionExecutor {
  public:
   XmlActionExecutor() = default;
 
-  /**
-   * Find or create a root XmlNodeAction that will be performed for the root XML
-   * element with the name `name`.
-   */
-  XmlNodeAction& operator[](const std::string& name) { return map_[name]; }
+  // Find or create a root XmlNodeAction that will be performed for the root XML element with the
+  // name `name`.
+  XmlNodeAction& operator[](const std::string& name) {
+    return map_[name];
+  }
 
-  /**
-   * Execute the defined actions for this XmlResource.
-   * Returns true if all actions return true, otherwise returns false.
-   */
+  // Execute the defined actions for this XmlResource.
+  // Returns true if all actions return true, otherwise returns false.
   bool Execute(XmlActionExecutorPolicy policy, IDiagnostics* diag, XmlResource* doc) const;
 
  private:

diff --git a/tools/aapt2/xml/XmlActionExecutor_test.cpp b/tools/aapt2/xml/XmlActionExecutor_test.cpp
index 0fe7ab0..d39854e 100644 (file)
--- a/tools/aapt2/xml/XmlActionExecutor_test.cpp
+++ b/tools/aapt2/xml/XmlActionExecutor_test.cpp
@@ -56,9 +56,13 @@ TEST(XmlActionExecutorTest, FailsWhenUndefinedHierarchyExists) {
   XmlActionExecutor executor;
   executor["manifest"]["application"];
 
-  std::unique_ptr<XmlResource> doc =
-      test::BuildXmlDom("<manifest><application /><activity /></manifest>");
+  std::unique_ptr<XmlResource> doc;
   StdErrDiagnostics diag;
+
+  doc = test::BuildXmlDom("<manifest><application /><activity /></manifest>");
+  ASSERT_FALSE(executor.Execute(XmlActionExecutorPolicy::kWhitelist, &diag, doc.get()));
+
+  doc = test::BuildXmlDom("<manifest><application><activity /></application></manifest>");
   ASSERT_FALSE(executor.Execute(XmlActionExecutorPolicy::kWhitelist, &diag, doc.get()));
 }