Issue 2711 - Refactored ChainedConfigParser, allowing manipulation of list items

chainedconfigparser.py

 # coding: utf-8
 
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import os
-import re
+import io
 import ConfigParser
+from StringIO import StringIO
 
 class Item(tuple):
   def __new__(cls, name, value, source):
     result = super(Item, cls).__new__(cls, (name, value))
     result.source = source
     return result
+
+class DiffForUnknownOptionError(ConfigParser.Error):
+  def __init__(self, option, section):
+    ConfigParser.Error.__init__(self, 'Failed to apply diff for unknown option '
+                                      '%r in section %r' % (option, section))
+    self.option = option
+    self.section = section
+    self.args = (option, section)
 
 class ChainedConfigParser(ConfigParser.SafeConfigParser):
   '''
     This class provides essentially the same interfaces as SafeConfigParser but
     allows chaining configuration files so that one config file provides the
     default values for the other. To specify the config file to inherit from
     a config file needs to contain the following option:
 
     [default]
     inherit = foo/bar.config
 
+    It is also possible to add values to or remove values from
+    whitespace-separated lists given by an inherited option:
+
+    [section]
+    opt1 += foo
+    opt2 -= bar
+
     The value of the inherit option has to be a relative path with forward
     slashes as delimiters. Up to 5 configuration files can be chained this way,
     longer chains are disallowed to deal with circular references.
 
-    A main API difference to SafeConfigParser is the way a class instance is
-    constructed: a file path has to be passed, this file is assumed to be
-    encoded as UTF-8. Also, ChainedConfigParser data is read-only and the
-    options are case-sensitive. An additional option_source(section, option)
-    method is provided to get the path of the configuration file defining this
-    option (for relative paths). Items returned by the items() function also
-    have a source attribute serving the same purpose.

[Wladimir Palant, 2015/06/23 09:43:36]

Extend documentation to mention the new += and -= syntax?

[Sebastian Noack, 2015/06/24 08:51:09]

Done.

+    As opposed to SafeConfigParser, files are decoded as UTF-8 while
+    reading. Also, ChainedConfigParser data is read-only. An additional
+    option_source(section, option) method is provided to get the path
+    of the configuration file defining this option (for relative paths).
+    Items returned by the items() function also have a source attribute
+    serving the same purpose.
   '''
 
   def __init__(self):
     ConfigParser.SafeConfigParser.__init__(self)
     self._origin = {}
 
-  def _get_parser_chain(self, file, filename):
+  def _make_parser(self, filename):
+    parser = ConfigParser.SafeConfigParser()
+    parser.optionxform = lambda option: option
+
+    with io.open(filename, encoding='utf-8') as file:
+      parser.readfp(file, filename)
+
+    return parser
+
+  def _get_parser_chain(self, parser, filename):
     parsers = []
-
-    parser = ConfigParser.SafeConfigParser()
-    parser._read(file, filename)

[Wladimir Palant, 2015/06/23 09:43:35]

Please use public API here:

  parser.readfp(file, filename)

[Sebastian Noack, 2015/06/24 08:51:10]

This code has been gone while addressing the comment below.

 
     while True:
       parsers.insert(0, (parser, filename))
 
       try:
         inherit = parser.get('default', 'inherit')
       except (ConfigParser.NoSectionError, ConfigParser.NoOptionError):
         return parsers
 
       filename = os.path.join(os.path.dirname(filename), *inherit.split('/'))
-      parser = ConfigParser.SafeConfigParser()
-      parser.read(filename)
+      parser = self._make_parser(filename)
 
   def _apply_diff(self, section, option, value):
-    addition = option.endswith('+')
-    removal  = option.endswith('-')
+    is_addition = option.endswith('+')
+    is_diff = is_addition or option.endswith('-')
 
-    if addition or removal:
+    if is_diff:
       option = option[:-1].rstrip()
-      old_value = self.get(section, option)

[Wladimir Palant, 2015/06/23 09:43:36]

This will throw when trying to change an option that doesn't exist yet. While we
probably don't want to sweep this condition under the carpet, a meaningful
warning would do better IMHO.

[Sebastian Noack, 2015/06/24 08:51:09]

Done.

+      try:
+        orig_value = self.get(section, option)
+      except ConfigParser.NoOptionError:
+        raise DiffForUnknownOptionError(option, section)
 
