Make relationship ids more reliable.

Relationship instances now keep track of their own id – this should be much more reliable than the old way of more or less “guessing” the relationship id based on the position of some object in some array. Fixes https://github.com/randym/axlsx/issues/212, especially.

Each relationship now has its own, unique id – except for the cases when it doesn’t: Some relationships need to share the same id, see `Relation#should_use_same_id_as?` for the gory details.

All tests pass, and the full example.xlsx is generated without errors and looks fine in Excel for Mac 2011.

The pivot table example still has the problems mentioned in https://github.com/randym/axlsx/issues/168 – but as far as I can tell I didn’t make it worse (Excel is still be able to “repair” the file, and the repaired file then contains the pivot table).

19 files changed, 122 insertions, 117 deletions

diff --git a/lib/axlsx/drawing/chart.rb b/lib/axlsx/drawing/chart.rb
index 1da3a253..c1d408e6 100644
--- a/lib/axlsx/drawing/chart.rb
+++ b/lib/axlsx/drawing/chart.rb
@@ -80,10 +80,10 @@ module Axlsx
     # Default :gap (although this really should vary by chart type and grouping)
     attr_reader :display_blanks_as
 
-    # returns a relationship object for the chart
-    # @return [Axlsx::Relationship]
+    # The relationship object for this chart.
+    # @return [Relationship]
     def relationship
-      Relationship.new(CHART_R, "../#{pn}")
+      Relationship.new(self, CHART_R, "../#{pn}")
     end
 
     # The index of this chart in the workbooks charts collection
diff --git a/lib/axlsx/drawing/drawing.rb b/lib/axlsx/drawing/drawing.rb
index 58f0f81e..1748e9f4 100644
--- a/lib/axlsx/drawing/drawing.rb
+++ b/lib/axlsx/drawing/drawing.rb
@@ -121,12 +121,6 @@ module Axlsx
       @worksheet.workbook.drawings.index(self)
     end
 
-    # The relation reference id for this drawing
-    # @return [String]
-    def rId
-      "rId#{index+1}"
-    end
-
     # The part name for this drawing
     # @return [String]
     def pn
@@ -140,15 +134,7 @@ module Axlsx
       "#{DRAWING_RELS_PN % (index+1)}"
     end
 
-    # The index of a chart, image or hyperlink object this drawing contains
-    def index_of(object)
-      child_objects.index(object)
-    end
-
-
-    # An ordered list of objects this drawing holds
-    # It is important that the objects are returned in the same order each time for 
-    # releationship indexing in the package
+    # A list of objects this drawing holds.
     # @return [Array]
     def child_objects
       charts + images + hyperlinks
diff --git a/lib/axlsx/drawing/graphic_frame.rb b/lib/axlsx/drawing/graphic_frame.rb
index 943caeae..9b921275 100644
--- a/lib/axlsx/drawing/graphic_frame.rb
+++ b/lib/axlsx/drawing/graphic_frame.rb
@@ -22,15 +22,10 @@ module Axlsx
       @chart = chart_type.new(self, options)
     end
 
-    # The relationship id for this graphic
+    # The relationship id for this graphic frame.
     # @return [String]
-    #
-    # NOTE: Discontinued. This should not be part of GraphicFrame.
-    # The drawing object maintains relationships and needs to be queried to determine the relationship id of any given graphic data child object.
-    #
     def rId
-      warn('axlsx::DEPRECIATED: GraphicFrame#rId has been depreciated. relationship id is determed by the drawing object')
-      "rId#{@anchor.index+1}"
+      @anchor.drawing.relationships.for(chart).Id
     end
 
     # Serializes the object
@@ -49,7 +44,7 @@ module Axlsx
       str << '</xdr:xfrm>'
       str << '<a:graphic>'
       str << '<a:graphicData uri="' << XML_NS_C << '">'
-      str << '<c:chart xmlns:c="' << XML_NS_C << '" xmlns:r="' << XML_NS_R << '" r:id="rId' << (@anchor.drawing.index_of(@chart)+1).to_s << '"/>'
+      str << '<c:chart xmlns:c="' << XML_NS_C << '" xmlns:r="' << XML_NS_R << '" r:id="' << rId << '"/>'
       str << '</a:graphicData>'
       str << '</a:graphic>'
       str << '</xdr:graphicFrame>'
