diff --git a/CHANGELOG.md b/CHANGELOG.md index c6aafa0a..474d1061 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/SIL.LCModel.FixData/SequenceFixer.cs b/src/SIL.LCModel.FixData/SequenceFixer.cs index b3a3651b..af745231 100644 --- a/src/SIL.LCModel.FixData/SequenceFixer.cs +++ b/src/SIL.LCModel.FixData/SequenceFixer.cs @@ -45,6 +45,7 @@ internal class SequenceFixer : RtFixer Dictionary m_candidateForRefAdjustment = new Dictionary(); Dictionary m_lexRefTypes = new Dictionary(); List m_lexReferencesToDelete = new List(); + List m_lexReferencesWithDuplicateTargets = new List(); List m_objsToDelete = new List(); List m_objsToAdjust = new List(); Dictionary> m_ownerThatWillLoseOwnee = new Dictionary>(); @@ -97,10 +98,16 @@ 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; @@ -108,17 +115,15 @@ internal override void InspectElement(XElement rt) } /// - /// 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. /// - /// - /// - /// - private static bool HoldsLessThanTwoInSequence(string propertyName, XElement xeObject) + private static List GetSequenceObjsurGuids(string propertyName, XElement xeObject) { var xeProperty = xeObject.Element(propertyName); - return xeProperty == null || xeProperty.Descendants("objsur").Count() < 2; + if (xeProperty == null) + return new List(); + return xeProperty.Descendants("objsur").Select(objsur => objsur.Attribute("guid").Value).ToList(); } /// @@ -378,11 +383,16 @@ 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; @@ -390,6 +400,26 @@ internal override bool FixElement(XElement rt, FwDataFixer.ErrorLogger errorLogg return true; } + /// + /// 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). + /// + 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(); + 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(); diff --git a/src/SIL.LCModel.FixData/Strings.Designer.cs b/src/SIL.LCModel.FixData/Strings.Designer.cs index 972d1d39..e40509db 100644 --- a/src/SIL.LCModel.FixData/Strings.Designer.cs +++ b/src/SIL.LCModel.FixData/Strings.Designer.cs @@ -204,6 +204,15 @@ public static string ksRemovingDuplicateAlternative { } } + /// + /// Looks up a localized string similar to Removing duplicate Target (guid='{0}') from LexReference (guid='{1}') owned by (guid='{2}').. + /// + public static string ksRemovingDuplicateLexReferenceTarget { + get { + return ResourceManager.GetString("ksRemovingDuplicateLexReferenceTarget", resourceCulture); + } + } + /// /// Looks up a localized string similar to Removing duplicate style {0}.. /// diff --git a/src/SIL.LCModel.FixData/Strings.resx b/src/SIL.LCModel.FixData/Strings.resx index 632d5765..13dfd122 100644 --- a/src/SIL.LCModel.FixData/Strings.resx +++ b/src/SIL.LCModel.FixData/Strings.resx @@ -215,6 +215,9 @@ Removing LexReference with too few references (Targets) (guid='{0}') from its owner (guid='{1}'). + + Removing duplicate Target (guid='{0}') from LexReference (guid='{1}') owned by (guid='{2}'). + Removing duplicate style {0}. diff --git a/tests/SIL.LCModel.FixData.Tests/FwDataFixerTests.cs b/tests/SIL.LCModel.FixData.Tests/FwDataFixerTests.cs index f2ab3413..f9c96fc3 100644 --- a/tests/SIL.LCModel.FixData.Tests/FwDataFixerTests.cs +++ b/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); + } + /// /// 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. diff --git a/tests/SIL.LCModel.FixData.Tests/TestData/DuplicateLexRefTargets/Test.fwdata b/tests/SIL.LCModel.FixData.Tests/TestData/DuplicateLexRefTargets/Test.fwdata new file mode 100644 index 00000000..41bd563c --- /dev/null +++ b/tests/SIL.LCModel.FixData.Tests/TestData/DuplicateLexRefTargets/Test.fwdata @@ -0,0 +1,66 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +