From 59d42c05747f9351656f8f874a006b24d2286f3c Mon Sep 17 00:00:00 2001 From: Lieven De Foor Date: Fri, 5 Jun 2026 16:27:24 +0200 Subject: [PATCH] Fix KeyNotFoundException/ArgumentException on renaming or removing standard/grouped drawings --- src/EPPlus/Drawing/ExcelDrawing.cs | 54 +++++++++++++++++++++++++ src/EPPlus/Drawing/ExcelGroupShape.cs | 18 ++++++++- src/EPPlusTest/Drawing/DrawingTest.cs | 57 +++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 2 deletions(-) diff --git a/src/EPPlus/Drawing/ExcelDrawing.cs b/src/EPPlus/Drawing/ExcelDrawing.cs index 73a182e69..05485c331 100644 --- a/src/EPPlus/Drawing/ExcelDrawing.cs +++ b/src/EPPlus/Drawing/ExcelDrawing.cs @@ -24,6 +24,7 @@ Date Author Change using OfficeOpenXml.Utils.FileUtils; using OfficeOpenXml.Utils.XML; using System; +using System.Collections.Generic; using System.Globalization; using System.IO; using System.Linq; @@ -336,6 +337,18 @@ public virtual string Name try { if (_nvPrPath == "") throw new NotImplementedException(); + string oldName = GetXmlNodeString(_nvPrPath + "/@name"); + // Only perform dictionary synchronization if the name has actually changed + if (!string.IsNullOrEmpty(oldName) && !oldName.Equals(value, StringComparison.Ordinal)) + { + // Validate name uniqueness in both worksheet and parent-group drawing collections. + // All checks must pass before any dictionaries are updated, preventing partial desynchronization. + ValidateNameUniquenessInCollections(value); + + // All uniqueness checks successfully passed! Safe to update name-lookup dictionaries. + UpdateNameInDictionaries(oldName, value); + } + SetXmlNodeString(_nvPrPath + "/@name", value); if (this is ExcelSlicer ts) { @@ -348,6 +361,10 @@ public virtual string Name pts.SlicerName = value; } } + catch (ArgumentException) + { + throw; + } catch { throw new NotImplementedException(); @@ -2467,6 +2484,43 @@ private XmlNode CopyShape(ExcelWorksheet worksheet, bool isGroupShape = false, X return drawNode; } + private void ValidateNameUniqueness(Dictionary drawingNames, IEnumerable collection, string name) + { + if (drawingNames?.ContainsKey(name) == true) + { + int index = drawingNames[name]; + var drawing = collection?.ElementAtOrDefault(index); + + if (drawing != null && drawing != this) + { + string collectionDescription = collection is ExcelDrawingsGroup ? "the group drawings collection" : "drawings collection"; + throw new ArgumentException($"Name '{name}' already exists in {collectionDescription}."); + } + } + } + + private void ValidateNameUniquenessInCollections(string name) + { + ValidateNameUniqueness(_drawings?._drawingNames, _drawings?._drawingsList, name); + ValidateNameUniqueness(_parent?.Drawings?._drawingNames, _parent?.Drawings, name); + } + + private void UpdateNameInDictionary(Dictionary drawingNames, string oldName, string newName) + { + if (drawingNames?.ContainsKey(oldName) == true) + { + int index = drawingNames[oldName]; + drawingNames.Remove(oldName); + drawingNames[newName] = index; + } + } + + private void UpdateNameInDictionaries(string oldName, string newName) + { + UpdateNameInDictionary(_drawings?._drawingNames, oldName, newName); + UpdateNameInDictionary(_parent?.Drawings?._drawingNames, oldName, newName); + } + internal ExcelAddressBase GetAddress() { GetFromBounds(out int fromRow, out _, out int fromCol, out _); diff --git a/src/EPPlus/Drawing/ExcelGroupShape.cs b/src/EPPlus/Drawing/ExcelGroupShape.cs index 2371c414d..656530bf1 100644 --- a/src/EPPlus/Drawing/ExcelGroupShape.cs +++ b/src/EPPlus/Drawing/ExcelGroupShape.cs @@ -38,7 +38,7 @@ internal ExcelDrawingsGroup(ExcelGroupShape parent, XmlNamespaceManager nsm, Xml _parent = parent; _nsm = nsm; _topNode = topNode; - _drawingNames = new Dictionary(); + _drawingNames = new Dictionary(StringComparer.OrdinalIgnoreCase); _drawingsCollectionType = drawingsCollectionType; AddDrawings(); } @@ -298,7 +298,21 @@ IEnumerator IEnumerable.GetEnumerator() public void Remove(ExcelDrawing drawing) { CheckNotDisposed(); - _groupDrawings.Remove(drawing); + // Find the 0-based list index of the drawing within this group + int index = _groupDrawings.IndexOf(drawing); + if (index < 0) return; + + // Remove the drawing from the list and its entry from the lookup dictionary + _groupDrawings.RemoveAt(index); + _drawingNames.Remove(drawing.Name); + + // Re-index all remaining drawings after the removed index in the group's name-to-index lookup. + // When an element is removed, all subsequent elements shift down, so we update their mapped indices accordingly. + for (int i = index; i < _groupDrawings.Count; i++) + { + _drawingNames[_groupDrawings[i].Name] = i; + } + AdjustXmlAndMoveFromGroup(drawing); var ix = _parent._drawings._drawingsList.IndexOf(_parent); _parent._drawings._drawingsList.Insert(ix, drawing); diff --git a/src/EPPlusTest/Drawing/DrawingTest.cs b/src/EPPlusTest/Drawing/DrawingTest.cs index ffed5c12f..6d955de72 100644 --- a/src/EPPlusTest/Drawing/DrawingTest.cs +++ b/src/EPPlusTest/Drawing/DrawingTest.cs @@ -1218,5 +1218,62 @@ public void AddTextBox() SaveAndCleanup(package); } } + [TestMethod] + public void DrawingNameChangeAndRemovalTest() + { + using (var package = new ExcelPackage()) + { + var ws = package.Workbook.Worksheets.Add("RenameTest"); + + // Add a drawing (shape) + var shape = ws.Drawings.AddShape("InitialName", eShapeStyle.Rect); + Assert.AreEqual("InitialName", shape.Name); + Assert.IsTrue(ws.Drawings._drawingNames.ContainsKey("InitialName")); + + // Rename drawing + shape.Name = "RenamedName"; + Assert.AreEqual("RenamedName", shape.Name); + + // Verify the internal dictionary was updated + Assert.IsFalse(ws.Drawings._drawingNames.ContainsKey("InitialName"), "Old name key should be removed from dictionary"); + Assert.IsTrue(ws.Drawings._drawingNames.ContainsKey("RenamedName"), "New name key should be added to dictionary"); + + // Test removing by shape object reference + ws.Drawings.Remove(shape); + Assert.AreEqual(0, ws.Drawings.Count, "Drawing should be removed successfully"); + Assert.IsFalse(ws.Drawings._drawingNames.ContainsKey("RenamedName"), "Dictionary should no longer contain renamed name"); + } + } + + [TestMethod] + public void GroupedDrawingNameChangeAndRemovalTest() + { + using (var package = new ExcelPackage()) + { + var ws = package.Workbook.Worksheets.Add("GroupRenameTest"); + + var shape1 = ws.Drawings.AddShape("Shape1", eShapeStyle.Rect); + var shape2 = ws.Drawings.AddShape("Shape2", eShapeStyle.Rect); + + // Group shapes using public EPPlus API + var group = shape1.Group(shape2); + + Assert.AreEqual("Shape1", shape1.Name); + Assert.IsTrue(group.Drawings._drawingNames.ContainsKey("Shape1")); + + // Rename grouped shape + shape1.Name = "GroupedShape1Renamed"; + Assert.AreEqual("GroupedShape1Renamed", shape1.Name); + + // Verify the internal group dictionary was updated + Assert.IsFalse(group.Drawings._drawingNames.ContainsKey("Shape1"), "Old name key should be removed from group dictionary"); + Assert.IsTrue(group.Drawings._drawingNames.ContainsKey("GroupedShape1Renamed"), "New name key should be added to group dictionary"); + + // Test removing grouped shape + group.Drawings.Remove(shape1); + Assert.AreEqual(1, group.Drawings.Count, "One shape should be left in group"); + Assert.IsFalse(group.Drawings._drawingNames.ContainsKey("GroupedShape1Renamed"), "Group dictionary should no longer contain removed name"); + } + } } }