diff --git a/lib/axlsx/drawing/hyperlink.rb b/lib/axlsx/drawing/hyperlink.rb
index e886f766..850baeef 100644
--- a/lib/axlsx/drawing/hyperlink.rb
+++ b/lib/axlsx/drawing/hyperlink.rb
@@ -83,27 +83,20 @@ module Axlsx
     # @return [String]
     attr_accessor :tooltip
 
-    # Returns a relationship object for this hyperlink
-    # @return [Axlsx::Relationship]
+    # The relationship object for this hyperlink.
+    # @return [Relationship]
     def relationship
-      Relationship.new(HYPERLINK_R, href, :target_mode => :External)
+      Relationship.new(self, HYPERLINK_R, href, :target_mode => :External)
     end
+
     # Serializes the object
     # @param [String] str
     # @return [String]
     def to_xml_string(str = '')
       str << '<a:hlinkClick '
-      serialized_attributes str, {:'r:id' => "rId#{id}", :'xmlns:r' => XML_NS_R }
+      serialized_attributes str, {:'r:id' => relationship.Id, :'xmlns:r' => XML_NS_R }
       str << '/>'
     end
 
-    private
-
-    # The relational ID for this hyperlink
-    # @return [Integer]
-    def id
-      @parent.anchor.drawing.index_of(self)+1
-    end
-
   end
 end
diff --git a/lib/axlsx/drawing/pic.rb b/lib/axlsx/drawing/pic.rb
index 8ed8d042..89f4c1ea 100644
--- a/lib/axlsx/drawing/pic.rb
+++ b/lib/axlsx/drawing/pic.rb
@@ -102,15 +102,10 @@ module Axlsx
       "#{IMAGE_PN % [(index+1), extname]}"
     end
 
-    # The relational id withing the drawing's relationships
-    def id
-      @anchor.drawing.charts.size + @anchor.drawing.images.index(self) + 1
-    end
-
-    # Returns a relationship object for this object
-    # @return Axlsx::Relationship
+    # The relationship object for this pic.
+    # @return [Relationship]
     def relationship
-      Relationship.new(IMAGE_R, "../#{pn}")
+      Relationship.new(self, IMAGE_R, "../#{pn}")
     end
 
     # providing access to the anchor's width attribute
@@ -177,7 +172,7 @@ module Axlsx
       picture_locking.to_xml_string(str)
       str << '</xdr:cNvPicPr></xdr:nvPicPr>'
       str << '<xdr:blipFill>'
-      str << '<a:blip xmlns:r ="' << XML_NS_R << '" r:embed="rId' << id.to_s <<  '"/>'
+      str << '<a:blip xmlns:r ="' << XML_NS_R << '" r:embed="' << relationship.Id <<  '"/>'
       str << '<a:stretch><a:fillRect/></a:stretch></xdr:blipFill><xdr:spPr>'
       str << '<a:xfrm><a:off x="0" y="0"/><a:ext cx="2336800" cy="2161540"/></a:xfrm>'
       str << '<a:prstGeom prst="rect"><a:avLst/></a:prstGeom></xdr:spPr></xdr:pic>'
diff --git a/lib/axlsx/package.rb b/lib/axlsx/package.rb
index 7e4bad60..a78b5f5d 100644
--- a/lib/axlsx/package.rb
+++ b/lib/axlsx/package.rb
@@ -339,9 +339,9 @@ module Axlsx
     # @private
     def relationships
       rels = Axlsx::Relationships.new
-      rels << Relationship.new(WORKBOOK_R, WORKBOOK_PN)
-      rels << Relationship.new(CORE_R, CORE_PN)
-      rels << Relationship.new(APP_R, APP_PN)
+      rels << Relationship.new(self, WORKBOOK_R, WORKBOOK_PN)
+      rels << Relationship.new(self, CORE_R, CORE_PN)
+      rels << Relationship.new(self, APP_R, APP_PN)
       rels.lock
       rels
     end
diff --git a/lib/axlsx/rels/relationship.rb b/lib/axlsx/rels/relationship.rb
index 385059f1..64356d46 100644
--- a/lib/axlsx/rels/relationship.rb
+++ b/lib/axlsx/rels/relationship.rb
@@ -3,7 +3,29 @@ module Axlsx
   # A relationship defines a reference between package parts.
   # @note Packages automatically manage relationships.
   class Relationship
