Refactor _add_directive into separate functions (#5786)

* Refactor _add_directive to separate functions

* UnspacedList isn't idempotent

* refactor parser in add_server_directives and update_or_add_server_directives

* update parser tests

* remove replace=False and add to update_or_add for replace=True in configurator

* remove replace=False and add to update_or_add for replace=True in http01

* update documentation

certbot-nginx/certbot_nginx/configurator.py

@@ -192,8 +192,8 @@ class NginxConfigurator(common.Installer):
         cert_directives = [['\n    ', 'ssl_certificate', ' ', fullchain_path],
                            ['\n    ', 'ssl_certificate_key', ' ', key_path]]
 
-        self.parser.add_server_directives(vhost,
+        self.parser.update_or_add_server_directives(vhost,
-                                          cert_directives, replace=True)
+                                          cert_directives)
         logger.info("Deploying Certificate to VirtualHost %s", vhost.filep)
 
         self.save_notes += ("Changed vhost at %s with addresses of %s\n" %
@@ -344,7 +344,7 @@ class NginxConfigurator(common.Installer):
         for name in vhost.names:
             name_block[0].append(' ')
             name_block[0].append(name)
-        self.parser.add_server_directives(vhost, name_block, replace=True)
+        self.parser.update_or_add_server_directives(vhost, name_block)
 
     def _get_default_vhost(self, port):
         vhost_list = self.parser.get_vhosts()
@@ -584,7 +584,7 @@ class NginxConfigurator(common.Installer):
         # have it continue to do so.
         if len(vhost.addrs) == 0:
             listen_block = [['\n    ', 'listen', ' ', self.DEFAULT_LISTEN_PORT]]
-            self.parser.add_server_directives(vhost, listen_block, replace=False)
+            self.parser.add_server_directives(vhost, listen_block)
 
         if vhost.ipv6_enabled():
             ipv6_block = ['\n    ',
@@ -618,7 +618,7 @@ class NginxConfigurator(common.Installer):
         ])
 
         self.parser.add_server_directives(
-            vhost, ssl_block, replace=False)
+            vhost, ssl_block)
 
     ##################################
     # enhancement methods (IInstaller)
@@ -683,7 +683,7 @@ class NginxConfigurator(common.Installer):
                 ['\n    ', 'add_header', ' ', header_substring, ' '] +
                     constants.HEADER_ARGS[header_substring],
                 ['\n']]
-            self.parser.add_server_directives(vhost, header_directives, replace=False)
+            self.parser.add_server_directives(vhost, header_directives)
 
     def _add_redirect_block(self, vhost, domain):
         """Add redirect directive to vhost
@@ -691,7 +691,7 @@ class NginxConfigurator(common.Installer):
         redirect_block = _redirect_block_for_domain(domain)
 
         self.parser.add_server_directives(
-            vhost, redirect_block, replace=False, insert_at_top=True)
+            vhost, redirect_block, insert_at_top=True)
 
     def _split_block(self, vhost, only_directives=None):
         """Splits this "virtual host" (i.e. this nginx server block) into
@@ -771,7 +771,7 @@ class NginxConfigurator(common.Installer):
 
             # Add this at the bottom to get the right order of directives
             return_404_directive = [['\n    ', 'return', ' ', '404']]
-            self.parser.add_server_directives(http_vhost, return_404_directive, replace=False)
+            self.parser.add_server_directives(http_vhost, return_404_directive)
 
             vhost = http_vhost
 
@@ -821,7 +821,7 @@ class NginxConfigurator(common.Installer):
 
         try:
             self.parser.add_server_directives(vhost,
-                                              stapling_directives, replace=False)
+                                              stapling_directives)
         except errors.MisconfigurationError as error:
             logger.debug(error)
             raise errors.PluginError("An error occurred while enabling OCSP "

certbot-nginx/certbot_nginx/http_01.py

@@ -199,9 +199,9 @@ class NginxHttp01(common.ChallengePerformer):
                                 ['return', ' ', '200', ' ', validation]]]]
 
         self.configurator.parser.add_server_directives(vhost,
-            location_directive, replace=False)
+            location_directive)
 
         rewrite_directive = [['rewrite', ' ', '^(/.well-known/acme-challenge/.*)',
                                 ' ', '$1', ' ', 'break']]
         self.configurator.parser.add_server_directives(vhost,
-            rewrite_directive, replace=False, insert_at_top=True)
+            rewrite_directive, insert_at_top=True)

