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).

32 files changed, 192 insertions, 171 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
diff --git a/test/drawing/tc_drawing.rb b/test/drawing/tc_drawing.rb
index f347001f..7014f1b2 100644
--- a/test/drawing/tc_drawing.rb
+++ b/test/drawing/tc_drawing.rb
@@ -53,11 +53,6 @@ class TestDrawing < Test::Unit::TestCase
     assert_equal(@ws.drawing.rels_pn, "drawings/_rels/drawing1.xml.rels")
   end
 
-  def test_rId
-    @ws.add_chart(Axlsx::Pie3DChart)
-    assert_equal(@ws.drawing.rId, "rId1")
-  end
-
   def test_index
     @ws.add_chart(Axlsx::Pie3DChart)
     assert_equal(@ws.drawing.index, @ws.workbook.drawings.index(@ws.drawing))
diff --git a/test/drawing/tc_graphic_frame.rb b/test/drawing/tc_graphic_frame.rb
index ce4b8214..eb638489 100644
--- a/test/drawing/tc_graphic_frame.rb
+++ b/test/drawing/tc_graphic_frame.rb
@@ -17,14 +17,11 @@ class TestGraphicFrame < Test::Unit::TestCase
   end
 
   def test_rId
-    assert_equal(@frame.rId, "rId1")
-    chart = @ws.add_chart Axlsx::Chart
-    assert_equal(chart.graphic_frame.rId, "rId2")
+    assert_equal @ws.drawing.relationships.for(@chart).Id, @frame.rId
   end
 
-  def test_rId_with_image_and_chart
-    image = @ws.add_image :image_src => (File.dirname(__FILE__) + "/../../examples/image1.jpeg"), :start_at => [0,25], :width => 200, :height => 200
-    assert_equal(2, image.id)
-    assert_equal(1, @chart.index+1)
+  def test_to_xml_has_correct_rId
+    doc = Nokogiri::XML(@frame.to_xml_string)
+    assert_equal @frame.rId, doc.xpath("//c:chart", doc.collect_namespaces).first["r:id"]
   end
 end
diff --git a/test/drawing/tc_hyperlink.rb b/test/drawing/tc_hyperlink.rb
index 292f1f96..b3f204b5 100644
--- a/test/drawing/tc_hyperlink.rb
+++ b/test/drawing/tc_hyperlink.rb
@@ -51,10 +51,6 @@ class TestHyperlink < Test::Unit::TestCase
     assert_equal(@hyperlink.highlightClick, false )
   end
 
-  def test_id
-    assert_equal(@hyperlink.send(:id), 2)
-  end
-
   def test_history
     assert_nothing_raised { @hyperlink.history = false }
     assert_raise(ArgumentError) {@hyperlink.history = "bob"}
diff --git a/test/drawing/tc_pic.rb b/test/drawing/tc_pic.rb
index a892678d..79a3194d 100644
--- a/test/drawing/tc_pic.rb
+++ b/test/drawing/tc_pic.rb
@@ -93,4 +93,10 @@ class TestPic < Test::Unit::TestCase
     assert(errors.empty?, "error free validation")
   end
 
+  def test_to_xml_has_correct_r_id
+    r_id = @image.anchor.drawing.relationships.for(@image).Id
+    doc = Nokogiri::XML(@image.anchor.drawing.to_xml_string)
+    assert_equal r_id, doc.xpath("//a:blip").first["r:embed"]
+  end
+  
 end
diff --git a/test/rels/tc_relationship.rb b/test/rels/tc_relationship.rb
index 32b9e4a1..add1654f 100644
--- a/test/rels/tc_relationship.rb
+++ b/test/rels/tc_relationship.rb
@@ -1,26 +1,44 @@
 require 'tc_helper.rb'
 
 class TestRelationships < Test::Unit::TestCase
-  def setup
+  
+  def test_instances_with_different_attributes_have_unique_ids
+    rel_1 = Axlsx::Relationship.new(Object.new, Axlsx::WORKSHEET_R, 'target')
+    rel_2 = Axlsx::Relationship.new(Object.new, Axlsx::COMMENT_R, 'foobar')
+    assert_not_equal rel_1.Id, rel_2.Id
   end
