Merge ~emitorino/review-tools:multi_value_attrs_in_decl into review-tools:master

[Emilia Torino]

- sr_declaration.py: add support for multi value attrs declarations
- test_sr_declaration.py: test cases to support the new functionality

The current code considers if the attribute in the constraint is a list, just one must match (it is
considered a list of alternatives, aka, a list of OR constraints).

As per https://bugs.launchpad.net/review-tools/+bug/1981637 review-tools needs to support a list of constraints that could be applied to a list of iface attr values. This MP fixes this issue.

Whats pending to do is to confirm if nested list attrs should be allowed. We can create a follow up MP to include it.

I added several test scenarios including the cases I could think of that should/should not be allowed after this change. Please suggest more that I could be missing so we make sure we dont break this important functionality.

The current test_sr_declaration.py already contains lots of test cases that keep passing with this change. If some should not anymore, please let me know.

[Alex Murray (alexmurray)]

LGTM - thanks for the detailed test cases.

review: Approve

[Samuele Pedroni (pedronis)]

question about more deep/recursive situations

[Emilia Torino (emitorino)]

Adding support for nested lists in declaration attrs

[Samuele Pedroni (pedronis)]

lgtm, wondering if the code can be simplified now though, as the code for lists _chec_attrib and the top loop do similar things ?

review: Approve

[Emilia Torino (emitorino)]

> lgtm, wondering if the code can be simplified now though, as the code for
> lists _chec_attrib and the top loop do similar things ?

Thanks for this comment. I indeed simplified the rules processing code but had to extend a bit more the _chec_attrib to support considering lists in "against" (rules) for non-lists in val (snap decl).

diff --git a/reviewtools/sr_declaration.py b/reviewtools/sr_declaration.py
index 58f5f3b..61bd313 100644
--- a/reviewtools/sr_declaration.py
+++ b/reviewtools/sr_declaration.py
@@ -811,61 +811,49 @@ class SnapReviewDeclaration(SnapReview):
             if type(val) not in [str, list, dict, bool]:
                 raise SnapDeclarationException("unknown type '%s'" % val)
 
-            # Keep in mind by this point each OR constraint is being iterated
-            # through such that 'against' as a list is a nested list.
-            # For now, punt on nested lists since they are impractical in use
-            # since they are a list of OR options where one option must match
-            # all of val, but if you have that you may as well just use a
-            # string. Eg for this snap.yaml:
-            #   snap.yaml:
-            #     plugs:
-            #       foo:
-            #         bar: [ baz, norf ]
-            #
-            # a snap decl that uses a list for 'bar' might be:
-            #   snap decl:
-            #     foo:
-            #       plug-attributes:
-            #         bar:
-            #         - baz|norf
-            #         - something|else
-            #
-            # but, 'something|else' is pointless so it will never match, so the
-            # decl should be rewritten more simply as:
-            #   snap decl:
-            #     foo:
-            #       plug-attributes:
-            #         bar: baz|norf
-            # Importantly, the type of the attribute in the snap decl means
-            # something different than the type in snap.yaml
-            if isinstance(against, list):
-                raise SnapDeclarationException(
-                    "attribute lists in the declaration not supported"
-                )
-
             matched = False
-            if isinstance(val, str) and isinstance(against, str):
-                if against.startswith("$"):
-                    if against == "$MISSING":
-                        matched = False  # value must not be set
-                    elif re.search(r"^\$PLUG\(%s\)$" % rules_attrib, against):
-                        matched = True
-                    elif re.search(r"^\$SLOT\(%s\)$" % rules_attrib, against):
+            if isinstance(val, str):
+                if isinstance(against, str):
+                    if against.startswith("$"):
+                        if against == "$MISSING":
+                            matched = False  # value must not be set
+                        elif re.search(r"^\$PLUG\(%s\)$" % rules_attrib, against):
+                            matched = True
+                        elif re.search(r"^\$SLOT\(%s\)$" % rules_attrib, against):
+                            matched = True
+                        else:
+                            raise SnapDeclarationException(
+                                "unknown special attrib '%s'" % against
+                            )
+                    elif re.search(r"^(%s)$" % against, val):
                         matched = True
-                    else:
-                        raise SnapDeclarationException(
-                            "unknown special attrib '%s'" % against
-                        )
-                elif re.search(r"^(%s)$" % against, val):
-                    matched = True
+                elif isinstance(against, list):
+                    for declaration_value in against:
+                        # if the attribute in the snap (val) is a string and the
+                        # declaration value (against) is a list, then to match,
+                        # val must match some entry in against
+                        if _check_attrib(val, declaration_value, side, rules_attrib):
+                            matched = True
+                            break
             elif isinstance(val, list):