-      if addition:
-        value = '%s %s' % (old_value, value)

[Wladimir Palant, 2015/06/23 09:43:35]

I don't think that we want duplicate values - we are essentially treating the
option as a set. So adding a value that is already there shouldn't have an
effect. Maybe:

  existing = old_value.split()
  value = ' '.join(existing + [v for v in value.split() if v not in existing])

[Sebastian Noack, 2015/06/24 08:51:10]

Done.

-      elif removal:

[Wladimir Palant, 2015/06/23 09:43:35]

Nit: no need for elif, can be simply else. But I don't mind leaving it as is for
readability.

[Sebastian Noack, 2015/06/24 08:51:09]

Done.

-        value = re.sub(r'\b(?:%s)\b\s*' % '|'.join(map(re.escape, value.split())), '', old_value).rstrip()

[Wladimir Palant, 2015/06/23 09:43:35]

I don't think that regular expressions are the right tool here. How about:

  blacklist = value.split()
  value = ' '.join([v for v in old_value.split() if v not in blacklist])

[Sebastian Noack, 2015/06/24 08:51:09]

Done.

+      orig_values = orig_value.split()
+      diff_values = value.split()
 
-    return option, value
+      if is_addition:
+        new_values = orig_values + [v for v in diff_values if v not in orig_values]
+      else:
+        new_values = [v for v in orig_values if v not in diff_values]
 
-  def _read(self, file, filename):

[Wladimir Palant, 2015/06/23 09:43:35]

Is it really a good idea to override private methods? There is relatively little
danger of this one changing of course but still. IMHO we should be overriding
public API - meaning read() and readfp(). This won't really add much to the
complexity.

[Sebastian Noack, 2015/06/24 08:51:10]

Overriding read() and readfp() would require quite some additional complexity
and duplication. I went for only supporting read() now, which doesn't seem to be
too bad.

-    parsers = self._get_parser_chain(file, filename)
+      value = ' '.join(new_values)
 
+    return is_diff, option, value
+
+  def _process_parsers(self, parsers):
     for parser, filename in parsers:
       for section in parser.sections():
+        if not self.has_section(section):
+          try:
+            ConfigParser.SafeConfigParser.add_section(self, section)
+          except ValueError:
+            # add_section() hardcodes 'default' and raises a ValueError if
+            # you try to add a section called like that (case insensitive).
+            # This bug has been fixed in Python 3.
+            ConfigParser.SafeConfigParser.readfp(self, StringIO('[%s]' % section))
+
         for option, value in parser.items(section):
-          option, value = self._apply_diff(section, option, value)
-          try:
-            self.set(section, option, value)
-          except ConfigParser.NoSectionError:
-            try:
-              self.add_section(section)
-            except ValueError:
-              # add_section() hardcodes 'default' and raises a ValueError if
-              # you try to add a section called like that (case insensitive).
-              # This bug has been fixed in Python 3.
-              self._sections[section] = self._dict()

[Wladimir Palant, 2015/06/23 09:43:36]

I cannot say that I like this hack. How about simply skipping section 'default'?
There is lots of magic attached to it, I don't think we want it for metadata
files. Would also make sense to have the "inherit" option no longer visible in
the result.

[Sebastian Noack, 2015/06/24 08:51:09]

"default" != "DEFAULT". The latter is handled specially and is already ignored
by our code. However, due to a bug, add_section() checks for "DEFAULT"
case-insensitive, and prevents adding a section called "default", which is just
handled as a regular section by the rest of the API.

[Wladimir Palant, 2015/06/25 14:11:40]

