From fd6a3d77fdcc40d06bd7a800a09562ea20921e67 Mon Sep 17 00:00:00 2001 From: Stefaan Lippens Date: Thu, 24 Nov 2022 23:20:28 +0100 Subject: [PATCH] fixup! issue #76 report missing parameter description --- src/openeo_aggregator/metadata/merging.py | 37 ++--- tests/metadata/test_merging.py | 165 +++++++++++++++++++--- 2 files changed, 163 insertions(+), 39 deletions(-) diff --git a/src/openeo_aggregator/metadata/merging.py b/src/openeo_aggregator/metadata/merging.py index 64af6064..a3b11c97 100644 --- a/src/openeo_aggregator/metadata/merging.py +++ b/src/openeo_aggregator/metadata/merging.py @@ -374,8 +374,11 @@ def _merge_process_parameters( params = process_metadata.get("parameters", []) if params: normalizer = ProcessParameterNormalizer( - backend_id, process_id, - strip_description=False, add_optionals=False + strip_description=False, + add_optionals=False, + report=functools.partial( + self.report, backend_id=backend_id, process_id=process_id + ), ) merged = normalizer.normalize_parameters(params) @@ -408,9 +411,11 @@ def _merge_process_parameters( ) for name in set(merged_params_by_name).intersection(params_by_name): normalizer = ProcessParameterNormalizer( - backend_id, process_id, strip_description=True, add_optionals=True, + report=functools.partial( + self.report, backend_id=backend_id, process_id=process_id + ), ) merged_param = normalizer.normalize_parameter( merged_params_by_name[name] @@ -505,20 +510,25 @@ class ProcessParameterNormalizer: e.g. for comparison purposes. """ - __slots__ = ["backend_id", "process_id", "strip_description", "add_optionals", "report"] + __slots__ = ["strip_description", "add_optionals", "report"] - def __init__(self, backend_id, process_id, - strip_description: bool = False, add_optionals: bool = True, - report: Callable = DEFAULT_REPORTER.report): - self.backend_id = backend_id - self.process_id = process_id + def __init__( + self, + strip_description: bool = False, + add_optionals: bool = True, + report: Callable = DEFAULT_REPORTER.report, + ): self.strip_description = strip_description self.add_optionals = add_optionals self.report = report def normalize_parameter(self, param: dict) -> dict: """Normalize a parameter metadata dict""" - # TODO: report missing name/description/schema? + for required in ["name", "schema", "description"]: + if required not in param: + self.report( + f"Missing required field {required!r} in parameter metadata {param!r}" + ) normalized = { "name": param.get("name", "n/a"), "schema": param.get("schema", {}), @@ -526,12 +536,7 @@ def normalize_parameter(self, param: dict) -> dict: if self.strip_description: normalized["description"] = "-" else: - normalized["description"] = param.get("description", "") - if normalized["description"] == "": - self.report("Missing description for parameter", - backend_id = self.backend_id, process_id = self.process_id, parameter = normalized["name"] - ) - normalized["description"] = normalized["name"] + normalized["description"] = param.get("description", normalized["name"]) for field, default_value in [ ("optional", False), ("deprecated", False), diff --git a/tests/metadata/test_merging.py b/tests/metadata/test_merging.py index 3326e9fc..0a3f407c 100644 --- a/tests/metadata/test_merging.py +++ b/tests/metadata/test_merging.py @@ -267,8 +267,16 @@ def test_merge_process_parameters_basic(self, merger, reporter): spec = { "id": "add", "parameters": [ - {"name": "x", "schema": {"type": "number"}}, - {"name": "y", "schema": {"type": "number"}}, + { + "name": "x", + "schema": {"type": "number"}, + "description": "The x value.", + }, + { + "name": "y", + "schema": {"type": "number"}, + "description": "The y value.", + }, ], } result = merger.merge_process_metadata( @@ -282,8 +290,16 @@ def test_merge_process_parameters_basic(self, merger, reporter): "id": "add", "description": "add", "parameters": [ - {"name": "x", "description": "x", "schema": {"type": "number"}}, - {"name": "y", "description": "y", "schema": {"type": "number"}}, + { + "name": "x", + "description": "The x value.", + "schema": {"type": "number"}, + }, + { + "name": "y", + "description": "The y value.", + "schema": {"type": "number"}, + }, ], "returns": {"schema": {}}, "federation:backends": ["b1", "b2"], @@ -299,7 +315,13 @@ def test_merge_process_parameters_invalid(self, merger, reporter): { "b1": { "id": "cos", - "parameters": [{"name": "x", "schema": {"type": "number"}}], + "parameters": [ + { + "name": "x", + "schema": {"type": "number"}, + "description": "x value", + } + ], }, "b2": {"id": "cos", "parameters": [1324]}, } @@ -310,13 +332,13 @@ def test_merge_process_parameters_invalid(self, merger, reporter): "federation:backends": ["b1", "b2"], "id": "cos", "parameters": [ - {"description": "x", "name": "x", "schema": {"type": "number"}} + {"name": "x", "schema": {"type": "number"}, "description": "x value"} ], "returns": {"schema": {}}, - 'deprecated': False, - 'experimental': False, - 'examples': [], - 'links': [] + "deprecated": False, + "experimental": False, + "examples": [], + "links": [], } assert reporter.logs == [ { @@ -334,12 +356,77 @@ def test_merge_process_parameters_invalid(self, merger, reporter): }, ] + def test_merge_process_parameters_missing_required(self, merger, reporter): + result = merger.merge_process_metadata( + { + "b1": { + "id": "cos", + "parameters": [ + { + "name": "x", + "schema": {"type": "number"}, + "description": "x value", + } + ], + }, + "b2": {"id": "cos", "parameters": [{"name": "x"}]}, + } + ) + + assert result == { + "description": "cos", + "federation:backends": ["b1", "b2"], + "id": "cos", + "parameters": [ + {"name": "x", "schema": {"type": "number"}, "description": "x value"} + ], + "returns": {"schema": {}}, + "deprecated": False, + "experimental": False, + "examples": [], + "links": [], + } + assert reporter.logs == [ + { + "backend_id": "b2", + "msg": "Missing required field 'schema' in parameter metadata {'name': 'x'}", + "process_id": "cos", + }, + { + "backend_id": "b2", + "msg": "Missing required field 'description' in parameter metadata {'name': 'x'}", + "process_id": "cos", + }, + { + "backend_id": "b2", + "msg": "Parameter 'x' field 'schema' value differs from merged.", + "diff": [ + "--- merged\n", + "+++ b2\n", + "@@ -1,3 +1 @@\n", + "-{\n", + '- "type": "number"\n', + "-}\n", + "+{}\n", + ], + "merged": {"type": "number"}, + "process_id": "cos", + "value": {}, + }, + ] + def test_merge_process_parameters_invalid_listing(self, merger, reporter): result = merger.merge_process_metadata( { "b1": { "id": "cos", - "parameters": [{"name": "x", "schema": {"type": "number"}}], + "parameters": [ + { + "name": "x", + "schema": {"type": "number"}, + "description": "x value", + } + ], }, "b2": {"id": "cos", "parameters": 1324}, } @@ -350,7 +437,7 @@ def test_merge_process_parameters_invalid_listing(self, merger, reporter): "federation:backends": ["b1", "b2"], "id": "cos", "parameters": [ - {"description": "x", "name": "x", "schema": {"type": "number"}} + {"name": "x", "schema": {"type": "number"}, "description": "x value"} ], "returns": {"schema": {}}, 'deprecated': False, @@ -379,7 +466,11 @@ def test_merge_process_parameters_differences(self, merger, reporter): "b1": { "id": "cos", "parameters": [ - {"name": "x", "schema": {"type": "number"}}, + { + "name": "x", + "schema": {"type": "number"}, + "description": "The X value.", + }, ], }, "b2": { @@ -387,7 +478,6 @@ def test_merge_process_parameters_differences(self, merger, reporter): "parameters": [ { "name": "x", - "description": "X value", "schema": {"type": "number"}, "deprecated": False, "optional": False, @@ -406,7 +496,11 @@ def test_merge_process_parameters_differences(self, merger, reporter): "id": "cos", "description": "cos", "parameters": [ - {"description": "x", "name": "x", "schema": {"type": "number"}} + { + "name": "x", + "schema": {"type": "number"}, + "description": "The X value.", + } ], "returns": {"schema": {}}, "federation:backends": ["b1", "b2", "b3"], @@ -416,6 +510,16 @@ def test_merge_process_parameters_differences(self, merger, reporter): 'links': [], } assert reporter.logs == [ + { + "backend_id": "b2", + "msg": "Missing required field 'description' in parameter metadata {'name': 'x', 'schema': {'type': 'number'}, 'deprecated': False, 'optional': False}", + "process_id": "cos", + }, + { + "backend_id": "b3", + "msg": "Missing required field 'description' in parameter metadata {'name': 'x', 'schema': {'type': 'array'}}", + "process_id": "cos", + }, { "msg": "Parameter 'x' field 'schema' value differs from merged.", "backend_id": "b3", @@ -445,10 +549,13 @@ def test_merge_process_parameters_recursive(self, merger, reporter): "parameters": [ { "name": "process", + "description": "Callback", "schema": { "type": "object", "subtype": "process-graph", - "parameters": [{"name": "x", "schema": {}}], + "parameters": [ + {"name": "x", "schema": {}, "description": "the x"} + ], "returns": {"schema": {}}, }, }, @@ -459,6 +566,7 @@ def test_merge_process_parameters_recursive(self, merger, reporter): "parameters": [ { "name": "process", + "description": "A Callback", "schema": { "type": "object", "subtype": "process-graph", @@ -466,6 +574,7 @@ def test_merge_process_parameters_recursive(self, merger, reporter): { "name": "x", "schema": {}, + "description": "The x", "optional": False, "deprecated": False, "experimental": False, @@ -487,11 +596,13 @@ def test_merge_process_parameters_recursive(self, merger, reporter): "parameters": [ { "name": "process", - "description": "process", + "description": "Callback", "schema": { "type": "object", "subtype": "process-graph", - "parameters": [{"name": "x", "description": "x", "schema": {}}], + "parameters": [ + {"name": "x", "description": "the x", "schema": {}} + ], "returns": {"schema": {}}, }, } @@ -514,12 +625,19 @@ def test_merge_process_parameters_recursive2(self, merger, reporter): "b1": { "id": "count", "parameters": [ - {"name": "data", "schema": {"type": "array"}}, + { + "name": "data", + "schema": {"type": "array"}, + "description": "Data", + }, { "name": "condition", + "description": "what to count", "schema": [ { - "parameters": [{"name": "x", "schema": {}}], + "parameters": [ + {"name": "x", "schema": {}, "description": "X."} + ], "returns": {"schema": {"type": "boolean"}}, "subtype": "process-graph", "type": "object", @@ -548,6 +666,7 @@ def test_merge_process_parameters_recursive2(self, merger, reporter): }, { "name": "condition", + "description": "Condition callback.", "deprecated": False, "experimental": False, "optional": True, @@ -581,10 +700,10 @@ def test_merge_process_parameters_recursive2(self, merger, reporter): "description": "count", "federation:backends": ["b1", "b2"], "parameters": [ - {"name": "data", "description": "data", "schema": {"type": "array"}}, + {"name": "data", "description": "Data", "schema": {"type": "array"}}, { "name": "condition", - "description": "condition", + "description": "what to count", "default": None, "optional": True, "schema": [ @@ -592,7 +711,7 @@ def test_merge_process_parameters_recursive2(self, merger, reporter): "type": "object", "subtype": "process-graph", "parameters": [ - {"name": "x", "description": "x", "schema": {}} + {"name": "x", "description": "X.", "schema": {}} ], "returns": {"schema": {"type": "boolean"}}, },