Fix LT-21598: Find and Fix repairs LexReferences with duplicate Targets

CHANGELOG.md

@@ -28,6 +28,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 
+- [SIL.LCModel.FixData] Find and Fix removes duplicate Targets from a LexReference (and deletes it if fewer than two distinct Targets remain) (LT-21598)
 - [SIL.LCModel] Data migration now serializes dates using the culture-neutral `ToLCMTimeFormatWithMillisString` (LT-20698)
 - [SIL.LCModel] `ReadWriteServices.LoadDateTime` now parses milliseconds correctly (LT-18205)
 - [SIL.LCModel.Core] Copy `SIL.LCModel.Core.dll.config` to output directory

src/SIL.LCModel.FixData/SequenceFixer.cs

@@ -45,6 +45,7 @@ internal class SequenceFixer : RtFixer
 		Dictionary<Guid, XElement> m_candidateForRefAdjustment = new Dictionary<Guid, XElement>();
 		Dictionary<Guid, XElement> m_lexRefTypes = new Dictionary<Guid, XElement>();
 		List<Guid> m_lexReferencesToDelete = new List<Guid>();
+		List<Guid> m_lexReferencesWithDuplicateTargets = new List<Guid>();
 		List<Guid> m_objsToDelete = new List<Guid>();
 		List<Guid> m_objsToAdjust = new List<Guid>();
 		Dictionary<Guid, List<Guid>> m_ownerThatWillLoseOwnee = new Dictionary<Guid, List<Guid>>();
@@ -97,28 +98,32 @@ internal override void InspectElement(XElement rt)
 					m_lexRefTypes.Add(guid, rt);
 					break;
 				case "LexReference":
-					// Check for less than two Targets
-					if (!HoldsLessThanTwoInSequence("Targets", rt))
-						return;
-					m_lexReferencesToDelete.Add(guid);
+					// A LexReference needs at least two distinct Targets. Count distinct targets, since
+					// duplicate Targets (the same object listed more than once) also make it invalid and can
+					// lead to a crash (LT-21598). Delete it if fewer than two distinct Targets remain;
+					// otherwise just remove the duplicates.
+					var targetGuids = GetSequenceObjsurGuids("Targets", rt);
+					var distinctTargetCount = targetGuids.Distinct().Count();
+					if (distinctTargetCount < 2)
+						m_lexReferencesToDelete.Add(guid);
+					else if (distinctTargetCount < targetGuids.Count)
+						m_lexReferencesWithDuplicateTargets.Add(guid);
 					break;
 				default:
 					break;
 			}
 		}
 
 		/// <summary>
-		/// Determines whether the sequence held by this XElement object is empty or has less
-		/// than two descendants or not.
-		/// N.B. At this point, there must only be one sequence held by this object.
+		/// Gets the guids (in order, including any duplicates) of the objsur elements in the named
+		/// sequence/collection property, or an empty list if the property is absent.
 		/// </summary>
-		/// <param name="propertyName"> </param>
-		/// <param name="xeObject"></param>
-		/// <returns></returns>
-		private static bool HoldsLessThanTwoInSequence(string propertyName, XElement xeObject)
+		private static List<string> GetSequenceObjsurGuids(string propertyName, XElement xeObject)
 		{
 			var xeProperty = xeObject.Element(propertyName);
-			return xeProperty == null || xeProperty.Descendants("objsur").Count() < 2;
+			if (xeProperty == null)
+				return new List<string>();
+			return xeProperty.Descendants("objsur").Select(objsur => objsur.Attribute("guid").Value).ToList();
 		}
 
 		/// <summary>
@@ -378,18 +383,43 @@ internal override bool FixElement(XElement rt, FwDataFixer.ErrorLogger errorLogg
 						objsur => danglingRefList.Contains(GetObjsurGuid(objsur))).Remove();
 					break;
 				case "LexReference":
