Skip to content

Commit

Permalink
Merge pull request #578 from tealeg/issue-574
Browse files Browse the repository at this point in the history
Make hyperllink loading non destructive
  • Loading branch information
tealeg authored Jun 9, 2020
2 parents 04a94a3 + 2962baa commit 414693d
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 53 deletions.
113 changes: 72 additions & 41 deletions lib.go
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,7 @@ func fillCellDataFromInlineString(rawcell xlsxC, cell *Cell) {
// rows from a XSLXWorksheet, populates them with Cells and resolves
// the value references from the reference table and stores them in
// the rows and columns.
func readRowsFromSheet(Worksheet *xlsxWorksheet, file *File, sheet *Sheet, rowLimit int) error {
func readRowsFromSheet(Worksheet *xlsxWorksheet, file *File, sheet *Sheet, rowLimit int, linkTable hyperlinkTable) error {
var row *Row
var maxCol, maxRow, colCount, rowCount int
var reftable *RefTable
Expand Down Expand Up @@ -543,7 +543,7 @@ func readRowsFromSheet(Worksheet *xlsxWorksheet, file *File, sheet *Sheet, rowLi
if err != nil {
return wrap(err)
}
x, _, err := GetCoordsFromCellIDString(rawcell.R)
x, y, err := GetCoordsFromCellIDString(rawcell.R)
if err != nil {
return wrap(err)
}
Expand All @@ -559,6 +559,11 @@ func readRowsFromSheet(Worksheet *xlsxWorksheet, file *File, sheet *Sheet, rowLi
cell.NumFmt, cell.parsedNumFmt = file.styles.getNumberFormat(rawcell.S)
}
cell.date1904 = file.Date1904

if hyperlink, found := linkTable[coord{x: x, y: y}]; found {
cell.Hyperlink = hyperlink
}

// Cell is considered hidden if the row or the column of this cell is hidden
col := sheet.Cols.FindColByIndex(cellX + 1)
cell.Hidden = rawrow.Hidden || (col != nil && col.Hidden != nil && *col.Hidden)
Expand Down Expand Up @@ -613,43 +618,19 @@ func readSheetViews(xSheetViews xlsxSheetViews) []SheetView {
return sheetViews
}

// readSheetFromFile is the logic of converting a xlsxSheet struct
// into a Sheet struct. This work can be done in parallel and so
// readSheetsFromZipFile will spawn an instance of this function per
// sheet and get the results back on the provided channel.
func readSheetFromFile(rsheet xlsxSheet, fi *File, sheetXMLMap map[string]string, rowLimit int) (sheet *Sheet, errRes error) {
defer func() {
if x := recover(); x != nil {
errRes = errors.New(fmt.Sprint(x))
}
}()

wrap := func(err error) (*Sheet, error) {
return nil, fmt.Errorf("readSheetFromFile: %w", err)
}

worksheet, err := getWorksheetFromSheet(rsheet, fi.worksheets, sheetXMLMap, rowLimit)
if err != nil {
return wrap(err)
}
type coord struct {
x int
y int
}

sheet, err = NewSheetWithCellStore(rsheet.Name, fi.cellStoreConstructor)
if err != nil {
return wrap(err)
}
type hyperlinkTable map[coord]Hyperlink

sheet.File = fi
err = readRowsFromSheet(worksheet, fi, sheet, rowLimit)
if err != nil {
return wrap(err)
func makeHyperlinkTable(worksheet *xlsxWorksheet, fi *File, rsheet *xlsxSheet) (hyperlinkTable, error) {
wrap := func(err error) (hyperlinkTable, error) {
return nil, fmt.Errorf("makeHyperlinkTable: %w", err)
}

sheet.Hidden = rsheet.State == sheetStateHidden || rsheet.State == sheetStateVeryHidden
sheet.SheetViews = readSheetViews(worksheet.SheetViews)
if worksheet.AutoFilter != nil {
autoFilterBounds := strings.Split(worksheet.AutoFilter.Ref, ":")
sheet.AutoFilter = &AutoFilter{autoFilterBounds[0], autoFilterBounds[1]}
}
table := make(hyperlinkTable)

// Convert xlsxHyperlinks to Hyperlinks
if worksheet.Hyperlinks != nil {
Expand Down Expand Up @@ -688,13 +669,63 @@ func readSheetFromFile(rsheet xlsxSheet, fi *File, sheetXMLMap map[string]string
if err != nil {
return wrap(err)
}
row, err := sheet.Row(y)
if err != nil {
return wrap(err)
}
cell := row.GetCell(x)
cell.Hyperlink = newHyperLink
table[coord{x: x, y: y}] = newHyperLink
}

// row, err := sheet.Row(y)
// if err != nil {
// return wrap(err)
// }
// fmt.Printf("%d, %d, %+v\n", x, y, row)

// // cell := row.GetCell(x)
// // cell.Hyperlink = newHyperLink
// }
}
return table, nil
}

// readSheetFromFile is the logic of converting a xlsxSheet struct
// into a Sheet struct. This work can be done in parallel and so
// readSheetsFromZipFile will spawn an instance of this function per
// sheet and get the results back on the provided channel.
func readSheetFromFile(rsheet xlsxSheet, fi *File, sheetXMLMap map[string]string, rowLimit int) (sheet *Sheet, errRes error) {
defer func() {
if x := recover(); x != nil {
errRes = errors.New(fmt.Sprint(x))
}
}()

wrap := func(err error) (*Sheet, error) {
return nil, fmt.Errorf("readSheetFromFile: %w", err)
}

worksheet, err := getWorksheetFromSheet(rsheet, fi.worksheets, sheetXMLMap, rowLimit)
if err != nil {
return wrap(err)
}

linkTable, err := makeHyperlinkTable(worksheet, fi, &rsheet)
if err != nil {
return wrap(err)
}

sheet, err = NewSheetWithCellStore(rsheet.Name, fi.cellStoreConstructor)
if err != nil {
return wrap(err)
}

sheet.File = fi
err = readRowsFromSheet(worksheet, fi, sheet, rowLimit, linkTable)
if err != nil {
return wrap(err)
}

sheet.Hidden = rsheet.State == sheetStateHidden || rsheet.State == sheetStateVeryHidden
sheet.SheetViews = readSheetViews(worksheet.SheetViews)
if worksheet.AutoFilter != nil {
autoFilterBounds := strings.Split(worksheet.AutoFilter.Ref, ":")
sheet.AutoFilter = &AutoFilter{autoFilterBounds[0], autoFilterBounds[1]}
}

sheet.SheetFormat.DefaultColWidth = worksheet.SheetFormatPr.DefaultColWidth
Expand Down
74 changes: 62 additions & 12 deletions lib_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,8 @@ func TestLib(t *testing.T) {
file.referenceTable = MakeSharedStringRefTable(sst)
sheet, err := NewSheet("test")
c.Assert(err, qt.IsNil)
err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit)
lt := make(hyperlinkTable)
err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit, lt)
c.Assert(err, qt.IsNil)
c.Assert(sheet.MaxRow, qt.Equals, 2)
c.Assert(sheet.MaxCol, qt.Equals, 2)
Expand Down Expand Up @@ -439,9 +440,12 @@ func TestLib(t *testing.T) {

sheet, err := NewSheetWithCellStore("test", constructor)
c.Assert(err, qt.IsNil)

lt := make(hyperlinkTable)

// Discarding all return values; this test is a regression for
// a panic due to an "index out of range."
readRowsFromSheet(worksheet, file, sheet, NoRowLimit)
readRowsFromSheet(worksheet, file, sheet, NoRowLimit, lt)
})

csRunC(c, "ReadRowsFromSheetWithLeadingEmptyRows", func(c *qt.C, constructor CellStoreConstructor) {
Expand Down Expand Up @@ -489,7 +493,9 @@ func TestLib(t *testing.T) {
file.referenceTable = MakeSharedStringRefTable(sst)
sheet, err := NewSheetWithCellStore("test", constructor)
c.Assert(err, qt.IsNil)
err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit)
lt := make(hyperlinkTable)

err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit, lt)
c.Assert(err, qt.IsNil)
c.Assert(sheet.MaxRow, qt.Equals, 5)
c.Assert(sheet.MaxCol, qt.Equals, 1)
Expand Down Expand Up @@ -569,7 +575,9 @@ func TestLib(t *testing.T) {
file.referenceTable = MakeSharedStringRefTable(sst)
sheet, err := NewSheetWithCellStore("test", constructor)
c.Assert(err, qt.IsNil)
err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit)
lt := make(hyperlinkTable)

err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit, lt)
c.Assert(err, qt.IsNil)
c.Assert(sheet.MaxRow, qt.Equals, 2)
c.Assert(sheet.MaxCol, qt.Equals, 4)
Expand Down Expand Up @@ -715,7 +723,9 @@ func TestLib(t *testing.T) {
file.referenceTable = MakeSharedStringRefTable(sst)
sheet, err := NewSheetWithCellStore("test", constructor)
c.Assert(err, qt.IsNil)
err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit)
lt := make(hyperlinkTable)

err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit, lt)
c.Assert(err, qt.IsNil)
c.Assert(sheet.MaxRow, qt.Equals, 3)
c.Assert(sheet.MaxCol, qt.Equals, 3)
Expand Down Expand Up @@ -761,7 +771,9 @@ func TestLib(t *testing.T) {
file.referenceTable = MakeSharedStringRefTable(sst)
sheet, err := NewSheetWithCellStore("test", constructor)
c.Assert(err, qt.IsNil)
err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit)
lt := make(hyperlinkTable)

err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit, lt)
c.Assert(err, qt.IsNil)
c.Assert(sheet.MaxCol, qt.Equals, 4)
c.Assert(sheet.MaxRow, qt.Equals, 8)
Expand Down Expand Up @@ -876,7 +888,10 @@ func TestLib(t *testing.T) {
file.referenceTable = MakeSharedStringRefTable(sst)
sheet, err := NewSheetWithCellStore("test", constructor)
c.Assert(err, qt.IsNil)
err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit)

lt := make(hyperlinkTable)

err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit, lt)
c.Assert(err, qt.IsNil)
c.Assert(sheet.MaxRow, qt.Equals, 2)
c.Assert(sheet.MaxCol, qt.Equals, 4)
Expand Down Expand Up @@ -955,7 +970,9 @@ func TestLib(t *testing.T) {
file.referenceTable = MakeSharedStringRefTable(sst)
sheet, err := NewSheetWithCellStore("test", constructor)
c.Assert(err, qt.IsNil)
err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit)
lt := make(hyperlinkTable)

err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit, lt)
c.Assert(err, qt.IsNil)
c.Assert(sheet.MaxRow, qt.Equals, 1)
c.Assert(sheet.MaxCol, qt.Equals, 6)
Expand Down Expand Up @@ -1032,7 +1049,9 @@ func TestLib(t *testing.T) {
file.referenceTable = MakeSharedStringRefTable(sst)
sheet, err := NewSheetWithCellStore("test", constructor)
c.Assert(err, qt.IsNil)
err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit)
lt := make(hyperlinkTable)

err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit, lt)
c.Assert(err, qt.IsNil)
c.Assert(sheet.MaxRow, qt.Equals, 1)
c.Assert(sheet.MaxCol, qt.Equals, 2)
Expand Down Expand Up @@ -1175,7 +1194,10 @@ func TestLib(t *testing.T) {
file.cellStoreConstructor = constructor
sheet, err := NewSheetWithCellStore("test", constructor)
c.Assert(err, qt.IsNil)
err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit)

