Apply [ 2714873 ] Fix for the overwritting of document attributes.

git-svn-id: http://svn.code.sf.net/p/docutils/code/trunk/docutils@7595 929543f6-e4f2-0310-98a6-ba3bd3dd1d04

5 files changed, 429 insertions, 20 deletions

diff --git a/HISTORY.txt b/HISTORY.txt
index f4b8e324e..978031ec9 100644
--- a/HISTORY.txt
+++ b/HISTORY.txt
@@ -16,6 +16,10 @@
 Changes Since 0.10
 ==================
 
+* General
+
+  - Apply [ 2714873 ] Fix for the overwritting of document attributes.
+
 * docutils/nodes.py
 
   - Fix [ 3601607 ] node.__repr__() must return `str` instance.
diff --git a/docutils/nodes.py b/docutils/nodes.py
index 86d113624..f20bb0295 100644
--- a/docutils/nodes.py
+++ b/docutils/nodes.py
@@ -432,10 +432,24 @@ class Element(Node):
     This is equivalent to ``element.extend([node1, node2])``.
     """
 
-    list_attributes = ('ids', 'classes', 'names', 'dupnames', 'backrefs')
+    basic_attributes = ('ids', 'classes', 'names', 'dupnames')
+    """List attributes which are defined for every Element-derived class
+    instance and can be safely transferred to a different node."""
+
+    local_attributes = ('backrefs',)
+    """A list of class-specific attributes that should not be copied with the
+    standard attributes when replacing a node.
+
+    NOTE: Derived classes should override this value to prevent any of its
+    attributes being copied by adding to the value in its parent class."""
+
+    list_attributes = basic_attributes + local_attributes
     """List attributes, automatically initialized to empty lists for
     all nodes."""
 
+    known_attributes = list_attributes + ('source',)
+    """List attributes that are known to the Element base class."""
+
     tagname = None
     """The element generic identifier. If None, it is set as an instance
     attribute to the name of the class."""
@@ -680,17 +694,231 @@ class Element(Node):
         else:
             return 1
 
-    def update_basic_atts(self, dict):
+    def update_basic_atts(self, dict_):
         """
         Update basic attributes ('ids', 'names', 'classes',
-        'dupnames', but not 'source') from node or dictionary `dict`.
+        'dupnames', but not 'source') from node or dictionary `dict_`.
+        """
+        if isinstance(dict_, Node):
+            dict_ = dict_.attributes
+        for att in self.basic_attributes:
+            self.append_attr_list(att, dict_.get(att, []))
+
+    def append_attr_list(self, attr, values):
+        """
+        For each element in values, if it does not exist in self[attr], append
+        it.
+
+        NOTE: Requires self[attr] and values to be sequence type and the
+        former should specifically be a list.
+        """
+        # List Concatenation
+        for value in values:
+            if not value in self[attr]:
+                self[attr].append(value)
+
+    def coerce_append_attr_list(self, attr, value):
+        """
+        First, convert both self[attr] and value to a non-string sequence
+        type; if either is not already a sequence, convert it to a list of one
+        element.  Then call append_attr_list.
+
+        NOTE: self[attr] and value both must not be None.
+        """
+        # List Concatenation
+        if not isinstance(self.get(attr), list):
+            self[attr] = [self[attr]]
+        if not isinstance(value, list):
+            value = [value]
+        self.append_attr_list(attr, value)
+
+    def replace_attr(self, attr, value, force = True):
+        """
+        If self[attr] does not exist or force is True or omitted, set
+        self[attr] to value, otherwise do nothing.
+        """
+        # One or the other
+        if force or self.get(attr) is None:
+            self[attr] = value
+
+    def copy_attr_convert(self, attr, value, replace = True):
+        """
+        If attr is an attribute of self, set self[attr] to
+        [self[attr], value], otherwise set self[attr] to value.
+
+        NOTE: replace is not used by this function and is kept only for
+              compatibility with the other copy functions.
+        """
+        if self.get(attr) is not value:
+            self.coerce_append_attr_list(attr, value)
+
+    def copy_attr_coerce(self, attr, value, replace):
+        """
+        If attr is an attribute of self and either self[attr] or value is a
+        list, convert all non-sequence values to a sequence of 1 element and
+        then concatenate the two sequence, setting the result to self[attr].
+        If both self[attr] and value are non-sequences and replace is True or
+        self[attr] is None, replace self[attr] with value. Otherwise, do
+        nothing.
+        """
+        if self.get(attr) is not value:
+            if isinstance(self.get(attr), list) or \
+               isinstance(value, list):
+                self.coerce_append_attr_list(attr, value)
+            else:
+                self.replace_attr(attr, value, replace)
+
+    def copy_attr_concatenate(self, attr, value, replace):
+        """
+        If attr is an attribute of self and both self[attr] and value are
+        lists, concatenate the two sequences, setting the result to
+        self[attr].  If either self[attr] or value are non-sequences and
+        replace is True or self[attr] is None, replace self[attr] with value.
+        Otherwise, do nothing.
+        """
+        if self.get(attr) is not value:
+            if isinstance(self.get(attr), list) and \
+               isinstance(value, list):
+                self.append_attr_list(attr, value)
+            else:
+                self.replace_attr(attr, value, replace)
+
+    def copy_attr_consistent(self, attr, value, replace):
+        """
+        If replace is True or selfpattr] is None, replace self[attr] with
+        value.  Otherwise, do nothing.
+        """
+        if self.get(attr) is not value:
+            self.replace_attr(attr, value, replace)
+
+    def update_all_atts(self, dict_, update_fun = copy_attr_consistent,
+                        replace = True, and_source = False):
+        """
+        Updates all attributes from node or dictionary `dict_`.
+
+        Appends the basic attributes ('ids', 'names', 'classes',
+        'dupnames', but not 'source') and then, for all other attributes in
+        dict_, updates the same attribute in self.  When attributes with the
+        same identifier appear in both self and dict_, the two values are
+        merged based on the value of update_fun.  Generally, when replace is
+        True, the values in self are replaced or merged with the values in
+        dict_; otherwise, the values in self may be preserved or merged.  When
+        and_source is True, the 'source' attribute is included in the copy.
+
+        NOTE: When replace is False, and self contains a 'source' attribute,
+              'source' is not replaced even when dict_ has a 'source'
+              attribute, though it may still be merged into a list depending
+              on the value of update_fun.
+        NOTE: It is easier to call the update-specific methods then to pass
+              the update_fun method to this function.
+        """
+        if isinstance(dict_, Node):
+            dict_ = dict_.attributes
+
+        # Include the source attribute when copying?
+        if and_source:
+            filter_fun = self.is_not_list_attribute
+        else:
+            filter_fun = self.is_not_known_attribute
+
+        # Copy the basic attributes
+        self.update_basic_atts(dict_)
+
+        # Grab other attributes in dict_ not in self except the
+        # (All basic attributes should be copied already)
+        for att in filter(filter_fun, dict_):
+            update_fun(self, att, dict_[att], replace)
+
+    def update_all_atts_consistantly(self, dict_, replace = True,
+                                     and_source = False):
         """
