From 950950fec7cbcff7318101ea0048db62ce11d0fc Mon Sep 17 00:00:00 2001 From: Zahari Date: Thu, 29 Apr 2021 15:28:29 +0100 Subject: [PATCH 1/5] Add failing test --- src/reportengine/tests/test_complexinput.py | 24 +++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/reportengine/tests/test_complexinput.py b/src/reportengine/tests/test_complexinput.py index d7542c3..12f4998 100644 --- a/src/reportengine/tests/test_complexinput.py +++ b/src/reportengine/tests/test_complexinput.py @@ -181,6 +181,9 @@ def produce_matched_datasets_from_dataspecs(self, dataspecs): res.sort(key=lambda x: (x['dataset_name'])) return res + def produce_dependent_namespace(self, pdf): + return {"pdfprop": pdf} + @make_argcheck def bad_check(pdf): @@ -193,8 +196,14 @@ def report(self, template_text): def plot_a_pdf(self, pdf): return "PLOT OF " + str(pdf) + def prop_table(self, pdfprop): + return f"Table: {pdfprop}" + dataspecs_speclabel = collect('speclabel', ('datasepcs',), element_default='label') + + props_collection = collect("prop_table", ("dependent_namespace",)) + @bad_check def bad_plot(self, pdf): return self.plot_a_pdf(pdf) @@ -399,5 +408,20 @@ def test_namespace_production(self): (('matched_datasets_from_dataspecs', 0), ('dataspecs', 1),))['dataset'] assert ds1 != ds2 + def test_dependent_rules(self): + c = Config({"pdf": "a", "Ns": {"pdf": "b"}}) + targets = [ + FuzzyTarget("props_collection", (), (), ()), + FuzzyTarget("props_collection", ("Ns",), (), ()), + ] + builder = resourcebuilder.ResourceBuilder(c, Providers(), targets) + builder.resolve_fuzzytargets() + builder.execute_sequential() + res1 = namespaces.resolve(builder.rootns, ())["props_collection"] + res2 = namespaces.resolve(builder.rootns, ('Ns',))["props_collection"] + assert res1 == ['Table: PDF: a'] + assert res2 == ['Table: PDF: b'] + + if __name__ == '__main__': unittest.main() From 182de49fe792d4e7527e296103cffa7139c44dc9 Mon Sep 17 00:00:00 2001 From: Zahari Date: Thu, 29 Apr 2021 14:06:21 +0100 Subject: [PATCH 2/5] Force fuzzyspec expansion to always resolve the ns If a namespace depends on a production rule (e.g. one that returns a dictionary or list of dictionaries that are then used in collect) the namespace would be resolved once but then be oblivious to the changes in the input of the production rule, that would make it required to reevaluate it. Rather than adding more special cases, just force all keys to be revaluated by the config object so as to keep them up to date within the current namespace. I cannot think of many downsides to this change, other than having to needlessly reevaluate a few inputs (which are assumed to be cheap in the first place). The test in test_complexinput.py represents a minimal example of input that would fail in master and work correctly with this change. --- src/reportengine/namespaces.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/reportengine/namespaces.py b/src/reportengine/namespaces.py index d78499d..2cfc332 100644 --- a/src/reportengine/namespaces.py +++ b/src/reportengine/namespaces.py @@ -99,9 +99,9 @@ def expand_fuzzyspec_partial(ns, fuzzyspec, currspec=None): results = [] #ns = ChainMap(d) key, remainder = fuzzyspec[0], fuzzyspec[1:] - if not key in ns: - yield key, currspec, ns + yield key, currspec, ns val = ns[key] + if isinstance(val, Mapping): cs_ = (*currspec, key) @@ -129,12 +129,17 @@ def expand_fuzzyspec(ns, fuzzyspec, currspec=None): """Return all the nsspecs that spawn from the fuzzyspec. Raise ElementNotFound if some part is missing.""" gen = expand_fuzzyspec_partial(ns, fuzzyspec, currspec) - try: - missing, nsspec, _ = gen.send(None) - except StopIteration as e: - return e.value - raise ElementNotFound("Could not resolve a fuzzyspec. " - "A key is missing: %s, at the level %r." % (missing, nsspec)) + while True: + try: + key, nsspec, currns = gen.send(None) + except StopIteration as e: + return e.value + else: + if key not in currns: + raise ElementNotFound( + "Could not resolve a fuzzyspec. " + f"A key is missing: '{key}', at the level {nsspec}." + ) def collect_fuzzyspec(ns, key, fuzzyspec, currspec=None): From e12ba6b077dc2dcd1df683e54dde9bbc29d1a3ac Mon Sep 17 00:00:00 2001 From: Zahari Date: Thu, 29 Apr 2021 15:42:35 +0100 Subject: [PATCH 3/5] Upgrade tests Replace old tests that rely on the behaviour of expand_partial. --- src/reportengine/tests/test_namespaces.py | 63 ++++++----------------- 1 file changed, 16 insertions(+), 47 deletions(-) diff --git a/src/reportengine/tests/test_namespaces.py b/src/reportengine/tests/test_namespaces.py index 803d086..fcad53b 100644 --- a/src/reportengine/tests/test_namespaces.py +++ b/src/reportengine/tests/test_namespaces.py @@ -52,52 +52,26 @@ def test_resolve(self): def test_expand(self): fuzzy = ('a', 'c') ns = ChainMap(self.d) - gen = namespaces.expand_fuzzyspec_partial(ns, fuzzy) - #self.assertFalse(list(gen)) - while True: - try: - next(gen) - except StopIteration as e: - self.assertEqual(e.value, [('a', ('c', 0)), ('a', ('c', 1))]) - break - else: - self.fail() - - fuzzy = ('a', 'x', 'c') - ns = ChainMap(self.d) - gen = namespaces.expand_fuzzyspec_partial(ns, fuzzy) - #self.assertFalse(list(gen)) - var, spec, cns = next(gen) - cns[var] = 'not ok' - with self.assertRaises(TypeError): - next(gen) - - fuzzy = ('a', 'xx', 'c') - ns = ChainMap(self.d) - gen = namespaces.expand_fuzzyspec_partial(ns, fuzzy) - #self.assertFalse(list(gen)) - var, spec, cns = next(gen) - cns[var] = [{'ok': True}, {'ok':'yes'}, {'ok':1}] - with self.assertRaises(StopIteration) as ec: - next(gen) - specs = ec.exception.value - self.assertEqual(set(specs), - set(itertools.product('a', [('xx', 0), ('xx', 1,), ('xx', 2)], - [('c', 0), ('c', 1)])) - ) + assert namespaces.expand_fuzzyspec(ns, fuzzy) == [ + ('a', ('c', 0)), + ('a', ('c', 1)), + ] + def test_nested_expand(self): d = self.d - d['c'][0]['l3'] = [{'x':1},{'x':2}] - d['c'][1]['l3'] = [{'x':1},] + d['c'][0]['l3'] = [{'x': 1}, {'x': 2}] + d['c'][1]['l3'] = [ + {'x': 1}, + ] ns = ChainMap(d) fuzzy = ('c', 'l3') - gen = namespaces.expand_fuzzyspec_partial(ns, fuzzy) - with self.assertRaises(StopIteration) as ec: - next(gen) - self.assertEqual(ec.exception.value, - [(('c', 0), ('l3', 0)), (('c', 0), ('l3', 1)), (('c', 1), ('l3', 0))] - ) + res = namespaces.expand_fuzzyspec(ns, fuzzy) + assert res == [ + (('c', 0), ('l3', 0)), + (('c', 0), ('l3', 1)), + (('c', 1), ('l3', 0)), + ] def test_identities(self): a = {1:'a'} @@ -118,12 +92,7 @@ def test_expand_identities(self): l1cp = copy.deepcopy(l1) d = {'root': root, 'l1':l1, 'l2':l2} - try: - next(namespaces.expand_fuzzyspec_partial(d, ('root', 'l1', 'l2'))) - except StopIteration as e: - specs = e.value - else: - raise RuntimeError() + specs = namespaces.expand_fuzzyspec(d, ('root', 'l1', 'l2')) self.assertEqual(len(specs), 3*3) From 26a0b4e4478c7906e27fa5da7135c64129bb892f Mon Sep 17 00:00:00 2001 From: Zahari Date: Thu, 29 Apr 2021 18:46:03 +0100 Subject: [PATCH 4/5] Add another failing test This fails because the collect node is resolved at compile time which works differently. --- src/reportengine/tests/test_complexinput.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/reportengine/tests/test_complexinput.py b/src/reportengine/tests/test_complexinput.py index 12f4998..a3ff729 100644 --- a/src/reportengine/tests/test_complexinput.py +++ b/src/reportengine/tests/test_complexinput.py @@ -184,6 +184,9 @@ def produce_matched_datasets_from_dataspecs(self, dataspecs): def produce_dependent_namespace(self, pdf): return {"pdfprop": pdf} + def produce_derived_prop(self, pdfprop): + return f"Derived: {pdfprop}" + @make_argcheck def bad_check(pdf): @@ -203,6 +206,7 @@ def prop_table(self, pdfprop): element_default='label') props_collection = collect("prop_table", ("dependent_namespace",)) + resolved_collection = collect("derived_prop", ("dependent_namespace",)) @bad_check def bad_plot(self, pdf): @@ -422,6 +426,21 @@ def test_dependent_rules(self): assert res1 == ['Table: PDF: a'] assert res2 == ['Table: PDF: b'] + def test_dependent_resolved(self): + c = Config({"pdf": "a", "Ns": {"pdf": "b"}}) + targets = [ + FuzzyTarget("resolved_collection", (), (), ()), + FuzzyTarget("resolved_collection", ("Ns",), (), ()), + ] + builder = resourcebuilder.ResourceBuilder(c, Providers(), targets) + builder.resolve_fuzzytargets() + builder.execute_sequential() + res1 = namespaces.resolve(builder.rootns, ())["resolved_collection"] + res2 = namespaces.resolve(builder.rootns, ('Ns',))["resolved_collection"] + assert res1 == ['Derived: PDF: a'] + assert res2 == ['Derived: PDF: b'] + + if __name__ == '__main__': unittest.main() From e6482da66543f449ac23bf391eb78cef8c8cfe5e Mon Sep 17 00:00:00 2001 From: Zahari Date: Thu, 29 Apr 2021 18:28:19 +0100 Subject: [PATCH 5/5] Force collect nodes to always be reprocessed This avoids and edge case where collect is resolved at compile time (because we are collecting over a production rule) and the dependencies have changed inadvertently. To remedy that we simply force the node to always be reevaluated. --- src/reportengine/resourcebuilder.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/reportengine/resourcebuilder.py b/src/reportengine/resourcebuilder.py index f31f971..a14fc80 100644 --- a/src/reportengine/resourcebuilder.py +++ b/src/reportengine/resourcebuilder.py @@ -402,11 +402,16 @@ def _process_requirement(self, name, nsspec, *, extraargs=None, log.debug("Processing requirement: %s" % (name,)) + ns = namespaces.resolve(self.rootns, nsspec) if extraargs is None: extraargs = () - + is_provider = self.is_provider_func(name) + if ( is_provider and isinstance(self.get_provider_func(name), collect)): + log.debug("Resolving collect node for %s", name) + yield from self._make_node(name, nsspec, extraargs, parents) + return #First try to find the name in the namespace try: put_index, val = self.input_parser.resolve_key(name, ns, parents=parents, currspec=nsspec)