fix: scanny#754 _Relationships.items() raises

Author: scanny

pptx/opc/package.py

@@ -96,7 +96,7 @@ def iter_rels(self):
         visited = set()
 
         def walk_rels(rels):
-            for rel in rels:
+            for rel in rels.values():
                 yield rel
                 # --- external items can have no relationships ---
                 if rel.is_external:
@@ -482,14 +482,14 @@ def from_xml(cls, content_types_xml):
 
 
 class _Relationships(Mapping):
-    """Collection of |_Relationship| instances, largely having dict semantics.
+    """Collection of |_Relationship| instances having `dict` semantics.
 
     Relationships are keyed by their rId, but may also be found in other ways, such as
-    by their relationship type. `rels` is a dict of |Relationship| objects keyed by
-    their rId.
+    by their relationship type. |Relationship| objects are keyed by their rId.
 
-    Note that iterating this collection generates |Relationship| references (values),
-    not rIds (keys) as it would for a dict.
+    Iterating this collection has normal mapping semantics, generating the keys (rIds)
+    of the mapping. `rels.keys()`, `rels.values()`, and `rels.items() can be used as
+    they would be for a `dict`.
     """
 
     def __init__(self, base_uri):
@@ -507,9 +507,8 @@ def __getitem__(self, rId):
             raise KeyError("no relationship with key '%s'" % rId)
 
     def __iter__(self):
-        """Implement iteration of relationships."""
-        rels = self._rels
-        return (rels[rId] for rId in sorted(rels.keys()))
+        """Implement iteration of rIds (iterating a mapping produces its keys)."""
+        return iter(self._rels)
 
     def __len__(self):
         """Return count of relationships in collection."""
@@ -593,8 +592,22 @@ def xml(self):
         a `<?xml` header with encoding as UTF-8.
         """
         rels_elm = CT_Relationships.new()
-        for rel in self:
+
+        # -- Sequence <Relationship> elements deterministically (in numerical order) to
+        # -- simplify testing and manual inspection.
+        def iter_rels_in_numerical_order():
+            sorted_num_rId_pairs = sorted(
+                (
+                    int(rId[3:]) if rId.startswith("rId") and rId[3:].isdigit() else 0,
+                    rId,
+                )
+                for rId in self.keys()
+            )
+            return (self[rId] for _, rId in sorted_num_rId_pairs)
+
+        for rel in iter_rels_in_numerical_order():
             rels_elm.add_rel(rel.rId, rel.reltype, rel.target_ref, rel.is_external)
+
         return rels_elm.xml
 
     def _add_relationship(self, reltype, target, is_external=False):
@@ -648,7 +661,7 @@ def _rels(self):
     def _rels_by_reltype(self):
         """defaultdict {reltype: [rels]} for all relationships in collection."""
         D = collections.defaultdict(list)
-        for rel in self:
+        for rel in self.values():
             D[rel.reltype].append(rel)
         return D
 

tests/opc/test_package.py

@@ -190,7 +190,7 @@ def it_can_iterate_over_its_relationships(self, request, _rels_prop_):
         part_0_, part_1_ = [
             instance_mock(request, Part, name="part_%d" % i) for i in range(2)
         ]
-        rels = tuple(
+        all_rels = tuple(
             instance_mock(
                 request,
                 _Relationship,
@@ -208,18 +208,15 @@ def it_can_iterate_over_its_relationships(self, request, _rels_prop_):
                 )
             )
         )
-        _rels_prop_.return_value = rels[:3]
-        part_0_.rels = rels[3:4]
-        part_1_.rels = rels[4:]
+        _rels_prop_.return_value = {r.rId: r for r in all_rels[:3]}
+        part_0_.rels = {r.rId: r for r in all_rels[3:4]}
+        part_1_.rels = {r.rId: r for r in all_rels[4:]}
         package = OpcPackage(None)
 
-        assert tuple(package.iter_rels()) == (
-            rels[0],
-            rels[3],
-            rels[4],
-            rels[1],
-            rels[2],
-        )
+        rels = set(package.iter_rels())
+
+        # -- sequence is not guaranteed, but count (len) and uniqueness are --
+        assert rels == set(all_rels)
 
     def it_provides_access_to_the_main_document_part(self, request):
         presentation_part_ = instance_mock(request, PresentationPart)
@@ -234,8 +231,7 @@ def it_provides_access_to_the_main_document_part(self, request):
         assert presentation_part is presentation_part_
 
     @pytest.mark.parametrize(
-        "ns, expected_n",
-        (((), 1), ((1,), 2), ((1, 2), 3), ((2, 4), 3), ((1, 4), 3)),
+        "ns, expected_n", (((), 1), ((1,), 2), ((1, 2), 3), ((2, 4), 3), ((1, 4), 3))
     )
     def it_can_find_the_next_available_partname(self, request, ns, expected_n):
         tmpl = "/x%d.xml"
@@ -659,13 +655,15 @@ def but_it_raises_KeyError_when_no_relationship_has_rId(self, _rels_prop_):
             _Relationships(None)["rId6"]
         assert str(e.value) == "\"no relationship with key 'rId6'\""
 
-    def it_can_iterate_the_relationships_it_contains(self, request, _rels_prop_):
+    def it_can_iterate_the_rIds_of_the_relationships_it_contains(
+        self, request, _rels_prop_
+    ):
         rels_ = set(instance_mock(request, _Relationship) for n in range(5))
         _rels_prop_.return_value = {"rId%d" % (i + 1): r for i, r in enumerate(rels_)}
         relationships = _Relationships(None)
 
-        for r in relationships:
-            rels_.remove(r)
+        for rId in relationships:
+            rels_.remove(relationships[rId])
 
         assert len(rels_) == 0
 
@@ -795,10 +793,10 @@ def it_can_pop_a_relationship_to_remove_it_from_the_collection(
 
     def it_can_serialize_itself_to_XML(self, request, _rels_prop_):
         _rels_prop_.return_value = {
-            "rId1": instance_mock(
+            "rId11": instance_mock(
                 request,
                 _Relationship,
-                rId="rId1",
+                rId="rId11",
                 reltype=RT.SLIDE,
                 target_ref="../slides/slide1.xml",
                 is_external=False,
@@ -811,6 +809,14 @@ def it_can_serialize_itself_to_XML(self, request, _rels_prop_):
                 target_ref="http://url",
                 is_external=True,
             ),
+            "foo7W": instance_mock(
+                request,
+                _Relationship,
+                rId="foo7W",
+                reltype=RT.IMAGE,
+                target_ref="../media/image1.png",
+                is_external=False,
+            ),
         }
         relationships = _Relationships(None)
 
@@ -867,12 +873,7 @@ def and_it_can_add_an_external_relationship_to_help(
         ),
     )
     def it_can_get_a_matching_relationship_to_help(
-        self,
-        request,
-        _rels_by_reltype_prop_,
-        target_ref,
-        is_external,
-        expected_value,
+        self, request, _rels_by_reltype_prop_, target_ref, is_external, expected_value
     ):
         part_1, part_2 = (instance_mock(request, Part) for _ in range(2))
         _rels_by_reltype_prop_.return_value = {
@@ -996,12 +997,7 @@ def it_can_construct_from_xml(self, request, part_):
         relationship = _Relationship.from_xml("/ppt", rel_elm, parts)
 
         _init_.assert_called_once_with(
-            relationship,
-            "/ppt",
-            "rId42",
-            RT.SLIDE,
-            RTM.INTERNAL,
-            part_,
+            relationship, "/ppt", "rId42", RT.SLIDE, RTM.INTERNAL, part_
         )
         assert isinstance(relationship, _Relationship)
 

tests/test_files/snippets/relationships.txt

@@ -1,2 +1,2 @@
 <?xml version='1.0' encoding='UTF-8' standalone='yes'?>
-<Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships"><Relationship Id="rId1" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/slide" Target="../slides/slide1.xml"/><Relationship Id="rId2" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/hyperlink" Target="http://url" TargetMode="External"/></Relationships>
+<Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships"><Relationship Id="foo7W" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/image" Target="../media/image1.png"/><Relationship Id="rId2" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/hyperlink" Target="http://url" TargetMode="External"/><Relationship Id="rId11" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/slide" Target="../slides/slide1.xml"/></Relationships>