-					// Remove a LexReference that only has one (or fewer) Targets.
-					if (!m_lexReferencesToDelete.Contains(guid))
-						return true;
-					ReportBadLexReference(guid, guidOwner, errorLogger);
-					return false; // delete this rt element
+					// Remove a LexReference that has fewer than two distinct Targets.
+					if (m_lexReferencesToDelete.Contains(guid))
+					{
+						ReportBadLexReference(guid, guidOwner, errorLogger);
+						return false; // delete this rt element
+					}
+					// Otherwise remove any duplicate Targets, keeping the first occurrence of each.
+					if (m_lexReferencesWithDuplicateTargets.Contains(guid))
+						RemoveDuplicateTargets(rt, guid, guidOwner, errorLogger);
+					break;
 
 				default:
 					break;
 			}
 			return true;
 		}
 
+		/// <summary>
+		/// Remove duplicate Targets from a LexReference, keeping the first occurrence of each. Duplicate
+		/// Targets (the same object listed more than once) can lead to a crash (LT-21598).
+		/// </summary>
+		private void RemoveDuplicateTargets(XElement rt, Guid guid, Guid guidOwner, FwDataFixer.ErrorLogger errorLogger)
+		{
+			var targets = rt.Element("Targets");
+			if (targets == null)
+				return;
+			var seen = new HashSet<string>();
+			foreach (var objsur in targets.Descendants("objsur").ToList())
+			{
+				var targetGuid = objsur.Attribute("guid").Value;
+				if (seen.Add(targetGuid))
+					continue;
+				objsur.Remove();
+				errorLogger(String.Format(Strings.ksRemovingDuplicateLexReferenceTarget, targetGuid, guid, guidOwner), true);
+			}
+		}
+
 		private void ReportBadLexReference(Guid guid, Guid guidOwner, FwDataFixer.ErrorLogger errorLogger)
 		{
 			// Example: if guid and rt belong to a "LexReference" that has fewer than two Targets,
@@ -477,6 +507,7 @@ internal override void Reset()
 			m_candidateForRefAdjustment.Clear();
 			m_lexRefTypes.Clear();
 			m_lexReferencesToDelete.Clear();
+			m_lexReferencesWithDuplicateTargets.Clear();
 			m_objsToDelete.Clear();
 			m_objsToAdjust.Clear();
 			m_ownerThatWillLoseOwnee.Clear();

src/SIL.LCModel.FixData/Strings.resx

@@ -215,6 +215,9 @@
   <data name="ksRemovingBadLexReference" xml:space="preserve">
     <value>Removing LexReference with too few references (Targets) (guid='{0}') from its owner (guid='{1}').</value>
   </data>
+  <data name="ksRemovingDuplicateLexReferenceTarget" xml:space="preserve">
+    <value>Removing duplicate Target (guid='{0}') from LexReference (guid='{1}') owned by (guid='{2}').</value>
+  </data>
   <data name="ksRemovingDuplicateStyle" xml:space="preserve">
     <value>Removing duplicate style {0}.</value>
   </data>

tests/SIL.LCModel.FixData.Tests/FwDataFixerTests.cs

@@ -53,7 +53,7 @@ internal XmlNodeList VerifyEntryExists(XmlDocument xmlDoc, string xPath)
 				"DuplicateGuid", "DanglingCustomListReference", "DanglingCustomProperty", "DanglingReference",
 				"DuplicateWs", "SequenceFixer", "EntryWithExtraMSA", "EntryWithMsaAndNoSenses", "EntryExtraMsaAndBustedSenseRef", "TagAndCellRefs", "GenericDates",
 				"HomographFixer", WordformswithsameformTestDir, "MorphBundleProblems", "MissingBasicCustomField", "DeletedMsaRefBySenseAndBundle",
-				"DuplicateNameCustomList", "SingleTargetLexRefs", "DuplicateStyles"
+				"DuplicateNameCustomList", "SingleTargetLexRefs", "DuplicateLexRefTargets", "DuplicateStyles"
 			};
 
 		private string TestDataDirectory => Path.Combine(TestDirectoryFinder.RootDirectory, "tests", "SIL.LCModel.FixData.Tests", "TestData");
@@ -415,6 +415,56 @@ public void SingleTargetLexReferences()
 				"//rt[@class=\"LexReference\"]", 1);
 		}
 
+		[Test]
+		public void DuplicateLexReferenceTargets()
+		{
+			var testPath = Path.Combine(_basePath, "DuplicateLexRefTargets");
+			// This test checks that LexReferences listing the same Target more than once (LT-21598) are repaired:
+			// duplicates are removed, and a LexReference left with fewer than two distinct Targets is deleted.
+			const string lexRefDedupeGuid = "f1f1f1f1-0000-0000-0000-000000000001";   // senseA, senseB, senseA -> senseA, senseB
+			const string lexRefDeleteGuid = "f1f1f1f1-0000-0000-0000-000000000002";   // senseC, senseC -> deleted
+			const string lexRefValidGuid = "f1f1f1f1-0000-0000-0000-000000000003";    // senseA, senseB -> untouched
+			const string senseAGuid = "aaaaaaaa-0000-0000-0000-00000000000a";
+			const string senseCGuid = "cccccccc-0000-0000-0000-00000000000c";
+			const string lexRefTypeGuid = "dddddddd-0000-0000-0000-00000000000d";
+
+			var data = new FwDataFixer(Path.Combine(testPath, "BasicFixup.fwdata"), new DummyProgressDlg(), LogErrors, ErrorCount);
+			data.FixErrorsAndSave();
+
+			// Two fixes: one duplicate Target removed, and one too-few-Targets LexReference removed (the
+			// owning LexRefType's reference to the deleted LexReference is removed too, but isn't logged separately).
+			Assert.AreEqual(2, _errorsFixed, "Unexpected number of fixes.");
+			Assert.That(_errors, Has.Some.StartsWith("Removing duplicate Target (guid='" + senseAGuid +
+				"') from LexReference (guid='" + lexRefDedupeGuid + "')"), "Duplicate-target error message missing."); // SequenceFixer--ksRemovingDuplicateLexReferenceTarget
+			Assert.That(_errors, Has.Some.StartsWith("Removing LexReference with too few references (Targets) (guid='" + lexRefDeleteGuid +
+				"')"), "Too-few-Targets error message missing."); // SequenceFixer--ksRemovingBadLexReference
+
+			// Original (bad) data: the dedupe LexReference had senseA twice (3 Targets total).
+			AssertThatXmlIn.File(Path.Combine(testPath, "BasicFixup.bak")).HasSpecifiedNumberOfMatchesForXpath(
+				"//rt[@guid=\"" + lexRefDedupeGuid + "\"]/Targets/objsur[@guid=\"" + senseAGuid + "\"]", 2);
+
+			var fixedFile = Path.Combine(testPath, "BasicFixup.fwdata");
+			// Dedupe LexReference: kept, but senseA now appears only once (two distinct Targets).
+			AssertThatXmlIn.File(fixedFile).HasSpecifiedNumberOfMatchesForXpath("//rt[@guid=\"" + lexRefDedupeGuid + "\"]", 1);
+			AssertThatXmlIn.File(fixedFile).HasSpecifiedNumberOfMatchesForXpath(
+				"//rt[@guid=\"" + lexRefDedupeGuid + "\"]/Targets/objsur[@guid=\"" + senseAGuid + "\"]", 1);
+			AssertThatXmlIn.File(fixedFile).HasSpecifiedNumberOfMatchesForXpath(
+				"//rt[@guid=\"" + lexRefDedupeGuid + "\"]/Targets/objsur", 2);
+
+			// All-duplicate LexReference: deleted, and the LexRefType no longer references it.
+			AssertThatXmlIn.File(fixedFile).HasSpecifiedNumberOfMatchesForXpath("//rt[@guid=\"" + lexRefDeleteGuid + "\"]", 0);
+			AssertThatXmlIn.File(fixedFile).HasSpecifiedNumberOfMatchesForXpath(
+				"//rt[@guid=\"" + lexRefTypeGuid + "\"]/Members/objsur[@guid=\"" + lexRefDeleteGuid + "\"]", 0);
+
+			// The targeted senses themselves are not deleted.
+			AssertThatXmlIn.File(fixedFile).HasSpecifiedNumberOfMatchesForXpath("//rt[@guid=\"" + senseAGuid + "\"]", 1);
+			AssertThatXmlIn.File(fixedFile).HasSpecifiedNumberOfMatchesForXpath("//rt[@guid=\"" + senseCGuid + "\"]", 1);
+
+			// The valid LexReference is untouched (two distinct Targets).
+			AssertThatXmlIn.File(fixedFile).HasSpecifiedNumberOfMatchesForXpath(
+				"//rt[@guid=\"" + lexRefValidGuid + "\"]/Targets/objsur", 2);
+		}
+
 		/// <summary>
 		/// This test checks that when a WfiMorphBundle has a dangling MSA pointer
 		///		- if it has a sense that has an MSA, the MSA is fixed to point to it.

