diff --git a/xl-ooxml/src/com/tjclp/xl/ooxml/XlsxWriter.scala b/xl-ooxml/src/com/tjclp/xl/ooxml/XlsxWriter.scala index 4937979..074aa0d 100644 --- a/xl-ooxml/src/com/tjclp/xl/ooxml/XlsxWriter.scala +++ b/xl-ooxml/src/com/tjclp/xl/ooxml/XlsxWriter.scala @@ -1008,10 +1008,10 @@ object XlsxWriter: if tracker.modifiedSheets.nonEmpty || tracker.modifiedMetadata then regenerate += "xl/sharedStrings.xml" - // When metadata is modified (add/remove/rename/reorder), regenerate ALL sheets. - // New sheets don't exist in source ZIP, and removed sheets change indices. - // This is the correct behavior - metadata changes are structural. - if tracker.modifiedMetadata then + // When metadata is modified or sheets are reordered, regenerate ALL sheets. + // New sheets don't exist in source ZIP, removed sheets change indices, + // and reordered sheets must be written to new positions. + if tracker.modifiedMetadata || tracker.reorderedSheets then // Metadata modified → regenerate all sheets and their relationships wb.sheets.indices.foreach { idx => regenerate += s"xl/worksheets/sheet${idx + 1}.xml" @@ -1404,10 +1404,11 @@ object XlsxWriter: else if sourceHasSharedStrings then sourceContext.foreach(ctx => copyPreservedPart(ctx.sourcePath, sharedStringsPath, zip)) - // When metadata is modified (add/remove/rename/reorder), we must regenerate ALL sheets - // because new sheets don't exist in source and sheet indices may have changed. + // When metadata is modified or sheets are reordered, we must regenerate ALL sheets + // because new sheets don't exist in source, sheet indices may have changed, + // or sheet positions must be remapped to match the new order. val sheetsToRegenerate = - if tracker.modifiedMetadata then workbook.sheets.indices.toSet + if tracker.modifiedMetadata || tracker.reorderedSheets then workbook.sheets.indices.toSet else tracker.modifiedSheets // Write sheets: regenerate modified, copy unmodified (if source available) diff --git a/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala b/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala index c78761f..c420993 100644 --- a/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala +++ b/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala @@ -1060,6 +1060,74 @@ class XlsxWriterCorruptionRegressionSpec extends FunSuite: Files.deleteIfExists(output) } + // ===== Regression Test for move-sheet Data Corruption (GH-207) ===== + + test("move-sheet preserves sheet data in correct positions after reorder (GH-207)") { + // Regression test for: move-sheet causes silent data corruption + // Bug: Workbook.reorder() sets reorderedSheets=true but NOT modifiedMetadata=true. + // The surgical writer only checked modifiedMetadata, so unmodified sheets were + // copied from the source ZIP in original positions while workbook.xml referenced + // them in the new order — causing name/data mismatch. + // Impact: P0 - Silent data corruption, sheet names point to wrong data + + // Create a workbook with 3 sheets, each with distinct content + val sheetA = Sheet("A").put(ref"A1" -> "I am Sheet A") + val sheetB = Sheet("B").put(ref"A1" -> "I am Sheet B") + val sheetC = Sheet("C").put(ref"A1" -> "I am Sheet C") + val wb = Workbook(Vector(sheetA, sheetB, sheetC)) + + // Write the initial workbook + val initial = Files.createTempFile("move-sheet-initial", ".xlsx") + XlsxWriter + .write(wb, initial) + .fold(err => fail(s"Failed to write initial: $err"), identity) + + // Read it back (to get SourceContext for surgical writes) + val wb2 = XlsxReader.read(initial).fold(err => fail(s"Failed to read: $err"), identity) + + // Reorder: move C to position 0 → order becomes [C, A, B] + val reordered = wb2 + .reorder(Vector(SheetName.unsafe("C"), SheetName.unsafe("A"), SheetName.unsafe("B"))) + .fold(err => fail(s"Failed to reorder: $err"), identity) + + // Write the reordered workbook + val output = Files.createTempFile("move-sheet-reordered", ".xlsx") + XlsxWriter + .write(reordered, output) + .fold(err => fail(s"Failed to write reordered: $err"), identity) + + // Read back and verify each sheet name maps to its correct data + val result = XlsxReader.read(output).fold(err => fail(s"Failed to read result: $err"), identity) + + // Verify sheet order + assertEquals(result.sheetNames.map(_.value), Vector("C", "A", "B")) + + // CRITICAL: Verify data integrity — each sheet must have its own data + val resultC = result("C").fold(err => fail(s"Sheet C not found: $err"), identity) + val resultA = result("A").fold(err => fail(s"Sheet A not found: $err"), identity) + val resultB = result("B").fold(err => fail(s"Sheet B not found: $err"), identity) + + assertEquals( + resultC(ref"A1").value, + CellValue.Text("I am Sheet C"), + "Sheet C has wrong data after reorder — data corruption!" + ) + assertEquals( + resultA(ref"A1").value, + CellValue.Text("I am Sheet A"), + "Sheet A has wrong data after reorder — data corruption!" + ) + assertEquals( + resultB(ref"A1").value, + CellValue.Text("I am Sheet B"), + "Sheet B has wrong data after reorder — data corruption!" + ) + + // Clean up + Files.deleteIfExists(initial) + Files.deleteIfExists(output) + } + private def createWorkbookWithoutSST(): Path = // Create a minimal workbook that uses inline strings (no sharedStrings.xml) val path = Files.createTempFile("test-no-sst", ".xlsx")