From a7b45654455a4c5b94ddd485ef958c2a4d5499b5 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 14 Mar 2024 10:11:35 +0100 Subject: [PATCH] meta: add tests for invalid python json/schema parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Based on the feedback from Tomáš in [0] this commit adds tests that ensure consistent behavior between the python and the json loader. It's not 100% because the python is extremly leaniant and does not even check if the required pieces of the json are there. I.e. it will load a module without a SCHEMA or SCHEMA_2 variable and the json loader code will warn about the issue but not raise an error. Fwiw, I have no strong opinion here but I do lean slightly towards staying close to the original code (but both approaches of failing with an exectption and continue with a warning have good arguments). [0] https://github.com/osbuild/osbuild/pull/1618#discussion_r1521141148 --- osbuild/meta.py | 12 +++++-- test/mod/test_meta.py | 75 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 69 insertions(+), 18 deletions(-) diff --git a/osbuild/meta.py b/osbuild/meta.py index 28ce0da8..44cbe3cd 100644 --- a/osbuild/meta.py +++ b/osbuild/meta.py @@ -472,12 +472,20 @@ class ModuleInfo: def _load_from_json(cls, path, klass, name) -> Optional["ModuleInfo"]: meta_json_suffix = ".meta.json" with open(path + meta_json_suffix, encoding="utf-8") as fp: - meta = json.load(fp) + try: + meta = json.load(fp) + except json.decoder.JSONDecodeError as e: + raise SyntaxError("Invalid schema: " + str(e)) from e schema = Schema(META_JSON_SCHEMA, "meta.json validator") res = schema.validate(meta) if not res.valid: - # XXX: should we raise an exception instead? + # the python code is very leaniant with invalid schemas + # so just print a warning here for now to stay close to + # what the old code was doing + errs = res.as_dict()["errors"] + # it would be nice to have a proper logger here + print(f"WARNING: schema for {path} is invalid: {errs}", file=sys.stderr) return None long_description = meta.get("description", "no description provided") diff --git a/test/mod/test_meta.py b/test/mod/test_meta.py index f1bd89f5..0294c501 100644 --- a/test/mod/test_meta.py +++ b/test/mod/test_meta.py @@ -1,5 +1,6 @@ import os import pathlib +import re import textwrap import pytest @@ -172,7 +173,7 @@ def test_schema(): assert res -def make_fake_meta_json(tmp_path, name, version): +def make_fake_meta_json(tmp_path, name, version, invalid=""): meta_json_path = pathlib.Path(f"{tmp_path}/stages/{name}.meta.json") meta_json_path.parent.mkdir(exist_ok=True) if version == 1: @@ -195,6 +196,16 @@ def make_fake_meta_json(tmp_path, name, version): """ else: pytest.fail(f"unexpected schema version {version}") + + if invalid == "json": + schema_part = """ + "xxx": {not-json}"' + """ + elif invalid == "schema": + schema_part = """ + "not": "following schema" + """ + meta_json_path.write_text(""" { "summary": "some json summary", @@ -209,18 +220,28 @@ def make_fake_meta_json(tmp_path, name, version): return meta_json_path -def make_fake_py_module(tmp_path, name): +def make_fake_py_module(tmp_path, name, invalid=""): py_path = pathlib.Path(f"{tmp_path}/stages/{name}") py_path.parent.mkdir(exist_ok=True) fake_py = '"""some py summary\nlong description\nwith newline"""' - fake_py += textwrap.dedent(""" - SCHEMA = '"properties": {"py_filename":{"type": "string"}}' - SCHEMA_2 = '"py_devices": {"type":"object"}' - CAPABILITIES = ['CAP_MAC_ADMIN'] + if invalid == "json": + fake_py += textwrap.dedent(""" + SCHEMA = '"xxx": {not-json}"' + """) + elif invalid == "schema": + fake_py += textwrap.dedent(""" + noT = "following schema" + """) + else: + fake_py += textwrap.dedent(""" + SCHEMA = '"properties": {"py_filename":{"type": "string"}}' + SCHEMA_2 = '"py_devices": {"type":"object"}' + CAPABILITIES = ['CAP_MAC_ADMIN'] """) py_path.write_text(fake_py, encoding="utf-8") +# keep the tests consistent between loading from python and json def test_load_from_json(tmp_path): make_fake_meta_json(tmp_path, "org.osbuild.noop", version=1) modinfo = osbuild.meta.ModuleInfo.load(tmp_path, "Stage", "org.osbuild.noop") @@ -233,6 +254,20 @@ def test_load_from_json(tmp_path): } +def test_load_from_json_invalid_json(tmp_path): + make_fake_meta_json(tmp_path, "org.osbuild.noop", version=2, invalid="json") + with pytest.raises(SyntaxError) as exc: + osbuild.meta.ModuleInfo.load(tmp_path, "Stage", "org.osbuild.noop") + assert str(exc.value) == "Invalid schema: Expecting property name enclosed in double quotes: line 2 column 17 (char 201)" + + +def test_load_from_json_invalid_schema(tmp_path, capsys): + make_fake_meta_json(tmp_path, "org.osbuild.noop", version=2, invalid="schema") + modinfo = osbuild.meta.ModuleInfo.load(tmp_path, "Stage", "org.osbuild.noop") + assert modinfo is None + assert re.match(r"WARNING: schema for .* is invalid: ", capsys.readouterr().err) + + def test_load_from_py(tmp_path): make_fake_py_module(tmp_path, "org.osbuild.noop") modinfo = osbuild.meta.ModuleInfo.load(tmp_path, "Stage", "org.osbuild.noop") @@ -245,6 +280,24 @@ def test_load_from_py(tmp_path): } +def test_load_from_py_invalid_json(tmp_path): + make_fake_py_module(tmp_path, "org.osbuild.noop", invalid="json") + with pytest.raises(SyntaxError) as exc: + osbuild.meta.ModuleInfo.load(tmp_path, "Stage", "org.osbuild.noop") + assert str(exc.value) == "Invalid schema: Expecting property name enclosed in double quotes (org.osbuild.noop, line 4)" + + +def test_load_from_py_invalid_schema(tmp_path): + make_fake_py_module(tmp_path, "org.osbuild.noop", invalid="schema") + modinfo = osbuild.meta.ModuleInfo.load(tmp_path, "Stage", "org.osbuild.noop") + assert modinfo.desc == "some py summary" + assert modinfo.info == "long description\nwith newline" + assert modinfo.opts == { + "1": {}, + "2": {}, + } + + def test_load_from_json_prefered(tmp_path): make_fake_meta_json(tmp_path, "org.osbuild.noop", version=2) make_fake_py_module(tmp_path, "org.osbuild.noop") @@ -258,16 +311,6 @@ def test_load_from_json_prefered(tmp_path): } -def test_load_from_json_validates(tmp_path): - meta_json_path = pathlib.Path(f"{tmp_path}/stages/org.osbuild.noop.meta-json") - meta_json_path.parent.mkdir(exist_ok=True) - # XXX: do a more direct test here that also validates the various - # error conditions - meta_json_path.write_text("""{"not": "following schema"}""") - modinfo = osbuild.meta.ModuleInfo.load(tmp_path, "Stage", "org.osbuild.noop") - assert modinfo is None - - @pytest.mark.parametrize("test_input,expected_err", [ # happy (