-
-  def teardown
+  
+  def test_instances_with_same_attributes_share_id
+    source_obj = Object.new
+    instance = Axlsx::Relationship.new(source_obj, Axlsx::WORKSHEET_R, 'target')
+    assert_equal instance.Id, Axlsx::Relationship.new(source_obj, Axlsx::WORKSHEET_R, 'target').Id
   end
-
+  
+  def test_target_is_only_considered_for_same_attributes_check_if_target_mode_is_external
+    source_obj = Object.new
+    rel_1 = Axlsx::Relationship.new(source_obj, Axlsx::WORKSHEET_R, 'target')
+    rel_2 = Axlsx::Relationship.new(source_obj, Axlsx::WORKSHEET_R, '../target')
+    assert_equal rel_1.Id, rel_2.Id
+    
+    rel_3 = Axlsx::Relationship.new(source_obj, Axlsx::HYPERLINK_R, 'target', :target_mode => :External)
+    rel_4 = Axlsx::Relationship.new(source_obj, Axlsx::HYPERLINK_R, '../target', :target_mode => :External)
+    assert_not_equal rel_3.Id, rel_4.Id
+  end
+  
   def test_type
-    assert_raise(ArgumentError) { Axlsx::Relationship.new 'type', 'target' }
-    assert_nothing_raised { Axlsx::Relationship.new Axlsx::WORKSHEET_R, 'target' }
-    assert_nothing_raised { Axlsx::Relationship.new Axlsx::COMMENT_R, 'target' }
+    assert_raise(ArgumentError) { Axlsx::Relationship.new nil, 'type', 'target' }
+    assert_nothing_raised { Axlsx::Relationship.new nil, Axlsx::WORKSHEET_R, 'target' }
+    assert_nothing_raised { Axlsx::Relationship.new nil, Axlsx::COMMENT_R, 'target' }
   end
 
   def test_target_mode
-    assert_raise(ArgumentError) { Axlsx::Relationship.new 'type', 'target', :target_mode => "FISH" }
-    assert_nothing_raised { Axlsx::Relationship.new( Axlsx::WORKSHEET_R, 'target', :target_mode => :External) }
+    assert_raise(ArgumentError) { Axlsx::Relationship.new nil, 'type', 'target', :target_mode => "FISH" }
+    assert_nothing_raised { Axlsx::Relationship.new( nil, Axlsx::WORKSHEET_R, 'target', :target_mode => :External) }
   end
 
   def test_ampersand_escaping_in_target
-    r = Axlsx::Relationship.new(Axlsx::HYPERLINK_R, "http://example.com?foo=1&bar=2", :target_mod => :External)
-    doc = Nokogiri::XML(r.to_xml_string(1))
+    r = Axlsx::Relationship.new(nil, Axlsx::HYPERLINK_R, "http://example.com?foo=1&bar=2", :target_mod => :External)
+    doc = Nokogiri::XML(r.to_xml_string)
     assert_equal(doc.xpath("//Relationship[@Target='http://example.com?foo=1&bar=2']").size, 1)
   end
 end
diff --git a/test/rels/tc_relationships.rb b/test/rels/tc_relationships.rb
index 356e4691..fe5a1ce5 100644
--- a/test/rels/tc_relationships.rb
+++ b/test/rels/tc_relationships.rb
@@ -2,6 +2,17 @@ require 'tc_helper.rb'
 
 class TestRelationships < Test::Unit::TestCase
 
+  def test_for
+    source_obj_1, source_obj_2 = Object.new, Object.new
+    rel_1 = Axlsx::Relationship.new(source_obj_1, Axlsx::WORKSHEET_R, "bar")
+    rel_2 = Axlsx::Relationship.new(source_obj_2, Axlsx::WORKSHEET_R, "bar")
+    rels = Axlsx::Relationships.new
+    rels << rel_1 
+    rels << rel_2
+    assert_equal rel_1, rels.for(source_obj_1)
+    assert_equal rel_2, rels.for(source_obj_2)
+  end
+  
   def test_valid_document
     @rels = Axlsx::Relationships.new
     schema = Nokogiri::XML::Schema(File.open(Axlsx::RELS_XSD))
