fix various bugs in list_present

fixes #31427
fixes #39875 (previous solution was incomplete)

Trying to convert the list and sub-objects into a set caused various
issues when checking for existance. Use a proper loop and `in` checks to
provide a more correct behavior.

Author: Sxderp

changelog/31427.fixed.md

@@ -0,0 +1,3 @@
+Fixed an issue with how existing entries are tracked in grains.list\_present. Previous entries were only considered if
+the grain previously exited. If not then the state would not "see" the duplicates. Removed the dubious tracking via
+"context" and focused on using checking for existance in the live grains.

changelog/39875.fixed.md

@@ -0,0 +1 @@
+Fixed issue with complex objects in grains.list\_present. Original fix #52710 did not fully address the problem.

salt/states/grains.py

@@ -38,30 +38,6 @@ def exists(name, delimiter=DEFAULT_TARGET_DELIM):
     return ret
 
 
-def make_hashable(list_grain, result=None):
-    """
-    Ensure that a list grain is hashable.
-
-    list_grain
-        The list grain that should be hashable
-
-    result
-        This function is recursive, so it must be possible to use a
-        sublist as parameter to the function. Should not be used by a caller
-        outside of the function.
-
-    Make it possible to compare two list grains to each other if the list
-    contains complex objects.
-    """
-    result = result or set()
-    for sublist in list_grain:
-        if type(sublist) == list:
-            make_hashable(sublist, result)
-        else:
-            result.add(frozenset(sublist))
-    return result
-
-
 def present(name, value, delimiter=DEFAULT_TARGET_DELIM, force=False):
     """
     Ensure that a grain is set
@@ -185,30 +161,32 @@ def list_present(name, value, delimiter=DEFAULT_TARGET_DELIM):
             ret["comment"] = f"Grain {name} is not a valid list"
             return ret
         if isinstance(value, list):
-            if make_hashable(value).issubset(
-                make_hashable(__salt__["grains.get"](name))
-            ):
+            # An older implementation tried to convert everything to a set. Information was lost
+            # during that conversion. It even converted str to set! Trying to add "racer" if
+            # "racecar" was already present would fail. Additionaly, dictionary values would also
+            # be lost. Trying to add {"foo": "bar"} and {"foo": "baz"} would also fail.
+            # While potentially slower, the only valid option is to actually check for existance
+            # within the existing grain.
+            # Hopefully the performance penality is reasonable in the effort for correctness.
+            # Very very very large grain lists will be probematic..
+            intersection = []
+            difference = []
+            for new_value in value:
+                if new_value in grain:
+                    intersection.append(new_value)
+                else:
+                    difference.append(new_value)
+            if difference:
+                value = difference
+            else:
                 ret["comment"] = f"Value {value} is already in grain {name}"
                 return ret
-            elif name in __context__.get("pending_grains", {}):
-                # elements common to both
-                intersection = set(value).intersection(
-                    __context__.get("pending_grains", {})[name]
-                )
-                if intersection:
-                    value = list(
-                        set(value).difference(__context__["pending_grains"][name])
-                    )
-                    ret["comment"] = (
-                        'Removed value {} from update due to context found in "{}".\n'.format(
-                            value, name
-                        )
+            if intersection:
+                ret["comment"] = (
+                    'Removed value {} from update due to value found in "{}".\n'.format(
+                        intersection, name
                     )
-            if "pending_grains" not in __context__:
-                __context__["pending_grains"] = {}
-            if name not in __context__["pending_grains"]:
-                __context__["pending_grains"][name] = set()
-            __context__["pending_grains"][name].update(value)
+                )
         else:
             if value in grain:
                 ret["comment"] = f"Value {value} is already in grain {name}"
@@ -227,17 +205,21 @@ def list_present(name, value, delimiter=DEFAULT_TARGET_DELIM):
         ret["changes"] = {"new": grain}
         return ret
     new_grains = __salt__["grains.append"](name, value)
+    # TODO: Rather than fetching the grains again can we just rely on the status of grains.append?
+    actual_grains = __salt__["grains.get"](name, [])
+    # Previous implementation tried to use a set for comparison. See above notes on why that is
+    # not good.
     if isinstance(value, list):
-        if not set(value).issubset(set(__salt__["grains.get"](name))):
-            ret["result"] = False
-            ret["comment"] = f"Failed append value {value} to grain {name}"
-            return ret
-    else:
-        if value not in __salt__["grains.get"](name, delimiter=DEFAULT_TARGET_DELIM):
-            ret["result"] = False
-            ret["comment"] = f"Failed append value {value} to grain {name}"
-            return ret
-    ret["comment"] = f"Append value {value} to grain {name}"
+        for new_value in value:
+            if new_value not in actual_grains:
+                ret["result"] = False
+                ret["comment"] = f"Failed append value {value} to grain {name}"
+                return ret
+    elif value not in actual_grains:
+        ret["result"] = False
+        ret["comment"] = f"Failed append value {value} to grain {name}"
+        return ret
+    ret["comment"] = "{0}Append value {1} to grain {2}".format(ret.get("comment", ""), value, name)
     ret["changes"] = {"new": new_grains}
     return ret
 

tests/pytests/unit/states/test_grains.py

@@ -73,15 +73,6 @@ def test_exists_found():
         assert ret["comment"] == "Grain exists"
         assert ret["changes"] == {}
 
-    # 'make_hashable' function tests: 1
-
-
-def test_make_hashable():
-    with set_grains({"cmplx_lst_grain": [{"a": "aval"}, {"foo": "bar"}]}):
-        hashable_list = {"cmplx_lst_grain": [{"a": "aval"}, {"foo": "bar"}]}
-        assert grains.make_hashable(grains.__grains__).issubset(
-            grains.make_hashable(hashable_list)
-        )
 
     # 'present' function tests: 12
 
@@ -643,7 +634,7 @@ def test_append_convert_to_list_empty():
         assert_grain_file_content("foo:\n- baz\n")
 
 
-# 'list_present' function tests: 7
+# 'list_present' function tests: 14
 
 
 def test_list_present():
@@ -721,6 +712,109 @@ def test_list_present_already():
         assert_grain_file_content("a: aval\nfoo:\n- bar\n")
 
 
+def test_list_present_complex_present():
+    with set_grains({"a": [{"foo": "bar"}]}):
+        ret = grains.list_present(name="a", value=[{"foo": "bar"}, {"foo": "baz"}])
+        assert grains.__grains__ == {"a": [{"foo": "bar"}, {"foo": "baz"}]}
+        assert ret["result"] is True
+        assert ret["comment"] == (
+            "Removed value [{'foo': 'bar'}] from update due to value found in \"a\".\n"
+            "Append value [{'foo': 'baz'}] to grain a"
+        )
+        assert ret["changes"] == {"new": {"a": [{"foo": "bar"}, {"foo": "baz"}]}}
+        assert_grain_file_content("a:\n- foo: bar\n- foo: baz\n")
+
+
+def test_list_present_racecar_present():
+    with set_grains({"foo": ["racecar"]}):
+        ret = grains.list_present(name="foo", value=["racer"])
+        assert grains.__grains__ == {"foo": ["racecar", "racer"]}
+        assert ret["result"] is True
+        assert ret["comment"] == "Append value ['racer'] to grain foo"
+        assert ret["changes"] == {"new": {"foo": ["racecar", "racer"]}}
+        assert_grain_file_content("foo:\n- racecar\n- racer\n")
+
+
+def test_list_present_multiple_calls_empty_as_str():
+    with set_grains({}):
+        # First Entry
+        ret = grains.list_present(name="foo", value="racecar")
+        assert grains.__grains__ == {"foo": ["racecar"]}
+        assert ret["result"] is True
+        assert ret["comment"] == "Append value racecar to grain foo"
+        assert ret["changes"] == {"new": {"foo": ["racecar"]}}
+
+        # Second Entry
+        ret = grains.list_present(name="foo", value=["racecar", "taxi"])
+        assert grains.__grains__ == {"foo": ["racecar", "taxi"]}
+        assert ret["result"] is True
+        assert ret["comment"] == (
+            "Removed value ['racecar'] from update due to value found in \"foo\".\n"
+            "Append value ['taxi'] to grain foo"
+        )
+        assert ret["changes"] == {"new": {"foo": ["racecar", "taxi"]}}
+
+
+def test_list_present_multiple_calls_empty_as_list():
+    with set_grains({}):
+        # First Entry
+        ret = grains.list_present(name="foo", value=["racecar"])
+        assert grains.__grains__ == {"foo": ["racecar"]}
+        assert ret["result"] is True
+        assert ret["comment"] == "Append value ['racecar'] to grain foo"
+        assert ret["changes"] == {"new": {"foo": ["racecar"]}}
+
+        # Second Entry
+        ret = grains.list_present(name="foo", value=["racecar", "taxi"])
+        assert grains.__grains__ == {"foo": ["racecar", "taxi"]}
+        assert ret["result"] is True
+        assert ret["comment"] == (
+            "Removed value ['racecar'] from update due to value found in \"foo\".\n"
+            "Append value ['taxi'] to grain foo"
+        )
+        assert ret["changes"] == {"new": {"foo": ["racecar", "taxi"]}}
+
+
+def test_list_present_multiple_calls_present_as_str():
+    with set_grains({"foo": ["nascar"]}):
+        # First Entry
+        ret = grains.list_present(name="foo", value="racecar")
+        assert grains.__grains__ == {"foo": ["nascar", "racecar"]}
+        assert ret["result"] is True
+        assert ret["comment"] == "Append value racecar to grain foo"
+        assert ret["changes"] == {"new": {"foo": ["nascar", "racecar"]}}
+
+        # Second Entry
+        ret = grains.list_present(name="foo", value=["racecar", "taxi"])
+        assert grains.__grains__ == {"foo": ["nascar", "racecar", "taxi"]}
+        assert ret["result"] is True
+        assert ret["comment"] == (
+            "Removed value ['racecar'] from update due to value found in \"foo\".\n"
+            "Append value ['taxi'] to grain foo"
+        )
+        assert ret["changes"] == {"new": {"foo": ["nascar", "racecar", "taxi"]}}
+
+
+def test_list_present_multiple_calls_present_as_list():
+    with set_grains({"foo": ["nascar"]}):
+        # First Entry
+        ret = grains.list_present(name="foo", value=["racecar"])
+        assert grains.__grains__ == {"foo": ["nascar", "racecar"]}
+        assert ret["result"] is True
+        assert ret["comment"] == "Append value ['racecar'] to grain foo"
+        assert ret["changes"] == {"new": {"foo": ["nascar", "racecar"]}}
+
+        # Second Entry
+        ret = grains.list_present(name="foo", value=["racecar", "taxi"])
+        assert grains.__grains__ == {"foo": ["nascar", "racecar", "taxi"]}
+        assert ret["result"] is True
+        assert ret["comment"] == (
+            "Removed value ['racecar'] from update due to value found in \"foo\".\n"
+            "Append value ['taxi'] to grain foo"
+        )
+        assert ret["changes"] == {"new": {"foo": ["nascar", "racecar", "taxi"]}}
+
+
 def test_list_present_unknown_failure():
     with set_grains({"a": "aval", "foo": ["bar"]}):
         # Unknown reason failure