I see. Still, should we access private variables in order to work around Python
bugs? How about throwing a more meaningful error here: raise
ValueError("SafeConfigParser.add_section() failed to add section %s, please use
a different section name" % section)

[Sebastian Noack, 2015/06/25 16:06:28]

The except block will only be reached in Python versions that have this bug. So
I don't see any problem with using internal APIs to workaround the bug here.

[Wladimir Palant, 2015/06/25 16:12:28]

That is: every Python version we support. And I'm pretty sure that there is a
reason why this is documented rather than fixed - it's a compatibility
constraint. There will never be a Python 2.x version without that bug.
The problem is that we might make it worse, e.g. by producing an unexpected data
structure that will fail further down without an obvious connection to this
piece of code. As I said, why not just produce a meaningful error message
instead of messing with internal data structures?

[Sebastian Noack, 2015/06/25 23:05:31]

In case I didn't stress this enough, this is a bug! The reason they didn't fix
it in Python 2.7 could be that they are in maintenance mode for years only,
fixing security issues, moving all improvements to Python 3 where this is fixed.
Another reason could be that they don't want to break the behavior of existing
APIs. And I agree there is no reason to have ChainedConfigParser behave
differently. But my fix doesn't. This behavior is only implemented for the set()
method, which isn't even implemented for external use by ChainedConfigParser.
The code here merely assist to implement the read() method, which correctly
parsed sections called "default" also when using SafeConfigParser. The line here
is equivalent to: SafeConfigParser.readfp(self, StringIO("[%s]" % section))

[Wladimir Palant, 2015/06/26 13:17:21]

It doesn't matter what you call it - it's documented meaning that it is a
compatibility constraint. This behavior won't be fixed, not that it is really
important for us.
Sure, at least not in the current Python version. You are making huge
assumptions here, for no good reason.
If that works then why not just use that line and get rid of private APIs? I
don't think we are terribly concerned about performance for this extremely
unlikely scenario.

[Sebastian Noack, 2015/06/26 13:37:53]

So your concerns here are only about using internal APIs. Then I got you wrong
in my previous reply. Fine with me using SafeConfigParser.readf() then instead.
I just realized last night that we could actually do that.

-            self.set(section, option, value)
-          self._origin[(section, option)] = filename

[Wladimir Palant, 2015/06/23 09:43:35]

Ok, we have a problem right here... Consider the following scenario:

foo/bar/metadata.chrome defines document_start = foo.js. foo/metadata.chrome
defines document_start += bar.js. The resulting value is document_start = foo.js
bar.js. Where are foo.js and bar.js supposed to be located?

With this change they will both be looked up in foo/ which is wrong. foo.js
should definitely be looked up in foo/bar/. Big question is bar.js - would be
logical to assume that its origin is foo/. However, that would be inconsistent
with -= directives - these use the paths from the original option, not paths
relative to the current metadata file. So I think that bar.js should also be
located in foo/bar/, additions and removals simply shouldn't change the origin
of the option.

[Sebastian Noack, 2015/06/24 08:51:09]

I first thought that we can simply rely on the source of the addition/removal as
it is merely a short hand syntax for repeating the pre-existing items. But you
are right.

Ideally, we would resolve the filenames relative to the location of the metadata
file they occurred in, before applying the diff. However, then we have to assume
that the option lists only filenames.

So without adding vague assumptions or too much complexity, it seems that the
best we can do is indeed preserving the source of the original option when
applying diffs. :(

+          is_diff, option, value = self._apply_diff(section, option, value)
+          ConfigParser.SafeConfigParser.set(self, section, option, value)
+
+          if not is_diff:
+            self._origin[(section, self.optionxform(option))] = filename
+
+  def read(self, filenames):
+    if isinstance(filenames, basestring):
+      filenames = [filenames]
+
+    read_ok = []
+    for filename in filenames:
+      try:
+        parser = self._make_parser(filename)
+      except IOError:
+        continue
+      self._process_parsers(self._get_parser_chain(parser, filename))
+      read_ok.append(filename)
+
+    return read_ok
 
   def items(self, section, *args, **kwargs):
     items = []
     for option, value in ConfigParser.SafeConfigParser.items(self, section, *args, **kwargs):
-      items.append(Item(option, value, self._origin[(section, option)]))
+      items.append(Item(
+        option, value,
+        self._origin[(section, self.optionxform(option))]
+      ))
     return items
 
   def option_source(self, section, option):
+    option = self.optionxform(option)
     try:
       return self._origin[(section, option)]
     except KeyError:
+      if not self.has_section(section):
+        raise ConfigParser.NoSectionError(section)
       raise ConfigParser.NoOptionError(option, section)
+
+  def readfp(self, fp, filename=None):
+    raise NotImplementedError
+
+  def set(self, section, option, value=None):
+    raise NotImplementedError
+
+  def add_section(self, section):
+    raise NotImplementedError
+
+  def remove_option(self, section, option):
+    raise NotImplementedError
+
+  def remove_section(self, section):
+    raise NotImplementedError