More file refactoring (#40114)

* Set src in the state functions rather than the toplevel

A good API should only require passing one version of a piece of data
around so do that for src

* Move the rewriting of path into additional_parameter_handling

When the path is a directory we can rewrite the path to be a file inside
of the directory

* Emit a warning when src is used with a state where it should be ignored

changelogs/fragments/file-disallow-src.yaml

@@ -0,0 +1,8 @@
+---
+bugfixes:
+- file module - The file module allowed the user to specify src as a parameter
+  when state was not link or hard.  This is documented as only applying to
+  state=link or state=hard but in previous Ansible, this could have an effect
+  in rare cornercases.  For instance, "ansible -m file -a 'state=directory
+  path=/tmp src=/var/lib'" would create /tmp/lib.  This has been disabled and
+  a warning emitted (will change to an error in Ansible-2.10).

docs/docsite/rst/porting_guides/porting_guide_2.6.rst

@@ -51,6 +51,18 @@ Noteworthy module changes
 * The ``win_iis_webapppool`` module no longer accepts a string for the ``atributes`` module option; use the free form dictionary value instead
 * The ``name`` module option for ``win_package`` has been removed; this is not used anywhere and should just be removed from your playbooks
 * The ``win_regedit`` module no longer automatically corrects the hive path ``HCCC`` to ``HKCC``; use ``HKCC`` because this is the correct hive path
+* The :ref:`file_module` now emits a deprecation warning when ``src`` is specified with a state
+  other than ``hard`` or ``link`` as it is only supposed to be useful with those.  This could have
+  an effect on people who were depending on a buggy interaction between src and other state's to
+  place files into a subdirectory.  For instance::
+
+    $ ansible localhost -m file -a 'path=/var/lib src=/tmp/ state=directory'
+
+  Would create a directory named ``/tmp/lib``.  Instead of the above, simply spell out the entire
+  destination path like this::
+
+    $ ansible localhost -m file -a 'path=/tmp/lib state=directory'
+
 
 Plugins
 =======

lib/ansible/modules/files/file.py

@@ -159,9 +159,6 @@ def _ansible_excepthook(exc_type, exc_value, tb):
 
 def additional_parameter_handling(params):
     """Additional parameter validation and reformatting"""
-
-    params['b_src'] = to_bytes(params['src'], errors='surrogate_or_strict', nonstring='passthru')
-
     # state should default to file, but since that creates many conflicts,
     # default state to 'current' when it exists.
     prev_state = get_state(to_bytes(params['path'], errors='surrogate_or_strict'))
@@ -179,6 +176,30 @@ def additional_parameter_handling(params):
         raise ParameterError(results={"msg": "recurse option requires state to be 'directory'",
                                       "path": params["path"]})
 
+    # Make sure that src makes sense with the state
+    if params['src'] and params['state'] not in ('link', 'hard'):
+        params['src'] = None
+        module.warn("The src option requires state to be 'link' or 'hard'.  This will become an"
+                    " error in Ansible 2.10")
+
+        # In 2.10, switch to this
+        # raise ParameterError(results={"msg": "src option requires state to be 'link' or 'hard'",
+        #                               "path": params["path"]})
+
+    # When path is a directory, rewrite the pathname to be the file inside of the directory
+    # TODO: Why do we exclude link?  Why don't we exclude directory?  Should we exclude touch?
+    if (params['state'] not in ("link", "absent") and os.path.isdir(to_bytes(params['path'], errors='surrogate_or_strict'))):
+        basename = None
+
+        # original_basename is used by other modules that depend on file
+        if params['original_basename']:
+            basename = params['original_basename']
+        elif params['src'] is not None:
+            basename = os.path.basename(params['src'])
+
+        if basename:
+            params['path'] = params['path'] = os.path.join(params['path'], basename)
+
 
 def get_state(path):
     ''' Find out current state '''
@@ -426,8 +447,9 @@ def ensure_directory(path, follow, recurse):
     return {'path': path, 'changed': changed, 'diff': diff}
 
 
-def ensure_symlink(path, src, b_src, follow, force):
+def ensure_symlink(path, src, follow, force):
     b_path = to_bytes(path, errors='surrogate_or_strict')
+    b_src = to_bytes(src, errors='surrogate_or_strict')
     prev_state = get_state(b_path)
     file_args = module.load_file_common_arguments(module.params)
 
@@ -437,7 +459,7 @@ def ensure_symlink(path, src, b_src, follow, force):
         if follow:
             # use the current target of the link as the source
             src = to_native(os.path.realpath(b_path), errors='strict')
-            b_src = to_bytes(os.path.realpath(b_path), errors='strict')
+            b_src = to_bytes(src, errors='surrogate_or_strict')
 
     if not os.path.islink(b_path) and os.path.isdir(b_path):
         relpath = path
@@ -537,8 +559,9 @@ def ensure_symlink(path, src, b_src, follow, force):
     return {'dest': path, 'src': src, 'changed': changed, 'diff': diff}
 
 
-def ensure_hardlink(path, src, b_src, follow, force):
+def ensure_hardlink(path, src, follow, force):
     b_path = to_bytes(path, errors='surrogate_or_strict')
+    b_src = to_bytes(src, errors='surrogate_or_strict')
     prev_state = get_state(b_path)
     file_args = module.load_file_common_arguments(module.params)
 
@@ -665,31 +688,20 @@ def main():
     follow = params['follow']
     path = params['path']
     src = params['src']
-    b_src = params['b_src']
 
     # short-circuit for diff_peek
     if params['_diff_peek'] is not None:
         appears_binary = execute_diff_peek(to_bytes(path, errors='surrogate_or_strict'))
         module.exit_json(path=path, changed=False, appears_binary=appears_binary)
 
-    # original_basename is used by other modules that depend on file.
-    if state not in ("link", "absent") and os.path.isdir(to_bytes(path, errors='surrogate_or_strict')):
-        basename = None
-        if params['original_basename']:
-            basename = params['original_basename']
-        elif b_src is not None:
-            basename = os.path.basename(b_src)
-        if basename:
-            params['path'] = path = os.path.join(path, basename)
-
     if state == 'file':
         result = ensure_file_attributes(path, follow)
     elif state == 'directory':
         result = ensure_directory(path, follow, recurse)
     elif state == 'link':
-        result = ensure_symlink(path, src, b_src, follow, force)
+        result = ensure_symlink(path, src, follow, force)
     elif state == 'hard':
-        result = ensure_hardlink(path, src, b_src, follow, force)
+        result = ensure_hardlink(path, src, follow, force)
     elif state == 'touch':
         result = execute_touch(path, follow)
     elif state == 'absent':

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

@@ -134,7 +134,9 @@
       - "file6_result.changed == true"
 
 - name: touch a hard link
-  file: src={{output_file}} dest={{output_dir}}/hard.txt state=touch
+  file:
+    dest: '{{ output_dir }}/hard.txt'
+    state: 'touch'
   register: file6_touch_result
 
 - name: verify that the hard link was touched