Repository: ansible/ansible
Pull request: Fix fileglob parameter order bug
##### SUMMARY

The order of the parameters given to `fileglob` can change the results.

Fixes #72873

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request


##### COMPONENT NAME
fileglob

##### ADDITIONAL INFORMATION
<!--- Include additional information to help people understand the change here -->
<!--- A step-by-step reproduction of the problem is helpful if there is no related issue -->

<!--- Paste verbatim command output below, e.g. before and after your change -->
```paste below

```


Problem statement:
fileglob lookup plugin returns inconsistent values

##### SUMMARY

The `fileglob` lookup plugin returns values inconsistently, depending on the order of arguments (terms) given it.

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME

fileglob

##### ANSIBLE VERSION

```paste below
ansible 2.10.3
  config file = /Users/jpm/take/training/bugs/fileglob/ansible.cfg
  configured module search path = ['/Users/jpm/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/jpm/take/training/bugs/fileglob/env.v3/lib/python3.8/site-packages/ansible
  executable location = /Users/jpm/take/training/bugs/fileglob/env.v3/bin/ansible
  python version = 3.8.5 (default, Sep 27 2020, 11:38:54) [Clang 12.0.0 (clang-1200.0.32.2)]
```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```paste below
ANSIBLE_NOCOWS(/Users/jpm/take/training/bugs/fileglob/ansible.cfg) = True
```

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->

macOS Catalina (10.15.7)

##### STEPS TO REPRODUCE

Run the example playbook

```yaml
- hosts: localhost
  connection: local
  gather_facts: false
  vars:
     dir: files
  tasks:
  - file: path='{{ dir }}' state=directory
  
  - file: path='setvars.bat' state=touch  # in current directory!

  - file: path='{{ dir }}/{{ item }}' state=touch
    loop:
        - json.c
        - strlcpy.c
        - base64.c
        - json.h
        - base64.h
        - strlcpy.h
        - jo.c

  - debug:
        msg: '{{ query("fileglob", "setvars.bat", "{{ dir }}/*.[ch]" ) }}'

  - debug:
        msg: '{{ query("fileglob", "{{ dir }}/*.[ch]", "setvars.bat" ) }}'
```


##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->

I expected the path `setvars.bat` to be contained in both result sets, but it's in the first only. The results are identical if I use `lookup()` instead of `query()` and when I use `with_fileglob:`

##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes -->
```paste below
PLAY [localhost] ***************************************************************

TASK [file] ********************************************************************
ok: [localhost]

TASK [file] ********************************************************************
changed: [localhost] => (item=json.c)
changed: [localhost] => (item=strlcpy.c)
changed: [localhost] => (item=base64.c)
changed: [localhost] => (item=json.h)
changed: [localhost] => (item=base64.h)
changed: [localhost] => (item=strlcpy.h)
changed: [localhost] => (item=jo.c)

TASK [debug] *******************************************************************
ok: [localhost] => {
    "msg": [
        "/Users/jpm/take/training/bugs/fileglob/setvars.bat",
        "/Users/jpm/take/training/bugs/fileglob/files/json.c",
        "/Users/jpm/take/training/bugs/fileglob/files/strlcpy.c",
        "/Users/jpm/take/training/bugs/fileglob/files/base64.h",
        "/Users/jpm/take/training/bugs/fileglob/files/json.h",
        "/Users/jpm/take/training/bugs/fileglob/files/base64.c",
        "/Users/jpm/take/training/bugs/fileglob/files/strlcpy.h",
        "/Users/jpm/take/training/bugs/fileglob/files/jo.c"
    ]
}

TASK [debug] *******************************************************************
ok: [localhost] => {
    "msg": [
        "/Users/jpm/take/training/bugs/fileglob/files/json.c",
        "/Users/jpm/take/training/bugs/fileglob/files/strlcpy.c",
        "/Users/jpm/take/training/bugs/fileglob/files/base64.h",
        "/Users/jpm/take/training/bugs/fileglob/files/json.h",
        "/Users/jpm/take/training/bugs/fileglob/files/base64.c",
        "/Users/jpm/take/training/bugs/fileglob/files/strlcpy.h",
        "/Users/jpm/take/training/bugs/fileglob/files/jo.c"
    ]
}

PLAY RECAP *********************************************************************
localhost                  : ok=4    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   
```

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/plugins/lookup/fileglob.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/lookup/fileglob.py)

If these files are incorrect, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
This looks to be a bug in `fileglob`, specifically, here:

https://github.com/ansible/ansible/blob/6608f3aab338f9e4e871afe77ded7b69e50f23f6/lib/ansible/plugins/lookup/fileglob.py#L76-L81

