From 9620cc75d4125066e4e676e299d3eecce09bbf84 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Mon, 9 Sep 2019 22:09:09 +0300 Subject: [PATCH] [Apache v2] Allow initialization of ParserNode instances using metadata dictionary instead of required arguments (#7366) Add metadata keyword argument to the ParserNode interface, allowing the initialization of the object from contents of the metadata - if the implementation allows it. As an example, Augeas implementation needs nothing more than the Augeas DOM path of a configuration directive to be able to populate the ParserNode instance with all data relevant to the DirectiveNode. The checks also allow skipping the otherwise required keyword arguments if metadata is provided. * Allow creating ParserNode instances using information from metadata dictionary * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: Brad Warren * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: Brad Warren * Address review comments * Fix filepath comment * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: Brad Warren --- certbot-apache/certbot_apache/interfaces.py | 37 +++++++++++-- .../certbot_apache/parsernode_util.py | 52 +++++++++++++++++-- .../certbot_apache/tests/parsernode_test.py | 3 +- .../tests/parsernode_util_test.py | 30 ++++++++++- 4 files changed, 111 insertions(+), 11 deletions(-) diff --git a/certbot-apache/certbot_apache/interfaces.py b/certbot-apache/certbot_apache/interfaces.py index 919aa7e93..cdfdfac91 100644 --- a/certbot-apache/certbot_apache/interfaces.py +++ b/certbot-apache/certbot_apache/interfaces.py @@ -45,8 +45,10 @@ and any other information relevant to the underlying parser engine. Access to the metadata should be handled by implementation specific methods, allowing the Configurator functionality to access the underlying information where needed. -A good example of this is file path of a node - something that is needed by the -reverter functionality within the Configurator. + +For some implementations the node can be initialized using the information carried +in metadata alone. This is useful especially when populating the ParserNode tree +while parsing the configuration. Apache implementation @@ -55,6 +57,20 @@ Apache implementation The Apache implementation of ParserNode interface requires some implementation specific functionalities that are not described by the interface itself. +Initialization + +When the user of a ParserNode class is creating these objects, they must specify +the parameters as described in the documentation for the __init__ methods below. +When these objects are created internally, however, some parameters may not be +needed because (possibly more detailed) information is included in the metadata +parameter. In this case, implementations can deviate from the required parameters +from __init__, however, they should still behave the same when metadata is not +provided. + +For consistency internally, if an argument is provided directly in the ParserNode +initialization parameters as well as within metadata it's recommended to establish +clear behavior around this scenario within the implementation. + Conditional blocks Apache configuration can have conditional blocks, for example: , @@ -86,7 +102,7 @@ For this reason the internal representation of data should not ignore the case. import abc import six -from acme.magic_typing import Optional, Tuple # pylint: disable=unused-import, no-name-in-module +from acme.magic_typing import Any, Dict, Optional, Tuple # pylint: disable=unused-import, no-name-in-module @six.add_metaclass(abc.ABCMeta) @@ -127,6 +143,10 @@ class ParserNode(object): # configuration file. Filepath can be None if a configuration directive is # defined in for example the httpd command line. filepath: Optional[str] + + # Metadata dictionary holds all the implementation specific key-value pairs + # for the ParserNode instance. + metadata: Dict[str, Any] """ @abc.abstractmethod @@ -146,6 +166,11 @@ class ParserNode(object): :param dirty: Boolean flag for denoting if this CommentNode has been created or changed after the last save. Default: False. :type dirty: bool + + :param metadata: Dictionary of metadata values for this ParserNode object. + Metadata information should be used only internally in the implementation. + Default: {} + :type metadata: dict """ @abc.abstractmethod @@ -209,7 +234,8 @@ class CommentNode(ParserNode): """ super(CommentNode, self).__init__(ancestor=kwargs['ancestor'], dirty=kwargs.get('dirty', False), - filepath=kwargs['filepath']) # pragma: no cover + filepath=kwargs['filepath'], + metadata=kwargs.get('metadata', {})) # pragma: no cover @six.add_metaclass(abc.ABCMeta) @@ -273,7 +299,8 @@ class DirectiveNode(ParserNode): """ super(DirectiveNode, self).__init__(ancestor=kwargs['ancestor'], dirty=kwargs.get('dirty', False), - filepath=kwargs['filepath']) # pragma: no cover + filepath=kwargs['filepath'], + metadata=kwargs.get('metadata', {})) # pragma: no cover @abc.abstractmethod def set_parameters(self, parameters): diff --git a/certbot-apache/certbot_apache/parsernode_util.py b/certbot-apache/certbot_apache/parsernode_util.py index 2450a1c15..d9646862a 100644 --- a/certbot-apache/certbot_apache/parsernode_util.py +++ b/certbot-apache/certbot_apache/parsernode_util.py @@ -31,14 +31,28 @@ def parsernode_kwargs(kwargs): dictionary, and hence the returned dictionary should be used instead in the caller function instead of the original kwargs. + If metadata is provided, the otherwise required argument "filepath" may be + omitted if the implementation is able to extract its value from the metadata. + This usecase is handled within this function. Filepath defaults to None. :param dict kwargs: Keyword argument dictionary to validate. :returns: Tuple of validated and prepared arguments. """ + + # As many values of ParserNode instances can be derived from the metadata, + # (ancestor being a common exception here) make sure we permit it here as well. + if "metadata" in kwargs: + # Filepath can be derived from the metadata in Augeas implementation. + # Default is None, as in this case the responsibility of populating this + # variable lies on the implementation. + kwargs.setdefault("filepath", None) + kwargs.setdefault("dirty", False) - kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath"]) - return kwargs["ancestor"], kwargs["dirty"], kwargs["filepath"] + kwargs.setdefault("metadata", {}) + + kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath", "metadata"]) + return kwargs["ancestor"], kwargs["dirty"], kwargs["filepath"], kwargs["metadata"] def commentnode_kwargs(kwargs): @@ -48,13 +62,29 @@ def commentnode_kwargs(kwargs): returned dictionary should be used instead in the caller function instead of the original kwargs. + If metadata is provided, the otherwise required argument "comment" may be + omitted if the implementation is able to extract its value from the metadata. + This usecase is handled within this function. :param dict kwargs: Keyword argument dictionary to validate. :returns: Tuple of validated and prepared arguments and ParserNode kwargs. """ + + # As many values of ParserNode instances can be derived from the metadata, + # (ancestor being a common exception here) make sure we permit it here as well. + if "metadata" in kwargs: + kwargs.setdefault("comment", None) + # Filepath can be derived from the metadata in Augeas implementation. + # Default is None, as in this case the responsibility of populating this + # variable lies on the implementation. + kwargs.setdefault("filepath", None) + kwargs.setdefault("dirty", False) - kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath", "comment"]) + kwargs.setdefault("metadata", {}) + + kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath", "comment", + "metadata"]) comment = kwargs.pop("comment") return comment, kwargs @@ -67,17 +97,31 @@ def directivenode_kwargs(kwargs): dictionary, and hence the returned dictionary should be used instead in the caller function instead of the original kwargs. + If metadata is provided, the otherwise required argument "name" may be + omitted if the implementation is able to extract its value from the metadata. + This usecase is handled within this function. + :param dict kwargs: Keyword argument dictionary to validate. :returns: Tuple of validated and prepared arguments and ParserNode kwargs. """ + # As many values of ParserNode instances can be derived from the metadata, + # (ancestor being a common exception here) make sure we permit it here as well. + if "metadata" in kwargs: + kwargs.setdefault("name", None) + # Filepath can be derived from the metadata in Augeas implementation. + # Default is None, as in this case the responsibility of populating this + # variable lies on the implementation. + kwargs.setdefault("filepath", None) + kwargs.setdefault("dirty", False) kwargs.setdefault("enabled", True) kwargs.setdefault("parameters", ()) + kwargs.setdefault("metadata", {}) kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath", "name", - "parameters", "enabled"]) + "parameters", "enabled", "metadata"]) name = kwargs.pop("name") parameters = kwargs.pop("parameters") diff --git a/certbot-apache/certbot_apache/tests/parsernode_test.py b/certbot-apache/certbot_apache/tests/parsernode_test.py index 0fba32b98..1a2288c82 100644 --- a/certbot-apache/certbot_apache/tests/parsernode_test.py +++ b/certbot-apache/certbot_apache/tests/parsernode_test.py @@ -13,10 +13,11 @@ class DummyParserNode(interfaces.ParserNode): """ Initializes the ParserNode instance. """ - ancestor, dirty, filepath = util.parsernode_kwargs(kwargs) + ancestor, dirty, filepath, metadata = util.parsernode_kwargs(kwargs) self.ancestor = ancestor self.dirty = dirty self.filepath = filepath + self.metadata = metadata super(DummyParserNode, self).__init__(**kwargs) def save(self, msg): # pragma: no cover diff --git a/certbot-apache/certbot_apache/tests/parsernode_util_test.py b/certbot-apache/certbot_apache/tests/parsernode_util_test.py index acb7a92db..a079759ee 100644 --- a/certbot-apache/certbot_apache/tests/parsernode_util_test.py +++ b/certbot-apache/certbot_apache/tests/parsernode_util_test.py @@ -48,10 +48,21 @@ class ParserNodeUtilTest(unittest.TestCase): params = self._setup_parsernode() ctrl = self._setup_parsernode() - ancestor, dirty, filepath = util.parsernode_kwargs(params) + ancestor, dirty, filepath, metadata = util.parsernode_kwargs(params) self.assertEqual(ancestor, ctrl["ancestor"]) self.assertEqual(dirty, ctrl["dirty"]) self.assertEqual(filepath, ctrl["filepath"]) + self.assertEqual(metadata, {}) + + def test_parsernode_from_metadata(self): + params = self._setup_parsernode() + params.pop("filepath") + md = {"some": "value"} + params["metadata"] = md + + # Just testing that error from missing required parameters is not raised + _, _, _, metadata = util.parsernode_kwargs(params) + self.assertEqual(metadata, md) def test_commentnode(self): params = self._setup_commentnode() @@ -60,6 +71,14 @@ class ParserNodeUtilTest(unittest.TestCase): comment, _ = util.commentnode_kwargs(params) self.assertEqual(comment, ctrl["comment"]) + def test_commentnode_from_metadata(self): + params = self._setup_commentnode() + params.pop("comment") + params["metadata"] = {} + + # Just testing that error from missing required parameters is not raised + util.commentnode_kwargs(params) + def test_directivenode(self): params = self._setup_directivenode() ctrl = self._setup_directivenode() @@ -69,6 +88,15 @@ class ParserNodeUtilTest(unittest.TestCase): self.assertEqual(parameters, ctrl["parameters"]) self.assertEqual(enabled, ctrl["enabled"]) + def test_directivenode_from_metadata(self): + params = self._setup_directivenode() + params.pop("filepath") + params.pop("name") + params["metadata"] = {"irrelevant": "value"} + + # Just testing that error from missing required parameters is not raised + util.directivenode_kwargs(params) + def test_missing_required(self): c_params = self._setup_commentnode() c_params.pop("comment")