Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] RecursiveDictDiffer has a bug - TypeError: string indices must be integers #59017

Closed
ITJamie opened this issue Nov 25, 2020 · 8 comments · Fixed by #63213
Closed

[BUG] RecursiveDictDiffer has a bug - TypeError: string indices must be integers #59017

ITJamie opened this issue Nov 25, 2020 · 8 comments · Fixed by #63213
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Milestone

Comments

@ITJamie
Copy link
Contributor

ITJamie commented Nov 25, 2020

Description
It is possible to cause a python error with RecursiveDictDiffer with some simple example diffs

Setup
When migrating a new restconf module from a custom differ to the built in RecursiveDictDiffer I have managed to make RecursiveDictDiffer to throw a python error

Steps to Reproduce the behavior

test data (output from minon debug):

[DEBUG   ] existing:
[DEBUG   ] {}
[DEBUG   ] dict_config:
[DEBUG   ] {'example-jukebox:artist': {'name': 'New Artist'}}

code:

diff = RecursiveDictDiffer(existing, dict_config, False)
ret["changes"]["new"] = diff.added()

Error:
appears when i try to access the .added() data

    ret["changes"]["new"] = diff.added()
  File "/usr/lib/python3/dist-packages/salt/utils/dictdiffer.py", line 279, in added
    return sorted(_added(self._diffs, prefix=""))
  File "/usr/lib/python3/dist-packages/salt/utils/dictdiffer.py", line 272, in _added
    diffs[key]["new"], prefix="{0}{1}.".format(prefix, key)
  File "/usr/lib/python3/dist-packages/salt/utils/dictdiffer.py", line 268, in _added
    elif diffs[key]["old"] == self.NONE_VALUE:
TypeError: string indices must be integers

Expected behavior
A dict to be returned of the new keys added

Versions Report

Salt Version:
           Salt: 3001.3

Dependency Versions:
           cffi: Not Installed
       cherrypy: unknown
       dateutil: 2.6.1
      docker-py: Not Installed
          gitdb: 2.0.3
      gitpython: 2.1.8
         Jinja2: 2.10
        libgit2: 0.26.0
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: 3.4.7
         pygit2: 0.26.2
         Python: 3.6.9 (default, Oct  8 2020, 12:12:24)
   python-gnupg: 0.4.1
         PyYAML: 3.12
          PyZMQ: 17.1.2
          smmap: 2.0.3
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.5

System Versions:
           dist: ubuntu 18.04 Bionic Beaver
         locale: UTF-8
        machine: x86_64
        release: 4.15.0-72-generic
         system: Linux
        version: Ubuntu 18.04 Bionic Beaver
@ITJamie ITJamie added the Bug broken, incorrect, or confusing behavior label Nov 25, 2020
@ITJamie
Copy link
Contributor Author

ITJamie commented Nov 25, 2020

FYI @github-abcde as I saw you submitted the PR for this class

@ITJamie ITJamie changed the title [BUG] RecurisveDictDiffer has a bug [BUG] RecursiveDictDiffer has a bug Nov 25, 2020
@ITJamie ITJamie changed the title [BUG] RecursiveDictDiffer has a bug [BUG] RecursiveDictDiffer has a bug - TypeError: string indices must be integers Nov 25, 2020
@github-abcde
Copy link
Contributor

@ITJamie The PR I submitted was for this function: https://github.com/saltstack/salt/blob/master/salt/utils/data.py#L1319
That does not seem the same as the one your are having a problem with: https://github.com/saltstack/salt/blob/master/salt/utils/dictdiffer.py#L26
Maybe you could try my function instead ;-)

@ITJamie
Copy link
Contributor Author

ITJamie commented Nov 25, 2020

my bad!. I will indeed try that

@ITJamie
Copy link
Contributor Author

ITJamie commented Nov 25, 2020

@github-abcde sadly i reached a new bug with your code. 🤣