In the broken case, the wildcard glob is being processed first, which makes `ret` contain a value. When `setvars.bat` is processed next, `glob.glob` will return `[]` for the first `found_path`, does not extend the return, and prematurely breaks out of the loop. We need an intermediary return structure.

Commit message: Fix fileglob parameter order bug

Patch to review:
diff --git a/changelogs/fragments/72873-fix-fileglob-ordering.yml b/changelogs/fragments/72873-fix-fileglob-ordering.yml
new file mode 100644
index 00000000000000..329befc9079be4
--- /dev/null
+++ b/changelogs/fragments/72873-fix-fileglob-ordering.yml
@@ -0,0 +1,2 @@
+bugfixes:
+- Fix fileglob bug where it could return different results for different order of parameters (https://github.com/ansible/ansible/issues/72873).
diff --git a/lib/ansible/plugins/lookup/fileglob.py b/lib/ansible/plugins/lookup/fileglob.py
index 84ae6312ff541e..9909361fb0fbab 100644
--- a/lib/ansible/plugins/lookup/fileglob.py
+++ b/lib/ansible/plugins/lookup/fileglob.py
@@ -59,6 +59,7 @@ def run(self, terms, variables=None, **kwargs):
 
         ret = []
         for term in terms:
+            term_results = []
             term_file = os.path.basename(term)
             found_paths = []
             if term_file != term:
@@ -76,7 +77,8 @@ def run(self, terms, variables=None, **kwargs):
             for dwimmed_path in found_paths:
                 if dwimmed_path:
                     globbed = glob.glob(to_bytes(os.path.join(dwimmed_path, term_file), errors='surrogate_or_strict'))
-                    ret.extend(to_text(g, errors='surrogate_or_strict') for g in globbed if os.path.isfile(g))
-                    if ret:
+                    term_results.extend(to_text(g, errors='surrogate_or_strict') for g in globbed if os.path.isfile(g))
+                    if term_results:
                         break
+            ret.extend(term_results)
         return ret
diff --git a/test/integration/targets/lookup_fileglob/issue72873/test.yml b/test/integration/targets/lookup_fileglob/issue72873/test.yml
new file mode 100644
index 00000000000000..218ee58d9035d1
--- /dev/null
+++ b/test/integration/targets/lookup_fileglob/issue72873/test.yml
@@ -0,0 +1,31 @@
+- hosts: localhost
+  connection: local
+  gather_facts: false
+  vars:
+     dir: files
+  tasks:
+  - file: path='{{ dir }}' state=directory
+  
+  - file: path='setvars.bat' state=touch  # in current directory!
+
+  - file: path='{{ dir }}/{{ item }}' state=touch
+    loop:
+        - json.c
+        - strlcpy.c
+        - base64.c
+        - json.h
+        - base64.h
+        - strlcpy.h
+        - jo.c
+
+  - name: Get working order results and sort them
+    set_fact:
+        working: '{{ query("fileglob", "setvars.bat", "{{ dir }}/*.[ch]") | sort }}'
+
+  - name: Get broken order results and sort them
+    set_fact:
+        broken: '{{ query("fileglob", "{{ dir }}/*.[ch]", "setvars.bat") | sort }}'
+
+  - assert:
+      that:
+        - working == broken
diff --git a/test/integration/targets/lookup_fileglob/runme.sh b/test/integration/targets/lookup_fileglob/runme.sh
index 1e0297c79d053e..be04421144f296 100755
--- a/test/integration/targets/lookup_fileglob/runme.sh
+++ b/test/integration/targets/lookup_fileglob/runme.sh
@@ -13,3 +13,6 @@ for seed in foo foo/bar foo/bar/baz
 do
 	ansible-playbook non_existent/play.yml -e "seed='${seed}'" "$@"
 done
+
+# test for issue 72873 fix
+ansible-playbook issue72873/test.yml "$@"


Code review comments:
diff_hunk: @@ -76,7 +77,8 @@ def run(self, terms, variables=None, **kwargs):
             for dwimmed_path in found_paths:
                 if dwimmed_path:
                     globbed = glob.glob(to_bytes(os.path.join(dwimmed_path, term_file), errors='surrogate_or_strict'))
-                    ret.extend(to_text(g, errors='surrogate_or_strict') for g in globbed if os.path.isfile(g))
-                    if ret:
+                    term_results.extend(to_text(g, errors='surrogate_or_strict') for g in globbed if os.path.isfile(g))
+                    if term_results:
path: lib/ansible/plugins/lookup/fileglob.py
text: @user1:
I'm not 100% sure this is correct, or at least maybe not exact enough, although it probably works correct.

You could probably just do:
```
term_results = [to_text(g, errors='surrogate_or_strict') for g in globbed if os.path.isfile(g)]
if term_results:
    ret.extend(term_results)
    break
```

@author:
Effectively, it's the same result. But your code is slightly more efficient with less lines, so I changed it.