Skip to content
Merged
6 changes: 6 additions & 0 deletions netjsonconfig/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ def _list_errors(e):
:param e: ``jsonschema.exceptions.ValidationError`` instance
"""
error_list = []
if not getattr(e, "context", None):
return error_list
if not getattr(e, "validator_value", None):
return error_list
for value, error in zip(e.validator_value, e.context):
error_list.append((value, error.message))
if error.context:
Expand All @@ -25,6 +29,8 @@ def __str__(self):
self.details,
)
errors = _list_errors(self.details)
if not errors:
return message
separator = "\nAgainst schema %s\n%s\n"
details = reduce(lambda x, y: x + separator % y, errors, "")
return message + details
Expand Down
27 changes: 22 additions & 5 deletions netjsonconfig/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
from collections import OrderedDict
from copy import deepcopy

from jsonschema import ValidationError as JsonSchemaError

from .exceptions import ValidationError


def merge_config(template, config, list_identifiers=None):
"""
Expand All @@ -19,14 +23,27 @@ def merge_config(template, config, list_identifiers=None):
:param config: config ``dict``
:param list_identifiers: ``list`` or ``None``
:returns: merged ``dict``
:raises ValidationError: if incompatible types are found
"""
result = deepcopy(template)
for key, value in config.items():
if isinstance(value, dict):
node = result.get(key, OrderedDict())
result[key] = merge_config(node, value)
elif isinstance(value, list) and isinstance(result.get(key), list):
result[key] = merge_list(result[key], value, list_identifiers)
existing = result.get(key)
if isinstance(value, dict) and isinstance(existing, dict):
result[key] = merge_config(existing, value, list_identifiers)
elif isinstance(value, list) and isinstance(existing, list):
result[key] = merge_list(existing, value, list_identifiers)
elif (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This guard applies at every recursive merge level, not only to the top level package shape conflict from issue 351. It now rejects cases where a matched custom UCI block overrides a list option with a scalar option, even though the merge contract says simple values in the config overwrite values in the template. For example, a template dnsmasq block with server: ["1.1.1.1"] and device config with server: "8.8.8.8" used to merge, but now raises because existing is a list and value is a string. Please scope the validation to the package shape conflict or other cases that would otherwise crash, unless this broader behavior change is intentional and documented.

existing is not None
and isinstance(existing, (dict, list))
and isinstance(value, (dict, list))
and type(value) is not type(existing)
):
raise ValidationError(
JsonSchemaError(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This constructs a JsonSchemaError with only a message. That gives the right exception type, but netjsonconfig.exceptions.ValidationError.__str__ later assumes the wrapped jsonschema error has iterable validator_value and context. For this path, str(exc) raises TypeError: 'Unset' object is not iterable, so an uncaught error is shown as <exception str() failed> instead of the validation message. Please either make the wrapper tolerate message only errors, or build the wrapped error with the fields that __str__ expects. The test should also assert that the raised error can be stringified.

f"Incompatible type for '{key}': expected {type(existing).__name__}, "
f"got {type(value).__name__}."
)
)
else:
result[key] = value
return result
Expand Down
26 changes: 26 additions & 0 deletions tests/openwrt/test_default.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from openwisp_utils.tests import capture_stdout

from netjsonconfig import OpenWrt
from netjsonconfig.exceptions import ValidationError
from netjsonconfig.utils import _TabsMixin


Expand Down Expand Up @@ -245,3 +246,28 @@ def test_render_invalid_uci_name(self):
option lan '0.0.0.0/24 domain=1'
""")
self.assertEqual(o.render(), expected)

def test_merge_invalid_format(self):
invalid = {
"dhcp": {
"lan": {
"interface": "lan",
"start": 100,
"limit": 150,
"leasetime": "12h",
}
}
}
valid = {
"dhcp": [
{
"dhcpv6": "disabled",
"ignore": True,
"ra": "disabled",
"config_value": "lan",
"config_name": "dhcp",
}
]
}
with self.assertRaises(ValidationError):
OpenWrt({}, templates=[valid, invalid])
15 changes: 15 additions & 0 deletions tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import unittest

from netjsonconfig.exceptions import ValidationError
from netjsonconfig.utils import evaluate_vars, get_copy, merge_config, merge_list


Expand Down Expand Up @@ -55,6 +56,20 @@ def test_merge_list_of_dicts_unchanged(self):
result, {"list": [{"a": "original"}, {"b": "changed"}, {"c": "original"}]}
)

def test_merge_config_list_scalar_overrides(self):
template = {"server": ["1.1.1.1"]}
config = {"server": "8.8.8.8"}
result = merge_config(template, config)
self.assertEqual(result, {"server": "8.8.8.8"})

def test_merge_config_incompatible_dict_list(self):
template = {"a": {"b": "c"}}
config = {"a": ["x"]}
with self.assertRaises(ValidationError) as context:
merge_config(template, config)
self.assertIn("Incompatible type", str(context.exception))
self.assertIn("ValidationError", str(context.exception))

def test_evaluate_vars(self):
self.assertEqual(evaluate_vars("{{ tz }}", {"tz": "UTC"}), "UTC")
self.assertEqual(evaluate_vars("tz: {{ tz }}", {"tz": "UTC"}), "tz: UTC")
Expand Down