Refactor of win_xml (2nd attempt) to add support for processing multiple nodes and counting nodes matched by xpath (#53362)

* add multi-node manipulation, delete on xpath match only and count capability to win_xml

* fix pep8 and yamllint errors identified by ci tests

* fixed bugs when handling multiple elements, multiple attribute nodes and handling for attribute nodes when using xpaths that only select attributes like //@lang.  Added more tests and tweaked documentation.

* fixed line-too-long error

* fixed trailing space errors

* trailing whitespace expunged

* bump version_added to 2.9 for new changes

* fix PSAvoidUsingPositionalParameters sanity check failure

* refix sanity check as it broke the msg return value

changelogs/fragments/win_xml_refactor.yaml

@@ -0,0 +1,5 @@
+minor_changes:
+  - win_xml - Behaviour change, module now processes all nodes specified by xpath, not just first encountered.
+  - win_xml - Behaviour change, fragment no longer required when processing element type nodes and state=absent.
+  - win_xml - Some output messages worded differently now the module uses a generic method to save changes.
+  - win_xml - Added 'count' module parameter which will return number of nodes matched by xpath if set to yes/true

lib/ansible/modules/windows/win_xml.ps1

@@ -80,6 +80,22 @@ function Compare-XmlDocs($actual, $expected) {
     }
 }
 
+
+function Save-ChangedXml($xmlorig, $result, $message, $check_mode, $backup) {
+    $result.changed = $true
+    if (-Not $check_mode) {
+        if ($backup) {
+            $result.backup_file = Backup-File -path $dest -WhatIf:$check_mode
+            # Ensure backward compatibility (deprecate in future)
+            $result.backup = $result.backup_file
+        }
+        $xmlorig.Save($dest)
+        $result.msg = $message
+    } else {
+        $result.msg += " check mode"
+    }
+}
+
 $params = Parse-Args $args -supports_check_mode $true
 $check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "bool" -default $false
 
@@ -87,12 +103,13 @@ $debug_level = Get-AnsibleParam -obj $params -name "_ansible_verbosity" -type "i
 $debug = $debug_level -gt 2
 
 $dest = Get-AnsibleParam $params "path" -type "path" -FailIfEmpty $true -aliases "dest", "file"
-$fragment = Get-AnsibleParam $params "fragment" -type "str" -FailIfEmpty $true -aliases "xmlstring"
+$fragment = Get-AnsibleParam $params "fragment" -type "str" -aliases "xmlstring"
 $xpath = Get-AnsibleParam $params "xpath" -type "str" -FailIfEmpty $true
-$backup = Get-AnsibleParam $params "backup" -type "bool" -default $false
+$backup = Get-AnsibleParam $params "backup" -type "bool" -Default $false
 $type = Get-AnsibleParam $params "type" -type "str" -Default "element" -ValidateSet "element", "attribute", "text"
 $attribute = Get-AnsibleParam $params "attribute" -type "str" -FailIfEmpty ($type -eq "attribute")
 $state = Get-AnsibleParam $params "state" -type "str" -Default "present"
