Give ported objects an HVO from the target cache, so a copied cache never shares an HVO between two objects

CHANGELOG.md

@@ -28,6 +28,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 
+- [SIL.LCModel] A cache copied with `InitializeFromSource`/`CreateCacheCopy` gives every ported object an HVO from the target cache, so objects activated or created in the copy no longer share an HVO with a dormant ported object and `GetObject(hvo)` no longer returns the wrong object
 - [SIL.LCModel] BaseStyleInfo now loads default font features (ktptFontVariations) from style rules, fixing font features set as style defaults being dropped on reload (LT-22351)
 - [SIL.LCModel] BaseStyleInfo no longer clobbers ws-specific font overrides inherited from a based-on style with the default font info's merely-inherited values; only explicitly-set default properties are propagated to ws overrides (LT-22351)
 - [SIL.LCModel] BulletInfo now decodes all properties of an encoded bullet font info instead of stopping at the first string property, fixing bullet font features (ktptFontVariations) being lost when a bullet font name was also set (LT-22351)

src/SIL.LCModel/Infrastructure/Impl/CmObjectSurrogate.cs

@@ -185,9 +185,8 @@ internal CmObjectSurrogate(LcmCache cache, ICmObjectSurrogate sourceSurrogate)
 				RawXmlBytes = surr.RawXmlBytes;
 			else
 				Xml = sourceSurrogate.XML;
-			ICmObjectId objId = surr.Id;
-			m_guid = objId is CmObjectIdWithHvo ? objId
-				: ((IServiceLocatorInternal)cache.ServiceLocator).IdentityMap.CreateObjectIdWithHvo(objId.Guid);
+			// An HVO from the source cache means nothing in this one, so always take one from this cache.
+			m_guid = ((IServiceLocatorInternal)cache.ServiceLocator).IdentityMap.CreateObjectIdWithHvo(surr.Id.Guid);
 			SetClassName(surr.m_classname);
 		}
 

tests/SIL.LCModel.Tests/Infrastructure/Impl/PersistingLayerTests.BEPPortTests.cs

@@ -250,6 +250,77 @@ public void PortAllBEPsTestsUsingAnUnopenedSource(
 			}
 			sourceGuids.Clear();
 		}
+
+		/// <summary>
+		/// A copy taken from a source whose objects are only partly activated must still give every
+		/// object in the target an HVO of its own, including objects created in the target afterwards.
+		/// </summary>
+		[Test]
+		public void PortFromPartlyActivatedSourceGivesMemoryOnlyTargetUniqueHvos()
+		{
+			var sourceProjectId = new TestProjectId(BackendProviderType.kXMLWithMemoryOnlyWsMgr,
+				GenerateBackendStartupParameters(false, BackendProviderType.kXMLWithMemoryOnlyWsMgr).ProjectId.Path);
+			using (var newCache = LcmCache.CreateCacheWithNewBlankLangProj(sourceProjectId, "en", "fr", "en", new DummyLcmUI(),
+				m_lcmDirectories, new LcmSettings()))
+			{
+				newCache.ServiceLocator.GetInstance<IUndoStackManager>().Save();
+			}
+
+			using (var sourceCache = LcmCache.CreateCacheFromExistingData(sourceProjectId, "en", new DummyLcmUI(),
+				m_lcmDirectories, new LcmSettings(), new DummyProgressDlg()))
+			{
+				var dormant = IdentityMapOf(sourceCache).AllObjectsOrSurrogates().Where(item => !item.HasObject).ToList();
+				Assert.That(dormant.Count, Is.GreaterThan(10), "The source should start mostly dormant.");
+				// Activate every other dormant object so the copy sees both activated and dormant sources.
+				for (var i = 0; i < dormant.Count; i += 2)
+					Assert.That(dormant[i].Object, Is.Not.Null);
+
+				var targetProjectId = new TestProjectId(BackendProviderType.kMemoryOnly, null);
+				using (var targetCache = LcmCache.CreateCacheCopy(targetProjectId, "en", new DummyLcmUI(),
+					m_lcmDirectories, new LcmSettings(), sourceCache))
+				{
+					AssertHvosAreUnique(targetCache, "after the copy");
+
+					var entryFactory = targetCache.ServiceLocator.GetInstance<ILexEntryFactory>();
+					NonUndoableUnitOfWorkHelper.Do(targetCache.ActionHandlerAccessor, () =>
+					{
+						for (var i = 0; i < dormant.Count; i++)
+							entryFactory.Create();
+					});
+					AssertHvosAreUnique(targetCache, "after creating objects in the target");
+
+					var repository = targetCache.ServiceLocator.GetInstance<ICmObjectRepository>();
+					foreach (var pair in HvosOf(targetCache).ToList())
+						Assert.That(repository.GetObject(pair.Value).Guid, Is.EqualTo(pair.Key), "Wrong object for HVO " + pair.Value);
+				}
+			}
+		}
+
+		private static IdentityMap IdentityMapOf(LcmCache cache)
+		{
+			return ((IServiceLocatorInternal)cache.ServiceLocator).IdentityMap;
+		}
+
+		/// <summary>
+		/// The HVO of every object in the cache that has one, read without activating any dormant object.
+		/// </summary>
+		private static IEnumerable<KeyValuePair<Guid, int>> HvosOf(LcmCache cache)
+		{
+			foreach (var item in IdentityMapOf(cache).AllObjectsOrSurrogates())
+			{
+				if (item.HasObject)
+					yield return new KeyValuePair<Guid, int>(item.Id.Guid, item.Object.Hvo);
+				else if (item.Id is CmObjectIdWithHvo idWithHvo)
+					yield return new KeyValuePair<Guid, int>(item.Id.Guid, idWithHvo.Hvo);
+			}
+		}
+
+		private static void AssertHvosAreUnique(LcmCache cache, string when)
+		{
+			var shared = HvosOf(cache).GroupBy(pair => pair.Value).Where(group => group.Count() > 1).ToList();
+			Assert.That(shared.Count, Is.Zero, "HVOs shared by more than one object " + when + ": "
+				+ string.Join(", ", shared.Take(5).Select(group => group.Key + " (" + group.Count() + " objects)")));
+		}
 		#endregion
 	}
 }