-        if isinstance(dict, Node):
-            dict = dict.attributes
-        for att in ('ids', 'classes', 'names', 'dupnames'):
-            for value in dict.get(att, []):
-                if not value in self[att]:
-                    self[att].append(value)
+        Updates all attributes from node or dictionary `dict_`.
+
+        Appends the basic attributes ('ids', 'names', 'classes',
+        'dupnames', but not 'source') and then, for all other attributes in
+        dict_, updates the same attribute in self.  When attributes with the
+        same identifier appear in both self and dict_ and replace is True, the
+        values in self are replaced with the values in dict_; otherwise, the
+        values in self are preserved.  When and_source is True, the 'source'
+        attribute is included in the copy.
+
+        NOTE: When replace is False, and self contains a 'source' attribute,
+              'source' is not replaced even when dict_ has a 'source'
+              attribute, though it may still be merged into a list depending
+              on the value of update_fun.
+        """
+        self.update_all_atts(dict_, Element.copy_attr_consistent, replace,
+                             and_source)
+
+    def update_all_atts_concatenating(self, dict_, replace = True,
+                                      and_source = False):
+        """
+        Updates all attributes from node or dictionary `dict_`.
+
+        Appends the basic attributes ('ids', 'names', 'classes',
+        'dupnames', but not 'source') and then, for all other attributes in
+        dict_, updates the same attribute in self.  When attributes with the
+        same identifier appear in both self and dict_ whose values aren't each
+        lists and replace is True, the values in self are replaced with the
+        values in dict_; if the values from self and dict_ for the given
+        identifier are both of list type, then the two lists are concatenated
+        and the result stored in self; otherwise, the values in self are
+        preserved.  When and_source is True, the 'source' attribute is
+        included in the copy.
+
+        NOTE: When replace is False, and self contains a 'source' attribute,
+              'source' is not replaced even when dict_ has a 'source'
+              attribute, though it may still be merged into a list depending
+              on the value of update_fun.
+        """
+        self.update_all_atts(dict_, Element.copy_attr_concatenate, replace,
+                             and_source)
+
+    def update_all_atts_coercion(self, dict_, replace = True,
+                                 and_source = False):
+        """
+        Updates all attributes from node or dictionary `dict_`.
+
+        Appends the basic attributes ('ids', 'names', 'classes',
+        'dupnames', but not 'source') and then, for all other attributes in
+        dict_, updates the same attribute in self.  When attributes with the
+        same identifier appear in both self and dict_ whose values are both
+        not lists and replace is True, the values in self are replaced with
+        the values in dict_; if either of the values from self and dict_ for
+        the given identifier are of list type, then first any non-lists are
+        converted to 1-element lists and then the two lists are concatenated
+        and the result stored in self; otherwise, the values in self are
+        preserved.  When and_source is True, the 'source' attribute is
+        included in the copy.
+
+        NOTE: When replace is False, and self contains a 'source' attribute,
+              'source' is not replaced even when dict_ has a 'source'
+              attribute, though it may still be merged into a list depending
+              on the value of update_fun.
+        """
+        self.update_all_atts(dict_, Element.copy_attr_coerce, replace,
+                             and_source)
+
+    def update_all_atts_convert(self, dict_, and_source = False):
+        """
+        Updates all attributes from node or dictionary `dict_`.
+
+        Appends the basic attributes ('ids', 'names', 'classes',
+        'dupnames', but not 'source') and then, for all other attributes in
+        dict_, updates the same attribute in self.  When attributes with the
+        same identifier appear in both self and dict_ then first any non-lists
+        are converted to 1-element lists and then the two lists are
+        concatenated and the result stored in self; otherwise, the values in
+        self are preserved.  When and_source is True, the 'source' attribute
+        is included in the copy.
+
+        NOTE: When replace is False, and self contains a 'source' attribute,
+              'source' is not replaced even when dict_ has a 'source'
+              attribute, though it may still be merged into a list depending
+              on the value of update_fun.
+        """
+        self.update_all_atts(dict_, Element.copy_attr_convert,
+                             and_source = and_source)
 
     def clear(self):
         self.children = []
@@ -721,7 +949,7 @@ class Element(Node):
         else:
             # `update` is a Text node or `new` is an empty list.
             # Assert that we aren't losing any attributes.
-            for att in ('ids', 'names', 'classes', 'dupnames'):
+            for att in self.basic_attributes:
                 assert not self[att], \
                        'Losing "%s" attribute: %s' % (att, self[att])
         self.parent.replace(self, new)
@@ -805,6 +1033,22 @@ class Element(Node):
             assert id is not None
             by_id.referenced = 1
 
+    @classmethod
+    def is_not_list_attribute(cls, attr):
+        """
+        Returns True if and only if the given attribute is NOT one of the
+        basic list attributes defined for all Elements.
+        """
+        return attr not in cls.list_attributes
+
+    @classmethod
+    def is_not_known_attribute(cls, attr):
+        """
+        Returns True if and only if the given attribute is NOT recognized by
+        this class.
+        """
+        return attr not in cls.known_attributes
+
 
 class TextElement(Element):
 
diff --git a/docutils/transforms/frontmatter.py b/docutils/transforms/frontmatter.py
index cd537f910..507d662af 100644
--- a/docutils/transforms/frontmatter.py
+++ b/docutils/transforms/frontmatter.py
@@ -49,13 +49,24 @@ class TitlePromoter(Transform):
 
         `node` is normally a document.
         """
