replace - fixed combined before and after usage (#31452)

When using before and after in combination, the opposite behavior was induced. This PR makes the the replacement happen between the specified patterns as intended.

* Added integration tests
* Add changelog, porting guide entry, and minor doc fixes

6 files changed, 317 insertions, 26 deletions

diff --git a/changelogs/fragments/replace-before-after.yaml b/changelogs/fragments/replace-before-after.yaml
new file mode 100644
index 0000000000..a50d238932
--- /dev/null
+++ b/changelogs/fragments/replace-before-after.yaml
@@ -0,0 +1,2 @@
+bugfixes:
+  - replace - fix behavior when ``before`` and ``after`` are used together (https://github.com/ansible/ansible/issues/31354)
diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.7.rst b/docs/docsite/rst/porting_guides/porting_guide_2.7.rst
index abd7a1b8ef..ac412bb4fc 100644
--- a/docs/docsite/rst/porting_guides/porting_guide_2.7.rst
+++ b/docs/docsite/rst/porting_guides/porting_guide_2.7.rst
@@ -224,6 +224,8 @@ Noteworthy module changes
 * The ``pip`` module has added a dependency on ``setuptools`` to support version requirements, this requirement is for
   the Python interpreter that executes the module and not the Python interpreter that the module is managing.
 
+* Prior to Ansible 2.7.10, the ``replace`` module did the opposite of what was intended when using the ``before`` and ``after`` options together. This now works properly but may require changes to tasks.
+
 
 Plugins
 =======
diff --git a/lib/ansible/modules/files/replace.py b/lib/ansible/modules/files/replace.py
index c816d64de1..6c49a4d07d 100644
--- a/lib/ansible/modules/files/replace.py
+++ b/lib/ansible/modules/files/replace.py
@@ -57,21 +57,24 @@ options:
       - The string to replace regexp matches.
       - May contain backreferences that will get expanded with the regexp capture groups if the regexp matches.
       - If not set, matches are removed entirely.
+      - Backreferences can be used ambiguously like C(\1), or explicitly like C(\g<1>).
     type: str
   after:
     description:
-      - If specified, the line after the replace/remove will start.
+      - If specified, only content after this match will be replaced/removed.
       - Can be used in combination with C(before).
       - Uses Python regular expressions; see
         U(http://docs.python.org/2/library/re.html).
+      - Uses DOTALL, which means the C(.) special character I(can match newlines).
     type: str
     version_added: "2.4"
   before:
     description:
-      - If specified, the line before the replace/remove will occur.
+      - If specified, only content before this match will be replaced/removed.
       - Can be used in combination with C(after).
       - Uses Python regular expressions; see
         U(http://docs.python.org/2/library/re.html).
+      - Uses DOTALL, which means the C(.) special character I(can match newlines).
     type: str
     version_added: "2.4"
   backup:
@@ -92,63 +95,78 @@ options:
     version_added: "2.4"
 notes:
   - As of Ansible 2.3, the I(dest) option has been changed to I(path) as default, but I(dest) still works as well.
+  - As of Ansible 2.7.10, the combined use of I(before) and I(after) works properly. If you were relying on the
+    previous incorrect behavior, you may be need to adjust your tasks.
+    See U(https://github.com/ansible/ansible/issues/31354) for details.
   - Option I(follow) has been removed in Ansible 2.5, because this module modifies the contents of the file so I(follow=no) doesn't make sense.
 '''
 
 EXAMPLES = r'''
-# Before Ansible 2.3, option 'dest', 'destfile' or 'name' was used instead of 'path'
-- replace:
+- name: Before Ansible 2.3, option 'dest', 'destfile' or 'name' was used instead of 'path'
+  replace:
     path: /etc/hosts
     regexp: '(\s+)old\.host\.name(\s+.*)?$'
     replace: '\1new.host.name\2'
-    backup: yes
 
 - name: Replace after the expression till the end of the file (requires Ansible >= 2.4)
   replace:
-    path: /etc/hosts
-    regexp: '(\s+)old\.host\.name(\s+.*)?$'
-    replace: '\1new.host.name\2'
-    after: Start after line.*
-    backup: yes
+    path: /etc/apache2/sites-available/default.conf
+    after: 'NameVirtualHost [*]'
+    regexp: '^(.+)$'
+    replace: '# \1'
 
 - name: Replace before the expression till the begin of the file (requires Ansible >= 2.4)
   replace:
-    path: /etc/hosts
-    regexp: '(\s+)old\.host\.name(\s+.*)?$'
-    replace: '\1new.host.name\2'
-    before: 'Start before line.*'
-    backup: yes
+    path: /etc/apache2/sites-available/default.conf
+    before: '# live site config'
+    regexp: '^(.+)$'
+    replace: '# \1'
 
+# Prior to Ansible 2.7.10, using before and after in combination did the opposite of what was intended.
+# see https://github.com/ansible/ansible/issues/31354 for details.
 - name: Replace between the expressions (requires Ansible >= 2.4)
   replace:
     path: /etc/hosts
-    regexp: '(\s+)old\.host\.name(\s+.*)?$'
-    replace: '\1new.host.name\2'
-    after: 'Start after line.*'
-    before: 'Start before line.*'
-    backup: yes
+    after: '<VirtualHost [*]>'
+    before: '</VirtualHost>'
+    regexp: '^(.+)$'
+    replace: '# \1'
 
-- replace:
+- name: Supports common file attributes
+  replace:
     path: /home/jdoe/.ssh/known_hosts
     regexp: '^old\.host\.name[^\n]*\n'
     owner: jdoe
     group: jdoe
     mode: '0644'
 
-- replace:
+- name: Supports a validate command
+  replace:
     path: /etc/apache/ports
     regexp: '^(NameVirtualHost|Listen)\s+80\s*$'
     replace: '\1 127.0.0.1:8080'
     validate: '/usr/sbin/apache2ctl -f %s -t'
 
 - name: Short form task (in ansible 2+) necessitates backslash-escaped sequences
-  replace: dest=/etc/hosts regexp='\\b(localhost)(\\d*)\\b' replace='\\1\\2.localdomain\\2 \\1\\2'
+  replace: path=/etc/hosts regexp='\\b(localhost)(\\d*)\\b' replace='\\1\\2.localdomain\\2 \\1\\2'
 
 - name: Long form task does not
   replace:
-    dest: /etc/hosts
+    path: /etc/hosts
     regexp: '\b(localhost)(\d*)\b'
     replace: '\1\2.localdomain\2 \1\2'
+
+- name: Explicitly specifying positional matched groups in replacement
+  replace:
+    path: /etc/ssh/sshd_config
+    regexp: '^(ListenAddress[ ]+)[^\n]+$'
+    replace: '\g<1>0.0.0.0'
+
+- name: Explicitly specifying named matched groups
+  replace:
+    path: /etc/ssh/sshd_config
+    regexp: '^(?P<dctv>ListenAddress[ ]+)(?P<host>[^\n]+)$'
+    replace: '#\g<dctv>\g<host>\n\g<dctv>0.0.0.0'
 '''
 
 import os
@@ -231,7 +249,7 @@ def main():
 
     pattern = u''
     if params['after'] and params['before']:
-        pattern = u'%s(?P<subsection>.*?)%s' % (params['before'], params['after'])
+        pattern = u'%s(?P<subsection>.*?)%s' % (params['after'], params['before'])
     elif params['after']:
         pattern = u'%s(?P<subsection>.*)' % params['after']
     elif params['before']:
@@ -242,6 +260,7 @@ def main():
         match = re.search(section_re, contents)
         if match:
             section = match.group('subsection')
+            indices = [match.start('subsection'), match.end('subsection')]
         else:
             res_args['msg'] = 'Pattern for before/after params did not match the given file: %s' % pattern
             res_args['changed'] = False
@@ -254,7 +273,7 @@ def main():
 
     if result[1] > 0 and section != result[0]:
         if pattern:
-            result = (contents.replace(section, result[0]), result[1])
+            result = (contents[:indices[0]] + result[0] + contents[indices[1]:], result[1])
         msg = '%s replacements made' % result[1]
         changed = True
         if module._diff:
diff --git a/test/integration/targets/replace/aliases b/test/integration/targets/replace/aliases
new file mode 100644
index 0000000000..a6dafcf8cd
--- /dev/null
+++ b/test/integration/targets/replace/aliases
@@ -0,0 +1 @@
+shippable/posix/group1
diff --git a/test/integration/targets/replace/meta/main.yml b/test/integration/targets/replace/meta/main.yml
new file mode 100644
index 0000000000..07faa21776
--- /dev/null
+++ b/test/integration/targets/replace/meta/main.yml
@@ -0,0 +1,2 @@
+dependencies:
+  - prepare_tests
diff --git a/test/integration/targets/replace/tasks/main.yml b/test/integration/targets/replace/tasks/main.yml
new file mode 100644
index 0000000000..24146ff31b
--- /dev/null
+++ b/test/integration/targets/replace/tasks/main.yml
@@ -0,0 +1,265 @@
+# setup
+- set_fact: output_dir_test={{output_dir}}/test_replace
+
+- name: make sure our testing sub-directory does not exist
+  file: path="{{ output_dir_test }}" state=absent
+
+- name: create our testing sub-directory
+  file: path="{{ output_dir_test }}" state=directory
+
+# tests
+- name: create test files
+  copy:
+      content: |-
+                The quick brown fox jumps over the lazy dog.
+                We promptly judged antique ivory buckles for the next prize.
+                Jinxed wizards pluck ivy from the big quilt.
+                Jaded zombies acted quaintly but kept driving their oxen forward.
+      dest: "{{ output_dir_test }}/pangrams.{{ item }}.txt"
+  with_sequence: start=0 end=6 format=%02x #increment as needed
+
+
+## test `before` option
+- name: remove all spaces before "quilt"
+  replace:
+    path: "{{ output_dir_test }}/pangrams.00.txt"
+    before: 'quilt'
+    regexp: ' '
+  register: replace_test0
+
+- command: "cat {{ output_dir_test }}/pangrams.00.txt"
+  register: replace_cat0
+
+- name: validate before assertions
+  assert:
+    that:
+      - replace_test0 is successful
+      - replace_test0 is changed
+      - replace_cat0.stdout_lines[0] == 'Thequickbrownfoxjumpsoverthelazydog.'
+      - replace_cat0.stdout_lines[-1] == 'Jaded zombies acted quaintly but kept driving their oxen forward.'
+
+
+## test `after` option
+- name: remove all spaces after "promptly"
+  replace:
+    path: "{{ output_dir_test }}/pangrams.01.txt"
+    after: 'promptly'
+    regexp: ' '
+  register: replace_test1
+
+- command: "cat {{ output_dir_test }}/pangrams.01.txt"
+  register: replace_cat1
+
+- name: validate after assertions
+  assert:
+    that:
+      - replace_test1 is successful
+      - replace_test1 is changed
+      - replace_cat1.stdout_lines[0] == 'The quick brown fox jumps over the lazy dog.'
+      - replace_cat1.stdout_lines[-1] == 'Jadedzombiesactedquaintlybutkeptdrivingtheiroxenforward.'
+
+
+## test combined `before` and `after` options
+- name: before "promptly" but after "quilt", replace every "e" with a "3"
+  replace:
+    path: "{{ output_dir_test }}/pangrams.02.txt"
+    before: 'promptly'
+    after: 'quilt'
+    regexp: 'e'
+    replace: '3'
+  register: replace_test2
+
+- name: validate after+before assertions
+  assert:
+    that:
+      - replace_test2 is successful
+      - not replace_test2 is changed
+      - replace_test2.msg.startswith("Pattern for before/after params did not match the given file")
+
+- name: before "quilt" but after "promptly", replace every "e" with a "3"
+  replace:
+    path: "{{ output_dir_test }}/pangrams.03.txt"
+    before: 'quilt'
+    after: 'promptly'
+    regexp: 'e'
+    replace: '3'
+  register: replace_test3
+
+- command: "cat {{ output_dir_test }}/pangrams.03.txt"
+  register: replace_cat3
+
+- name: validate before+after assertions
+  assert:
+    that:
+      - replace_test3 is successful
+      - replace_test3 is changed
+      - replace_cat3.stdout_lines[1] == 'We promptly judg3d antiqu3 ivory buckl3s for th3 n3xt priz3.'
+
+
+## test ^$ behavior in MULTILINE, and . behavior in absense of DOTALL
+- name: quote everything between bof and eof
+  replace:
+    path: "{{ output_dir_test }}/pangrams.04.txt"
+    regexp: ^([\S\s]+)$
+    replace: '"\1"'
+  register: replace_test4_0
+
+- command: "cat {{ output_dir_test }}/pangrams.04.txt"
+  register: replace_cat4_0
+
+- name: quote everything between bol and eol
+  replace:
+    path: "{{ output_dir_test }}/pangrams.04.txt"
+    regexp: ^(.+)$
+    replace: '"\1"'
+  register: replace_test4_1
+
+- command: "cat {{ output_dir_test }}/pangrams.04.txt"
+  register: replace_cat4_1
+
+- name: validate before+after assertions
+  assert:
+    that:
+      - replace_test4_0 is successful
+      - replace_test4_0 is changed
+      - replace_test4_1 is successful
+      - replace_test4_1 is changed
+      - replace_cat4_0.stdout_lines[0] == '"The quick brown fox jumps over the lazy dog.'
+      - replace_cat4_0.stdout_lines[-1] == 'Jaded zombies acted quaintly but kept driving their oxen forward."'
+      - replace_cat4_1.stdout_lines[0] == '""The quick brown fox jumps over the lazy dog."'
+      - replace_cat4_1.stdout_lines[-1] == '"Jaded zombies acted quaintly but kept driving their oxen forward.""'
+
+
+## test \b escaping in short and long form
+- name: short form with unescaped word boundaries
+  replace: path="{{ output_dir_test }}/pangrams.05.txt" regexp='\b(.+)\b' replace='"\1"'
+  register: replace_test5_0
+
+- name: short form with escaped word boundaries
+  replace: path="{{ output_dir_test }}/pangrams.05.txt" regexp='\\b(.+)\\b' replace='"\1"'
+  register: replace_test5_1
+
+- command: "cat {{ output_dir_test }}/pangrams.05.txt"
+  register: replace_cat5_1
+
+- name: long form with unescaped word boundaries
+  replace:
+    path: "{{ output_dir_test }}/pangrams.05.txt"
+    regexp: '\b(.+)\b'
+    replace: '"\1"'
+  register: replace_test5_2
+
+- command: "cat {{ output_dir_test }}/pangrams.05.txt"
+  register: replace_cat5_2
+
+- name: long form with escaped word boundaries
+  replace:
+    path: "{{ output_dir_test }}/pangrams.05.txt"
+    regexp: '\\b(.+)\\b'
+    replace: '"\1"'
+  register: replace_test5_3
+
+- name: validate before+after assertions
+  assert:
+    that:
+      - not replace_test5_0 is changed
+      - replace_test5_1 is changed
+      - replace_test5_2 is changed
+      - not replace_test5_3 is changed
+      - replace_cat5_1.stdout_lines[0] == '"The quick brown fox jumps over the lazy dog".'
+      - replace_cat5_1.stdout_lines[-1] == '"Jaded zombies acted quaintly but kept driving their oxen forward".'
+      - replace_cat5_2.stdout_lines[0] == '""The quick brown fox jumps over the lazy dog"".'
+      - replace_cat5_2.stdout_lines[-1] == '""Jaded zombies acted quaintly but kept driving their oxen forward"".'
+
+
+## test backup behaviors
+- name: replacement with backup
+  replace:
+    path: "{{ output_dir_test }}/pangrams.06.txt"
+    regexp: ^(.+)$
+    replace: '"\1"'
+    backup: true
+  register: replace_test6
+
+- command: "cat {{ output_dir_test }}/pangrams.06.txt"
+  register: replace_cat6_0
+
+- command: "cat {{ replace_test6.backup_file }}"
+  register: replace_cat6_1
+
+- name: validate backup
+  assert:
+    that:
+      - replace_test6 is successful
+      - replace_test6 is changed
+      - replace_test6.backup_file is search('/pangrams.06.txt.')
+      - replace_cat6_0.stdout != replace_cat6_1.stdout
+
+
+## test filesystem failures
+- name: fail on directory
+  replace:
+    path: "{{ output_dir_test }}"
+    regexp: ^(.+)$
+  register: replace_test7_1
+  ignore_errors: true
+
+- name: fail on missing file
+  replace:
+    path: "{{ output_dir_test }}/missing_file.txt"
+    regexp: ^(.+)$
+  register: replace_test7_2
+  ignore_errors: true
+
+- name: validate backup
+  assert:
+    that:
+      - replace_test7_1 is failure
+      - replace_test7_2 is failure
+      - replace_test7_1.msg.endswith(" is a directory !")
+      - replace_test7_2.msg.endswith(" does not exist !")
+
+
+## test subsection replacement when before/after potentially match more than once
+- name: test file for subsection replacement gone awry
+  copy:
+      content: |-
+                # start of group
+                0.0.0.0
+                127.0.0.1
+                127.0.1.1
+                # end of group
+
+                # start of group
+                0.0.0.0
+                127.0.0.1
+                127.0.1.1
+                # end of group
+
+                # start of group
+                0.0.0.0
+                127.0.0.1
+                127.0.1.1
+                # end of group
+      dest: "{{ output_dir_test }}/addresses.txt"
+
+- name: subsection madness
+  replace:
+    path: "{{ output_dir_test }}/addresses.txt"
+    after: '# start of group'
+    before: '# end of group'
+    regexp: '0'
+    replace: '9'
+  register: replace_test8
+
+- command: "cat {{ output_dir_test }}/addresses.txt"
+  register: replace_cat8
+
+- name: validate before+after assertions
+  assert:
+    that:
+      - replace_test8 is successful
+      - replace_test8 is changed
+      - replace_cat8.stdout_lines[1] == "9.9.9.9"
+      - replace_cat8.stdout_lines[7] == "0.0.0.0"
+      - replace_cat8.stdout_lines[13] == "0.0.0.0"