AddonInfo: const-ified loading and improved errorhandling (#5834)

Author: firewave

lib/addoninfo.cpp

@@ -52,47 +52,73 @@ static std::string parseAddonInfo(AddonInfo& addoninfo, const picojson::value &j
         return "Loading " + fileName + " failed. " + json_error;
     }
     if (!json.is<picojson::object>())
-        return "Loading " + fileName + " failed. Bad json.";
-    picojson::object obj = json.get<picojson::object>();
-    if (obj.count("args")) {
-        if (!obj["args"].is<picojson::array>())
-            return "Loading " + fileName + " failed. args must be array.";
-        for (const picojson::value &v : obj["args"].get<picojson::array>())
-            addoninfo.args += " " + v.get<std::string>();
-    }
+        return "Loading " + fileName + " failed. JSON is not an object.";
 
-    if (obj.count("ctu")) {
-        // ctu is specified in the config file
-        if (!obj["ctu"].is<bool>())
-            return "Loading " + fileName + " failed. ctu must be boolean.";
-        addoninfo.ctu = obj["ctu"].get<bool>();
-    } else {
-        addoninfo.ctu = false;
+    // TODO: remove/complete default value handling for missing fields
+    const picojson::object& obj = json.get<picojson::object>();
+    {
+        const auto it = obj.find("args");
+        if (it != obj.cend()) {
+            const auto& val = it->second;
+            if (!val.is<picojson::array>())
+                return "Loading " + fileName + " failed. 'args' must be an array.";
+            for (const picojson::value &v : val.get<picojson::array>()) {
+                if (!v.is<std::string>())
+                    return "Loading " + fileName + " failed. 'args' entry is not a string.";
+                addoninfo.args += " " + v.get<std::string>();
+            }
+        }
     }
 
-    if (obj.count("python")) {
-        // Python was defined in the config file
-        if (obj["python"].is<picojson::array>()) {
-            return "Loading " + fileName +" failed. python must not be an array.";
+    {
+        const auto it = obj.find("ctu");
+        if (it != obj.cend()) {
+            const auto& val = it->second;
+            // ctu is specified in the config file
+            if (!val.is<bool>())
+                return "Loading " + fileName + " failed. 'ctu' must be a boolean.";
+            addoninfo.ctu = val.get<bool>();
+        }
+        else {
+            addoninfo.ctu = false;
         }
-        addoninfo.python = obj["python"].get<std::string>();
-    } else {
-        addoninfo.python = "";
     }
 
-    if (obj.count("executable")) {
-        if (!obj["executable"].is<std::string>())
-            return "Loading " + fileName + " failed. executable must be a string.";
-        addoninfo.executable = getFullPath(obj["executable"].get<std::string>(), fileName);
-        return "";
+    {
+        const auto it = obj.find("python");
+        if (it != obj.cend()) {
+            const auto& val = it->second;
+            // Python was defined in the config file
+            if (!val.is<std::string>()) {
+                return "Loading " + fileName +" failed. 'python' must be a string.";
+            }
+            addoninfo.python = val.get<std::string>();
+        }
+        else {
+            addoninfo.python = "";
+        }
     }
 
-    if (!obj.count("script") || !obj["script"].is<std::string>())
     {
-        return "Loading " + fileName + " failed. script must be set to a string value.";
+        const auto it = obj.find("executable");
+        if (it != obj.cend()) {
+            const auto& val = it->second;
+            if (!val.is<std::string>())
+                return "Loading " + fileName + " failed. 'executable' must be a string.";
+            addoninfo.executable = getFullPath(val.get<std::string>(), fileName);
+            return ""; // TODO: why bail out?
+        }
     }
 
-    return addoninfo.getAddonInfo(obj["script"].get<std::string>(), exename);
+    const auto it = obj.find("script");
+    if (it == obj.cend())
+        return "Loading " + fileName + " failed. 'script' is missing.";
+
+    const auto& val = it->second;
+    if (!val.is<std::string>())
+        return "Loading " + fileName + " failed. 'script' must be a string.";
+
+    return addoninfo.getAddonInfo(val.get<std::string>(), exename);
 }
 
 std::string AddonInfo::getAddonInfo(const std::string &fileName, const std::string &exename) {

test/cli/test-other.py

@@ -4,6 +4,7 @@
 import os
 import sys
 import pytest
+import json
 
 from testutils import cppcheck, assert_cppcheck
 
@@ -278,22 +279,6 @@ def test_addon_threadsafety(tmpdir):
     assert stderr == '{}:4:12: warning: strerror is MT-unsafe [threadsafety-unsafe-call]\n'.format(test_file)
 
 
-def test_addon_invalidjson(tmpdir):
-    addon_file = os.path.join(tmpdir, 'invalid.json')
-    with open(addon_file, 'wt') as f:
-        f.write("""
-{
-    "Script": "addons/something.py"
-}
-                """)
-
-    args = ['--addon={}'.format(addon_file), '--enable=all', 'nonexistent.cpp']
-
-    exitcode, stdout, stderr = cppcheck(args)
-    assert exitcode != 0
-    assert stdout == 'Loading {} failed. script must be set to a string value.\n'.format(addon_file)
-
-
 def test_addon_naming(tmpdir):
     # the addon does nothing without a config
     addon_file = os.path.join(tmpdir, 'naming1.json')
@@ -644,12 +629,10 @@ def test_invalid_addon_json(tmpdir):
                 """)
 
     test_file = os.path.join(tmpdir, 'file.cpp')
-    with open(test_file, 'wt') as f:
-        f.write("""
-typedef int MISRA_5_6_VIOLATION;
-                """)
+    with open(test_file, 'wt'):
+        pass
 
-    args = ['--addon={}'.format(addon_file), '--enable=all', test_file]
+    args = ['--addon={}'.format(addon_file), test_file]
 
     exitcode, stdout, stderr = cppcheck(args)
     assert exitcode == 1
@@ -1124,3 +1107,56 @@ def test_build_dir_j_memleak(tmpdir): #12111
     ]
 
     assert_cppcheck(args, ec_exp=0, err_exp=[], out_exp=out_lines)
+
+
+def __test_addon_json_invalid(tmpdir, addon_json, expected):
+    addon_file = os.path.join(tmpdir, 'invalid.json')
+    with open(addon_file, 'wt') as f:
+        f.write(addon_json)
+
+    test_file = os.path.join(tmpdir, 'file.cpp')
+    with open(test_file, 'wt'):
+        pass
+
+    args = ['--addon={}'.format(addon_file), test_file]
+
+    exitcode, stdout, stderr = cppcheck(args)
+    assert exitcode == 1
+    lines = stdout.splitlines()
+    assert len(lines) == 1
+    assert lines == [
+        'Loading {} failed. {}'.format(addon_file, expected)
+    ]
+    assert stderr == ''
+
+
+def test_addon_json_invalid_no_obj(tmpdir):
+    __test_addon_json_invalid(tmpdir, json.dumps([]), "JSON is not an object.")
+
+
+def test_addon_json_invalid_args_1(tmpdir):
+    __test_addon_json_invalid(tmpdir, json.dumps({'args':0}), "'args' must be an array.")
+
+
+def test_addon_json_invalid_args_2(tmpdir):
+    __test_addon_json_invalid(tmpdir, json.dumps({'args':[0]}), "'args' entry is not a string.")
+
+
+def test_addon_json_invalid_ctu(tmpdir):
+    __test_addon_json_invalid(tmpdir, json.dumps({'ctu':0}), "'ctu' must be a boolean.")
+
+
+def test_addon_json_invalid_python(tmpdir):
+    __test_addon_json_invalid(tmpdir, json.dumps({'python':0}), "'python' must be a string.")
+
+
+def test_addon_json_invalid_executable(tmpdir):
+    __test_addon_json_invalid(tmpdir, json.dumps({'executable':0}), "'executable' must be a string.")
+
+
+def test_addon_json_invalid_script_1(tmpdir):
+    __test_addon_json_invalid(tmpdir, json.dumps({'Script':''}), "'script' is missing.")
+
+
+def test_addon_json_invalid_script_2(tmpdir):
+    __test_addon_json_invalid(tmpdir, json.dumps({'script':0}), "'script' must be a string.")