[DEBUG   ] existing_config:
[DEBUG   ] <class 'dict'>
[DEBUG   ] {'example-jukebox:artist': [{'name': 'Foo Fighters', 'album': [{'name': 'Wasting Light', 'genre': 'example-jukebox:alternative', 'year': 2011, 'song': [{'name': 'Wasting Light', 'location': '/media/foo/a7/wasting-light.mp3', 'format': 'MP3', 'length': 286}, {'name': 'Rope', 'location': '/media/foo/a7/rope.mp3', 'format': 'MP3', 'length': 259}]}]}]}
[DEBUG   ] proposed_config:
[DEBUG   ] <class 'dict'>
[DEBUG   ] {'example-jukebox:artist': {'name': 'New Artist'}}
[DEBUG   ] An exception occurred in this state: dictionary update sequence element #0 has length 4; 2 is required
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/salt/state.py", line 2154, in call
    *cdata["args"], **cdata["kwargs"]
  File "/usr/lib/python3/dist-packages/salt/loader.py", line 2087, in wrapper
    return f(*args, **kwargs)
  File "/var/cache/salt/proxy/extmods/states/restconf.py", line 152, in config_manage
    ret["changes"] = recursive_diff(uri_check[1]['request_restponse'], proposed_config, )
  File "/usr/lib/python3/dist-packages/salt/utils/data.py", line 1403, in recursive_diff
    ignore_missing_keys=ignore_missing_keys,
  File "/usr/lib/python3/dist-packages/salt/utils/data.py", line 1455, in recursive_diff
    if list_old or list_new
ValueError: dictionary update sequence element #0 has length 4; 2 is required
[ERROR   ] An exception occurred in this state: Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/salt/state.py", line 2154, in call
    *cdata["args"], **cdata["kwargs"]
  File "/usr/lib/python3/dist-packages/salt/loader.py", line 2087, in wrapper
    return f(*args, **kwargs)
  File "/var/cache/salt/proxy/extmods/states/restconf.py", line 152, in config_manage
    ret["changes"] = recursive_diff(uri_check[1]['request_restponse'], proposed_config, )
  File "/usr/lib/python3/dist-packages/salt/utils/data.py", line 1403, in recursive_diff
    ignore_missing_keys=ignore_missing_keys,
  File "/usr/lib/python3/dist-packages/salt/utils/data.py", line 1455, in recursive_diff
    if list_old or list_new
ValueError: dictionary update sequence element #0 has length 4; 2 is required

dealing with restconf is slowly killing my soul 👎

@github-abcde
Copy link
Contributor

This is caused by comparing differently structured data.
Note that example-jukebox:artist contains a list in the first data-element, and a dict in the second.
The code does not know how to compare a dict with a list (and is not handling this very well). I'll submit a PR to remedy this in recursive_diff, however I doubt that will help you much as it will then simply return your complete input (from my locally patched salt):

