diff --git a/src/BloomExe/Spreadsheet/SpreadsheetIO.cs b/src/BloomExe/Spreadsheet/SpreadsheetIO.cs index b9ef488fc178..5affd788ba96 100644 --- a/src/BloomExe/Spreadsheet/SpreadsheetIO.cs +++ b/src/BloomExe/Spreadsheet/SpreadsheetIO.cs @@ -185,9 +185,29 @@ 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)) - imageName = $"{imageName}-{rowNum}"; + // 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.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; 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." + ); + } + } } }