Merge pull request #786 from tealeg/cell-store-sheet-name-is-stable

stable cellStoreName in they key for the cellstore

Author: tealeg

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

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

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
+}