@@ -12,7 +23,7 @@ class TestRelationships < Test::Unit::TestCase
       errors << error
     end
 
-    @rels << Axlsx::Relationship.new(Axlsx::WORKSHEET_R, "bar")
+    @rels << Axlsx::Relationship.new(nil, Axlsx::WORKSHEET_R, "bar")
     doc = Nokogiri::XML(@rels.to_xml_string)
     errors = []
     schema.validate(doc).each do |error|
diff --git a/test/tc_helper.rb b/test/tc_helper.rb
index af40a1e4..c09446b7 100644
--- a/test/tc_helper.rb
+++ b/test/tc_helper.rb
@@ -8,3 +8,7 @@ end
 require 'test/unit'
 require "timecop"
 require "axlsx.rb"
+
+# Make sure all valid rIds are > 1000 - this should help catching the cases where rId is still
+# determined by looking at the index of an object in an array etc.
+Axlsx::Relationship.instance_variable_set :@next_free_id_counter, 1000 
diff --git a/test/workbook/tc_workbook.rb b/test/workbook/tc_workbook.rb
index 591160f4..32b6935a 100644
--- a/test/workbook/tc_workbook.rb
+++ b/test/workbook/tc_workbook.rb
@@ -116,5 +116,10 @@ class TestWorkbook < Test::Unit::TestCase
     assert_equal(doc.xpath('//xmlns:workbook/xmlns:definedNames/xmlns:definedName').inner_text, @wb.worksheets[0].auto_filter.defined_name)
   end
 
-
+  def test_to_xml_uses_correct_rIds_for_pivotCache
+    ws = @wb.add_worksheet
+    pivot_table = ws.add_pivot_table('G5:G6', 'A1:D5')
+    doc = Nokogiri::XML(@wb.to_xml_string)
+    assert_equal pivot_table.cache_definition.rId, doc.xpath("//xmlns:pivotCache").first["r:id"]
+  end
 end
diff --git a/test/workbook/worksheet/tc_pivot_table.rb b/test/workbook/worksheet/tc_pivot_table.rb
index d909eeb6..ee90bec2 100644
--- a/test/workbook/worksheet/tc_pivot_table.rb
+++ b/test/workbook/worksheet/tc_pivot_table.rb
@@ -72,11 +72,6 @@ class TestPivotTable < Test::Unit::TestCase
     assert_equal(@ws.pivot_tables.first.pn, "pivotTables/pivotTable1.xml")
   end
 
-  def test_rId
-    @ws.add_pivot_table('G5:G6', 'A1:D5')
-    assert_equal(@ws.pivot_tables.first.rId, "rId1")
-  end
-
   def test_index
     @ws.add_pivot_table('G5:G6', 'A1:D5')
     assert_equal(@ws.pivot_tables.first.index, @ws.workbook.pivot_tables.index(@ws.pivot_tables.first))
diff --git a/test/workbook/worksheet/tc_pivot_table_cache_definition.rb b/test/workbook/worksheet/tc_pivot_table_cache_definition.rb
index 2b4389b7..a38e808a 100644
--- a/test/workbook/worksheet/tc_pivot_table_cache_definition.rb
+++ b/test/workbook/worksheet/tc_pivot_table_cache_definition.rb
@@ -21,7 +21,7 @@ class TestPivotTableCacheDefinition < Test::Unit::TestCase
   end
 
   def test_rId
-    assert_equal('rId1', @cache_definition.rId)
+    assert_equal @pivot_table.relationships.for(@cache_definition).Id, @cache_definition.rId
   end
 
   def test_index
diff --git a/test/workbook/worksheet/tc_table.rb b/test/workbook/worksheet/tc_table.rb
index de86b886..6f39bb13 100644
--- a/test/workbook/worksheet/tc_table.rb
+++ b/test/workbook/worksheet/tc_table.rb
@@ -36,8 +36,8 @@ class TestTable < Test::Unit::TestCase
   end
 
   def test_rId
-    @ws.add_table("A1:D5")
-    assert_equal(@ws.tables.first.rId, "rId1")
+    table = @ws.add_table("A1:D5")
+    assert_equal @ws.relationships.for(table).Id, table.rId
   end
 
   def test_index