tests/SIL.LCModel.FixData.Tests/TestData/DuplicateLexRefTargets/Test.fwdata

@@ -0,0 +1,66 @@
+<?xml version="1.0" encoding="utf-8"?>
+<languageproject version="7000061">
+
+<rt class="LexEntry" guid="11111111-0000-0000-0000-000000000001">
+  <Senses>
+	<objsur guid="aaaaaaaa-0000-0000-0000-00000000000a" t="o" />
+  </Senses>
+</rt>
+<rt class="LexSense" guid="aaaaaaaa-0000-0000-0000-00000000000a" ownerguid="11111111-0000-0000-0000-000000000001" />
+
+<rt class="LexEntry" guid="11111111-0000-0000-0000-000000000002">
+  <Senses>
+	<objsur guid="bbbbbbbb-0000-0000-0000-00000000000b" t="o" />
+  </Senses>
+</rt>
+<rt class="LexSense" guid="bbbbbbbb-0000-0000-0000-00000000000b" ownerguid="11111111-0000-0000-0000-000000000002" />
+
+<rt class="LexEntry" guid="11111111-0000-0000-0000-000000000003">
+  <Senses>
+	<objsur guid="cccccccc-0000-0000-0000-00000000000c" t="o" />
+  </Senses>
+</rt>
+<rt class="LexSense" guid="cccccccc-0000-0000-0000-00000000000c" ownerguid="11111111-0000-0000-0000-000000000003" />
+
+<rt class="CmPossibilityList" guid="eeeeeeee-0000-0000-0000-00000000000e">
+	<Possibilities>
+		<objsur guid="dddddddd-0000-0000-0000-00000000000d" t="o" />
+	</Possibilities>
+</rt>
+
+<rt class="LexRefType" guid="dddddddd-0000-0000-0000-00000000000d" ownerguid="eeeeeeee-0000-0000-0000-00000000000e">
+	<Members>
+		<objsur guid="f1f1f1f1-0000-0000-0000-000000000001" t="o" />
+		<objsur guid="f1f1f1f1-0000-0000-0000-000000000002" t="o" />
+		<objsur guid="f1f1f1f1-0000-0000-0000-000000000003" t="o" />
+	</Members>
+</rt>
+
+<!-- Duplicate target (senseA listed twice) but two distinct targets remain.
+		The duplicate should be removed and the LexReference kept. -->
+<rt class="LexReference" guid="f1f1f1f1-0000-0000-0000-000000000001" ownerguid="dddddddd-0000-0000-0000-00000000000d">
+  <Targets>
+	<objsur guid="aaaaaaaa-0000-0000-0000-00000000000a" t="r" />
+	<objsur guid="bbbbbbbb-0000-0000-0000-00000000000b" t="r" />
+	<objsur guid="aaaaaaaa-0000-0000-0000-00000000000a" t="r" />
+  </Targets>
+</rt>
+
+<!-- All targets are the same object (senseC twice): fewer than two distinct
+		Targets, so the whole LexReference should be deleted. -->
+<rt class="LexReference" guid="f1f1f1f1-0000-0000-0000-000000000002" ownerguid="dddddddd-0000-0000-0000-00000000000d">
+  <Targets>
+	<objsur guid="cccccccc-0000-0000-0000-00000000000c" t="r" />
+	<objsur guid="cccccccc-0000-0000-0000-00000000000c" t="r" />
+  </Targets>
+</rt>
+
+<!-- A valid LexReference with two distinct Targets: should be left untouched. -->
+<rt class="LexReference" guid="f1f1f1f1-0000-0000-0000-000000000003" ownerguid="dddddddd-0000-0000-0000-00000000000d">
+  <Targets>
+	<objsur guid="aaaaaaaa-0000-0000-0000-00000000000a" t="r" />
+	<objsur guid="bbbbbbbb-0000-0000-0000-00000000000b" t="r" />
+  </Targets>
+</rt>
+
+</languageproject>