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.

Sign up for GitHub

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

Enhance find_json garbage filtering #688

Open
wants to merge 2 commits into
base: openSUSE/release/3006.0
Choose a base branch
from
Open

Enhance find_json garbage filtering #688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
992906a
Enhance find_json garbage filtering
m-czernek Oct 31, 2024
b86c08c
Enhance error handling in transactional_update module
m-czernek Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion salt/modules/transactional_update.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -984,7 +984,7 @@ def call(function, *args, **kwargs):
return local.get("return", local)
else:
return local
except ValueError:
except (ValueError, AttributeError):
return {"result": False, "retcode": 1, "comment": ret_stdout}
finally:
# Check if reboot is needed
Expand Down
12 changes: 10 additions & 2 deletions salt/utils/json.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ def find_json(raw):
# Search for possible starts end ends of the json fragments
for ind, _ in enumerate(lines):
line = lines[ind].lstrip()
line = line[0] if line else line
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm bit confused with this part, maybe there are no such examples, but for me it looks like it can work with only pretty formated json. It's confusing because looks like it just takes only first symbol from the string with ignoring the rest part completely.

Copy link
Contributor Author

@m-czernek m-czernek Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does work with pretty formatted JSONs only indeed. I don't think json.find_json supports unformatted JSONS - except for the best-effort search at the end, which just filters the text before it and then tries to load whatever is left.

Our implementation that you authored definitely doesn't support unformatted JSONs:

    for ind, _ in enumerate(lines):
        line = lines[ind].lstrip()
        line = line[0] if line else line
        if line == "{" or line == "[": # <-- this will never work for unformatted JSON
            starts.append((ind, line))
        if line == "}" or line == "]": # <-- this will never work for unformatted JSON
            ends.append((ind, line))

In other words, if I'm not mistaken, we didn't support unformatted JSONs before, and we still don't, right?

if line == "{" or line == "[":
starts.append((ind, line))
if line == "}" or line == "]":
Expand All @@ -61,10 +62,17 @@ def find_json(raw):
working = "\n".join(lines[start : end + 1])
try:
ret = json.loads(working)
return ret
except ValueError:
continue
if ret:
pass
# Try filtering non-JSON text right after the last closing curly brace
end_str = lines[end].lstrip()[0]
working = "\n".join(lines[start : end]) + end_str
try:
ret = json.loads(working)
return ret
except ValueError:
continue

# Fall back to old implementation for backward compatibility
# excpecting json after the text
Expand Down
5 changes: 5 additions & 0 deletions tests/unit/utils/test_json.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ def test_find_json(self):
ret = salt.utils.json.find_json(garbage_prepend_json)
self.assertDictEqual(ret, expected_ret)

# Pre-pend garbage right after closing bracket of the JSON
garbage_prepend_json = "{}{}".format(test_sample_json.rstrip(), LOREM_IPSUM)
ret = salt.utils.json.find_json(garbage_prepend_json)
self.assertDictEqual(ret, expected_ret)

# Test to see if a ValueError is raised if no JSON is passed in
self.assertRaises(ValueError, salt.utils.json.find_json, LOREM_IPSUM)

Expand Down