Repository: ansible/ansible
Pull request: test: Update regex for required and default in FieldAttributes
##### SUMMARY

Fixes: #61460

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>

##### ISSUE TYPE
- Bugfix Pull Request




Problem statement:
False positive on required-and-default-attributes sanity test
##### SUMMARY
The required-and-default-attributes sanity test is somehow giving a false positive on a FieldAttribute that only specifies `default`.

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
required-and-default-attributes sanity test

##### ANSIBLE VERSION
2.9.0.dev0

##### CONFIGURATION

##### OS / ENVIRONMENT


##### STEPS TO REPRODUCE
Add a `default` to the collections FIeldAttribute in `lib/ansible/playbook/collectionsearch.py` (which does not specify `required`)

##### EXPECTED RESULTS
No error from the sanity test.

##### ACTUAL RESULTS
```
ERROR: Found 1 required-and-default-attributes issue(s) which need to be resolved:
ERROR: lib/ansible/playbook/collectionsearch.py:35:20: use only one of `default` or `required` with `FieldAttribute`
ERROR: The 1 sanity test(s) listed below (out of 1) failed. See error output above for details.
required-and-default-attributes
```

Hints from issue discussion:
Files identified in the description:
* [`test/sanity/code-smell/required-and-default-attributes.py`](https://github.com/ansible/ansible/blob/devel/test/sanity/code-smell/required-and-default-attributes.py)

If these files are inaccurate, 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 --->
cc @mattclay
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
The regex:

```
(FieldAttribute.*(default|required).*(default|required))
```

Is matching because the `default` is a variable that contains the word `default`:

```
default=_ensure_default_collection
```

```
>>> match.groups()
("FieldAttribute(isa='list', listof=string_types, priority=100, default=_ensure_default",
 'default',
 'default')
```
Might be able to make it better by adding `=` to the regex:

```
(FieldAttribute.*(default|required)=.*(default|required)=)
```

Commit message: Review requests

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>

Patch to review:
diff --git a/changelogs/fragments/required-and-default.yml b/changelogs/fragments/required-and-default.yml
new file mode 100644
index 00000000000000..86f9d02bf6a5f2
--- /dev/null
+++ b/changelogs/fragments/required-and-default.yml
@@ -0,0 +1,3 @@
+---
+bugfixes:
+  - tests - update regex for detecting default and required values in FieldAttributes (https://github.com/ansible/ansible/issues/61460).
diff --git a/test/sanity/code-smell/required-and-default-attributes.py b/test/sanity/code-smell/required-and-default-attributes.py
index 900829dce7409e..b2635f674d2424 100644
--- a/test/sanity/code-smell/required-and-default-attributes.py
+++ b/test/sanity/code-smell/required-and-default-attributes.py
@@ -1,19 +1,31 @@
 from __future__ import annotations
 
-import re
 import sys
+import ast
+
+
+class CallVisitor(ast.NodeVisitor):
+    def __init__(self, path):
+        self.path = path
+
+    def visit_Call(self, node):
+        if getattr(node.func, "id", "").endswith("FieldAttribute"):
+            count = 0
+            for kw in node.keywords:
+                if kw.arg in ("default", "required"):
+                    count += 1
+            if count > 1:
+                print(
+                    f"{self.path}:{node.lineno}:{node.col_offset}: use only one of `default` or `required` with `{node.func.id}`"
+                )
 
 
 def main():
     for path in sys.argv[1:] or sys.stdin.read().splitlines():
-        with open(path, 'r') as path_fd:
-            for line, text in enumerate(path_fd.readlines()):
-                match = re.search(r'(FieldAttribute.*(default|required).*(default|required))', text)
-
-                if match:
-                    print('%s:%d:%d: use only one of `default` or `required` with `FieldAttribute`' % (
-                        path, line + 1, match.start(1) + 1))
+        with open(path, "r") as path_fd:
+            tree = ast.parse(path_fd.read())
+            CallVisitor(path).visit(tree)
 
 
-if __name__ == '__main__':
+if __name__ == "__main__":
     main()
diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt
index 5736094ef8d810..79873410f54ec1 100644
--- a/test/sanity/ignore.txt
+++ b/test/sanity/ignore.txt
@@ -51,7 +51,6 @@ lib/ansible/module_utils/six/__init__.py pylint:unidiomatic-typecheck
 lib/ansible/module_utils/six/__init__.py replace-urlopen
 lib/ansible/module_utils/urls.py replace-urlopen
 lib/ansible/parsing/yaml/objects.py pylint:arguments-renamed
-lib/ansible/playbook/collectionsearch.py required-and-default-attributes  # https://github.com/ansible/ansible/issues/61460
 lib/ansible/playbook/role/include.py pylint:arguments-renamed
 lib/ansible/plugins/action/normal.py action-plugin-docs # default action plugin for modules without a dedicated action plugin
 lib/ansible/plugins/cache/base.py ansible-doc!skip  # not a plugin, but a stub for backwards compatibility


Code review comments:
diff_hunk: @@ -0,0 +1,3 @@
+---
+bugfixes:
+  - tests - update regex for detecting default and required values in FieldAttributes (https://github.com/ansible/ansible/issues/61460).
path: changelogs/fragments/required-and-default.yml
text: @user1:
Changelog entries shouldn't be created for changes to core-only sanity tests.

diff_hunk: @@ -1,19 +1,31 @@
 from __future__ import annotations
 
-import re
 import sys
+import ast
+
+
+class CallVisitor(ast.NodeVisitor):
+    def __init__(self, path):
+        self.path = path
+
+    def visit_Call(self, node):
+        if getattr(node.func, "id", "").endswith("FieldAttribute"):
+            count = 0
+            for kw in node.keywords:
+                if kw.arg in ("default", "required"):
+                    count += 1
+            if count > 1:
+                print(
+                    f"{self.path}:{node.lineno}:{node.col_offset}: use only one of `default` or `required` with `{node.func.id}`"
+                )
path: test/sanity/code-smell/required-and-default-attributes.py
text: @user1:
Since we're reworking this test, there are several other things that can be improved as well:

* sorting imports
* including type annotations
* passing the path to `ast.parse` for more helpful error messages
* using `pathlib` to simplify file access
* using `isinstance` instead of attribute checks
* using a comprehension to simplify the logic

With those changes we'd end up with something like this:

```python
from __future__ import annotations

import ast
import pathlib
import sys


class CallVisitor(ast.NodeVisitor):
    def __init__(self, path: str) -> None:
        self.path = path

    def visit_Call(self, node: ast.Call) -> None:
        if isinstance(node.func, ast.Name) and node.func.id.endswith("FieldAttribute"):
            if len([kw for kw in node.keywords if kw.arg in ("default", "required")]) > 1:
                print(f"{self.path}:{node.lineno}:{node.col_offset}: use only one of `default` or `required` with `{node.func.id}`")


def main() -> None:
    for path in sys.argv[1:] or sys.stdin.read().splitlines():
        tree = ast.parse(pathlib.Path(path).read_text(), path)
        CallVisitor(path).visit(tree)


if __name__ == "__main__":
    main()
```