+    
+    class << self
+      # Keeps track of all instances of this class.
+      # @return [Array]
+      def instances
+        @instances ||= []
+      end
+      
+      # Generate and return a unique id. Used for setting {#Id}.
+      # @return [String]
+      def next_free_id
+        @next_free_id_counter ||= 0
+        @next_free_id_counter += 1
+        "rId#{@next_free_id_counter}"
+      end
+    end
 
+    # The id of the relationship (eg. "rId123"). Most instances get their own unique id. 
+    # However, some instances need to share the same id – see {#should_use_same_id_as?}
+    # for details.
+    # @return [String]
+    attr_reader :Id
+    
     # The location of the relationship target
     # @return [String]
     attr_reader :Target
@@ -30,14 +52,26 @@ module Axlsx
     # Target mode must be :external for now.
     attr_reader :TargetMode
 
-    # creates a new relationship
+    # The source object the relations belongs to (e.g. a hyperlink, drawing, ...). Needed when
+    # looking up the relationship for a specific object (see {Relationships#for}).
+    attr_reader :source_obj
+    
+    # Initializes a new relationship. 
+    # @param [Object] source_obj see {#source_obj}
     # @param [String] type The type of the relationship
     # @param [String] target The target for the relationship
     # @option [Symbol] :target_mode only accepts :external.
-    def initialize(type, target, options={})
+    def initialize(source_obj, type, target, options={})
+      @source_obj = source_obj
       self.Target=target
       self.Type=type
-      self.TargetMode = options.delete(:target_mode) if options[:target_mode]
+      self.TargetMode = options[:target_mode] if options[:target_mode]
+      @Id = if (existing = self.class.instances.find{ |i| should_use_same_id_as?(i) })
+        existing.Id
+      else
+        self.class.next_free_id
+      end
+      self.class.instances << self
     end
 
     # @see Target
@@ -50,15 +84,32 @@ module Axlsx
 
     # serialize relationship
     # @param [String] str
-    # @param [Integer] rId the id for this relationship
     # @return [String]
-    def to_xml_string(rId, str = '')
-      h = self.instance_values
-      h[:Id] = 'rId' << rId.to_s
+    def to_xml_string(str = '')
+      h = self.instance_values.reject{|k, _| k == "source_obj"}
       str << '<Relationship '
       str << h.map { |key, value| '' << key.to_s << '="' << Axlsx::coder.encode(value.to_s) << '"'}.join(' ')
       str << '/>'
     end
-
+    
+    # Whether this relationship should use the same id as `other`.
+    #
+    # Instances designating the same relationship need to use the same id. We can not simply
+    # compare the {#Target} attribute, though: `foo/bar.xml`, `../foo/bar.xml`, 
+    # `../../foo/bar.xml` etc. are all different but probably mean the same file (this 
+    # is especially an issue for relationships in the context of pivot tables). So lets
+    # just ignore this attribute for now (except when {#TargetMode} is set to `:External` –
+    # then {#Target} will be an absolute URL and thus can safely be compared).
+    #
+    # @todo Implement comparison of {#Target} based on normalized path names.
+    # @param other [Relationship]
+    def should_use_same_id_as?(other)
+      result = self.source_obj == other.source_obj && self.Type == other.Type && self.TargetMode == other.TargetMode
+      if self.TargetMode == :External
+        result &&= self.Target == other.Target
+      end
+      result
+    end
+    
   end
 end
diff --git a/lib/axlsx/rels/relationships.rb b/lib/axlsx/rels/relationships.rb
index 521d7689..a9c73f7d 100644
--- a/lib/axlsx/rels/relationships.rb
+++ b/lib/axlsx/rels/relationships.rb
@@ -11,10 +11,17 @@ require 'axlsx/rels/relationship.rb'
       super Relationship
     end
     
+    # The relationship instance for the given source object, or nil if none exists.
+    # @see Relationship#source_obj
+    # @return [Relationship]
+    def for(source_obj)
+      @list.find{ |rel| rel.source_obj == source_obj }
+    end
+    
     def to_xml_string(str = '')
       str << '<?xml version="1.0" encoding="UTF-8"?>'
       str << '<Relationships xmlns="' << RELS_R << '">'