certbot-nginx/certbot_nginx/parser.py

@@ -276,15 +276,13 @@ class NginxParser(object):
 
         return False
 
-    def add_server_directives(self, vhost, directives, replace, insert_at_top=False):
+    def add_server_directives(self, vhost, directives, insert_at_top=False):
-        """Add or replace directives in the server block identified by vhost.
+        """Add directives to the server block identified by vhost.
 
         This method modifies vhost to be fully consistent with the new directives.
 
-        ..note :: If replace is True and the directive already exists, the first
+        ..note :: It's an error to try and add a nonrepeatable directive that already
-        instance will be replaced. Otherwise, the directive is added.
+            exists in the config block with a conflicting value.
-        ..note :: If replace is False nothing gets added if an identical
-        block exists already.
 
         ..todo :: Doesn't match server blocks whose server_name directives are
             split across multiple conf files.
@@ -292,13 +290,34 @@ class NginxParser(object):
         :param :class:`~certbot_nginx.obj.VirtualHost` vhost: The vhost
             whose information we use to match on
         :param list directives: The directives to add
-        :param bool replace: Whether to only replace existing directives
         :param bool insert_at_top: True if the directives need to be inserted at the top
             of the server block instead of the bottom
 
         """
         self._modify_server_directives(vhost,
-            functools.partial(_add_directives, directives, replace, insert_at_top))
+            functools.partial(_add_directives, directives, insert_at_top))
+
+    def update_or_add_server_directives(self, vhost, directives, insert_at_top=False):
+        """Add or replace directives in the server block identified by vhost.
+
+        This method modifies vhost to be fully consistent with the new directives.
+
+        ..note :: When a directive with the same name already exists in the
+        config block, the first instance will be replaced. Otherwise, the directive
+        will be appended/prepended to the config block as in add_server_directives.
+
+        ..todo :: Doesn't match server blocks whose server_name directives are
+            split across multiple conf files.
+
+        :param :class:`~certbot_nginx.obj.VirtualHost` vhost: The vhost
+            whose information we use to match on
+        :param list directives: The directives to add
+        :param bool insert_at_top: True if the directives need to be inserted at the top
+            of the server block instead of the bottom
+
+        """
+        self._modify_server_directives(vhost,
+            functools.partial(_update_or_add_directives, directives, insert_at_top))
 
     def remove_server_directives(self, vhost, directive_name, match_func=None):
         """Remove all directives of type directive_name.
@@ -524,26 +543,17 @@ def _is_ssl_on_directive(entry):
             len(entry) == 2 and entry[0] == 'ssl' and
             entry[1] == 'on')
 
-def _add_directives(directives, replace, insert_at_top, block):
+def _add_directives(directives, insert_at_top, block):
-    """Adds or replaces directives in a config block.
+    """Adds directives to a config block."""
-
-    When replace=False, it's an error to try and add a nonrepeatable directive that already
-    exists in the config block with a conflicting value.
-
-    When replace=True and a directive with the same name already exists in the
-    config block, the first instance will be replaced. Otherwise, the directive
-    will be added to the config block.
-
-    ..todo :: Find directives that are in included files.
-
-    :param list directives: The new directives.
-    :param bool replace: Described above.
-    :param bool insert_at_top: Described above.
-    :param list block: The block to replace in
-
-    """
     for directive in directives:
-        _add_directive(block, directive, replace, insert_at_top)
+        _add_directive(block, directive, insert_at_top)
+    if block and '\n' not in block[-1]:  # could be "   \n  " or ["\n"] !
+        block.append(nginxparser.UnspacedList('\n'))
+
+def _update_or_add_directives(directives, insert_at_top, block):
+    """Adds or replaces directives in a config block."""
+    for directive in directives:
+        _update_or_add_directive(block, directive, insert_at_top)
     if block and '\n' not in block[-1]:  # could be "   \n  " or ["\n"] !
         block.append(nginxparser.UnspacedList('\n'))
 