+$count = Get-AnsibleParam $params "count" -type "bool" -Default $false
 
 $result = @{
     changed = $false
@@ -117,121 +134,132 @@ $localname = $xmlorig.DocumentElement.LocalName
 
 $namespaceMgr.AddNamespace($xmlorig.$localname.SchemaInfo.Prefix, $namespace)
 
+$nodeList = $xmlorig.SelectNodes($xpath, $namespaceMgr)
+$nodeListCount = $nodeList.get_Count()
+if ($count) {
+    $result.count = $nodeListCount
+    if (-not $fragment) {
+       Exit-Json $result
+    }
+}
+## Exit early if xpath did not match any nodes
+if ($nodeListCount -eq 0) {
+    $result.msg = "The supplied xpath did not match any nodes.  If this is unexpected, check your xpath is valid for the xml file at supplied path."
+    Exit-Json $result
+}
+
+$changed = $false
+$result.msg = "not changed"
+
 if ($type -eq "element") {
-    $xmlchild = $null
-    Try {
-        $xmlchild = [xml]$fragment
-    } Catch {
-        Fail-Json $result "Failed to parse fragment as XML: $($_.Exception.Message)"
-    }
-
-    $child = $xmlorig.CreateElement($xmlchild.get_DocumentElement().get_Name(), $xmlorig.get_DocumentElement().get_NamespaceURI())
-    Copy-Xml -dest $child -src $xmlchild.DocumentElement -xmlorig $xmlorig
-
-    $node = $xmlorig.SelectSingleNode($xpath, $namespaceMgr)
-    if ($node.get_NodeType() -eq "Document") {
-        $node = $node.get_DocumentElement()
-    }
-    $elements = $node.get_ChildNodes()
-    [bool]$present = $false
-    [bool]$changed = $false
-    if ($elements.get_Count()) {
-        if ($debug) {
-            $err = @()
-            $result.err = {$err}.Invoke()
-        }
-        foreach ($element in $elements) {
-            try {
-                Compare-XmlDocs $child $element
-                $present = $true
-                break
-            } catch {
+    if ($state -eq "absent") {
+        foreach ($node in $nodeList) {
+            # there are some nodes that match xpath, delete without comparing them to fragment
+            if (-Not $check_mode) {
+                $removedNode = $node.get_ParentNode().RemoveChild($node)
+                $changed = $true
                 if ($debug) {
-                    $result.err.Add($_.Exception.ToString())
+                    $result.removed += $result.removed + $removedNode.get_OuterXml()
                 }
             }
         }
-        if (!$present -and ($state -eq "present")) {
-            [void]$node.AppendChild($child)
-            $result.msg = "xml added"
-            $changed = $true
-        } elseif ($present -and ($state -eq "absent")) {
-            [void]$node.RemoveChild($element)
-            $result.msg = "xml removed"
-            $changed = $true
+    } else { # state -eq 'present'
+        $xmlfragment = $null
+        Try {
+            $xmlfragment = [xml]$fragment
+        } Catch {
+            Fail-Json $result "Failed to parse fragment as XML: $($_.Exception.Message)"
         }
-    } else {
-        if ($state -eq "present") {
-            [void]$node.AppendChild($child)
-            $result.msg = "xml added"
-            $changed = $true
-        }
-    }
 
-    if ($changed) {
-        if ($backup) {
-            $result.backup_file = Backup-File -path $dest -WhatIf:$check_mode
-            # Ensure backward compatibility (deprecate in future)
-            $result.backup = $result.backup_file
+        foreach ($node in $nodeList) {
+            $candidate = $xmlorig.CreateElement($xmlfragment.get_DocumentElement().get_Name(), $xmlorig.get_DocumentElement().get_NamespaceURI())
+            Copy-Xml -dest $candidate -src $xmlfragment.DocumentElement -xmlorig $xmlorig
+
+            if ($node.get_NodeType() -eq "Document") {
+                $node = $node.get_DocumentElement()
+            }
+            $elements = $node.get_ChildNodes()
+            [bool]$present = $false
+            [bool]$changed = $false
+            $element_count = $elements.get_Count()
+            $nstatus = "node: " + $node.get_Value() + " element: " + $elements.get_OuterXml() + " Element count is $element_count"
+            Add-Warning $result $nstatus
+            if ($elements.get_Count()) {
+                if ($debug) {
+                    $err = @()
+                    $result.err = {$err}.Invoke()
+                }
+                foreach ($element in $elements) {
+                    $estatus = "element is " + $element.get_OuterXml()
+                    Add-Warning $result $estatus
+                    try {
+                        Compare-XmlDocs $candidate $element
+                        $present = $true
+                        break
+                    } catch {
+                        if ($debug) {
+                            $result.err.Add($_.Exception.ToString())
+                        }
+                    }
+                }
+                if (-Not $present -and ($state -eq "present")) {
+                    [void]$node.AppendChild($candidate)
+                    $result.msg = $result.msg + "xml added "
+                    $changed = $true
+                }
+            }
         }
-        if (-not $check_mode) {
-            $xmlorig.Save($dest)
-        }
-        $result.changed = $true
-    } else {
-        $result.msg = "not changed"
     }
 } elseif ($type -eq "text") {
-    $node = $xmlorig.SelectSingleNode($xpath, $namespaceMgr)
-    [bool]$add = ($node.get_InnerText() -ne $fragment)
-    if ($add) {
-        if ($backup) {
-            $result.backup_file = Backup-File -path $dest -WhatIf:$check_mode
-            # Ensure backward compatibility (deprecate in future)
-            $result.backup = $result.backup_file
+    foreach ($node in $nodeList) {
+        if ($node.get_InnerText() -ne $fragment) {
+            $node.set_InnerText($fragment)
+            $changed = $true
         }
-        $node.set_InnerText($fragment)
-        if (-not $check_mode) {
-            $xmlorig.Save($dest)
-        }
-        $result.changed = $true
-        $result.msg = "text changed"
-    } else {
-        $result.msg = "not changed"
     }
 } elseif ($type -eq "attribute") {
-    $node = $xmlorig.SelectSingleNode($xpath, $namespaceMgr)
-    [bool]$add = !$node.HasAttribute($attribute) -Or ($node.$attribute -ne $fragment)
-    if ($add -And ($state -eq "present")) {
-        if ($backup) {
-            $result.backup_file = Backup-File -path $dest -WhatIf:$check_mode
-            # Ensure backward compatibility (deprecate in future)
-            $result.backup = $result.backup_file
+    foreach ($node in $nodeList) {
+        if ($state -eq 'present') {
+            if ($node.NodeType -eq 'Attribute') {
+                if ($node.Name -eq $attribute -and $node.Value -ne $fragment ) {
+                    # this is already the attribute with the right name, so just set its value to match fragment
+                    $node.Value = $fragment
+                    $changed = $true
+                }
+            } else { # assume NodeType is Element
+                if ($node.$attribute -ne $fragment) {
+                    if (!$node.HasAttribute($attribute)) { # add attribute to Element if missing
+                        $node.SetAttributeNode($attribute, $xmlorig.get_DocumentElement().get_NamespaceURI())
+                    }
+                    #set the attribute into the element
+                    $node.SetAttribute($attribute, $fragment)
+                    $changed = $true
+                }
+            }
+        } elseif ($state -eq 'absent') {
+            if ($node.NodeType -eq 'Attribute') {
+                $attrNode = [System.Xml.XmlAttribute]$node
+                $parent = $attrNode.OwnerElement
+                $parent.RemoveAttribute($attribute)
+                $changed = $true
+            } else { # element node processing
+                if ($node.Name -eq $attribute ) { # note not caring about the state of 'fragment' at this point
+                   $node.RemoveAttribute($attribute)
+                   $changed = $true
+                }
+            }
+        } else {
+            Add-Warning $result "Unexpected state when processing attribute $($attribute), add was $add, state was $state"
         }
-        if (!$node.HasAttribute($attribute)) {
-            $node.SetAttributeNode($attribute, $xmlorig.get_DocumentElement().get_NamespaceURI())
-        }
-        $node.SetAttribute($attribute, $fragment)
-        if (-not $check_mode) {
-            $xmlorig.Save($dest)
-        }
-        $result.changed = $true
-        $result.msg = "text changed"
-    } elseif (!$add -And ($state -eq "absent")) {
-        if ($backup) {
-            $result.backup_file = Backup-File -path $dest -WhatIf:$check_mode
-            # Ensure backward compatibility (deprecate in future)
-            $result.backup = $result.backup_file
-        }
-        $node.RemoveAttribute($attribute)
-        if (-not $check_mode) {
-            $xmlorig.Save($dest)
-        }
-        $result.changed = $true
-        $result.msg = "text changed"
-    } else {
-        $result.msg = "not changed"
     }
 }
+if ($changed) {
+    if ($state -eq "absent") {
+        $summary = "$type removed"
+    } else {
+        $summary = "$type changed"
+    }
+    Save-ChangedXml -xmlorig $xmlorig -result $result -message $summary -check_mode $check_mode -backup $backup
+}
 
 Exit-Json $result

lib/ansible/modules/windows/win_xml.py

@@ -15,28 +15,23 @@ DOCUMENTATION = r'''
 ---
 module: win_xml
 version_added: "2.7"
-short_description: Add XML fragment to an XML parent
+short_description: Manages XML file content on Windows hosts
 description:
-    - Adds XML fragments formatted as strings to existing XML on remote servers.
+    - Manages XML nodes, attributes and text, using xpath to select which xml nodes need to be managed.
+    - XML fragments, formatted as strings, are used to specify the desired state of a part or parts of XML files on remote Windows servers.
     - For non-Windows targets, use the M(xml) module instead.
 options:
-    path:
+    attribute:
         description:
-        - The path of remote servers XML.
-        type: path
-        required: true
-        aliases: [ dest, file ]
-    fragment:
-        description:
-        - The string representation of the XML fragment to be added.
+        - The attribute name if the type is 'attribute'.
+        - Required if C(type=attribute).
         type: str
-        required: true
-        aliases: [ xmlstring ]
-    xpath:
+    count:
         description:
-        - The node of the remote server XML where the fragment will go.
-        type: str
-        required: true
+        - When set to C(yes), return the number of nodes matched by I(xpath).
+        type: bool
+        default: false
+        version_added: 2.9
     backup:
         description:
         - Determine whether a backup should be created.
@@ -44,20 +39,49 @@ options:
           so you can get the original file back if you somehow clobbered it incorrectly.
         type: bool
         default: no
+    fragment:
+        description:
+        - The string representation of the XML fragment expected at xpath.  Since ansible 2.9 not required when I(state=absent), or when I(count=yes).
+        type: str
+        required: false
+        aliases: [ xmlstring ]
+    path:
+        description:
+        - Path to the file to operate on.
+        type: path
+        required: true
+        aliases: [ dest, file ]
+    state:
+        description:
+        - Set or remove the nodes (or attributes) matched by I(xpath).
+        type: str
+        default: present
+        choices: [ present, absent ]
+        version_added: 2.9
     type:
         description:
-        - The type of XML you are working with.
+        - The type of XML node you are working with.
         type: str
         required: yes
         default: element
         choices: [ attribute, element, text ]
-    attribute:
+    xpath:
         description:
-        - The attribute name if the type is 'attribute'.
-        - Required if C(type=attribute).
+        - Xpath to select the node or nodes to operate on.
         type: str
+        required: true
 author:
     - Richard Levenberg (@richardcs)
+    - Jon Hawkesworth (@jhawkesworth)
+notes:
+    - Only supports operating on xml elements, attributes and text.
+    - Namespace, processing-instruction, command and document node types cannot be modified with this module.
+seealso:
+    - module: xml
+      description: XML manipulation for Posix hosts.
+    - name: w3shools XPath tutorial
+      description: A useful tutorial on XPath
+      link: https://www.w3schools.com/xml/xpath_intro.asp
 '''
 
 EXAMPLES = r'''
@@ -74,6 +98,32 @@ EXAMPLES = r'''
    attribute: 'sslEnabledProtocols'
    fragment: 'TLSv1,TLSv1.1,TLSv1.2'
    type: attribute
+
+- name: remove debug configuration nodes from nlog.conf
+  win_xml:
+   path: C:\IISApplication\nlog.conf
+   xpath: /nlog/rules/logger[@name="debug"]/descendant::*
+   state: absent
+
+- name: count configured connectors in Tomcat's server.xml
+  win_xml:
+   path: C:\Tomcat\conf\server.xml
+   xpath: //Server/Service/Connector
+   count: yes
+  register: connector_count
+
+- name: show connector count
+  debug:
+    msg="Connector count is {{connector_count.count}}"
+
+- name: ensure all lang=en attributes to lang=nl
+  win_xml:
+   path: C:\Data\Books.xml
+   xpath: //@[lang="en"]
+   attribute: lang
+   fragment: nl
+   type: attribute
+
 '''
 
 RETURN = r'''
@@ -82,6 +132,11 @@ backup_file:
     returned: if backup=yes
     type: str
     sample: C:\Path\To\File.txt.11540.20150212-220915.bak
+count:
+    description: Number of nodes matched by xpath.
+    returned: if count=yes
+    type: int
+    sample: 33
 msg:
     description: What was done.
     returned: always

test/integration/targets/win_xml/files/books.xml

@@ -0,0 +1,10 @@
+<?xml version='1.0' encoding='utf-8'?>
+<books>
+  <works lang="en">
+    <title lang="en" isbn13="123412341234X">A Great Book</title>
+    <title lang="en" isbn13="1234109823400">Best Book Ever</title>
+    <title lang="en" isbn13="123412121234X">Worst Book Ever</title>
+    <title lang="en" isbn13="423412341234X">Another Book</title>
+    <title lang="en" isbn13="523412341234X">Worst Book Ever Two</title>
+  </works>
+</books>

test/integration/targets/win_xml/tasks/main.yml

@@ -142,3 +142,166 @@
   assert:
     that:
       - sha1_checksum.stat.checksum == 'de86f79b409383447cf4cf112b20af8ffffcfdbf'
+
+# features added ansible 2.8
+# count
+
+- name: count logger nodes in log4j.xml
+  win_xml:
+    path: "{{ win_output_dir }}\\log4j.xml"
+    xpath: //logger
+    count: yes
+  register: logger_node_count
+
+- name: verify node count
+  assert:
+    that:
+      - logger_node_count.count == 5
+
+# multiple attribute change
+- name: ensure //logger/level value attributes are set to debug
+  win_xml:
+    path: "{{ win_output_dir }}\\log4j.xml"
+    xpath: '//logger/level[@value="error"]'
+    type: attribute
+    attribute: value
+    fragment: debug
+    count: yes
+  register: logger_level_value_attrs
+ 
+- name: verify //logger/level value attributes
+  assert:
+    that:
+      - logger_level_value_attrs.count == 4
+      - logger_level_value_attrs.changed == true
+      - logger_level_value_attrs.msg == 'attribute changed'
+
+- name: ensure //logger/level value attributes are set to debug (idempotency)
+  win_xml:
+    path: "{{ win_output_dir }}\\log4j.xml"
+    xpath: '//logger/level[@value="error"]'
+    type: attribute
+    attribute: value
+    fragment: debug
+    count: yes
+  register: logger_level_value_attrs_again
+ 
+- name: verify //logger/level value attributes again (idempotency)
+  assert:
+    that:
+      - logger_level_value_attrs_again.count == 0
+      - logger_level_value_attrs_again.changed == false
+      - logger_level_value_attrs_again.msg == 'The supplied xpath did not match any nodes.  If this is unexpected, check your xpath is valid for the xml file at supplied path.'
+
+# multiple text nodes
+- name: ensure test books.xml is present
+  win_copy:
+    src: books.xml
+    dest: '{{ win_output_dir }}\books.xml'
+
+- name: demonstrate multi text replace by replacing all title text elements
+  win_xml:
+    path: '{{ win_output_dir }}\books.xml'
+    xpath: //works/title
+    type: text
+    fragment: _TITLE_TEXT_REMOVED_BY_WIN_XML_MODULE_
+    count: yes
+  register: multi_text
+
+- name: verify multi text change
+  assert:
+    that:
+      - multi_text.changed == true
+      - multi_text.count == 5
+      - multi_text.msg == 'text changed'
+
+- name: demonstrate multi text replace by replacing all title text elements again (idempotency)
+  win_xml:
+    path: '{{ win_output_dir }}\books.xml'
+    xpath: //works/title
+    type: text
+    fragment: _TITLE_TEXT_REMOVED_BY_WIN_XML_MODULE_
+    count: yes
+  register: multi_text_again
+
+- name: verify multi text again change (idempotency)
+  assert:
+    that:
+      - multi_text_again.changed == false
+      - multi_text_again.count == 5
+      - multi_text_again.msg == 'not changed'
+
+# multiple element 
+
+#- name: ensure a fresh test books.xml is present
+#  win_copy:
+#    src: books.xml
+#    dest: '{{ win_output_dir }}\books.xml'
+
+- name: demonstrate multi element should append new information element from fragment
+  win_xml:
+    path: '{{ win_output_dir }}\books.xml'
+    xpath: //works/title
+    type: element
+    fragment: <information>This element added by ansible</information>
+    count: yes
+  register: multi_element
+
+- name: verify multi element
+  assert:
+    that:
+      - multi_element.changed == true
+      - multi_element.count == 5
+      - multi_element.msg == 'element changed'
+
+- name: demonstrate multi element unchanged (idempotency)
+  win_xml:
+    path: '{{ win_output_dir }}\books.xml'
+    xpath: //works/title
+    type: element
+    fragment: <information>This element added by ansible</information>
+    count: yes
+  register: multi_element_again
+
+- name: verify multi element again (idempotency)
+  assert:
+    that:
+      - multi_element_again.changed == false
+      - multi_element_again.count == 5
+      - multi_element_again.msg == 'not changed'
+
+# multiple attributes on differing parent nodes
+
+- name: ensure all attribute lang=nl
+  win_xml:
+    path: '{{ win_output_dir }}\books.xml'
+    xpath: //@lang
+    type: attribute
+    attribute: lang
+    fragment: nl
+    count: yes
+  register: multi_attr
+
+- name: verify multi attribute
+  assert:
+    that:
+      - multi_attr.changed == true
+      - multi_attr.count == 6
+      - multi_attr.msg == 'attribute changed'
+
+- name: ensure all attribute lang=nl (idempotency)
+  win_xml:
+    path: '{{ win_output_dir }}\books.xml'
+    xpath: //@lang
+    type: attribute
+    attribute: lang
+    fragment: nl
+    count: yes
+  register: multi_attr_again
+
+- name: verify multi attribute (idempotency)
+  assert:
+    that:
+      - multi_attr_again.changed == false
+      - multi_attr_again.count == 6
+      - multi_attr_again.msg == 'not changed'