-      each_with_index { |rel, index| rel.to_xml_string(index+1, str) }
+      each{ |rel| rel.to_xml_string(str) }
       str << '</Relationships>'
     end
 
diff --git a/lib/axlsx/workbook/workbook.rb b/lib/axlsx/workbook/workbook.rb
index 1397552a..d122f253 100644
--- a/lib/axlsx/workbook/workbook.rb
+++ b/lib/axlsx/workbook/workbook.rb
@@ -270,14 +270,14 @@ require 'axlsx/workbook/worksheet/selection.rb'
     def relationships
       r = Relationships.new
       @worksheets.each do |sheet|
-        r << Relationship.new(WORKSHEET_R, WORKSHEET_PN % (r.size+1))
+        r << Relationship.new(sheet, WORKSHEET_R, WORKSHEET_PN % (r.size+1))
       end
       pivot_tables.each_with_index do |pivot_table, index|
-        r << Relationship.new(PIVOT_TABLE_CACHE_DEFINITION_R, PIVOT_TABLE_CACHE_DEFINITION_PN % (index+1))
+        r << Relationship.new(pivot_table.cache_definition, PIVOT_TABLE_CACHE_DEFINITION_R, PIVOT_TABLE_CACHE_DEFINITION_PN % (index+1))
       end
-      r << Relationship.new(STYLES_R,  STYLES_PN)
+      r << Relationship.new(self, STYLES_R,  STYLES_PN)
       if use_shared_strings
-          r << Relationship.new(SHARED_STRINGS_R,  SHARED_STRINGS_PN)
+          r << Relationship.new(self, SHARED_STRINGS_R,  SHARED_STRINGS_PN)
       end
       r
     end
@@ -336,9 +336,8 @@ require 'axlsx/workbook/worksheet/selection.rb'
       defined_names.to_xml_string(str)
       unless pivot_tables.empty?
         str << '<pivotCaches>'
-        pivot_tables.each_with_index do |pivot_table, index|
-          rId = "rId#{@worksheets.size + index + 1  }"
-          str << '<pivotCache cacheId="' << pivot_table.cache_definition.cache_id.to_s << '" r:id="' << rId << '"/>'
+        pivot_tables.each do |pivot_table|
+          str << '<pivotCache cacheId="' << pivot_table.cache_definition.cache_id.to_s << '" r:id="' << pivot_table.cache_definition.rId << '"/>'
         end
         str << '</pivotCaches>'
       end
diff --git a/lib/axlsx/workbook/worksheet/comments.rb b/lib/axlsx/workbook/worksheet/comments.rb
index 25da9de2..03cce339 100644
--- a/lib/axlsx/workbook/worksheet/comments.rb
+++ b/lib/axlsx/workbook/worksheet/comments.rb
@@ -56,9 +56,9 @@ module Axlsx
     # The relationships required by this object
     # @return [Array]
     def relationships
-      [Relationship.new(VML_DRAWING_R, "../#{vml_drawing.pn}"),
-       Relationship.new(COMMENT_R, "../#{pn}"),
-       Relationship.new(COMMENT_R_NULL, "NULL")]
+      [Relationship.new(self, VML_DRAWING_R, "../#{vml_drawing.pn}"),
+       Relationship.new(self, COMMENT_R, "../#{pn}"),
+       Relationship.new(self, COMMENT_R_NULL, "NULL")]
     end
 
     # serialize the object
diff --git a/lib/axlsx/workbook/worksheet/pivot_table.rb b/lib/axlsx/workbook/worksheet/pivot_table.rb
index da87162d..632765f9 100644
--- a/lib/axlsx/workbook/worksheet/pivot_table.rb
+++ b/lib/axlsx/workbook/worksheet/pivot_table.rb
@@ -135,20 +135,14 @@ module Axlsx
       @cache_definition ||= PivotTableCacheDefinition.new(self)
     end
 
-    # The worksheet relationships. This is managed automatically by the worksheet
+    # The relationships for this pivot table.
     # @return [Relationships]
     def relationships
       r = Relationships.new
-      r << Relationship.new(PIVOT_TABLE_CACHE_DEFINITION_R, "../#{cache_definition.pn}")
+      r << Relationship.new(cache_definition, PIVOT_TABLE_CACHE_DEFINITION_R, "../#{cache_definition.pn}")
       r
     end
 