@@ -601,28 +611,20 @@ def _find_location(block, directive_name, match_func=None):
     return next((index for index, line in enumerate(block) \
         if line and line[0] == directive_name and (match_func is None or match_func(line))), None)
 
-def _add_directive(block, directive, replace, insert_at_top):
+def _is_whitespace_or_comment(directive):
-    """Adds or replaces a single directive in a config block.
+    """Is this directive either a whitespace or comment directive?"""
+    return len(directive) == 0 or directive[0] == '#'
 
-    See _add_directives for more documentation.
+def _add_directive(block, directive, insert_at_top):
-
+    if not isinstance(directive, nginxparser.UnspacedList):
-    """
+        directive = nginxparser.UnspacedList(directive)
-    directive = nginxparser.UnspacedList(directive)
+    if _is_whitespace_or_comment(directive):
-    def is_whitespace_or_comment(directive):
-        """Is this directive either a whitespace or comment directive?"""
-        return len(directive) == 0 or directive[0] == '#'
-    if is_whitespace_or_comment(directive):
         # whitespace or comment
         block.append(directive)
         return
 
     location = _find_location(block, directive[0])
 
-    if replace:
-        if location is not None:
-            block[location] = directive
-            comment_directive(block, location)
-            return
     # Append or prepend directive. Fail if the name is not a repeatable directive name,
     # and there is already a copy of that directive with a different value
     # in the config file.
@@ -647,7 +649,7 @@ def _add_directive(block, directive, replace, insert_at_top):
         for included_directive in included_directives:
             included_dir_loc = _find_location(block, included_directive[0])
             included_dir_name = included_directive[0]
-            if not is_whitespace_or_comment(included_directive) \
+            if not _is_whitespace_or_comment(included_directive) \
                 and not can_append(included_dir_loc, included_dir_name):
                 if block[included_dir_loc] != included_directive:
                     raise errors.MisconfigurationError(err_fmt.format(included_directive,
@@ -668,6 +670,27 @@ def _add_directive(block, directive, replace, insert_at_top):
     elif block[location] != directive:
         raise errors.MisconfigurationError(err_fmt.format(directive, block[location]))
 
+def _update_directive(block, directive, location):
+    block[location] = directive
+    comment_directive(block, location)
+
+def _update_or_add_directive(block, directive, insert_at_top):
+    if not isinstance(directive, nginxparser.UnspacedList):
+        directive = nginxparser.UnspacedList(directive)
+    if _is_whitespace_or_comment(directive):
+        # whitespace or comment
+        block.append(directive)
+        return
+
+    location = _find_location(block, directive[0])
+
+    # we can update directive
+    if location is not None:
+        _update_directive(block, directive, location)
+        return
+
+    _add_directive(block, directive, insert_at_top)
+
 def _is_certbot_comment(directive):
     return '#' in directive and COMMENT in directive
 

certbot-nginx/certbot_nginx/tests/configurator_test.py

@@ -113,8 +113,7 @@ class NginxConfiguratorTest(util.NginxTest):
                                      None, [0])
         self.config.parser.add_server_directives(
             mock_vhost,
-            [['listen', ' ', '5001', ' ', 'ssl']],
+            [['listen', ' ', '5001', ' ', 'ssl']])
-            replace=False)
         self.config.save()
 
         # pylint: disable=protected-access
@@ -206,9 +205,9 @@ class NginxConfiguratorTest(util.NginxTest):
             "example/chain.pem",
             None)
 
