diff --git a/xl-ooxml/src/com/tjclp/xl/ooxml/style/StyleIndex.scala b/xl-ooxml/src/com/tjclp/xl/ooxml/style/StyleIndex.scala index c677833..f61f1fe 100644 --- a/xl-ooxml/src/com/tjclp/xl/ooxml/style/StyleIndex.scala +++ b/xl-ooxml/src/com/tjclp/xl/ooxml/style/StyleIndex.scala @@ -160,22 +160,20 @@ object StyleIndex: * Build style index for workbook with source, preserving original styles. * * This variant is used during surgical modification to avoid corruption: - * - Deduplicates styles ONLY from modified sheets (optimal compression) - * - Preserves original styles from source for unmodified sheets (no remapping needed) - * - Ensures unmodified sheets' style references remain valid after write + * - Preserves ALL original styles from source (including duplicates) + * - Deduplicates only truly new styles from modified sheets + * - Generates remappings for ALL sheets (not just modified ones) * - * Strategy: - * 1. Parse original styles.xml to get complete WorkbookStyles - * 2. Deduplicate styles from modified sheets only - * 3. Ensure all original styles are present in output (fill gaps if needed) - * 4. Return remappings ONLY for modified sheets (unmodified sheets use original IDs) + * All sheets need remappings because metadata-only changes (add-sheet, reorder) cause ALL sheets + * to be regenerated from the domain model, not copied verbatim from source. Without remappings, + * unmodified-but-regenerated sheets would fall back to styleId 0, stripping all styles (TJC-751). * * @param wb * The workbook with modified sheets * @param ctx * Source context providing modification tracker and original file path * @return - * (StyleIndex with all original + deduplicated styles, Map[modifiedSheetIdx -> remapping]) + * (StyleIndex with all original + new styles, Map[sheetIdx -> remapping]) */ @SuppressWarnings( Array( @@ -188,9 +186,8 @@ object StyleIndex: wb: Workbook, ctx: SourceContext ): (StyleIndex, Map[Int, Map[Int, Int]]) = - // Extract values from context val tracker = ctx.modificationTracker - val modifiedSheetIndices = tracker.modifiedSheets + val needsRemappingForAll = tracker.modifiedMetadata || tracker.reorderedSheets val sourcePath = ctx.sourcePath import scala.collection.mutable import java.util.zip.ZipInputStream @@ -251,9 +248,13 @@ object StyleIndex: })) var nextNumFmtId = if numFmtsBuilder.isEmpty then 164 else numFmtsBuilder.map(_._1).max + 1 - // Step 4: Process ONLY modified sheets for style remapping - val remappings = wb.sheets.zipWithIndex.flatMap { case (sheet, sheetIdx) => - if modifiedSheetIndices.contains(sheetIdx) then + // Step 4: Process sheets for style remapping + // When metadata or reorder changes force ALL sheets to be regenerated from domain model, + // every sheet needs a remapping — otherwise unmodified sheets fall back to styleId 0, + // stripping all styles (TJC-751). When only specific sheets are modified, unmodified + // sheets are copied verbatim from source and don't need remappings. + val remappings = wb.sheets.zipWithIndex.map { case (sheet, sheetIdx) => + if needsRemappingForAll || tracker.modifiedSheets.contains(sheetIdx) then val registry = sheet.styleRegistry val remapping = mutable.Map[Int, Int]() @@ -302,10 +303,10 @@ object StyleIndex: case _ => () } - Some(sheetIdx -> remapping.toMap) + sheetIdx -> remapping.toMap else - // Unmodified sheet - no remapping needed (uses original style IDs) - None + // Unmodified sheet copied verbatim from source — remapping unused + sheetIdx -> Map.empty[Int, Int] }.toMap // Step 5: Finalize component vectors (original + any new components) 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 c420993..92352b4 100644 --- a/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala +++ b/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala @@ -1128,6 +1128,247 @@ class XlsxWriterCorruptionRegressionSpec extends FunSuite: Files.deleteIfExists(output) } + // ===== Regression Test for Style Stripping on Untouched Sheets (TJC-751) ===== + + test("add-sheet preserves styles on untouched sheets (TJC-751)") { + // Regression test for: styles stripped on ALL untouched sheets during metadata-only writes + // Bug: StyleIndex.fromWorkbookWithSource only created remappings for modified sheets, + // but metadata changes (add-sheet) force ALL sheets to be regenerated from domain model. + // Unmodified sheets got Map.empty remapping → all styles fell back to styleId 0 (default). + // Impact: P0 - All styling (bold, colors, number formats) silently stripped from untouched sheets + + val source = createWorkbookWithRichStyles() + val output = Files.createTempFile("tjc-751-add-sheet", ".xlsx") + + try + // Read the workbook (populates SourceContext) + val wb = XlsxReader.read(source).fold(err => fail(s"Failed to read: $err"), identity) + assertEquals(wb.sheets.size, 2, "Source should have 2 sheets") + + // Add a new sheet — triggers modifiedMetadata=true, modifiedSheets stays empty + val withNewSheet = wb.put(Sheet("NewComps").put(ref"A1" -> "New data")) + + // Verify tracker state: metadata modified but no existing sheets marked modified + val tracker = withNewSheet.sourceContext.get.modificationTracker + assert(tracker.modifiedMetadata, "modifiedMetadata should be true after add-sheet") + assert( + !tracker.modifiedSheets.contains(0) && !tracker.modifiedSheets.contains(1), + "Existing sheets should NOT be in modifiedSheets" + ) + + // Write the modified workbook + XlsxWriter + .write(withNewSheet, output) + .fold(err => fail(s"Failed to write: $err"), identity) + + // Read back and verify + val result = XlsxReader.read(output).fold(err => fail(s"Failed to reload: $err"), identity) + assertEquals(result.sheets.size, 3, "Output should have 3 sheets") + + // CRITICAL: Verify styles on "Styled" sheet are preserved (not stripped to default) + val styled = result("Styled").fold(err => fail(s"Sheet Styled not found: $err"), identity) + + // A1 should be bold + val a1Style = styled(ref"A1").styleId + .flatMap(id => styled.styleRegistry.get(id)) + assert(a1Style.isDefined, "A1 should have a style") + assert(a1Style.get.font.bold, "A1 should be bold (was stripped to default!)") + + // B1 should have a colored fill + val b1Style = styled(ref"B1").styleId + .flatMap(id => styled.styleRegistry.get(id)) + assert(b1Style.isDefined, "B1 should have a style") + assert( + b1Style.get.fill != Fill.default, + "B1 should have colored fill (was stripped to default!)" + ) + + // CRITICAL: Verify styles on "AlsoStyled" sheet are preserved + val alsoStyled = + result("AlsoStyled").fold(err => fail(s"Sheet AlsoStyled not found: $err"), identity) + + val a1Style2 = alsoStyled(ref"A1").styleId + .flatMap(id => alsoStyled.styleRegistry.get(id)) + assert(a1Style2.isDefined, "AlsoStyled A1 should have a style") + assert(a1Style2.get.font.bold, "AlsoStyled A1 should be bold (was stripped!)") + + // Also verify at the raw XML level: check that cells in untouched sheets have s= attributes + val outputZip = new ZipFile(output.toFile) + try + val sheet1Xml = readEntryString(outputZip, outputZip.getEntry("xl/worksheets/sheet1.xml")) + + // Cells with styles should have s="N" where N > 0 + val cellsWithStyles = + """s="(\d+)"""".r.findAllMatchIn(sheet1Xml).map(_.group(1).toInt).toList + assert( + cellsWithStyles.exists(_ > 0), + s"Sheet1 cells should have non-zero style indices but found: $cellsWithStyles" + ) + finally outputZip.close() + finally + Files.deleteIfExists(source) + Files.deleteIfExists(output) + } + + test("reorder preserves styles on all sheets (TJC-751 variant)") { + // Same bug trigger via reorderedSheets=true instead of modifiedMetadata=true + val source = createWorkbookWithRichStyles() + val output = Files.createTempFile("tjc-751-reorder", ".xlsx") + + try + val wb = XlsxReader.read(source).fold(err => fail(s"Failed to read: $err"), identity) + + // Reorder sheets: [Styled, AlsoStyled] → [AlsoStyled, Styled] + val reordered = wb + .reorder(Vector(SheetName.unsafe("AlsoStyled"), SheetName.unsafe("Styled"))) + .fold(err => fail(s"Failed to reorder: $err"), identity) + + val tracker = reordered.sourceContext.get.modificationTracker + assert(tracker.reorderedSheets, "reorderedSheets should be true") + + XlsxWriter + .write(reordered, output) + .fold(err => fail(s"Failed to write: $err"), identity) + + val result = XlsxReader.read(output).fold(err => fail(s"Failed to reload: $err"), identity) + assertEquals(result.sheetNames.map(_.value), Vector("AlsoStyled", "Styled")) + + // Verify styles survived the reorder + val styled = result("Styled").fold(err => fail(s"Sheet not found: $err"), identity) + val a1Style = styled(ref"A1").styleId.flatMap(id => styled.styleRegistry.get(id)) + assert(a1Style.isDefined, "A1 should have a style after reorder") + assert(a1Style.get.font.bold, "A1 should be bold after reorder (was stripped!)") + + val b1Style = styled(ref"B1").styleId.flatMap(id => styled.styleRegistry.get(id)) + assert(b1Style.isDefined, "B1 should have a style after reorder") + assert(b1Style.get.fill != Fill.default, "B1 fill stripped after reorder!") + + val alsoStyled = + result("AlsoStyled").fold(err => fail(s"Sheet not found: $err"), identity) + val a1Style2 = alsoStyled(ref"A1").styleId.flatMap(id => alsoStyled.styleRegistry.get(id)) + assert(a1Style2.isDefined, "AlsoStyled A1 should have a style after reorder") + assert(a1Style2.get.font.bold, "AlsoStyled A1 should be bold after reorder!") + finally + Files.deleteIfExists(source) + Files.deleteIfExists(output) + } + + private def createWorkbookWithRichStyles(): Path = + val path = Files.createTempFile("test-rich-styles", ".xlsx") + val out = new ZipOutputStream(Files.newOutputStream(path)) + out.setLevel(1) + + try + writeEntry( + out, + "[Content_Types].xml", + """ + + + + + + + +""" + ) + + writeEntry( + out, + "_rels/.rels", + """ + + +""" + ) + + writeEntry( + out, + "xl/workbook.xml", + """ + + + + + +""" + ) + + writeEntry( + out, + "xl/_rels/workbook.xml.rels", + """ + + + + +""" + ) + + // Styles: bold font, red solid fill, currency number format + writeEntry( + out, + "xl/styles.xml", + """ + + + + + + + + + + + + + + + + + + + + + + +""" + ) + + // Sheet 1: "Styled" — A1 is bold, B1 has red fill, C1 has currency format + writeEntry( + out, + "xl/worksheets/sheet1.xml", + """ + + + + Bold Header + Red Cell + 1234.56 + + +""" + ) + + // Sheet 2: "AlsoStyled" — A1 is bold + writeEntry( + out, + "xl/worksheets/sheet2.xml", + """ + + + + Bold in Sheet 2 + + +""" + ) + + finally out.close() + + path + private def createWorkbookWithoutSST(): Path = // Create a minimal workbook that uses inline strings (no sharedStrings.xml) val path = Files.createTempFile("test-no-sst", ".xlsx")