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

13 files changed, 70 insertions, 54 deletions

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