From aad3a7e15f4757cc8409851a87bc93e6bc181a9d Mon Sep 17 00:00:00 2001 From: Geoffrey Teale Date: Tue, 24 Oct 2023 06:24:46 +0200 Subject: [PATCH] stable cellStoreName in they key for the cellstore --- file.go | 18 +++++++----------- file_test.go | 33 ++++++++++++++++++++++++++++++++- sheet.go | 26 +++++++++++++++++++++++++- 3 files changed, 64 insertions(+), 13 deletions(-) diff --git a/file.go b/file.go index dd9fd955..1d34a478 100644 --- a/file.go +++ b/file.go @@ -10,7 +10,6 @@ import ( "os" "strconv" "strings" - "unicode/utf8" ) // File is a high level structure providing a slice of Sheet structs @@ -203,22 +202,16 @@ func (f *File) AddSheetWithCellStore(sheetName string, constructor CellStoreCons if _, exists := f.Sheet[sheetName]; exists { return nil, fmt.Errorf("duplicate sheet name '%s'.", sheetName) } - runeLength := utf8.RuneCountInString(sheetName) - if runeLength > 31 || runeLength == 0 { - return nil, fmt.Errorf("sheet name must be 31 or fewer characters long. It is currently '%d' characters long", runeLength) - } - // Iterate over the runes - for _, r := range sheetName { - // Excel forbids : \ / ? * [ ] - if r == ':' || r == '\\' || r == '/' || r == '?' || r == '*' || r == '[' || r == ']' { - return nil, fmt.Errorf("sheet name must not contain any restricted characters : \\ / ? * [ ] but contains '%s'", string(r)) - } + + if err := IsSaneSheetName(sheetName); err != nil { + return nil, fmt.Errorf("sheet name is not valid: %w", err) } sheet := &Sheet{ Name: sheetName, File: f, Selected: len(f.Sheets) == 0, Cols: &ColStore{}, + cellStoreName: sheetName, } sheet.cellStore, err = constructor() @@ -235,6 +228,9 @@ func (f *File) AppendSheet(sheet Sheet, sheetName string) (*Sheet, error) { if _, exists := f.Sheet[sheetName]; exists { return nil, fmt.Errorf("duplicate sheet name '%s'.", sheetName) } + if err := IsSaneSheetName(sheetName); err != nil { + return nil, fmt.Errorf("sheet name is not valid: %w", err) + } sheet.Name = sheetName sheet.File = f sheet.Selected = len(f.Sheets) == 0 diff --git a/file_test.go b/file_test.go index c8e74019..14ec2687 100644 --- a/file_test.go +++ b/file_test.go @@ -360,7 +360,7 @@ func TestFile(t *testing.T) { csRunO(c, "TestAddSheetWithEmptyName", func(c *qt.C, option FileOption) { f := NewFile(option) _, err := f.AddSheet("") - c.Assert(err.Error(), qt.Equals, "sheet name must be 31 or fewer characters long. It is currently '0' characters long") + c.Assert(err.Error(), qt.Contains, "sheet name must be 31 or fewer characters long. It is currently '0' characters long") }) // Test that we can append a sheet to a File @@ -386,6 +386,37 @@ func TestFile(t *testing.T) { c.Assert(err.Error(), qt.Equals, "duplicate sheet name 'MySheet'.") }) + // Test that AppendSheet doesn't lose rows because of a change in the sheet name (this really occurred see https://github.com/tealeg/xlsx/issues/783 ) + csRunO(c, "TestAppendSheetWithNewSheetNameDoesNotLoseRows", func(c *qt.C, option FileOption) { + + sourceFile, err := OpenFile("./testdocs/original.xlsx") + c.Assert(err, qt.IsNil) + c.Assert(len(sourceFile.Sheets), qt.Equals, 1) + s := sourceFile.Sheets[0] + + f := NewFile(option) + _, err = f.AppendSheet(*s, "Dave") + c.Assert(err, qt.IsNil) + + tmp := c.TempDir() + p := filepath.Join(tmp, "blokes.xlsx") + err = f.Save(p) + c.Assert(err, qt.IsNil) + + blokes, err := OpenFile(p) + c.Assert(err, qt.IsNil) + + + dave := blokes.Sheets[0] + if dave.currentRow != nil { + c.Assert(dave.cellStore.WriteRow(dave.currentRow), qt.IsNil) + } + cell, err := dave.Cell(0, 0) + c.Assert(err, qt.IsNil) + c.Assert(cell.String(), qt.Equals, "Column A") + + }) + // Test that we can read & create a 31 rune sheet name csRunO(c, "TestMaxSheetNameLength", func(c *qt.C, option FileOption) { // Open a genuine xlsx created by Microsoft Excel 2007 diff --git a/sheet.go b/sheet.go index c7a75a8b..06b2d6b7 100644 --- a/sheet.go +++ b/sheet.go @@ -7,6 +7,7 @@ import ( "io" "strconv" "strings" + "unicode/utf8" "github.com/shabbyrobe/xmlwriter" ) @@ -28,6 +29,9 @@ type Sheet struct { DataValidations []*xlsxDataValidation cellStore CellStore currentRow *Row + cellStoreName string // The first part of the key used in + // the cellStore. This name is stable, + // unlike the Name, which can change } // NewSheet constructs a Sheet with the default CellStore and returns @@ -39,9 +43,13 @@ func NewSheet(name string) (*Sheet, error) { // NewSheetWithCellStore constructs a Sheet, backed by a CellStore, // for which you must provide the constructor function. func NewSheetWithCellStore(name string, constructor CellStoreConstructor) (*Sheet, error) { + if err := IsSaneSheetName(name); err != nil { + return nil, fmt.Errorf("sheet name is invalid: %w", err) + } sheet := &Sheet{ Name: name, Cols: &ColStore{}, + cellStoreName: name, } var err error sheet.cellStore, err = constructor() @@ -207,7 +215,7 @@ func (s *Sheet) AddRow() *Row { } func makeRowKey(s *Sheet, i int) string { - return fmt.Sprintf("%s:%06d", s.Name, i) + return fmt.Sprintf("%s:%06d", s.cellStoreName, i) } // Add a new Row to a Sheet at a specific index @@ -936,3 +944,19 @@ func handleNumFmtIdForXLSX(NumFmtId int, styles *xlsxStyleSheet) (XfId int) { XfId = styles.addCellXf(xCellXf) return } + + +func IsSaneSheetName(sheetName string) error { + runeLength := utf8.RuneCountInString(sheetName) + if runeLength > 31 || runeLength == 0 { + return fmt.Errorf("sheet name must be 31 or fewer characters long. It is currently '%d' characters long", runeLength) + } + // Iterate over the runes + for _, r := range sheetName { + // Excel forbids : \ / ? * [ ] + if r == ':' || r == '\\' || r == '/' || r == '?' || r == '*' || r == '[' || r == ']' { + return fmt.Errorf("sheet name must not contain any restricted characters : \\ / ? * [ ] but contains '%s'", string(r)) + } + } + return nil +}