-                # if the attribute in the snap (val) is a list and the
-                # declaration value (against) is a string, then to match,
-                # against must be a regex that matches all entries in val
                 num_matched = 0
-                for i in val:
-                    if _check_attrib(i, against, side, rules_attrib):
-                        num_matched += 1
+                for snap_attribute in val:
+                    if isinstance(against, str):
+                        # if the attribute in the snap (val) is a list and the
+                        # declaration value (against) is a string, then to match,
+                        # against must be a regex that matches all entries in val
+                        if _check_attrib(snap_attribute, against, side, rules_attrib):
+                            num_matched += 1
+                    elif isinstance(against, list):
+                        for declaration_value in against:
+                            # if the attribute in the snap (val) is a list and the
+                            # declaration value (against) is a list, then to match,
+                            # every item in against must match some entry in val
+                            if _check_attrib(
+                                snap_attribute, declaration_value, side, rules_attrib
+                            ):
+                                num_matched += 1
+                                break
                 if num_matched == len(val):
                     matched = True
             elif isinstance(val, dict):
@@ -880,6 +868,14 @@ class SnapReviewDeclaration(SnapReview):
                         )
                         if not matched:
                             break
+                elif isinstance(against, list):
+                    for declaration_value in against:
+                        # if the attribute in the snap (val) is a dict and the
+                        # declaration value (against) is a list, then to match,
+                        # val must match some entry in against
+                        if _check_attrib(val, declaration_value, side, rules_attrib):
+                            matched = True
+                            break
                 else:
                     matched = False
             else:  # bools
@@ -973,18 +969,8 @@ class SnapReviewDeclaration(SnapReview):
                     val = iface[rules_attrib]
                     against = rules[rules_key][rules_attrib]
 
-                    if isinstance(against, list):
-                        # As a practical matter, if the attribute in the
-                        # constraint is a list, just one must match (it is
-                        # considered a list of alternatives, aka, a list of
-                        # OR constraints).
-                        for i in against:
-                            if _check_attrib(val, i, side, rules_attrib):
-                                attributes_matched[rules_key]["matched"] += 1
-                                break
-                    else:
-                        if _check_attrib(val, against, side, rules_attrib):
-                            attributes_matched[rules_key]["matched"] += 1
+                    if _check_attrib(val, against, side, rules_attrib):
+                        attributes_matched[rules_key]["matched"] += 1
                 else:
                     # when the attribute is missing from the interface don't
                     # mark as checked (missing attributes are checked
diff --git a/reviewtools/tests/test_sr_declaration.py b/reviewtools/tests/test_sr_declaration.py
index 1b36992..38154f0 100644
--- a/reviewtools/tests/test_sr_declaration.py
+++ b/reviewtools/tests/test_sr_declaration.py
@@ -3892,6 +3892,403 @@ slots:
         }
         self.check_results(r, expected=expected)
 