diff --git a/test/workbook/worksheet/tc_worksheet.rb b/test/workbook/worksheet/tc_worksheet.rb
index 980c9e01..00983fec 100644
--- a/test/workbook/worksheet/tc_worksheet.rb
+++ b/test/workbook/worksheet/tc_worksheet.rb
@@ -123,9 +123,7 @@ class TestWorksheet < Test::Unit::TestCase
   end
 
   def test_rId
-    assert_equal(@ws.rId, "rId1")
-    ws = @ws.workbook.add_worksheet
-    assert_equal(ws.rId, "rId2")
+    assert_equal @ws.workbook.relationships.for(@ws).Id, @ws.rId
   end
 
   def test_index
@@ -341,16 +339,16 @@ class TestWorksheet < Test::Unit::TestCase
   def test_to_xml_string_drawing
     @ws.add_chart Axlsx::Pie3DChart
     doc = Nokogiri::XML(@ws.to_xml_string)
-    assert_equal(doc.xpath('//xmlns:worksheet/xmlns:drawing[@r:id="rId1"]').size, 1)
+    assert_equal @ws.send(:worksheet_drawing).relationship.Id, doc.xpath('//xmlns:worksheet/xmlns:drawing').first["r:id"]
   end
 
   def test_to_xml_string_tables
     @ws.add_row ["one", "two"]
     @ws.add_row [1, 2]
-    @ws.add_table "A1:B2"
+    table = @ws.add_table "A1:B2"
     doc = Nokogiri::XML(@ws.to_xml_string)
     assert_equal(doc.xpath('//xmlns:worksheet/xmlns:tableParts[@count="1"]').size, 1)
-    assert_equal(doc.xpath('//xmlns:worksheet/xmlns:tableParts/xmlns:tablePart[@r:id="rId1"]').size, 1)
+    assert_equal table.rId, doc.xpath('//xmlns:worksheet/xmlns:tableParts/xmlns:tablePart').first["r:id"]
   end
 
   def test_to_xml_string
diff --git a/test/workbook/worksheet/tc_worksheet_hyperlink.rb b/test/workbook/worksheet/tc_worksheet_hyperlink.rb
index 278c5add..748082ac 100644
--- a/test/workbook/worksheet/tc_worksheet_hyperlink.rb
+++ b/test/workbook/worksheet/tc_worksheet_hyperlink.rb
@@ -32,22 +32,13 @@ class TestWorksheetHyperlink < Test::Unit::TestCase
     assert_equal(@options[:ref], @a.ref)
   end
 
-  def test_id
-    @a.target = :external
-
-    assert_equal("rId1", @a.id)
-    @a.target = :internal
-    assert_equal(nil, @a.id)
-  end
-
-
   def test_to_xml_string_with_non_external
     doc = Nokogiri::XML(@ws.to_xml_string)
     assert_equal(doc.xpath("//xmlns:hyperlink[@ref='#{@a.ref}']").size, 1)
     assert_equal(doc.xpath("//xmlns:hyperlink[@tooltip='#{@a.tooltip}']").size, 1)
     assert_equal(doc.xpath("//xmlns:hyperlink[@location='#{@a.location}']").size, 1)
     assert_equal(doc.xpath("//xmlns:hyperlink[@display='#{@a.display}']").size, 1)
-    assert_equal(doc.xpath("//xmlns:hyperlink[@r:id='#{@a.id}']").size, 0)
+    assert_equal(doc.xpath("//xmlns:hyperlink[@r:id]").size, 0)
   end
 
   def test_to_xml_stirng_with_external
@@ -57,7 +48,7 @@ class TestWorksheetHyperlink < Test::Unit::TestCase
     assert_equal(doc.xpath("//xmlns:hyperlink[@tooltip='#{@a.tooltip}']").size, 1)
     assert_equal(doc.xpath("//xmlns:hyperlink[@display='#{@a.display}']").size, 1)
     assert_equal(doc.xpath("//xmlns:hyperlink[@location='#{@a.location}']").size, 0)
-    assert_equal(doc.xpath("//xmlns:hyperlink[@r:id='#{@a.id}']").size, 1)
+    assert_equal(doc.xpath("//xmlns:hyperlink[@r:id='#{@a.relationship.Id}']").size, 1)
   end
 end