[mapguide-commits] r10224 - in branches/4.0/MgDev: Common/Foundation/Data Server/src/UnitTesting

[svn_mapguide at osgeo.org]

Author: jng
Date: 2026-10-21 15:45:33 -0700 (Wed, 21 Oct 2026)
New Revision: 10224

Modified:
   branches/4.0/MgDev/Common/Foundation/Data/NamedCollection.cpp
   branches/4.0/MgDev/Server/src/UnitTesting/TestMisc.cpp
Log:
Guard against stale refs (by name) in MgNamedCollection

Fixes #1268

Modified: branches/4.0/MgDev/Common/Foundation/Data/NamedCollection.cpp
===================================================================
--- branches/4.0/MgDev/Common/Foundation/Data/NamedCollection.cpp	2026-10-21 22:40:44 UTC (rev 10223)
+++ branches/4.0/MgDev/Common/Foundation/Data/NamedCollection.cpp	2026-10-21 22:45:33 UTC (rev 10224)
@@ -159,8 +159,12 @@
                 // the requested object.
                 if (Compare(obj->GetName(), name) != 0)
                 {
-                    // The names do not match so we need to do a linear search
-                    // to find it because the collection and the map don't match.
+                    // The names do not match, so the object was renamed after it
+                    // was added to the map and the entry is keyed by a name it no
+                    // longer has. Drop it, and do a linear search to find the
+                    // requested object because the collection and the map don't
+                    // match.
+                    RemoveMap(obj);
                     obj = NULL;
                 }
             }
@@ -184,6 +188,13 @@
                 }
             }
         }
+
+        // The object was not indexed under its current name, so index it now.
+        if (m_pNameMap && obj != NULL)
+        {
+            RemoveMap(obj);
+            InsertMap(obj);
+        }
     }
 
     return obj.Detach();
@@ -332,9 +343,12 @@
 
     if (m_pNameMap)
     {
-        // If map is built, use it since it is faster.
+        // If map is built, use it since it is faster. Its entry for a name can
+        // be stale though, because an element's name can be changed after it
+        // was added, so only a hit for the element itself is conclusive.
         Ptr<MgNamedSerializable> item = GetMap( ((MgNamedSerializable*)value)->GetName() );
-        return(item  != NULL );
+        if (item.p == value)
+            return true;
     }
 
     // otherwise, linear search
@@ -551,11 +565,16 @@
 // Remove the given element from the map.
 void MgNamedCollection::RemoveMap(const MgNamedSerializable* value)
 {
-    // handle the current case sensitivity of the element name.
-    if (m_bCaseSensitive)
-        m_pNameMap->erase( ((MgNamedSerializable*)value)->GetName() );
-    else
-        m_pNameMap->erase( Lower(((MgNamedSerializable*)value)->GetName()) );
+    // The element's name may have changed since it was added to the map, so its
+    // entries have to be located by element rather than by name.
+    std::map<STRING, MgNamedSerializable* >::iterator iter = m_pNameMap->begin();
+    while (iter != m_pNameMap->end())
+    {
+        if (iter->second == value)
+            m_pNameMap->erase(iter++);
+        else
+            ++iter;
+    }
 }
 
 

Modified: branches/4.0/MgDev/Server/src/UnitTesting/TestMisc.cpp
===================================================================
--- branches/4.0/MgDev/Server/src/UnitTesting/TestMisc.cpp	2026-10-21 22:40:44 UTC (rev 10223)
+++ branches/4.0/MgDev/Server/src/UnitTesting/TestMisc.cpp	2026-10-21 22:45:33 UTC (rev 10224)
@@ -1017,6 +1017,76 @@
     }
 }
 
+TEST_CASE("TracTicket1268", "[Misc]")
+{
+    try
+    {
+        // MgNamedCollection indexes its elements by name once the collection
+        // outgrows COLL_MAP_THRESHOLD. An element can be renamed after that,
+        // and removal used to erase the map entry by the element's current
+        // name, so the entry keyed by the old name survived and kept pointing
+        // at the removed element.
+        Ptr<MgNamedCollection> coll = new MgNamedCollection(false);
+
+        Ptr<MgFeatureSchema> renamed;
+        for (INT32 i = 0; i < COLL_MAP_THRESHOLD + 2; i++)
+        {
+            STRING suffix;
+            MgUtil::Int32ToString(i, suffix);
+
+            STRING name = L"Schema" + suffix;
+            Ptr<MgFeatureSchema> schema = new MgFeatureSchema(name, L"");
+            coll->Add(schema);
+
+            if (i == 0)
+                renamed = schema;
+        }
+
+        // ========================== Begin Test ======================== //
+
+        // Renaming an element does not take it out of the collection.
+        renamed->SetName(L"RenamedSchema");
+
+        CHECK(coll->Contains(renamed));
+
+        Ptr<MgNamedSerializable> found = coll->FindItem(L"RenamedSchema");
+        CHECK(found.p == renamed.p);
+
+        // Removing the element must remove it completely: neither of the names
+        // it has had may resolve to it afterwards.
+        REQUIRE(coll->Remove(renamed));
+        CHECK(coll->FindItem(L"RenamedSchema") == nullptr);
+        CHECK(coll->FindItem(L"Schema0") == nullptr);
+
+        // An element that merely carries the vacated name is not in the
+        // collection until it is added.
+        Ptr<MgFeatureSchema> replacement = new MgFeatureSchema(L"Schema0", L"");
+        CHECK(!coll->Contains(replacement));
+
+        // Drop the last reference to the removed element. A lookup by the name
+        // it used to have must not reach the freed object.
+        found = NULL;
+        renamed = NULL;
+        CHECK(coll->FindItem(L"Schema0") == nullptr);
+
+        // The vacated name is free to be used again.
+        coll->Add(replacement);
+        found = coll->FindItem(L"Schema0");
+        REQUIRE(found.p == replacement.p);
+        CHECK(coll->Contains(replacement));
+    }
+    catch (MgException* e)
+    {
+        STRING message = e->GetDetails(TestServiceFactory::TEST_LOCALE);
+        SAFE_RELEASE(e);
+        FAIL(MG_WCHAR_TO_CHAR(message.c_str()));
+    }
+    catch (...)
+    {
+        throw;
+    }
+}
+
 TEST_CASE("TracTicket1304", "[Misc]")
 {
     try