From d34355b927b4830bec182b114ff1609bdea7504a Mon Sep 17 00:00:00 2001 From: Steve McConnel Date: Tue, 7 Jul 2026 12:09:10 -0600 Subject: [PATCH 1/3] Fix SS export when image names (without extension) conflict (BL-16498) --- src/BloomExe/Spreadsheet/SpreadsheetIO.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/BloomExe/Spreadsheet/SpreadsheetIO.cs b/src/BloomExe/Spreadsheet/SpreadsheetIO.cs index b9ef488fc178..6a4689fecae5 100644 --- a/src/BloomExe/Spreadsheet/SpreadsheetIO.cs +++ b/src/BloomExe/Spreadsheet/SpreadsheetIO.cs @@ -185,8 +185,14 @@ int embedImage(string imageSrcPath, int rowNum, int colNum) using (Image image = Image.FromFile(imageSrcPath)) { string imageName = Path.GetFileNameWithoutExtension(imageSrcPath); - // Allow for image reuse even though it shouldn't happen. - if (worksheet.Drawings.Any(xx => xx.Name == imageName)) + // Image.jpg and image.png are different files, but the same name for storing here. (BL-16498) + // Names are treated as case-insensitive (this is Windows after all!), so we need to allow for that. + // Image reuse might conceivably happen as well, and this allows for that too. + if ( + worksheet.Drawings.Any(xx => + xx.Name.ToLowerInvariant() == imageName.ToLowerInvariant() + ) + ) imageName = $"{imageName}-{rowNum}"; var origImageHeight = image.Size.Height; var origImageWidth = image.Size.Width; From 74e34419d32b88749265fa1a654048a4a319ba69 Mon Sep 17 00:00:00 2001 From: Steve McConnel Date: Tue, 7 Jul 2026 14:05:11 -0600 Subject: [PATCH 2/3] Guarantee unique EPPlus drawing name in SS export (BL-16498) The initial fix renamed a colliding image to `{name}-{rowNum}` but appended the suffix blindly without re-checking, and compared case-insensitively while renaming with the original casing. So the renamed value could still collide (e.g. three images sharing a base name in one row, or two same-row names differing only in case), causing AddPicture to throw the very EPPlus name-collision exception BL-16498 set out to prevent. Loop until the name is truly unique (case-insensitively), matching EPPlus semantics. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BloomExe/Spreadsheet/SpreadsheetIO.cs | 30 +++++++++++++++++------ 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/src/BloomExe/Spreadsheet/SpreadsheetIO.cs b/src/BloomExe/Spreadsheet/SpreadsheetIO.cs index 6a4689fecae5..5affd788ba96 100644 --- a/src/BloomExe/Spreadsheet/SpreadsheetIO.cs +++ b/src/BloomExe/Spreadsheet/SpreadsheetIO.cs @@ -185,15 +185,29 @@ int embedImage(string imageSrcPath, int rowNum, int colNum) using (Image image = Image.FromFile(imageSrcPath)) { string imageName = Path.GetFileNameWithoutExtension(imageSrcPath); - // Image.jpg and image.png are different files, but the same name for storing here. (BL-16498) - // Names are treated as case-insensitive (this is Windows after all!), so we need to allow for that. - // Image reuse might conceivably happen as well, and this allows for that too. - if ( + // Image.jpg and image.png are different files, but reduce to the same name for storing here. (BL-16498) + // EPPlus treats drawing names as case-insensitive (this is Windows after all!) and rejects + // duplicates, so we must ensure the name we use is unique without regard to case. Image reuse + // might conceivably happen as well, and this handles that too. Because more than two images + // can reduce to the same name (even within one row), we keep incrementing the suffix until + // the name really is unique; otherwise AddPicture below would still throw. + bool NameIsTaken(string candidate) => worksheet.Drawings.Any(xx => - xx.Name.ToLowerInvariant() == imageName.ToLowerInvariant() - ) - ) - imageName = $"{imageName}-{rowNum}"; + xx.Name.Equals( + candidate, + StringComparison.InvariantCultureIgnoreCase + ) + ); + if (NameIsTaken(imageName)) + { + string baseName = imageName; + int suffix = rowNum; + do + { + imageName = $"{baseName}-{suffix}"; + suffix++; + } while (NameIsTaken(imageName)); + } var origImageHeight = image.Size.Height; var origImageWidth = image.Size.Width; int finalWidth = defaultImageWidth; From e3c3d371e7dace8322f40b002dbcc589896e13e6 Mon Sep 17 00:00:00 2001 From: Steve McConnel Date: Tue, 7 Jul 2026 14:36:40 -0600 Subject: [PATCH 3/3] Add regression test for SS export image-name conflicts (BL-16498) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exports a small book whose three image files reduce to the same EPPlus drawing name once the extension is dropped — including names that differ only in case (conflict.png / Conflict.jpg / conflict.gif) — and asserts every image embeds with no error message and no warning. Verified the test fails against the pre-fix code (the case-differing image was reported as a "Bad image file" because EPPlus rejected the duplicate drawing name) and passes with the fix. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Spreadsheet/SpreadsheetImagesTests.cs | 162 ++++++++++++++++++ 1 file changed, 162 insertions(+) diff --git a/src/BloomTests/Spreadsheet/SpreadsheetImagesTests.cs b/src/BloomTests/Spreadsheet/SpreadsheetImagesTests.cs index d09233b31fe3..e66c3c8f05b8 100644 --- a/src/BloomTests/Spreadsheet/SpreadsheetImagesTests.cs +++ b/src/BloomTests/Spreadsheet/SpreadsheetImagesTests.cs @@ -490,5 +490,167 @@ public void displayThumbnail_svg_svgErrorMessage(string source) ); Assert.That(svgRow.GetCell(thumbnailColumn).Text, Is.EqualTo("Can't display SVG")); } + + // A minimal book whose three images all reduce to the same EPPlus drawing name once the + // extension is dropped: "conflict.png" and "conflict.gif" match exactly, and "Conflict.jpg" + // matches them case-insensitively. EPPlus treats drawing names as case-insensitive and + // rejects duplicates, so this is the situation that used to throw during export (BL-16498). + private const string conflictingImageNamesBook = + @" + + + + +
+
+
Image
+
+
+
+
+
+ +
+
+
+
+
+
+
Image
+
+
+
+
+
+ +
+
+
+
+
+
+
Image
+
+
+
+
+
+ +
+
+
+
+
+ + +"; + + /// + /// Regression test for BL-16498: exporting a book whose image file names collide once the + /// extension is dropped (including names that differ only in case) must embed every image + /// without error. Before the fix, the second colliding image threw inside EPPlus's + /// AddPicture (which treats drawing names as case-insensitive and rejects duplicates); the + /// exporter caught the throw and reported the image as a "Bad image file". + /// + [Test] + public void Export_ImagesWithConflictingNames_AllEmbeddedWithoutError() + { + using (var bookFolder = new TemporaryFolder("SpreadsheetImageConflict_Book")) + using (var outputFolder = new TemporaryFolder("SpreadsheetImageConflict_Out")) + { + // Copy one known-good image to three names that collide once the extension is dropped. + var sourceImage = Path.Combine( + SIL.IO.FileLocationUtilities.GetDirectoryDistributedWithApplication( + _pathToTestImages + ), + "man.jpg" + ); + var conflictingNames = new[] { "conflict.png", "Conflict.jpg", "conflict.gif" }; + foreach (var name in conflictingNames) + RobustFile.Copy(sourceImage, Path.Combine(bookFolder.FolderPath, name)); + + // Sanity check: the setup really did create three distinct, non-empty image files + // whose names collide case-insensitively once the extension is removed. + Assert.That( + conflictingNames + .Select(n => Path.GetFileNameWithoutExtension(n).ToLowerInvariant()) + .Distinct() + .Count(), + Is.EqualTo(1), + "Setup sanity check: all test image names should reduce to the same case-insensitive base name." + ); + foreach (var name in conflictingNames) + { + var path = Path.Combine(bookFolder.FolderPath, name); + Assert.That( + RobustFile.Exists(path), + $"Setup sanity check: {name} should exist." + ); + Assert.That( + new FileInfo(path).Length, + Is.GreaterThan(0), + $"Setup sanity check: {name} should not be empty." + ); + } + + var mockLangDisplayNameResolver = new Mock(); + mockLangDisplayNameResolver + .Setup(x => x.GetLanguageDisplayName("en")) + .Returns("English"); + var exporter = new SpreadsheetExporter(mockLangDisplayNameResolver.Object); + var progressSpy = new ProgressSpy(); + + var sheet = exporter.ExportToFolder( + new HtmlDom(conflictingImageNamesBook, true), + bookFolder.FolderPath, + outputFolder.FolderPath, + out string outputPath, + progressSpy, + OverwriteOptions.Overwrite + ); + + // Sanity check: all three images made it into the export as image content rows. + var imageSourceColumn = sheet.GetColumnForTag( + InternalSpreadsheet.ImageSourceColumnLabel + ); + var exportedImageRows = sheet + .ContentRows.Where(r => + r.GetCell(imageSourceColumn).Text.ToLowerInvariant().Contains("conflict") + ) + .ToList(); + Assert.That( + exportedImageRows.Count, + Is.EqualTo(3), + "Setup sanity check: expected all three conflicting images to be exported as content rows." + ); + + // The images are actually embedded (and any errors recorded) while the .xlsx file is + // written, so read the file back to check the results. + var sheetFromFile = InternalSpreadsheet.ReadFromFile(outputPath); + var thumbnailColumn = sheetFromFile.GetColumnForTag( + InternalSpreadsheet.ImageThumbnailColumnLabel + ); + foreach ( + var row in sheetFromFile.ContentRows.Where(r => + r.GetCell(imageSourceColumn).Text.ToLowerInvariant().Contains("conflict") + ) + ) + { + Assert.That( + row.GetCell(thumbnailColumn).Text, + Is.EqualTo(""), + $"Image '{row.GetCell(imageSourceColumn).Text}' should have embedded without an error, " + + "but its thumbnail cell holds an error message (the duplicate-name collision was not resolved)." + ); + } + + // And no warning should have been reported about embedding these images. + Assert.That( + progressSpy.Warnings, + Is.Empty, + "Exporting images with conflicting names should not report any warnings." + ); + } + } } }