+        # Type check
+        if not isinstance(node, nodes.Element):
+            raise TypeError, 'node must be of Element-derived type.'
+
         # `node` must not have a title yet.
         assert not (len(node) and isinstance(node[0], nodes.title))
         section, index = self.candidate_index(node)
         if index is None:
             return None
+
         # Transfer the section's attributes to the node:
-        node.attributes.update(section.attributes)
+        # NOTE: Change second parameter to False to NOT replace
+        #       attributes that already exist in node with those in
+        #       section
+        # NOTE: Remove third parameter to NOT copy the 'source'
+        #       attribute from section
+        node.update_all_atts_concatenating(section, True, True)
+
         # setup_child is called automatically for all nodes.
         node[:] = (section[:1]        # section title
                    + node[:index]     # everything that was in the
@@ -81,18 +92,23 @@ class TitlePromoter(Transform):
                 <subtitle>
                 ...
         """
+        # Type check
+        if not isinstance(node, nodes.Element):
+            raise TypeError, 'node must be of Element-derived type.'
+
         subsection, index = self.candidate_index(node)
         if index is None:
             return None
         subtitle = nodes.subtitle()
-        # Transfer the subsection's attributes to the new subtitle:
-        # This causes trouble with list attributes!  To do: Write a
-        # test case which catches direct access to the `attributes`
-        # dictionary and/or write a test case which shows problems in
-        # this particular case.
-        subtitle.attributes.update(subsection.attributes)
-        # We're losing the subtitle's attributes here!  To do: Write a
-        # test case which shows this behavior.
+
+        # Transfer the subsection's attributes to the new subtitle
+        # NOTE: Change second parameter to False to NOT replace
+        #       attributes that already exist in node with those in
+        #       section
+        # NOTE: Remove third parameter to NOT copy the 'source'
+        #       attribute from section
+        subtitle.update_all_atts_concatenating(subsection, True, True)
+
         # Transfer the contents of the subsection's title to the
         # subtitle:
         subtitle[:] = subsection[0][:]
diff --git a/test/test_nodes.py b/test/test_nodes.py
index 6a785b516..ecfa98108 100755
--- a/test/test_nodes.py
+++ b/test/test_nodes.py
@@ -166,6 +166,117 @@ class ElementTests(unittest.TestCase):
         # 'test' is not overwritten because it is not a basic attribute.
         self.assertEqual(element1['test'], ['test1'])
 
+    def test_update_all_atts(self):
+        # Note: Also tests is_not_list_attribute and is_not_known_attribute
+        # and various helpers
+        ## Test for full attribute replacement
+        element1 = nodes.Element(ids=['foo', 'bar'], parent_only='parent',
+                                 all_nodes='mom')
+        element2 = nodes.Element(ids=['baz', 'qux'], child_only='child',
+                                 all_nodes='dad', source='source')
+
+        # Test for when same fields are replaced as well as source...
+        element1.update_all_atts_consistantly(element2, True, True)
+        # 'ids' are appended because 'ids' is a basic attribute.
+        self.assertEquals(element1['ids'], ['foo', 'bar', 'baz', 'qux'])
+        # 'parent_only' should remain unaffected.
+        self.assertEquals(element1['parent_only'], 'parent')
+        # 'all_nodes' is overwritten due to the second parameter == True.
+        self.assertEquals(element1['all_nodes'], 'dad')
+        # 'child_only' should have been added.
+        self.assertEquals(element1['child_only'], 'child')
+        # 'source' is also overwritten due to the third parameter == True.
+        self.assertEquals(element1['source'], 'source')
+
+        # Test for when same fields are replaced but not source...
+        element1 = nodes.Element(ids=['foo', 'bar'], parent_only='parent',
+                                 all_nodes='mom')
+        element1.update_all_atts_consistantly(element2)
+        # 'ids' are appended because 'ids' is a basic attribute.
+        self.assertEquals(element1['ids'], ['foo', 'bar', 'baz', 'qux'])
+        # 'parent_only' should remain unaffected.
+        self.assertEquals(element1['parent_only'], 'parent')
+        # 'all_nodes' is overwritten due to the second parameter default of True.
+        self.assertEquals(element1['all_nodes'], 'dad')
+        # 'child_only' should have been added.
+        self.assertEquals(element1['child_only'], 'child')
+        # 'source' remains unset due to the third parameter default of False.
+        self.assertEquals(element1.get('source'), None)
+
+        # Test for when fields are NOT replaced but source is...
+        element1 = nodes.Element(ids=['foo', 'bar'], parent_only='parent',
+                                 all_nodes='mom')
+        element1.update_all_atts_consistantly(element2, False, True)
+        # 'ids' are appended because 'ids' is a basic attribute.
+        self.assertEquals(element1['ids'], ['foo', 'bar', 'baz', 'qux'])
+        # 'parent_only' should remain unaffected.
+        self.assertEquals(element1['parent_only'], 'parent')
+        # 'all_nodes' is preserved due to the second parameter == False.
+        self.assertEquals(element1['all_nodes'], 'mom')
+        # 'child_only' should have been added.
+        self.assertEquals(element1['child_only'], 'child')
+        # 'source' is added due to the third parameter == True.
+        self.assertEquals(element1['source'], 'source')
+        element1 = nodes.Element(source='destination')
+        element1.update_all_atts_consistantly(element2, False, True)
+        # 'source' remains unchanged due to the second parameter == False.
+        self.assertEquals(element1['source'], 'destination')
+
+        # Test for when same fields are replaced but not source...
+        element1 = nodes.Element(ids=['foo', 'bar'], parent_only='parent',
+                                 all_nodes='mom')
+        element1.update_all_atts_consistantly(element2, False)
+        # 'ids' are appended because 'ids' is a basic attribute.
+        self.assertEquals(element1['ids'], ['foo', 'bar', 'baz', 'qux'])
+        # 'parent_only' should remain unaffected.
+        self.assertEquals(element1['parent_only'], 'parent')
+        # 'all_nodes' is preserved due to the second parameter == False.
+        self.assertEquals(element1['all_nodes'], 'mom')
+        # 'child_only' should have been added.
+        self.assertEquals(element1['child_only'], 'child')
+        # 'source' remains unset due to the third parameter default of False.
+        self.assertEquals(element1.get('source'), None)
+
+        ## Test for List attribute merging
+        # Attribute Concatination
+        element1 = nodes.Element(ss='a', sl='1', ls=['I'], ll=['A'])
+        element2 = nodes.Element(ss='b', sl=['2'], ls='II', ll=['B'])
+        element1.update_all_atts_concatenating(element2)
+        # 'ss' is replaced because non-list
+        self.assertEquals(element1['ss'], 'b')
+        # 'sl' is replaced because they are both not lists
+        self.assertEquals(element1['sl'], ['2'])
+        # 'ls' is replaced because they are both not lists
+        self.assertEquals(element1['ls'], 'II')
+        # 'll' is extended because they are both lists
+        self.assertEquals(element1['ll'], ['A', 'B'])
+
+        # Attribute Coercion
+        element1 = nodes.Element(ss='a', sl='1', ls=['I'], ll=['A'])
+        element2 = nodes.Element(ss='b', sl=['2'], ls='II', ll=['B'])
+        element1.update_all_atts_coercion(element2)
+        # 'ss' is replaced because non-list
+        self.assertEquals(element1['ss'], 'b')
+        # 'sl' is converted to a list and appended because element2 has a list
+        self.assertEquals(element1['sl'], ['1', '2'])
+        # 'ls' has element2's value appended to the list
+        self.assertEquals(element1['ls'], ['I', 'II'])
+        # 'll' is extended because they are both lists
+        self.assertEquals(element1['ll'], ['A', 'B'])
+
+        # Attribute Conversion
+        element1 = nodes.Element(ss='a', sl='1', ls=['I'], ll=['A'])
+        element2 = nodes.Element(ss='b', sl=['2'], ls='II', ll=['B'])
+        element1.update_all_atts_convert(element2)
+        # 'ss' is converted to a list with the values from each element
+        self.assertEquals(element1['ss'], ['a', 'b'])
+        # 'sl' is converted to a list and appended
+        self.assertEquals(element1['sl'], ['1', '2'])
+        # 'ls' has element2's value appended to the list
+        self.assertEquals(element1['ls'], ['I', 'II'])
+        # 'll' is extended
+        self.assertEquals(element1['ll'], ['A', 'B'])
+
     def test_replace_self(self):
         parent = nodes.Element(ids=['parent'])
         child1 = nodes.Element(ids=['child1'])
diff --git a/test/test_transforms/test_doctitle.py b/test/test_transforms/test_doctitle.py
index 0807bce70..c00113e63 100755
--- a/test/test_transforms/test_doctitle.py
+++ b/test/test_transforms/test_doctitle.py
@@ -10,8 +10,23 @@ Tests for docutils.transforms.frontmatter.DocTitle.
 
 from __init__ import DocutilsTestSupport
 from docutils.transforms.frontmatter import DocTitle, SectionSubTitle
-from docutils.parsers.rst import Parser
+from docutils.parsers.rst import Parser, Directive
+from docutils.parsers.rst.directives import register_directive
 
+# dummy directive to test attribute merging:
+class AddNameToDocumentTitle(Directive):
+    required_arguments = 0
+    optional_arguments = 0
+    final_argument_whitespace = True
+    option_spec = { }
+    has_content = False
+
+    def run(self):
+        document = self.state_machine.document
+        document['names'].append('Name')
+        return []
+
+register_directive('add-name-to-title', AddNameToDocumentTitle)
 
 def suite():
     parser = Parser()
@@ -221,6 +236,25 @@ Another Subtitle
             <subtitle ids="another-subtitle" names="another\ subtitle">
                 Another Subtitle
 """],
+["""\
+-----
+Title
+-----
+
+This is a document, it flows nicely, so the attributes of it are at the
+bottom.
+
+.. add-name-to-title::
+
+""",
+"""\
+<document ids="title" names="Name title" source="test data" title="Title">
+    <title>
+        Title
+    <paragraph>
+        This is a document, it flows nicely, so the attributes of it are at the
+        bottom.
+"""]
 ])