{'old': {'example-jukebox:artist': [{'name': 'Foo Fighters', 'album': [{'name': 'Wasting Light', 'genre': 'example-jukebox:alternative', 'year': 2011, 'song': [{'name': 'Wasting Light', 'location': '/media/foo/a7/wasting-light.mp3', 'format': 'MP3', 'length': 286}, {'name': 'Rope', 'location': '/media/foo/a7/rope.mp3', 'format': 'MP3', 'length': 259}]}]}]}, 'new': {'example-jukebox:artist': {'name': 'New Artist'}}}```

@sagetherage sagetherage assigned s0undt3ch and unassigned Akm0d Feb 9, 2021
@sagetherage sagetherage assigned sagetherage and unassigned s0undt3ch Mar 2, 2021
@sagetherage sagetherage added severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around and removed needs-triage labels Apr 6, 2021
@sagetherage sagetherage added this to the Approved milestone Apr 6, 2021
@sagetherage
Copy link
Contributor

@github-abcde do you have time to submit a PR on this? We will gladly review, thank you!

@github-abcde
Copy link
Contributor

I should have the change still lying around somewhere, will try to find time to submit it in a PR.

@lkubb
Copy link
Contributor

lkubb commented Mar 23, 2022

Discovered this behavior regarding added/removed nested dictionaries as well. PoC:

import salt.utils.dictdiffer as dd

initial = {
    'foo': 'bar'
}

new_works = {
    'foo': 'bar',
    'baz': True
}

new_fails = {
    'nested': {
        'baz': True,
    }
}

works = dd.recursive_diff(initial, new_works)
print(works.diffs)
print(works.added())

fails = dd.recursive_diff(initial, new_fails)
print(fails.diffs)
print(fails.added())
{'baz': {'new': True, 'old': '<_null_>'}}
['baz']
{'nested': {'new': {'baz': True}, 'old': '<_null_>'}}
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "salt/salt/utils/dictdiffer.py", line 269, in added
    return sorted(_added(self._diffs, prefix=""))
  File "salt/salt/utils/dictdiffer.py", line 261, in _added
    _added(
  File "salt/salt/utils/dictdiffer.py", line 258, in _added
    elif diffs[key]["old"] == self.NONE_VALUE:
TypeError: 'bool' object is not subscriptable

Knowing about salt.utils.data.recursive_diff was helpful, although I discovered it too late. Fwiw, here are two patched variants of salt.utils.dictdiffer.RecursiveDictDiffer that behave as expected.

The first is just a slight modification and skips over nested dicts, only including the new parent key:

class PatchedRecursiveDiffer(salt.utils.dictdiffer.RecursiveDictDiffer):
    def added(self):
        def _added(diffs, prefix):
            keys = []
            for key in diffs.keys():
                if isinstance(diffs[key], dict):
                    if "old" not in diffs[key]:
                        keys.extend(_added(diffs[key], prefix="{}{}.".format(prefix, key)))
                    elif diffs[key]["old"] == self.NONE_VALUE:
                        keys.append("{}{}".format(prefix, key))
            return keys

        return sorted(_added(self._diffs, prefix=""))

    def removed(self):
        def _removed(diffs, prefix):
            keys = []
            for key in diffs.keys():
                if isinstance(diffs[key], dict):
                    if "old" not in diffs[key]:
                        keys.extend(
                            _removed(diffs[key], prefix="{}{}.".format(prefix, key))
                        )
                    elif diffs[key]["new"] == self.NONE_VALUE:
                        keys.append("{}{}".format(prefix, key))
            return keys

        return sorted(_removed(self._diffs, prefix=""))

Output using the same data as PoC for failing statement:

['nested']

The second one is quite ugly, but includes the possibility to return nested keys as well:

class PatchedRecursiveDiffer(salt.utils.dictdiffer.RecursiveDictDiffer):
    def added(self, include_nested=False):
        return sorted(self._it("old", "new", include_nested))

    def removed(self, include_nested=False):
        return sorted(self._it("new", "old", include_nested))

    def _it(
        self, key_a, key_b, include_nested=False, diffs=None, prefix="", is_nested=False
    ):
        keys = []
        if diffs is None:
            diffs = self.diffs

        for key in diffs.keys():
            if is_nested:
                keys.append("{}{}".format(prefix, key))

            if not isinstance(diffs[key], dict):
                continue

            if is_nested:
                keys.extend(
                    self._it(
                        key_a,
                        key_b,
                        diffs=diffs[key],
                        prefix="{}{}.".format(prefix, key),
                        is_nested=is_nested,
                        include_nested=include_nested,
                    )
                )
            elif "old" not in diffs[key]:
                keys.extend(
                    self._it(
                        key_a,
                        key_b,
                        diffs=diffs[key],
                        prefix="{}{}.".format(prefix, key),
                        is_nested=is_nested,
                        include_nested=include_nested,
                    )
                )
            elif diffs[key][key_a] == self.NONE_VALUE:
                keys.append("{}{}".format(prefix, key))

                if isinstance(diffs[key][key_b], dict) and include_nested:
                    keys.extend(
                        self._it(
                            key_a,
                            key_b,
                            diffs=diffs[key][key_b],
                            is_nested=True,
                            prefix="{}{}.".format(prefix, key),
                            include_nested=include_nested,
                        )
                    )
        return keys

Output using the same data as PoC for failing statement, passing include_nested=False/True:

['nested']
['nested', 'nested.baz']

This works for arbitrarily nested added/removed dictionaries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants