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, codecs, ConfigParser
+import os
+import re
+import ConfigParser
 
 class Item(tuple):
   def __new__(cls, name, value, source):
     result = super(Item, cls).__new__(cls, (name, value))
     result.source = source
     return result
 
-class ChainedConfigParser:
-  """
+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
 
     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.

-  """
+  '''
 
-  def __init__(self, path):
-    self.chain = []
-    self.read_path(path)
+  def __init__(self):
+    ConfigParser.SafeConfigParser.__init__(self)
+    self._origin = {}
 
-  def read_path(self, path):
-    if len(self.chain) >= 5:
-      raise Exception('Too much inheritance in config files')
+  def _get_parser_chain(self, file, filename):
+    parsers = []
 
-    config = ConfigParser.SafeConfigParser()
-    config.optionxform = str
-    config.source_path = path
-    handle = codecs.open(path, 'rb', encoding='utf-8')
-    config.readfp(handle)
-    handle.close()
-    self.chain.append(config)
+    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.

 
-    if config.has_section('default') and config.has_option('default', 'inherit'):
-      parts = config.get('default', 'inherit').split('/')
-      defaults_path = os.path.join(os.path.dirname(path), *parts)
-      self.read_path(defaults_path)
+    while True:
+      parsers.insert(0, (parser, filename))
 
-  def defaults(self):
-    result = {}
-    for config in reverse(self.chain):
-      for key, value in config.defaults().iteritems():
-        result[key] = value
-    return result
+      try:
+        inherit = parser.get('default', 'inherit')
+      except (ConfigParser.NoSectionError, ConfigParser.NoOptionError):
+        return parsers
 
-  def sections(self):
-    result = set()
-    for config in self.chain:
-      for section in config.sections():
-        result.add(section)
-    return list(result)
+      filename = os.path.join(os.path.dirname(filename), *inherit.split('/'))
+      parser = ConfigParser.SafeConfigParser()
+      parser.read(filename)
 
-  def has_section(self, section):
-    for config in self.chain:
-      if config.has_section(section):
-        return True
-    return False
+  def _apply_diff(self, section, option, value):
+    addition = option.endswith('+')
+    removal  = option.endswith('-')
 
-  def options(self, section):
-    result = set()
-    for config in self.chain:
-      if config.has_section(section):
-        for option in config.options(section):
-          result.add(option)
-    return list(result)
+    if addition or removal:
+      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.

 
-  def has_option(self, section, option):
-    for config in self.chain:
-      if config.has_section(section) and config.has_option(section, option):
-        return True
-    return False
+      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.

 
-  def get(self, section, option):
-    for config in self.chain:
-      if config.has_section(section) and config.has_option(section, option):
-        return config.get(section, option)
-    raise ConfigParser.NoOptionError(option, section)
+    return option, value
 
-  def items(self, section):
-    seen = set()
-    result = []
-    for config in self.chain:
-      if config.has_section(section):
-        for name, value in config.items(section):
-          if name not in seen:
-            seen.add(name)
-            result.append(Item(name, value, config.source_path))
-    return result
+  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)
+
+    for parser, filename in parsers:
+      for section in parser.sections():
+        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. :(

+
+  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)]))
+    return items
 
   def option_source(self, section, option):
-    for config in self.chain:
-      if config.has_section(section) and config.has_option(section, option):
-        return config.source_path
-    raise ConfigParser.NoOptionError(option, section)
+    try:
+      return self._origin[(section, option)]
+    except KeyError:
+      raise ConfigParser.NoOptionError(option, section)