-    # The relation reference id for this table
-    # @return [String]
-    def rId
-      "rId#{index+1}"
-    end
-
     # Serializes the object
     # @param [String] str
     # @return [String]
diff --git a/lib/axlsx/workbook/worksheet/pivot_table_cache_definition.rb b/lib/axlsx/workbook/worksheet/pivot_table_cache_definition.rb
index c1683520..340b94ff 100644
--- a/lib/axlsx/workbook/worksheet/pivot_table_cache_definition.rb
+++ b/lib/axlsx/workbook/worksheet/pivot_table_cache_definition.rb
@@ -35,10 +35,11 @@ module Axlsx
       index + 1
     end
 
-    # The relation reference id for this table
+    # The relationship id for this pivot table cache definition.
+    # @see Relationship#Id
     # @return [String]
     def rId
-      "rId#{index + 1}"
+      pivot_table.relationships.for(self).Id
     end
 
     # Serializes the object
diff --git a/lib/axlsx/workbook/worksheet/pivot_tables.rb b/lib/axlsx/workbook/worksheet/pivot_tables.rb
index f5625fc0..912d9f41 100644
--- a/lib/axlsx/workbook/worksheet/pivot_tables.rb
+++ b/lib/axlsx/workbook/worksheet/pivot_tables.rb
@@ -17,7 +17,7 @@ module Axlsx
     # returns the relationships required by this collection
     def relationships
       return [] if empty?
-      map{ |pivot_table| Relationship.new(PIVOT_TABLE_R, "../#{pivot_table.pn}") }
+      map{ |pivot_table| Relationship.new(pivot_table, PIVOT_TABLE_R, "../#{pivot_table.pn}") }
     end
   end
 
diff --git a/lib/axlsx/workbook/worksheet/table.rb b/lib/axlsx/workbook/worksheet/table.rb
index dce1a40e..94474c57 100644
--- a/lib/axlsx/workbook/worksheet/table.rb
+++ b/lib/axlsx/workbook/worksheet/table.rb
@@ -47,10 +47,11 @@ module Axlsx
       "#{TABLE_PN % (index+1)}"
     end
 
-    # The relation reference id for this table
+    # The relationship id for this table.
+    # @see Relationship#Id
     # @return [String]
     def rId
-      "rId#{index+1}"
+      @sheet.relationships.for(self).Id
     end
 
     # The name of the Table.
diff --git a/lib/axlsx/workbook/worksheet/tables.rb b/lib/axlsx/workbook/worksheet/tables.rb
index 2d9a1f3c..814f995f 100644
--- a/lib/axlsx/workbook/worksheet/tables.rb
+++ b/lib/axlsx/workbook/worksheet/tables.rb
@@ -17,7 +17,7 @@ module Axlsx
     # returns the relationships required by this collection
     def relationships
       return [] if empty?
-      map{ |table| Relationship.new(TABLE_R, "../#{table.pn}") }
+      map{ |table| Relationship.new(table, TABLE_R, "../#{table.pn}") }
     end
 
     def to_xml_string(str = "")
diff --git a/lib/axlsx/workbook/worksheet/worksheet.rb b/lib/axlsx/workbook/worksheet/worksheet.rb
index 7324181a..ecba9254 100644
--- a/lib/axlsx/workbook/worksheet/worksheet.rb
+++ b/lib/axlsx/workbook/worksheet/worksheet.rb
@@ -348,10 +348,11 @@ module Axlsx
       "#{WORKSHEET_RELS_PN % (index+1)}"
     end
 
-    # The relationship Id of thiw worksheet
+    # The relationship id of this worksheet.
     # @return [String]
+    # @see Relationship#Id
     def rId
-      "rId#{index+1}"
+      @workbook.relationships.for(self).Id
     end
 
     # The index of this worksheet in the owning Workbook's worksheets list.
@@ -574,14 +575,6 @@ module Axlsx
       r
     end
 
-    # identifies the index of an object withing the collections used in generating relationships for the worksheet
-    # @param [Any] object the object to search for
-    # @return [Integer] The index of the object
-    def relationships_index_of(object)
-      objects = [tables.to_a, worksheet_comments.comments.to_a, hyperlinks.to_a, worksheet_drawing.drawing].flatten.compact || []
-      objects.index(object)
-    end
-
     # Returns the cell or cells defined using excel style A1:B3 references.
     # @param [String|Integer] cell_def the string defining the cell or range of cells, or the rownumber
     # @return [Cell, Array]