+    def test_check_declaration_slots_multi_value_attrs_issue(self,):
+        review_ok_output = {
+            "error": dict(),
+            "warn": dict(),
+            "info": {
+                "declaration-snap-v2:slots:fram-devices:custom-device": {"text": "OK"}
+            },
+        }
+
+        review_err_output = {
+            "error": {
+                "declaration-snap-v2:slots_installation:fram-devices:custom-device": {
+                    "text": "human review required due to 'allow-installation' constraint (interface attributes)"
+                }
+            },
+            "warn": dict(),
+            "info": dict(),
+        }
+
+        multi_value_attrs_test_data = {
+            "iface_with_multiple_multi_value_attrs_and_multi_value_decl_matches": (
+                {
+                    "read-devices": [
+                        "/dev/chunkfs/geom",
+                        "/dev/chunkfs/stats",
+                        "/dev/chunkfs/sizes",
+                    ],
+                    "devices": ["/dev/mtd[0-9]", "/dev/chunkfs/chunkfs[0-2]"],
+                },
+                {
+                    "read-devices": [
+                        "/dev/chunkfs/geom",
+                        "/dev/chunkfs/stats",
+                        "/dev/chunkfs/sizes",
+                    ],
+                    "devices": ["/dev/mtd\\[0-9\\]", "/dev/chunkfs/chunkfs\\[0-2\\]"],
+                },
+                {"info": 1, "warn": 0, "error": 0},
+                review_ok_output,
+            ),
+            "iface_with_multiple_multi_value_attrs_and_multi_value_decl_different_order_matches": (
+                {
+                    "read-devices": [
+                        "/dev/chunkfs/stats",
+                        "/dev/chunkfs/sizes",
+                        "/dev/chunkfs/geom",
+                    ],
+                    "devices": ["/dev/chunkfs/chunkfs[0-2]", "/dev/mtd[0-9]"],
+                },
+                {
+                    "read-devices": [
+                        "/dev/chunkfs/geom",
+                        "/dev/chunkfs/stats",
+                        "/dev/chunkfs/sizes",
+                    ],
+                    "devices": ["/dev/mtd\\[0-9\\]", "/dev/chunkfs/chunkfs\\[0-2\\]"],
+                },
+                {"info": 1, "warn": 0, "error": 0},
+                review_ok_output,
+            ),
+            "iface_with_multiple_multi_value_attrs_and_single_value_list_decl_matches": (
+                {"devices": ["/dev/mtd1", "/dev/mtd2"]},
+                {"devices": ["/dev/mtd[0-9]"]},
+                {"info": 1, "warn": 0, "error": 0},
+                review_ok_output,
+            ),
+            "iface_with_single_value_attrs_and_single_value_list_decl_matches": (
+                {"devices": "/dev/mtd1"},
+                {"devices": ["/dev/mtd[0-9]"]},
+                {"info": 1, "warn": 0, "error": 0},
+                review_ok_output,
+            ),
+            "iface_with_single_value_attrs_and_single_value_decl_matches": (
+                {"devices": "/dev/mtd1"},
+                {"devices": "/dev/mtd[0-9]"},
+                {"info": 1, "warn": 0, "error": 0},
+                review_ok_output,
+            ),
+            "iface_with_one_multi_value_attrs_all_matching": (
+                {"devices": ["/dev/mtd[0-9]", "/dev/chunkfs/chunkfs[0-2]"]},
+                {"devices": ["/dev/mtd\\[0-9\\]", "/dev/chunkfs/chunkfs\\[0-2\\]"]},
+                {"info": 1, "warn": 0, "error": 0},
+                review_ok_output,
+            ),
+            "regex_in_list_of_single_attr_matches": (  # This case represents the current workaround
+                {
+                    "read-devices": [
+                        "/dev/chunkfs/geom",
+                        "/dev/chunkfs/stats",
+                        "/dev/chunkfs/sizes",
+                    ],
+                    "devices": ["/dev/mtd[0-9]", "/dev/chunkfs/chunkfs[0-2]"],
+                },
+                {
+                    "devices": ["(/dev/mtd\\[0-9\\]|/dev/chunkfs/chunkfs\\[0-2\\])"],
+                    "read-devices": [
+                        "(/dev/chunkfs/geom|/dev/chunkfs/stats|/dev/chunkfs/sizes)"
+                    ],
+                },
+                {"info": 1, "warn": 0, "error": 0},
+                review_ok_output,
+            ),
+            "extra_iface_attr_matches": (  # Even though this test passes here,
+                #  unknown attrs are detected as part _verify_declaration check
+                {
+                    "read-devices": [
+                        "/dev/chunkfs/geom",
+                        "/dev/chunkfs/stats",
+                        "/dev/chunkfs/sizes",
+                    ],
+                    "devices": ["/dev/mtd[0-9]", "/dev/chunkfs/chunkfs[0-2]"],
+                },
+                {
+                    "read-devices": [
+                        "/dev/chunkfs/geom",
+                        "/dev/chunkfs/stats",
+                        "/dev/chunkfs/sizes",
+                    ],
+                },
+                {"info": 1, "warn": 0, "error": 0},
+                review_ok_output,
+            ),
+            "less_items_in_iface_attr_list_but_all_matching_matches": (
+                {"read-devices": ["/dev/chunkfs/geom", "/dev/chunkfs/stats"]},
+                {
+                    "read-devices": [
+                        "/dev/chunkfs/geom",
+                        "/dev/chunkfs/stats",
+                        "/dev/chunkfs/sizes",
+                    ],
+                },
+                {"info": 1, "warn": 0, "error": 0},
+                review_ok_output,
+            ),
+            "missing_attr_in_iface_fails": (
+                {
+                    "read-devices": [
+                        "/dev/chunkfs/geom",
+                        "/dev/chunkfs/stats",
+                        "/dev/chunkfs/sizes",
+                    ],
+                },
+                {
+                    "read-devices": [
+                        "/dev/chunkfs/geom",
+                        "/dev/chunkfs/stats",
+                        "/dev/chunkfs/sizes",
+                    ],
+                    "devices": ["/dev/mtd\\[0-9\\]", "/dev/chunkfs/chunkfs\\[0-2\\]"],
+                },
+                {"info": 0, "warn": 0, "error": 1},
+                review_err_output,
+            ),
+            "no_matching_item_in_iface_attr_list_fails": (
+                {
+                    "read-devices": [
+                        "/dev/chunkfs/geom",
+                        "/dev/chunkfs/stats",
+                        "/dev/chunkfs/sizes",
+                    ]
+                },
+                {"read-devices": ["/dev/chunkfs/geom", "/dev/chunkfs/stats"]},
+                {"info": 0, "warn": 0, "error": 1},
+                review_err_output,
+            ),
+            "iface_with_single_value_attrs_and_multiple_value_decl_matches": (
+                {"devices": "/dev/mtd1"},
+                {"devices": ["/dev/mtd[0-9]", "/dev/chunkfs/chunkfs[0-2]"]},
+                {"info": 1, "warn": 0, "error": 0},
+                review_ok_output,
+            ),
+        }
+
+        for test, test_data in multi_value_attrs_test_data.items():
+            (
+                iface_attr_value,
+                rule_value,
+                expected_counts,
+                expected_output_data,
+            ) = test_data
+            with self.subTest(
+                iface_attr_value=iface_attr_value,
+                rule_value=iface_attr_value,
+                expected_counts=expected_counts,
+                expected_output_data=expected_output_data,
+            ):
+                iface_data = {"interface": "custom-device"}
+                iface_data.update(iface_attr_value)
+                slots = {"fram-devices": iface_data}
+                self.set_test_snap_yaml("slots", slots)
+                c = SnapReviewDeclaration(self.test_name)
+                base = {
+                    "slots": {
+                        "custom-device": {
+                            "allow-installation": {
+                                "slot-attributes": rule_value,
+                                "slot-names": ["fram-devices"],
+                            }
+                        }
+                    }
+                }
+                self._set_base_declaration(c, base)
+                c.check_declaration()
+                r = c.review_report
+                self.check_results(r, expected_counts)
+                self.check_results(r, expected=expected_output_data)
+
+    def test_check_declaration_slots_multi_value_attrs_issue_support_nested(self,):
+        review_ok_output = {
+            "error": dict(),
+            "warn": dict(),
+            "info": {"declaration-snap-v2:plugs:mnt:mount-control": {"text": "OK"}},
+        }
+
+        review_err_output = {
+            "error": {
+                "declaration-snap-v2:plugs_installation:mnt:mount-control": {
+                    "text": "human review required due to 'allow-installation' constraint (interface attributes)"
+                }
+            },
+            "warn": dict(),
+            "info": dict(),
+        }
+
+        multi_value_attrs_test_data = {
+            "iface_with_list_attrs_and_single_value_decl_list_matches": (
+                {
+                    "mnt": [
+                        {
+                            "what": "/dev/x*",
+                            "where": "/foo/*",
+                            "options": ["rw", "nodev"],
+                        },
+                        {
+                            "what": "/bar/*",
+                            "where": "/baz/*",
+                            "options": ["rw", "bind"],
+                        },
+                    ]
+                },
+                {
+                    "mnt": [
+                        {
+                            "what": r"/(bar/|dev/x)\*",
+                            "where": r"/(foo|baz)/\*",
+                            "options": "rw|bind|nodev",
+                        }
+                    ]
+                },
+                {"info": 1, "warn": 0, "error": 0},
+                review_ok_output,
+            ),
+            "iface_with_single_list_attrs_and_single_value_decl_list_matches": (
+                {
+                    "mnt": [
+                        {
+                            "what": "/dev/x*",
+                            "where": "/foo/*",
+                            "options": ["rw", "nodev"],
+                        }
+                    ]
+                },
+                {
+                    "mnt": [
+                        {
+                            "what": r"/(bar/|dev/x)\*",
+                            "where": r"/(foo|baz)/\*",
+                            "options": "rw|bind|nodev",
+                        }
+                    ]
+                },
+                {"info": 1, "warn": 0, "error": 0},
+                review_ok_output,
+            ),
+            "iface_with_list_attrs_and_multiple_value_decl_list_matches": (
+                {
+                    "mnt": [
+                        {
+                            "what": "/dev/x*",
+                            "where": "/foo/*",
+                            "options": ["rw", "nodev"],
+                        },
+                        {
+                            "what": "/bar/*",
+                            "where": "/baz/*",
+                            "options": ["rw", "bind"],
+                        },
+                    ]
+                },
+                {
+                    "mnt": [
+                        {
+                            "what": r"/dev/x\*",
+                            "where": r"/foo/\*",
+                            "options": ["nodev", "rw"],
+                        },
+                        {
+                            "what": r"/bar/\*",
+                            "where": r"/baz/\*",
+                            "options": ["rw", "bind"],
+                        },
+                    ]
+                },
+                {"info": 1, "warn": 0, "error": 0},
+                review_ok_output,
+            ),
+            "iface_with_list_attrs_and_multiple_value_decl_list_with_no_alternatives_fails": (
+                {
+                    "mnt": [
+                        {
+                            "what": "/dev/x*",
+                            "where": "/foo/*",
+                            "options": ["rw", "nodev"],
+                        },
+                        {
+                            "what": "/bar/*",
+                            "where": "/baz/*",
+                            "options": ["rw", "bind"],
+                        },
+                    ]
+                },
+                {
+                    "mnt": [
+                        {"what": r"/dev/x\*", "where": r"/foo/\*", "options": ["rw"]},
+                        {
+                            "what": r"/bar/\*",
+                            "where": r"/baz/\*",
+                            "options": ["rw", "bind"],
+                        },
+                    ]
+                },
+                {"info": 0, "warn": 0, "error": 1},
+                review_err_output,
+            ),
+            "iface_with_extra_list_attrs_and_multiple_value_decl_list_fails": (
+                {
+                    "mnt": [
+                        {
+                            "what": "/dev/x*",
+                            "where": "/foo/*",
+                            "options": ["rw", "nodev"],
+                        },
+                        {
+                            "what": "/bar/*",
+                            "where": "/baz/*",
+                            "foo_options": ["rw", "bind"],
+                        },
+                    ]
+                },
+                {
+                    "mnt": [
+                        {
+                            "what": r"/(bar/|dev/x)\*",
+                            "where": r"/(foo|baz)/\*",
+                            "options": "rw|bind|nodev",
+                        }
+                    ]
+                },
+                {"info": 0, "warn": 0, "error": 1},
+                review_err_output,
+            ),
+        }
+
+        for test, test_data in multi_value_attrs_test_data.items():
+            (
+                iface_attr_value,
+                rule_value,
+                expected_counts,
+                expected_output_data,
+            ) = test_data
+            with self.subTest(
+                iface_attr_value=iface_attr_value,
+                rule_value=iface_attr_value,
+                expected_counts=expected_counts,
+                expected_output_data=expected_output_data,
+            ):
+                iface_data = {"interface": "mount-control"}
+                iface_data.update(iface_attr_value)
+                plugs = {"mnt": iface_data}
+                self.set_test_snap_yaml("plugs", plugs)
+                c = SnapReviewDeclaration(self.test_name)
+                base = {
+                    "plugs": {
+                        "mount-control": {
+                            "allow-installation": {
+                                "plug-attributes": rule_value,
+                                "plug-names": ["mnt"],
+                            }
+                        }
+                    }
+                }
+                self._set_base_declaration(c, base)
+                c.check_declaration()
+                r = c.review_report
+                self.check_results(r, expected_counts)
+                self.check_results(r, expected=expected_output_data)
+
     def test_check_declaration_slots_deny_connection_attrib_list_match_unmatching_plug_names(
         self,
     ):
@@ -3973,7 +4370,7 @@ slots:
         expected["info"][name] = {"text": "OK"}
         self.check_results(r, expected=expected)
 
-    def test_check_declaration_slots_allow_connection_attrib_lists_exception(self):
+    def test_check_declaration_slots_allow_connection_attrib_lists_supported(self):
         """Test check_declaration - slots/allow-connection/attrib - lists match snap attribute list in decl list of lists"""
         slots = {"iface-foo": {"interface": "foo", "attrib1": ["b", "a"]}}
         self.set_test_snap_yaml("slots", slots)
@@ -3991,8 +4388,7 @@ slots:
         try:
             c.check_declaration()
         except SnapDeclarationException:
-            return
-        raise Exception("base declaration should be invalid")  # pragma: nocover
+            raise Exception("base declaration should not be invalid")  # pragma: nocover
 
     def test_check_declaration_slots_allow_connection_attrib_list_nomatch(self):
         """Test check_declaration - slots/allow-connection/attrib - list nomatch snap attribute str not in decl list"""