lt := make(hyperlinkTable)

err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit, lt)
c.Assert(err, qt.IsNil)
c.Assert(sheet.MaxCol, qt.Equals, 3)
c.Assert(sheet.MaxRow, qt.Equals, 2)
Expand Down Expand Up @@ -1320,7 +1342,10 @@ func TestLib(t *testing.T) {

sheet, err := NewSheetWithCellStore("test", constructor)
c.Assert(err, qt.IsNil)
err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit)

lt := make(hyperlinkTable)

err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit, lt)
c.Assert(err, qt.IsNil)
row, err := sheet.Row(3)
c.Assert(err, qt.Equals, nil)
Expand Down Expand Up @@ -1414,7 +1439,8 @@ func TestReadRowsFromSheet(t *testing.T) {
worksheet.mapMergeCells()
sheet, err := NewSheetWithCellStore("test", constructor)
c.Assert(err, qt.IsNil)
err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit)
lt := make(hyperlinkTable)
err = readRowsFromSheet(worksheet, file, sheet, NoRowLimit, lt)
c.Assert(err, qt.IsNil)
row, err := sheet.Row(0)
c.Assert(err, qt.Equals, nil)
Expand Down Expand Up @@ -1772,3 +1798,27 @@ func TestGrowRowCellSliceDuringFileLoad(t *testing.T) {
c.Assert(err, qt.Equals, nil)
})
}

func TestIssueSheetsWithHyperlinksHaveLegibleValues(t *testing.T) {
c := qt.New(t)

// Issue 574 concerned a sheet with cell values that
// incorrectly showed up blank. This issue was caused by
// mutable state being abused during the data load
// (essentially using Sheet.Row(n) before the sheet was fully
// loaded. The file: testdocs/issue574.xlsx illustrates this issue.
f, err := OpenFile("testdocs/issue574.xlsx")
c.Assert(err, qt.Equals, nil)

sheet, ok := f.Sheet["Sheet1"]
c.Assert(ok, qt.Equals, true)
c.Assert(sheet.MaxRow, qt.Equals, 4)
sheet.ForEachRow(func(r *Row) error {
r.ForEachCell(func(cell *Cell) error {
c.Assert(cell, qt.Not(qt.IsNil))
c.Assert(cell.Value, qt.Not(qt.Equals), "")
return nil
})
return nil
})
}
Binary file added testdocs/issue574.xlsx
Binary file not shown.

0 comments on commit 414693d

Please sign in to comment.