From c7628470c863d06fe65c29ff1a5c60ae1eb175e9 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Thu, 13 Aug 2026 16:35:14 -0400 Subject: [PATCH 1/3] Escape custom list range ids at the point they are written A custom possibility list's name reaches two attributes: the range id in the .lift header and the range id in the .lift-ranges file. It was escaped once as element content when the exporter collected it, which is the wrong rule for an attribute and the wrong place to apply it. The header escaped it a second time, so a list named "Birds & Beasts" arrived as "Birds & Beasts" and disagreed with its own ranges file. The ranges file did not escape it at all, so a name holding a quotation mark closed the id attribute early and left the document unparseable. Hold the collected names unescaped and escape at each write site, as the rest of the exporter does. Co-Authored-By: Claude Opus 5 (1M context) --- .../LexTextControlsTests/LiftExportTests.cs | 78 +++++++++++++++++++ Src/LexText/LexTextControls/LiftExporter.cs | 19 +++-- 2 files changed, 91 insertions(+), 6 deletions(-) diff --git a/Src/LexText/LexTextControls/LexTextControlsTests/LiftExportTests.cs b/Src/LexText/LexTextControls/LexTextControlsTests/LiftExportTests.cs index abe90cc5a7..c530c04db9 100644 --- a/Src/LexText/LexTextControls/LexTextControlsTests/LiftExportTests.cs +++ b/Src/LexText/LexTextControls/LexTextControlsTests/LiftExportTests.cs @@ -1135,6 +1135,84 @@ public void LiftExport() VerifyExportRanges(xdocRangeFile); } + ///-------------------------------------------------------------------------------------- + /// + /// A custom list name reaches the ranges file as a range id, where it needs the escaping + /// rules for an attribute. It used to be escaped once as element content on the way into + /// the exporter's map and never again, so a name holding a quotation mark left the ranges + /// document unparseable. + /// + ///-------------------------------------------------------------------------------------- + [Test] + public void LiftExportRanges_CustomListRangeIdWithQuoteIsEscapedForAnAttribute() + { + const string ksListName = "So-called \"words\""; + XmlDocument xdocLift; + XmlDocument xdocRangeFile; + var customList = ExportWithCustomList(ksListName, out xdocLift, out xdocRangeFile); + + var range = xdocRangeFile.SelectSingleNode(string.Format("//range[@guid='{0}']", customList.Guid)); + Assert.That(range, Is.Not.Null, "the custom list must be written as a range"); + Assert.That(range.Attributes["id"].Value, Is.EqualTo(ksListName), + "the range id must read back as the name stored"); + } + + ///-------------------------------------------------------------------------------------- + /// + /// The same name is written into the .lift header and into the ranges file, so the two must + /// agree. Escaping once on the way in and again at the header write site turned an ampersand + /// into & there while the ranges file kept a single escape. + /// + ///-------------------------------------------------------------------------------------- + [Test] + public void LiftExportRanges_CustomListRangeIdWithAmpersandIsEscapedExactlyOnce() + { + const string ksListName = "Birds & Beasts"; + XmlDocument xdocLift; + XmlDocument xdocRangeFile; + var customList = ExportWithCustomList(ksListName, out xdocLift, out xdocRangeFile); + + var rangesRange = xdocRangeFile.SelectSingleNode(string.Format("//range[@guid='{0}']", customList.Guid)); + Assert.That(rangesRange, Is.Not.Null); + Assert.That(rangesRange.Attributes["id"].Value, Is.EqualTo(ksListName), + "one level of escaping, undone by the parser, must give back the name stored"); + var headerIds = xdocLift.SelectNodes("//header/ranges/range/@id"); + Assert.That(headerIds, Is.Not.Null); + Assert.That(headerIds.Cast().Any(id => id.Value == ksListName), + "the .lift header must name the range the same way the ranges file does"); + } + + /// + /// Exports both files from one exporter, so that the custom list is discovered by the .lift + /// pass and is therefore written by the ranges pass. + /// + private ICmPossibilityList ExportWithCustomList(string listName, out XmlDocument xdocLift, out XmlDocument xdocRangeFile) + { + ICmPossibilityList customList = null; + NonUndoableUnitOfWorkHelper.Do(m_cache.ActionHandlerAccessor, () => + { + customList = AddCustomList(listName); + MakeCustomField("CustomFieldForCustomList", LexEntryTags.kClassId, + WritingSystemServices.kwsAnal, CustomFieldType.ListRefAtomic, customList.Guid); + }); + var exporter = new LiftExporter(m_cache); + xdocLift = new XmlDocument(); + using (var w = new StringWriter()) + { + // SUT: the .lift pass populates the map the ranges pass writes from. + exporter.ExportLift(w, LiftFolder); + xdocLift.LoadXml(w.ToString()); + } + xdocRangeFile = new XmlDocument(); + using (var w = new StringWriter()) + { + // SUT + exporter.ExportLiftRanges(w); + xdocRangeFile.LoadXml(w.ToString()); + } + return customList; + } + /// /// LT-15467 documents a Flex to WeSay S/R which had pronunciation audio files multiplying like bunny rabbits. /// Make sure the export doesn't make new files when two different references point to the same file. diff --git a/Src/LexText/LexTextControls/LiftExporter.cs b/Src/LexText/LexTextControls/LiftExporter.cs index dfb2807aa5..25cd6bbb8a 100644 --- a/Src/LexText/LexTextControls/LiftExporter.cs +++ b/Src/LexText/LexTextControls/LiftExporter.cs @@ -57,7 +57,13 @@ public class LiftExporter /// /// This contains the possibility lists that are custom or that are referenced by a custom field. /// + /// + /// Range names are held unescaped. Each write site escapes for the context it writes into, as + /// everything else in this exporter does; escaping on the way in instead escaped a second time + /// at the write site, and picked the element rules for a value only ever written as an attribute. + /// private Dictionary m_CmPossListsReferencedOrCustom = new Dictionary(); + /// Range names are held unescaped, as for . private Dictionary m_ListsGuidToRangeName = new Dictionary(); private readonly ICmPossibilityListRepository m_repoCmPossibilityLists; private readonly ISilDataAccessManaged m_sda; @@ -1979,10 +1985,11 @@ private void WritePossibilityListAsRange(TextWriter w, string rangeId, ICmPossib { if (list.PossibilitiesOS.Count == 0) return; + var sRangeId = MakeSafeAndNormalizedAttribute(rangeId); if (String.IsNullOrEmpty(guid)) //only output the guid for custom lists - w.WriteLine("", rangeId); + w.WriteLine("", sRangeId); else - w.WriteLine("", rangeId, MakeSafeAndNormalizedAttribute(guid)); + w.WriteLine("", sRangeId, MakeSafeAndNormalizedAttribute(guid)); foreach (var poss in list.ReallyReallyAllPossibilities) { var liftId = poss.Name.BestAnalysisVernacularAlternative.Text; @@ -2348,7 +2355,7 @@ private Dictionary GetCustomListsAndReferencedLists(IEnumerable cmPoss var rangeName = RangeNames.GetRangeNameForLiftExport(m_mdc, possList); if (!cmPossibilityListsReferencedByFields.ContainsKey(possListGuid)) { - cmPossibilityListsReferencedByFields.Add(possListGuid, MakeSafeAndNormalizedXml(rangeName)); + cmPossibilityListsReferencedByFields.Add(possListGuid, rangeName); } } } @@ -2463,7 +2470,7 @@ private void MapCmPossibilityListGuidsToLiftRangeNames(Dictionary //by a custom field and which are not already included yet. foreach (var list in cmPossibilityListsReferencedByFields) { - m_ListsGuidToRangeName.Add(list.Key, MakeSafeAndNormalizedXml(list.Value)); + m_ListsGuidToRangeName.Add(list.Key, list.Value); } } From 8b45cd7af393d68b5cb5c7e8d2ec7998883f02ea Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Thu, 13 Aug 2026 18:55:20 -0400 Subject: [PATCH 2/3] State the escaping invariant without narrating past behavior The doc comments on the two range-name maps and on the two new export tests described how the exporter used to escape range names, which the repository comment standard bans. State the current contract instead: range names are held unescaped, and each is escaped by the rules of the context it is written into. Also reset m_customListsGuids in the per-test setup. NUnit shares one fixture instance across the tests in a class, so the list outlived the cache whose lists it indexes and grew by two entries per test; the assertions that index it were correct only for whichever test ran first. Co-Authored-By: Claude Opus 5 (1M context) --- .../LexTextControlsTests/LiftExportTests.cs | 13 ++++++------- Src/LexText/LexTextControls/LiftExporter.cs | 12 ++++++------ 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/Src/LexText/LexTextControls/LexTextControlsTests/LiftExportTests.cs b/Src/LexText/LexTextControls/LexTextControlsTests/LiftExportTests.cs index c530c04db9..dd95dd8850 100644 --- a/Src/LexText/LexTextControls/LexTextControlsTests/LiftExportTests.cs +++ b/Src/LexText/LexTextControls/LexTextControlsTests/LiftExportTests.cs @@ -493,6 +493,8 @@ public void CreateMockCache() m_mapPartsOfSpeech.Clear(); m_mapAcademicDomains.Clear(); m_mapPublications.Clear(); + // NUnit shares one fixture instance across the tests, so this outlives the cache it indexes. + m_customListsGuids.Clear(); var mockProjectName = "xxyyzProjectFolderForLIFTTest"; MockProjectFolder = Path.Combine(Path.GetTempPath(), mockProjectName); var mockProjectPath = Path.Combine(MockProjectFolder, mockProjectName + ".fwdata"); @@ -1137,10 +1139,8 @@ public void LiftExport() ///-------------------------------------------------------------------------------------- /// - /// A custom list name reaches the ranges file as a range id, where it needs the escaping - /// rules for an attribute. It used to be escaped once as element content on the way into - /// the exporter's map and never again, so a name holding a quotation mark left the ranges - /// document unparseable. + /// A custom list name reaches the ranges file as a range id, which is an attribute, so a name + /// holding a quotation mark has to be escaped by the attribute rules for the document to parse. /// ///-------------------------------------------------------------------------------------- [Test] @@ -1159,9 +1159,8 @@ public void LiftExportRanges_CustomListRangeIdWithQuoteIsEscapedForAnAttribute() ///-------------------------------------------------------------------------------------- /// - /// The same name is written into the .lift header and into the ranges file, so the two must - /// agree. Escaping once on the way in and again at the header write site turned an ampersand - /// into & there while the ranges file kept a single escape. + /// A custom list name is written as a range id in both the .lift header and the ranges file, + /// escaped once in each, so that the two files name the range the same way. /// ///-------------------------------------------------------------------------------------- [Test] diff --git a/Src/LexText/LexTextControls/LiftExporter.cs b/Src/LexText/LexTextControls/LiftExporter.cs index 25cd6bbb8a..eba666d8b6 100644 --- a/Src/LexText/LexTextControls/LiftExporter.cs +++ b/Src/LexText/LexTextControls/LiftExporter.cs @@ -56,14 +56,14 @@ public class LiftExporter private readonly int m_wsBestVernAnal; /// /// This contains the possibility lists that are custom or that are referenced by a custom field. + /// Range names are held unescaped, so that each is escaped by the rules of the context it is + /// written into. /// - /// - /// Range names are held unescaped. Each write site escapes for the context it writes into, as - /// everything else in this exporter does; escaping on the way in instead escaped a second time - /// at the write site, and picked the element rules for a value only ever written as an attribute. - /// private Dictionary m_CmPossListsReferencedOrCustom = new Dictionary(); - /// Range names are held unescaped, as for . + /// + /// Maps a possibility list to the name of its LIFT range. Range names are held unescaped, so + /// that each is escaped by the rules of the context it is written into. + /// private Dictionary m_ListsGuidToRangeName = new Dictionary(); private readonly ICmPossibilityListRepository m_repoCmPossibilityLists; private readonly ISilDataAccessManaged m_sda; From c4b3bf2abbf3ae7f9aa891500cc0789cc2213774 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 14 Aug 2026 09:26:18 -0400 Subject: [PATCH 3/3] Reset every shared custom-field id list between tests The fixture instance outlives each test's cache, so the id lists that VerifyExport indexes positionally kept entries from earlier caches; they matched only because a fresh cache reassigns custom-field flids in the same order. Co-Authored-By: Claude Opus 5 (1M context) --- .../LexTextControls/LexTextControlsTests/LiftExportTests.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Src/LexText/LexTextControls/LexTextControlsTests/LiftExportTests.cs b/Src/LexText/LexTextControls/LexTextControlsTests/LiftExportTests.cs index dd95dd8850..9737775708 100644 --- a/Src/LexText/LexTextControls/LexTextControlsTests/LiftExportTests.cs +++ b/Src/LexText/LexTextControls/LexTextControlsTests/LiftExportTests.cs @@ -493,7 +493,11 @@ public void CreateMockCache() m_mapPartsOfSpeech.Clear(); m_mapAcademicDomains.Clear(); m_mapPublications.Clear(); - // NUnit shares one fixture instance across the tests, so this outlives the cache it indexes. + // NUnit shares one fixture instance across the tests, so these outlive the cache they index. + m_customFieldEntryIds.Clear(); + m_customFieldSenseIds.Clear(); + m_customFieldAllomorphsIds.Clear(); + m_customFieldExampleSentencesIds.Clear(); m_customListsGuids.Clear(); var mockProjectName = "xxyyzProjectFolderForLIFTTest"; MockProjectFolder = Path.Combine(Path.GetTempPath(), mockProjectName);