From 487000b9577b68aea0fdb82f00ead30eca38c7a7 Mon Sep 17 00:00:00 2001
From: Dino Chiesa <dchiesa@google.com>
Date: Mon, 4 Mar 2024 09:04:16 -0800
Subject: [PATCH] fix: relax PO034 check of Remove/* in AssignMessage

---
 lib/package/plugins/PO034-am-hygiene.js       | 60 ++++++++++++++-----
 .../Remove-Headers-Header-Non-Boolean.xml     |  7 +++
 .../PO034/fail/Remove-Payload-NonBoolean.xml  |  5 ++
 .../fixtures/resources/PO034/fail/messages.js |  6 ++
 .../pass/Remove-Headers-Header-Empty.xml      |  7 +++
 .../pass/Remove-Headers-Header-False.xml      |  7 +++
 .../PO034/pass/Remove-Headers-Header-True.xml |  7 +++
 ...e-Payload.xml => Remove-Payload-Empty.xml} |  0
 .../PO034/pass/Remove-Payload-False.xml       |  5 ++
 .../PO034/pass/Remove-Payload-True.xml        |  5 ++
 10 files changed, 94 insertions(+), 15 deletions(-)
 create mode 100644 test/fixtures/resources/PO034/fail/Remove-Headers-Header-Non-Boolean.xml
 create mode 100644 test/fixtures/resources/PO034/fail/Remove-Payload-NonBoolean.xml
 create mode 100644 test/fixtures/resources/PO034/pass/Remove-Headers-Header-Empty.xml
 create mode 100644 test/fixtures/resources/PO034/pass/Remove-Headers-Header-False.xml
 create mode 100644 test/fixtures/resources/PO034/pass/Remove-Headers-Header-True.xml
 rename test/fixtures/resources/PO034/pass/{Remove-Payload.xml => Remove-Payload-Empty.xml} (100%)
 create mode 100644 test/fixtures/resources/PO034/pass/Remove-Payload-False.xml
 create mode 100644 test/fixtures/resources/PO034/pass/Remove-Payload-True.xml

diff --git a/lib/package/plugins/PO034-am-hygiene.js b/lib/package/plugins/PO034-am-hygiene.js
index f5a9876..2bb4925 100644
--- a/lib/package/plugins/PO034-am-hygiene.js
+++ b/lib/package/plugins/PO034-am-hygiene.js
@@ -424,8 +424,43 @@ const onPolicy = function (policy, cb) {
         });
       });
 
-      // children of Remove/Copy, if they exist, should have no text
+      // children of Remove/Copy, if they exist, should have no text,
+      // or the text should be a boolean.
       ["Remove", "Copy"].forEach((tag) => {
+        const checkSingleChildTextBoolean = (child, tag1, tag2) => {
+          //debug(child);
+          if (child.childNodes.length > 1) {
+            foundIssue = true;
+            _addIssue(
+              policy,
+              `there should be at most a single element under <${tag1}>/<${tag2}>.`,
+              child.lineNumber,
+              child.columnNumber
+            );
+          } else {
+            if (child.firstChild.nodeType != TEXT_NODE) {
+              foundIssue = true;
+              _addIssue(
+                policy,
+                `there should be no non-text elements under <${tag1}>/<${tag2}>.`,
+                child.lineNumber,
+                child.columnNumber
+              );
+            } else {
+              const text = child.firstChild.data.trim().toLowerCase();
+              if (text != "false" && text != "true") {
+                foundIssue = true;
+                _addIssue(
+                  policy,
+                  `if there is a text element under <${tag1}>/<${tag2}>, it should be a boolean.`,
+                  child.lineNumber,
+                  child.columnNumber
+                );
+              }
+            }
+          }
+        };
+
         const elements = xpath.select(`AssignMessage/${tag}`, policyRoot);
         elements.forEach((element) => {
           debug(`checking(3) ${element.tagName}...`);
@@ -498,14 +533,13 @@ const onPolicy = function (policy, cb) {
                         );
                       }
 
-                      // check that innerChild has no text value or child elements
                       if (innerChild.hasChildNodes()) {
-                        foundIssue = true;
-                        _addIssue(
-                          policy,
-                          `there should be no text or child elements under element <${child.tagName}>/<${innerChild.tagName}>.`,
-                          innerChild.lineNumber,
-                          innerChild.columnNumber
+                        // check that there is a single innerChild,
+                        // it is text, and it represents a boolean
+                        checkSingleChildTextBoolean(
+                          innerChild,
+                          `${tag}>/<${child.tagName}`,
+                          innerChild.tagName
                         );
                       }
                     }
@@ -514,13 +548,9 @@ const onPolicy = function (policy, cb) {
               } else {
                 // Payload, Verb, etc
                 if (child.hasChildNodes()) {
-                  foundIssue = true;
-                  _addIssue(
-                    policy,
-                    `there should be no text or child elements under element <${tag}>/<${child.tagName}>.`,
-                    child.lineNumber,
-                    child.columnNumber
-                  );
+                  // check that there is a single innerChild,
+                  // it is text, and it represents a boolean
+                  checkSingleChildTextBoolean(child, tag, child.tagName);
                 }
                 if (child.hasAttributes()) {
                   foundIssue = true;
diff --git a/test/fixtures/resources/PO034/fail/Remove-Headers-Header-Non-Boolean.xml b/test/fixtures/resources/PO034/fail/Remove-Headers-Header-Non-Boolean.xml
new file mode 100644
index 0000000..fabc75d
--- /dev/null
+++ b/test/fixtures/resources/PO034/fail/Remove-Headers-Header-Non-Boolean.xml
@@ -0,0 +1,7 @@
+<AssignMessage name="Remove-Headers-Header-Non-Boolean">
+  <Remove>
+    <Headers>
+      <Header name='foo'>non-bool-value</Header>
+    </Headers>
+  </Remove>
+</AssignMessage>
diff --git a/test/fixtures/resources/PO034/fail/Remove-Payload-NonBoolean.xml b/test/fixtures/resources/PO034/fail/Remove-Payload-NonBoolean.xml
new file mode 100644
index 0000000..47e3d5b
--- /dev/null
+++ b/test/fixtures/resources/PO034/fail/Remove-Payload-NonBoolean.xml
@@ -0,0 +1,5 @@
+<AssignMessage name="Remove-Payload-NonBoolean">
+  <Remove>
+    <Payload>non-boolean-value</Payload>
+  </Remove>
+</AssignMessage>
diff --git a/test/fixtures/resources/PO034/fail/messages.js b/test/fixtures/resources/PO034/fail/messages.js
index c24100a..0cbfde9 100644
--- a/test/fixtures/resources/PO034/fail/messages.js
+++ b/test/fixtures/resources/PO034/fail/messages.js
@@ -47,5 +47,11 @@ module.exports = {
   ],
   "Copy-with-multiple-attrs.xml": [
     "incorrect attribute (extraneous) on element <Copy>."
+  ],
+  "Remove-Payload-NonBoolean.xml": [
+    "if there is a text element under <Remove>/<Payload>, it should be a boolean."
+  ],
+  "Remove-Headers-Header-Non-Boolean.xml": [
+    "if there is a text element under <Remove>/<Headers>/<Header>, it should be a boolean."
   ]
 };
diff --git a/test/fixtures/resources/PO034/pass/Remove-Headers-Header-Empty.xml b/test/fixtures/resources/PO034/pass/Remove-Headers-Header-Empty.xml
new file mode 100644
index 0000000..bf99e08
--- /dev/null
+++ b/test/fixtures/resources/PO034/pass/Remove-Headers-Header-Empty.xml
@@ -0,0 +1,7 @@
+<AssignMessage name="Remove-Headers-Header-Empty">
+  <Remove>
+    <Headers>
+      <Header name='foo'/>
+    </Headers>
+  </Remove>
+</AssignMessage>
diff --git a/test/fixtures/resources/PO034/pass/Remove-Headers-Header-False.xml b/test/fixtures/resources/PO034/pass/Remove-Headers-Header-False.xml
new file mode 100644
index 0000000..68a9e86
--- /dev/null
+++ b/test/fixtures/resources/PO034/pass/Remove-Headers-Header-False.xml
@@ -0,0 +1,7 @@
+<AssignMessage name="Remove-Headers-Header-False">
+  <Remove>
+    <Headers>
+      <Header name='foo'>false</Header>
+    </Headers>
+  </Remove>
+</AssignMessage>
diff --git a/test/fixtures/resources/PO034/pass/Remove-Headers-Header-True.xml b/test/fixtures/resources/PO034/pass/Remove-Headers-Header-True.xml
new file mode 100644
index 0000000..d78baec
--- /dev/null
+++ b/test/fixtures/resources/PO034/pass/Remove-Headers-Header-True.xml
@@ -0,0 +1,7 @@
+<AssignMessage name="Remove-Headers-Header-True">
+  <Remove>
+    <Headers>
+      <Header name='foo'>true</Header>
+    </Headers>
+  </Remove>
+</AssignMessage>
diff --git a/test/fixtures/resources/PO034/pass/Remove-Payload.xml b/test/fixtures/resources/PO034/pass/Remove-Payload-Empty.xml
similarity index 100%
rename from test/fixtures/resources/PO034/pass/Remove-Payload.xml
rename to test/fixtures/resources/PO034/pass/Remove-Payload-Empty.xml
diff --git a/test/fixtures/resources/PO034/pass/Remove-Payload-False.xml b/test/fixtures/resources/PO034/pass/Remove-Payload-False.xml
new file mode 100644
index 0000000..c617111
--- /dev/null
+++ b/test/fixtures/resources/PO034/pass/Remove-Payload-False.xml
@@ -0,0 +1,5 @@
+<AssignMessage name="Remove-Payload-False">
+  <Remove>
+    <Payload>false</Payload>
+  </Remove>
+</AssignMessage>
diff --git a/test/fixtures/resources/PO034/pass/Remove-Payload-True.xml b/test/fixtures/resources/PO034/pass/Remove-Payload-True.xml
new file mode 100644
index 0000000..652930f
--- /dev/null
+++ b/test/fixtures/resources/PO034/pass/Remove-Payload-True.xml
@@ -0,0 +1,5 @@
+<AssignMessage name="Remove-Payload-True">
+  <Remove>
+    <Payload>true</Payload>
+  </Remove>
+</AssignMessage>