-    @mock.patch('certbot_nginx.parser.NginxParser.add_server_directives')
+    @mock.patch('certbot_nginx.parser.NginxParser.update_or_add_server_directives')
-    def test_deploy_cert_raise_on_add_error(self, mock_add_server_directives):
+    def test_deploy_cert_raise_on_add_error(self, mock_update_or_add_server_directives):
-        mock_add_server_directives.side_effect = errors.MisconfigurationError()
+        mock_update_or_add_server_directives.side_effect = errors.MisconfigurationError()
         self.assertRaises(
             errors.PluginError,
             self.config.deploy_cert,

certbot-nginx/certbot_nginx/tests/parser_test.py

@@ -206,8 +206,7 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods
         mock_vhost.path = [0]
         nparser.add_server_directives(mock_vhost,
                                       [['foo', 'bar'], ['ssl_certificate',
-                                                        '/etc/ssl/cert2.pem']],
+                                                        '/etc/ssl/cert2.pem']])
-                                      replace=False)
         nparser.remove_server_directives(mock_vhost, 'foo')
         nparser.remove_server_directives(mock_vhost, 'ssl_certificate')
         self.assertEqual(nparser.parsed[example_com],
@@ -226,8 +225,7 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods
                                      None, [10, 1, 9])
         nparser.add_server_directives(mock_vhost,
                                       [['foo', 'bar'], ['\n ', 'ssl_certificate', ' ',
-                                                        '/etc/ssl/cert.pem']],
+                                                        '/etc/ssl/cert.pem']])
-                                      replace=False)
         ssl_re = re.compile(r'\n\s+ssl_certificate /etc/ssl/cert.pem')
         dump = nginxparser.dumps(nparser.parsed[nparser.abs_path('nginx.conf')])
         self.assertEqual(1, len(re.findall(ssl_re, dump)))
@@ -239,10 +237,8 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods
         mock_vhost.path = [0]
         nparser.add_server_directives(mock_vhost,
                                       [['foo', 'bar'], ['ssl_certificate',
-                                                        '/etc/ssl/cert2.pem']],
+                                                        '/etc/ssl/cert2.pem']])
-                                      replace=False)
+        nparser.add_server_directives(mock_vhost, [['foo', 'bar']])
-        nparser.add_server_directives(mock_vhost, [['foo', 'bar']],
-                                      replace=False)
         from certbot_nginx.parser import COMMENT
         self.assertEqual(nparser.parsed[example_com],
             [[['server'], [['listen', '69.50.225.155:9000'],
@@ -264,8 +260,7 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods
                           nparser.add_server_directives,
                           mock_vhost,
                           [['foo', 'bar'],
-                           ['ssl_certificate', '/etc/ssl/cert2.pem']],
+                           ['ssl_certificate', '/etc/ssl/cert2.pem']])
-                          replace=False)
 
     def test_comment_is_repeatable(self):
         nparser = parser.NginxParser(self.config_path)
@@ -275,12 +270,10 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods
                                      set(['.example.com', 'example.*']),
                                      None, [0])
         nparser.add_server_directives(mock_vhost,
-                                      [['\n  ', '#', ' ', 'what a nice comment']],
+                                      [['\n  ', '#', ' ', 'what a nice comment']])
-                                      replace=False)
         nparser.add_server_directives(mock_vhost,
                                       [['\n  ', 'include', ' ',
-                                      nparser.abs_path('comment_in_file.conf')]],
+                                      nparser.abs_path('comment_in_file.conf')]])
-                                      replace=False)
         from certbot_nginx.parser import COMMENT
         self.assertEqual(nparser.parsed[example_com],
             [[['server'], [['listen', '69.50.225.155:9000'],
@@ -299,8 +292,8 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods
         target = set(['.example.com', 'example.*'])
         filep = nparser.abs_path('sites-enabled/example.com')
         mock_vhost = obj.VirtualHost(filep, None, None, None, target, None, [0])
-        nparser.add_server_directives(
+        nparser.update_or_add_server_directives(
-            mock_vhost, [['server_name', 'foobar.com']], replace=True)
+            mock_vhost, [['server_name', 'foobar.com']])
         from certbot_nginx.parser import COMMENT
         self.assertEqual(
             nparser.parsed[filep],
@@ -310,8 +303,8 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods
                            ['server_name', 'example.*'], []
                            ]]])
         mock_vhost.names = set(['foobar.com', 'example.*'])
-        nparser.add_server_directives(
+        nparser.update_or_add_server_directives(
-            mock_vhost, [['ssl_certificate', 'cert.pem']], replace=True)
+            mock_vhost, [['ssl_certificate', 'cert.pem']])
         self.assertEqual(
             nparser.parsed[filep],
             [[['server'], [['listen', '69.50.225.155:9000'],