EPPlus icon indicating copy to clipboard operation
EPPlus copied to clipboard

Fix generated file corruption for Pivot Tables with a named range as …

Open lieszkol opened this issue 6 years ago • 2 comments

We've been having trouble generating some Excel workbooks with all versions after 4.0.5 (1/8/2016, f3e3852c718e0109e7384fa781491dc4cc51a071).

After several hours of research we tracked the issue down to two problems:

  1. Changeset 92b972b9d8b3d90f3ff263e5cdd1493fea7d747c introduced a bug where, if a row is inserted on a worksheet that has a named range of the type "$C:$F", then EPPlus will redefine the range with an ending address that is greater than ExcelPackage.MaxRows.

The problem is that ExcelWorksheet.InsertRow() calls ExcelNamedRangeCollection.InsertRows(), which redefines the named range with a new address namedRange.Start.Row + rows without checking if this will result in an invalid address. I've added some rudimentary checks but a better solution would be to return from ExcelNamedRangeCollection.InsertRows() if the named range is of the type "$C:$F" (i.e. a named column range). However, I don't see how this could be done easily, because ExcelNamedRange does not have any property like "IsColumnReference" (or whatever). The same holds for InsertColumns.

In any case the fix works.

  1. Changeset 7b78083d1a0060dae2e12ff642dbab2fe704c60f introduced a bug where, if a pivot table refers to a named range, then the "SourceNamePath" is replaced with a "SourceAddressPath", leading to a corrupt workbook.

The problem is in ExcelWorksheet.SavePivotTables(), with this check:

if (pt.CacheDefinition.SourceRange.IsName)

This returns "false" even if the pivot table's source range is an ExcelNamedRange, because ExcelNamedRange.IsName only returns true if the named range refers to another named range (is that even possible?).

I've replaced this with a more exact condition, which fixes the problem:

if (pt.CacheDefinition.SourceRange is ExcelNamedRange && !string.IsNullOrEmpty(((ExcelNamedRange)pt.CacheDefinition.SourceRange).Name))

But again, there may be a better approach.


I've not added any unit tests as I've already spend 7+ hours tracking down these problems and am unfamiliar with how unit tests work on this project. The work was made more difficult by changeset 7b78083d1a0060dae2e12ff642dbab2fe704c60f which basically includes all files in the solution (with a minor change in the header) PLUS the one breaking change described in 2) above.

I believe this fix should help some of the folks who've commented on https://github.com/JanKallman/EPPlus/issues/38

lieszkol avatar Aug 14 '18 13:08 lieszkol

How is work progressing on this issue? I really look forward to these corrections.

nsemikov avatar Nov 21 '19 15:11 nsemikov

I cannot do any more than I already have in regards to this issue. @JanKallman would need to review and merge it into master if he approves of the fixes.

lieszkol avatar Nov 22 '19 15:11 lieszkol