From af1c66b28f5cad7c33c7da06feb2cc3c2bd8dc30 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Fri, 30 Aug 2019 23:42:18 +0300 Subject: [PATCH] [Apache v2] Modifications to ParserNode interfaces (#7330) This PR contains the changes requested in initial pre-review comments of #7308 Move properties to class pydocs in interfaces.py Prefer class ABC register() functionality instead of class inheritance for interface classes Add apache implementation specific functions to interfaces * Move class argument definitions to class pydoc * Add apache specific functionality to the interface * Bring inheritance back * Define initialization for different ParserNode classes * Add parsernode utils to check keyword arguments and document the defaults in pydoc * Fix pydocs and make BlockNode a child of DirectiveNode * Refine docs, and remove unused __init__ from BlockNode * Split parsernode util tests to their own respective file * Skip cover for dummy calls to super * Add types to method documentation * Add documentation for children --- certbot-apache/certbot_apache/interfaces.py | 248 +++++++++--------- .../certbot_apache/parsernode_util.py | 85 ++++++ .../certbot_apache/tests/parsernode_test.py | 101 ++++--- .../tests/parsernode_util_test.py | 87 ++++++ 4 files changed, 360 insertions(+), 161 deletions(-) create mode 100644 certbot-apache/certbot_apache/parsernode_util.py create mode 100644 certbot-apache/certbot_apache/tests/parsernode_util_test.py diff --git a/certbot-apache/certbot_apache/interfaces.py b/certbot-apache/certbot_apache/interfaces.py index a614f85c9..919aa7e93 100644 --- a/certbot-apache/certbot_apache/interfaces.py +++ b/certbot-apache/certbot_apache/interfaces.py @@ -86,6 +86,8 @@ 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 + @six.add_metaclass(abc.ABCMeta) class ParserNode(object): @@ -110,35 +112,42 @@ class ParserNode(object): it's responsibility of each ParserNode object itself to store its prepending whitespaces in order to be able to reconstruct the complete configuration file as it was when originally read from the disk. + + ParserNode objects should have the following attributes: + + # Reference to ancestor node, or None if the node is the root node of the + # configuration tree. + ancestor: Optional[ParserNode] + + # True if this node has been modified since last save. + dirty: bool + + # Filepath of the file where the configuration element for this ParserNode + # object resides. For root node, the value for filepath is the httpd root + # configuration file. Filepath can be None if a configuration directive is + # defined in for example the httpd command line. + filepath: Optional[str] """ - @property @abc.abstractmethod - def ancestor(self): # pragma: no cover + def __init__(self, **kwargs): """ - This property contains a reference to ancestor node, or None if the node - is the root node of the configuration tree. + Initializes the ParserNode instance, and sets the ParserNode specific + instance variables. This is not meant to be used directly, but through + specific classes implementing ParserNode interface. - :returns: The ancestor BlockNode object, or None for root node. - :rtype: ParserNode + :param ancestor: BlockNode ancestor for this CommentNode. Required. + :type ancestor: BlockNode or None + + :param filepath: Filesystem path for the file where this CommentNode + does or should exist in the filesystem. Required. + :type filepath: str or None + + :param dirty: Boolean flag for denoting if this CommentNode has been + created or changed after the last save. Default: False. + :type dirty: bool """ - raise NotImplementedError - - @property - @abc.abstractmethod - def dirty(self): # pragma: no cover - """ - This property contains a boolean value of the information if this node has - been modified since last save (or after the initial parse). - - :returns: True if this node has had changes that have not yet been written - to disk. - :rtype: bool - """ - - raise NotImplementedError - @abc.abstractmethod def save(self, msg): """ @@ -154,10 +163,13 @@ class ParserNode(object): file writes properly, the file specific temporary trees should be extracted from the full ParserNode tree where necessary when writing to disk. + :param str msg: Message describing the reason for the save. + """ -@six.add_metaclass(abc.ABCMeta) +# Linter rule exclusion done because of https://github.com/PyCQA/pylint/issues/179 +@six.add_metaclass(abc.ABCMeta) # pylint: disable=abstract-method class CommentNode(ParserNode): """ CommentNode class is used for representation of comments within the parsed @@ -167,20 +179,38 @@ class CommentNode(ParserNode): CommentNode stores its contents in class variable 'comment' and does not have a specific name. + CommentNode objects should have the following attributes in addition to + the ones described in ParserNode: + + # Contains the contents of the comment without the directive notation + # (typically # or /* ... */). + comment: str + """ - @property @abc.abstractmethod - def comment(self): # pragma: no cover + def __init__(self, **kwargs): """ - Comment property contains the contents of the comment without the comment - directives (typically # or /* ... */). + Initializes the CommentNode instance and sets its instance variables. - :returns: A string containing the comment - :rtype: str + :param comment: Contents of the comment. Required. + :type comment: str + + :param ancestor: BlockNode ancestor for this CommentNode. Required. + :type ancestor: BlockNode or None + + :param filepath: Filesystem path for the file where this CommentNode + does or should exist in the filesystem. Required. + :type filepath: str or None + + :param dirty: Boolean flag for denoting if this CommentNode has been + created or changed after the last save. Default: False. + :type dirty: bool """ + super(CommentNode, self).__init__(ancestor=kwargs['ancestor'], + dirty=kwargs.get('dirty', False), + filepath=kwargs['filepath']) # pragma: no cover - raise NotImplementedError @six.add_metaclass(abc.ABCMeta) class DirectiveNode(ParserNode): @@ -189,45 +219,61 @@ class DirectiveNode(ParserNode): It can have zero or more parameters attached to it. Because of the nature of single directives, it is not able to have child nodes and hence it is always treated as a leaf node. + + If a this directive was defined on the httpd command line, the ancestor instance + variable for this DirectiveNode should be None, and it should be inserted to the + beginning of root BlockNode children sequence. + + DirectiveNode objects should have the following attributes in addition to + the ones described in ParserNode: + + # True if this DirectiveNode is enabled and False if it is inside of an + # inactive conditional block. + enabled: bool + + # Name, or key of the configuration directive. If BlockNode subclass of + # DirectiveNode is the root configuration node, the name should be None. + name: Optional[str] + + # Tuple of parameters of this ParserNode object, excluding whitespaces. + parameters: Tuple[str, ...] + """ - @property @abc.abstractmethod - def enabled(self): # pragma: no cover + def __init__(self, **kwargs): """ - Configuration blocks may have conditional statements enabling or disabling - their contents. This property returns the state of this DirectiveNode. + Initializes the DirectiveNode instance and sets its instance variables. + + :param name: Name or key of the DirectiveNode object. Required. + :type name: str or None + + :param tuple parameters: Tuple of str parameters for this DirectiveNode. + Default: (). + :type parameters: tuple + + :param ancestor: BlockNode ancestor for this DirectiveNode, or None for + root configuration node. Required. + :type ancestor: BlockNode or None + + :param filepath: Filesystem path for the file where this DirectiveNode + does or should exist in the filesystem, or None for directives introduced + in the httpd command line. Required. + :type filepath: str or None + + :param dirty: Boolean flag for denoting if this DirectiveNode has been + created or changed after the last save. Default: False. + :type dirty: bool + + :param enabled: True if this DirectiveNode object is parsed in the active + configuration of the httpd. False if the DirectiveNode exists within a + unmatched conditional configuration block. Default: True. + :type enabled: bool - :returns: True if the DirectiveNode is parsed and enabled in the configuration. - :rtype: bool """ - - raise NotImplementedError - - @property - @abc.abstractmethod - def name(self): # pragma: no cover - """ - Name property contains the name of the directive. - - :returns: Name of this node - :rtype: str - """ - - raise NotImplementedError - - @property - @abc.abstractmethod - def parameters(self): # pragma: no cover - """ - This property contains a tuple of parameters of this ParserNode object - excluding whitespaces. - - :returns: A tuple of parameters for this node - :rtype: tuple - """ - - raise NotImplementedError + super(DirectiveNode, self).__init__(ancestor=kwargs['ancestor'], + dirty=kwargs.get('dirty', False), + filepath=kwargs['filepath']) # pragma: no cover @abc.abstractmethod def set_parameters(self, parameters): @@ -242,7 +288,7 @@ class DirectiveNode(ParserNode): @six.add_metaclass(abc.ABCMeta) -class BlockNode(ParserNode): +class BlockNode(DirectiveNode): """ BlockNode class represents a block of nested configuration directives, comments and other blocks as its children. A BlockNode can have zero or more parameters @@ -272,6 +318,15 @@ class BlockNode(ParserNode): The applicable parameters are dependent on the underlying configuration language and its grammar. + + BlockNode objects should have the following attributes in addition to + the ones described in DirectiveNode: + + # Tuple of direct children of this BlockNode object. The order of children + # in this tuple retain the order of elements in the parsed configuration + # block. + children: Tuple[ParserNode, ...] + """ @abc.abstractmethod @@ -335,33 +390,6 @@ class BlockNode(ParserNode): """ - @property - @abc.abstractmethod - def children(self): # pragma: no cover - """ - This property contains a list ParserNode objects that are the children - for this node. The order of children is the same as that of the parsed - configuration block. - - :returns: A tuple of this block's children - :rtype: tuple - """ - - raise NotImplementedError - - @property - @abc.abstractmethod - def enabled(self): - """ - Configuration blocks may have conditional statements enabling or disabling - their contents. This property returns the state of this configuration block. - In case of unmatched conditional statement in block, this block itself should - be set enabled while its children should be set disabled. - - :returns: True if the BlockNode is parsed and enabled in the configuration. - :rtype: bool - """ - @abc.abstractmethod def find_blocks(self, name, exclude=True): """ @@ -424,44 +452,6 @@ class BlockNode(ParserNode): of children of the callee. """ - @property - @abc.abstractmethod - def name(self): # pragma: no cover - """ - Name property contains the name of the block. As an example for config: - ... - the name would be "VirtualHost". - - :returns: Name of this node - :rtype: str - """ - - raise NotImplementedError - - @property - @abc.abstractmethod - def parameters(self): # pragma: no cover - """ - This property contains a tuple of parameters of this ParserNode object - excluding whitespaces. - - :returns: A tuple of parameters for this node - :rtype: tuple - """ - - raise NotImplementedError - - @abc.abstractmethod - def set_parameters(self, parameters): - """ - Sets the sequence of parameters for this ParserNode object without - whitespaces. While the whitespaces for parameters are discarded when using - this method, the whitespacing preceeding the ParserNode itself should be - kept intact. - - :param list parameters: sequence of parameters - """ - @abc.abstractmethod def unsaved_files(self): """ diff --git a/certbot-apache/certbot_apache/parsernode_util.py b/certbot-apache/certbot_apache/parsernode_util.py new file mode 100644 index 000000000..2450a1c15 --- /dev/null +++ b/certbot-apache/certbot_apache/parsernode_util.py @@ -0,0 +1,85 @@ +"""ParserNode utils""" + + +def validate_kwargs(kwargs, required_names): + """ + Ensures that the kwargs dict has all the expected values. This function modifies + the kwargs dictionary, and hence the returned dictionary should be used instead + in the caller function instead of the original kwargs. + + :param dict kwargs: Dictionary of keyword arguments to validate. + :param list required_names: List of required parameter names. + """ + + validated_kwargs = dict() + for name in required_names: + try: + validated_kwargs[name] = kwargs.pop(name) + except KeyError: + raise TypeError("Required keyword argument: {} undefined.".format(name)) + + # Raise exception if unknown key word arguments are found. + if kwargs: + unknown = ", ".join(kwargs.keys()) + raise TypeError("Unknown keyword argument(s): {}".format(unknown)) + return validated_kwargs + + +def parsernode_kwargs(kwargs): + """ + Validates keyword arguments for ParserNode. This function modifies the kwargs + dictionary, and hence the returned dictionary should be used instead in the + caller function instead of the original kwargs. + + + :param dict kwargs: Keyword argument dictionary to validate. + + :returns: Tuple of validated and prepared arguments. + """ + kwargs.setdefault("dirty", False) + kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath"]) + return kwargs["ancestor"], kwargs["dirty"], kwargs["filepath"] + + +def commentnode_kwargs(kwargs): + """ + Validates keyword arguments for CommentNode and sets the default values for + optional kwargs. This function modifies the kwargs dictionary, and hence the + returned dictionary should be used instead in the caller function instead of + the original kwargs. + + + :param dict kwargs: Keyword argument dictionary to validate. + + :returns: Tuple of validated and prepared arguments and ParserNode kwargs. + """ + kwargs.setdefault("dirty", False) + kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath", "comment"]) + + comment = kwargs.pop("comment") + return comment, kwargs + + +def directivenode_kwargs(kwargs): + """ + Validates keyword arguments for DirectiveNode and BlockNode and sets the + default values for optional kwargs. This function modifies the kwargs + dictionary, and hence the returned dictionary should be used instead in the + caller function instead of the original kwargs. + + :param dict kwargs: Keyword argument dictionary to validate. + + :returns: Tuple of validated and prepared arguments and ParserNode kwargs. + """ + + kwargs.setdefault("dirty", False) + kwargs.setdefault("enabled", True) + kwargs.setdefault("parameters", ()) + + kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath", "name", + "parameters", "enabled"]) + + name = kwargs.pop("name") + parameters = kwargs.pop("parameters") + enabled = kwargs.pop("enabled") + return name, parameters, enabled, kwargs diff --git a/certbot-apache/certbot_apache/tests/parsernode_test.py b/certbot-apache/certbot_apache/tests/parsernode_test.py index 6b45a4d6d..0fba32b98 100644 --- a/certbot-apache/certbot_apache/tests/parsernode_test.py +++ b/certbot-apache/certbot_apache/tests/parsernode_test.py @@ -2,84 +2,121 @@ import unittest -from acme.magic_typing import Optional, Tuple # pylint: disable=unused-import, no-name-in-module - from certbot_apache import interfaces +from certbot_apache import parsernode_util as util +class DummyParserNode(interfaces.ParserNode): + """ A dummy class implementing ParserNode interface """ -class DummyCommentNode(interfaces.CommentNode): + def __init__(self, **kwargs): + """ + Initializes the ParserNode instance. + """ + ancestor, dirty, filepath = util.parsernode_kwargs(kwargs) + self.ancestor = ancestor + self.dirty = dirty + self.filepath = filepath + super(DummyParserNode, self).__init__(**kwargs) + + def save(self, msg): # pragma: no cover + """Save""" + pass + + +class DummyCommentNode(DummyParserNode): """ A dummy class implementing CommentNode interface """ - ancestor = None - comment = "" - dirty = False - def save(self, msg): # pragma: no cover - pass + def __init__(self, **kwargs): + """ + Initializes the CommentNode instance and sets its instance variables. + """ + comment, kwargs = util.commentnode_kwargs(kwargs) + self.comment = comment + super(DummyCommentNode, self).__init__(**kwargs) -class DummyDirectiveNode(interfaces.DirectiveNode): +class DummyDirectiveNode(DummyParserNode): """ A dummy class implementing DirectiveNode interface """ - ancestor = None - parameters = tuple() # type: Tuple[str, ...] - dirty = False - enabled = True - name = "" - def save(self, msg): # pragma: no cover - pass + # pylint: disable=too-many-arguments + def __init__(self, **kwargs): + """ + Initializes the DirectiveNode instance and sets its instance variables. + """ + name, parameters, enabled, kwargs = util.directivenode_kwargs(kwargs) + self.name = name + self.parameters = parameters + self.enabled = enabled + + super(DummyDirectiveNode, self).__init__(**kwargs) def set_parameters(self, parameters): # pragma: no cover + """Set parameters""" pass -class DummyBlockNode(interfaces.BlockNode): +class DummyBlockNode(DummyDirectiveNode): """ A dummy class implementing BlockNode interface """ - ancestor = None - parameters = tuple() # type: Tuple[str, ...] - children = tuple() # type: Tuple[interfaces.ParserNode, ...] - dirty = False - enabled = True - name = "" - - def save(self, msg): # pragma: no cover - pass def add_child_block(self, name, parameters=None, position=None): # pragma: no cover + """Add child block""" pass def add_child_directive(self, name, parameters=None, position=None): # pragma: no cover + """Add child directive""" pass def add_child_comment(self, comment="", position=None): # pragma: no cover + """Add child comment""" pass def find_blocks(self, name, exclude=True): # pragma: no cover + """Find blocks""" pass def find_directives(self, name, exclude=True): # pragma: no cover + """Find directives""" pass def find_comments(self, comment, exact=False): # pragma: no cover + """Find comments""" pass def delete_child(self, child): # pragma: no cover - pass - - def set_parameters(self, parameters): # pragma: no cover + """Delete child""" pass def unsaved_files(self): # pragma: no cover + """Unsaved files""" pass +interfaces.CommentNode.register(DummyCommentNode) +interfaces.DirectiveNode.register(DummyDirectiveNode) +interfaces.BlockNode.register(DummyBlockNode) + class ParserNodeTest(unittest.TestCase): """Dummy placeholder test case for ParserNode interfaces""" def test_dummy(self): - dummyblock = DummyBlockNode() - dummydirective = DummyDirectiveNode() - dummycomment = DummyCommentNode() + dummyblock = DummyBlockNode( + name="None", + parameters=(), + ancestor=None, + dirty=False, + filepath="/some/random/path" + ) + dummydirective = DummyDirectiveNode( + name="Name", + ancestor=None, + filepath="/another/path" + ) + dummycomment = DummyCommentNode( + comment="Comment", + ancestor=dummyblock, + filepath="/some/file" + ) if __name__ == "__main__": diff --git a/certbot-apache/certbot_apache/tests/parsernode_util_test.py b/certbot-apache/certbot_apache/tests/parsernode_util_test.py new file mode 100644 index 000000000..acb7a92db --- /dev/null +++ b/certbot-apache/certbot_apache/tests/parsernode_util_test.py @@ -0,0 +1,87 @@ +""" Tests for ParserNode utils """ +import unittest + +from certbot_apache import parsernode_util as util + + +class ParserNodeUtilTest(unittest.TestCase): + """Tests for ParserNode utils""" + + def _setup_parsernode(self): + """ Sets up kwargs dict for ParserNode """ + return { + "ancestor": None, + "dirty": False, + "filepath": "/tmp", + } + + def _setup_commentnode(self): + """ Sets up kwargs dict for CommentNode """ + + pn = self._setup_parsernode() + pn["comment"] = "x" + return pn + + def _setup_directivenode(self): + """ Sets up kwargs dict for DirectiveNode """ + + pn = self._setup_parsernode() + pn["name"] = "Name" + pn["parameters"] = ("first",) + pn["enabled"] = True + return pn + + def test_unknown_parameter(self): + params = self._setup_parsernode() + params["unknown"] = "unknown" + self.assertRaises(TypeError, util.parsernode_kwargs, params) + + params = self._setup_commentnode() + params["unknown"] = "unknown" + self.assertRaises(TypeError, util.commentnode_kwargs, params) + + params = self._setup_directivenode() + params["unknown"] = "unknown" + self.assertRaises(TypeError, util.directivenode_kwargs, params) + + def test_parsernode(self): + params = self._setup_parsernode() + ctrl = self._setup_parsernode() + + ancestor, dirty, filepath = util.parsernode_kwargs(params) + self.assertEqual(ancestor, ctrl["ancestor"]) + self.assertEqual(dirty, ctrl["dirty"]) + self.assertEqual(filepath, ctrl["filepath"]) + + def test_commentnode(self): + params = self._setup_commentnode() + ctrl = self._setup_commentnode() + + comment, _ = util.commentnode_kwargs(params) + self.assertEqual(comment, ctrl["comment"]) + + def test_directivenode(self): + params = self._setup_directivenode() + ctrl = self._setup_directivenode() + + name, parameters, enabled, _ = util.directivenode_kwargs(params) + self.assertEqual(name, ctrl["name"]) + self.assertEqual(parameters, ctrl["parameters"]) + self.assertEqual(enabled, ctrl["enabled"]) + + def test_missing_required(self): + c_params = self._setup_commentnode() + c_params.pop("comment") + self.assertRaises(TypeError, util.commentnode_kwargs, c_params) + + d_params = self._setup_directivenode() + d_params.pop("ancestor") + self.assertRaises(TypeError, util.directivenode_kwargs, d_params) + + p_params = self._setup_parsernode() + p_params.pop("filepath") + self.assertRaises(TypeError, util.parsernode_kwargs, p_params) + + +if __name__ == "__main__": + unittest.main() # pragma: no cover