diff --git a/lib/axlsx/workbook/worksheet/worksheet_comments.rb b/lib/axlsx/workbook/worksheet/worksheet_comments.rb
index 8c700aa0..f9e4c8cd 100644
--- a/lib/axlsx/workbook/worksheet/worksheet_comments.rb
+++ b/lib/axlsx/workbook/worksheet/worksheet_comments.rb
@@ -40,10 +40,11 @@ module Axlsx
       !comments.empty?
     end
 
-    # The index in the worksheet's relationships for the VML drawing that will render the comments
-    # @return [Integer]
-    def index
-      worksheet.relationships.index { |r| r.Type == VML_DRAWING_R } + 1
+    # The relationship id of the VML drawing that will render the comments.
+    # @see Relationship#Id
+    # @return [String]
+    def drawing_rId
+      comments.relationships.find{ |r| r.Type == VML_DRAWING_R }.Id
     end
 
     # Seraalize the object
@@ -51,7 +52,7 @@ module Axlsx
     # @return [String]
     def to_xml_string(str = '')
       return unless has_comments?
-      str << "<legacyDrawing r:id='rId#{index}' />"
+      str << "<legacyDrawing r:id='#{drawing_rId}' />"
     end
   end
 end
diff --git a/lib/axlsx/workbook/worksheet/worksheet_drawing.rb b/lib/axlsx/workbook/worksheet/worksheet_drawing.rb
index 9deeeec3..08cad1f7 100644
--- a/lib/axlsx/workbook/worksheet/worksheet_drawing.rb
+++ b/lib/axlsx/workbook/worksheet/worksheet_drawing.rb
@@ -41,24 +41,18 @@ module Axlsx
       @drawing.is_a? Drawing
     end
 
-    # The relationship required by this object
+    # The relationship instance for this drawing.
     # @return [Relationship]
     def relationship
       return unless has_drawing?
-      Relationship.new(DRAWING_R, "../#{drawing.pn}") 
-    end
-
-    # returns the index of the worksheet releationship that defines this drawing.
-    # @return [Integer]
-    def index
-       worksheet.relationships.index{ |r| r.Type == DRAWING_R } +1 
+      Relationship.new(self, DRAWING_R, "../#{drawing.pn}") 
     end
 
     # Serialize the drawing for the worksheet
     # @param [String] str
     def to_xml_string(str = '')
       return unless has_drawing? 
-      str << "<drawing r:id='rId#{index}'/>"
+      str << "<drawing r:id='#{relationship.Id}'/>"
     end
   end
 end
diff --git a/lib/axlsx/workbook/worksheet/worksheet_hyperlink.rb b/lib/axlsx/workbook/worksheet/worksheet_hyperlink.rb
index d352ec90..2a241287 100644
--- a/lib/axlsx/workbook/worksheet/worksheet_hyperlink.rb
+++ b/lib/axlsx/workbook/worksheet/worksheet_hyperlink.rb
@@ -45,18 +45,13 @@ module Axlsx
       @ref = cell_reference
     end
 
-       # The relationship required by this hyperlink when the taget is :external
+    # The relationship instance for this hyperlink.
+    # A relationship is only required if `@target` is `:external`. If not, this method will simply return `nil`.
+    # @see #target=
     # @return [Relationship]
     def relationship
       return unless @target == :external
-      Relationship.new HYPERLINK_R, location, :target_mode => :External
-    end
-
-    # The id of the relationship for this object
-    # @return [String]
-    def id
-      return unless @target == :external
-      "rId#{(@worksheet.relationships_index_of(self)+1)}"
+      Relationship.new(self, HYPERLINK_R, location, :target_mode => :External)
     end
 
     # Seralize the object
@@ -73,7 +68,7 @@ module Axlsx
     # r:id should only be specified for external targets.
     # @return [Hash]
     def location_or_id
-      @target == :external ?  { :"r:id" => id } : { :location => Axlsx::coder.encode(location) }
+      @target == :external ?  { :"r:id" => relationship.Id } : { :location => Axlsx::coder.encode(location) }
     end
   end
 end