From 92b73ffb1e5cdfb001da684c463856f79aff6e11 Mon Sep 17 00:00:00 2001 From: Stefan Daschek Date: Fri, 5 Jul 2013 04:22:35 +0200 Subject: Make relationship ids more reliable. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- lib/axlsx/drawing/chart.rb | 6 +- lib/axlsx/drawing/drawing.rb | 16 +----- lib/axlsx/drawing/graphic_frame.rb | 11 +--- lib/axlsx/drawing/hyperlink.rb | 17 ++---- lib/axlsx/drawing/pic.rb | 13 ++--- lib/axlsx/package.rb | 6 +- lib/axlsx/rels/relationship.rb | 67 +++++++++++++++++++--- lib/axlsx/rels/relationships.rb | 9 ++- lib/axlsx/workbook/workbook.rb | 13 ++--- lib/axlsx/workbook/worksheet/comments.rb | 6 +- lib/axlsx/workbook/worksheet/pivot_table.rb | 10 +--- .../worksheet/pivot_table_cache_definition.rb | 5 +- lib/axlsx/workbook/worksheet/pivot_tables.rb | 2 +- lib/axlsx/workbook/worksheet/table.rb | 5 +- lib/axlsx/workbook/worksheet/tables.rb | 2 +- lib/axlsx/workbook/worksheet/worksheet.rb | 13 +---- lib/axlsx/workbook/worksheet/worksheet_comments.rb | 11 ++-- lib/axlsx/workbook/worksheet/worksheet_drawing.rb | 12 +--- .../workbook/worksheet/worksheet_hyperlink.rb | 15 ++--- test/drawing/tc_drawing.rb | 5 -- test/drawing/tc_graphic_frame.rb | 11 ++-- test/drawing/tc_hyperlink.rb | 4 -- test/drawing/tc_pic.rb | 6 ++ test/rels/tc_relationship.rb | 40 +++++++++---- test/rels/tc_relationships.rb | 13 ++++- test/tc_helper.rb | 4 ++ test/workbook/tc_workbook.rb | 7 ++- test/workbook/worksheet/tc_pivot_table.rb | 5 -- .../worksheet/tc_pivot_table_cache_definition.rb | 2 +- test/workbook/worksheet/tc_table.rb | 4 +- test/workbook/worksheet/tc_worksheet.rb | 10 ++-- test/workbook/worksheet/tc_worksheet_hyperlink.rb | 13 +---- 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 << '' str << '' str << '' - str << '' + str << '' str << '' str << '' str << '' 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 << ' "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 << '' str << '' - str << '' + str << '' str << '' str << '' str << '' 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 << '' 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 << '' str << '' - each_with_index { |rel, index| rel.to_xml_string(index+1, str) } + each{ |rel| rel.to_xml_string(str) } str << '' 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 << '' - pivot_tables.each_with_index do |pivot_table, index| - rId = "rId#{@worksheets.size + index + 1 }" - str << '' + pivot_tables.each do |pivot_table| + str << '' end str << '' 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 << "" + str << "" 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 << "" + str << "" 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 -- cgit v1.2.3