diff --git a/CHANGELOG.md b/CHANGELOG.md index 34c276c4a..e32c11c37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed +- [SIL.Chorus.LibChorus] Match `range-element` on its `guid` rather than its `id`, so that renaming or respelling a range element merges as an edit instead of a deletion plus an addition - Prevent S&R to Internet without full URL - [SIL.Chorus.LibChorus] Correctly handle & and other special characters in passwords - [SIL.Chorus] Fix collection-modified exception when UsbDrives property is read while background scan thread updates the list diff --git a/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs b/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs index 56cfdaf58..b653d7e6d 100644 --- a/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs +++ b/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs @@ -42,7 +42,15 @@ internal static void AddLiftRangeElementStrategies(MergeStrategies mergeStrategi // This element appears to not be in the main lift file, so it will be 'extra', but ought not cause harm. // [Optional, multitext] holds zero or more
elements diff --git a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs index 5656ebcd2..d3092725f 100644 --- a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs +++ b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs @@ -15,9 +15,11 @@ namespace Chorus.merge.xml.generic public interface IFindNodeToMerge { /// - /// Should return null if parentToSearchIn is null. Non-null result should be a value in acceptableTargets, - /// which will be a subset (or all) of the children of parentToSearchIn; any other result will - /// be treated as no match. + /// Should return null if parentToSearchIn is null, and otherwise either null or a + /// child of parentToSearchIn. acceptableTargets is a subset (or all) of the children + /// of parentToSearchIn; an implementation holding only one possible match may ignore + /// it and return that match, so a caller passing a strict subset is responsible for + /// discarding a result outside it. /// XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, HashSet acceptableTargets); } @@ -210,6 +212,183 @@ public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) } } + /// + /// Search for a matching element on the value of one of two key attributes: the preferred + /// attribute where the element carries a non-empty value for it, the fallback attribute only + /// where it does not. The fallback value is not consulted where a non-empty preferred value + /// is present. + /// + /// + /// Use this where the fallback key attribute's value is derived from user data, and so + /// can change while the object it identifies does not, and the preferred key attribute holds + /// a value that stays with that object. Matching on the changeable value alone turns such a + /// change into a deletion plus an addition. + /// Elements carrying a preferred value and elements carrying none are matched + /// separately, and never against each other, which keeps matching an equivalence relation: + /// no element is paired with two partners, and no edit lands on an object it was not made + /// against. An attribute that is present but empty counts as absent throughout. + /// The price is that an element whose preferred value one revision writes and another + /// omits reads as a deletion plus an addition, and as a removed-versus-edited conflict where + /// the other revision edited it. Choose a preferred key attribute only where every writer can + /// be relied on to preserve the value it finds; writers that disagree pay that cost on every + /// merge between them. + /// + public class FindByPreferredKeyAttribute : IFindMatchingNodesToMerge + { + private readonly string _preferredKeyAttribute; + private readonly string _fallbackKeyAttribute; + private readonly Dictionary _indexedParents = new Dictionary(); + + public FindByPreferredKeyAttribute(string preferredKeyAttribute, string fallbackKeyAttribute) + { + _preferredKeyAttribute = preferredKeyAttribute; + _fallbackKeyAttribute = fallbackKeyAttribute; + } + + public XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, HashSet acceptableTargets) + { + if (nodeToMatch == null || parentToSearchIn == null) + return null; + + var preferredKey = XmlUtilities.GetOptionalAttributeString(nodeToMatch, _preferredKeyAttribute); + var fallbackKey = XmlUtilities.GetOptionalAttributeString(nodeToMatch, _fallbackKeyAttribute); + if (string.IsNullOrEmpty(preferredKey) && string.IsNullOrEmpty(fallbackKey)) + return null; + + var index = GetIndexFor(parentToSearchIn); + XmlNode match; + if (string.IsNullOrEmpty(preferredKey)) + { + // Only among elements that likewise carry no preferred value, so that this + // cannot claim an element that a preferred value already speaks for. + index.ByFallbackKeyWherePreferredAbsent.TryGetValue(IndexKey(nodeToMatch, fallbackKey), out match); + return match; // May be null, which is fine. + } + index.ByPreferredKey.TryGetValue(IndexKey(nodeToMatch, preferredKey), out match); + return match; + } + + /// + /// The children of one parent, indexed on (element name, attribute value) for each key + /// attribute this finder can match on, so siblings are not rescanned once per child. + /// Built at the parent's first search and never revisited: children added or removed + /// between searches of one parent are not seen. + /// + private class ParentIndex + { + internal readonly Dictionary, XmlNode> ByPreferredKey = new Dictionary, XmlNode>(); + /// Only those children that carry no preferred value of their own. + internal readonly Dictionary, XmlNode> ByFallbackKeyWherePreferredAbsent = new Dictionary, XmlNode>(); + } + + private ParentIndex GetIndexFor(XmlNode parentToSearchIn) + { + ParentIndex index; + if (_indexedParents.TryGetValue(parentToSearchIn, out index)) + return index; + + index = new ParentIndex(); + _indexedParents.Add(parentToSearchIn, index); + foreach (XmlNode childNode in parentToSearchIn.ChildNodes) + { + if (childNode.NodeType != XmlNodeType.Element) + continue; + var preferredKey = XmlUtilities.GetOptionalAttributeString(childNode, _preferredKeyAttribute); + var fallbackKey = XmlUtilities.GetOptionalAttributeString(childNode, _fallbackKeyAttribute); + if (!string.IsNullOrEmpty(preferredKey)) + IndexFirstOnly(index.ByPreferredKey, childNode, preferredKey); + else if (!string.IsNullOrEmpty(fallbackKey)) + IndexFirstOnly(index.ByFallbackKeyWherePreferredAbsent, childNode, fallbackKey); + } + return index; + } + + /// + /// Two children can carry the same key value, which is not an error here: keep the first + /// and ignore the rest, matching how the merger resolves siblings it cannot tell apart. + /// + private static void IndexFirstOnly(IDictionary, XmlNode> index, XmlNode childNode, string keyValue) + { + var indexKey = IndexKey(childNode, keyValue); + if (!index.ContainsKey(indexKey)) + index.Add(indexKey, childNode); + } + + /// + /// The element name is part of the key, so elements of different names never match, + /// however their key values compare. + /// + private static Tuple IndexKey(XmlNode element, string keyValue) + { + return new Tuple(element.Name, keyValue); + } + + /// + /// Get all matching nodes, or an empty collection, if there are no matches. + /// + /// A collection of zero, or more, matching nodes. + /// may, or may not, be a child of . + public IEnumerable GetMatchingNodes(XmlNode nodeToMatch, XmlNode parentToSearchIn) + { + var matches = new List(); + if (nodeToMatch == null || parentToSearchIn == null) + return matches; + + var preferredKey = XmlUtilities.GetOptionalAttributeString(nodeToMatch, _preferredKeyAttribute); + var fallbackKey = XmlUtilities.GetOptionalAttributeString(nodeToMatch, _fallbackKeyAttribute); + if (string.IsNullOrEmpty(preferredKey) && string.IsNullOrEmpty(fallbackKey)) + return matches; + + foreach (XmlNode childNode in parentToSearchIn.ChildNodes) + { + if (childNode.NodeType != XmlNodeType.Element) + continue; + if (nodeToMatch == childNode) + { + matches.Add(childNode); + continue; + } + if (childNode.Name != nodeToMatch.Name) + continue; + if (IsMatch(childNode, preferredKey, fallbackKey)) + matches.Add(childNode); + } + return matches; + } + + private bool IsMatch(XmlNode candidate, string soughtPreferredKey, string soughtFallbackKey) + { + var candidatePreferredKey = XmlUtilities.GetOptionalAttributeString(candidate, _preferredKeyAttribute); + // One carries a preferred value and the other does not, so they are matched apart. + if (string.IsNullOrEmpty(soughtPreferredKey) != string.IsNullOrEmpty(candidatePreferredKey)) + return false; + // Both carry one, and it decides alone: the fallback value may have changed. + if (!string.IsNullOrEmpty(soughtPreferredKey)) + return soughtPreferredKey == candidatePreferredKey; + if (string.IsNullOrEmpty(soughtFallbackKey)) + return false; + return soughtFallbackKey == XmlUtilities.GetOptionalAttributeString(candidate, _fallbackKeyAttribute); + } + + /// + /// Get a basic message that is suitable for use in a warning report where ambiguous nodes are found in the same parent node. + /// + /// A message string or null/empty string, if no message is needed for ambiguous nodes. + public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) + { + Guard.AgainstNull(nodeForMessage, "nodeForMessage"); + + // Whether the element carries a preferred value decides which key attribute it was + // matched on, so that is the attribute whose values are the same; naming the other + // would accuse an attribute the match never consulted. + var preferredKey = XmlUtilities.GetOptionalAttributeString(nodeForMessage, _preferredKeyAttribute); + var matchedOnPreferredKey = !string.IsNullOrEmpty(preferredKey); + return string.Format("The key attribute '{0}' has values that are the same '{1}'", + matchedOnPreferredKey ? _preferredKeyAttribute : _fallbackKeyAttribute, + matchedOnPreferredKey ? preferredKey : XmlUtilities.GetOptionalAttributeString(nodeForMessage, _fallbackKeyAttribute)); + } + } + /// /// Assuming the children of the parent to search in form a list (order matters, duplicates allowed), and so do the /// children of the nodeToMatch, find the corresponding object in the list. A corresponding node will have the same key, diff --git a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs index fdf060820..c6cb9e39a 100644 --- a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs +++ b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs @@ -1,12 +1,15 @@ using System; using System.IO; using System.Linq; +using System.Text; using Chorus.FileTypeHandlers; using Chorus.merge; +using Chorus.merge.xml.generic; using LibChorus.TestUtilities; using NUnit.Framework; using SIL.IO; using SIL.Progress; +using SIL.TestUtilities; namespace LibChorus.Tests.FileHandlers.LiftRanges { @@ -251,6 +254,222 @@ public void BothEditWithConflictAndWeWin() Assert.AreEqual(ours.Replace("\r\n", "\n"), result); } + /// + /// A range-element id is the possibility's own name, so FLEx respelling names on export + /// (LT-22697 normalizes them) moves the id without the possibility having changed. + /// + [Test] + public void RespelledIdMergesAsAnEditWhenTheGuidIsUnchanged() + { + var common = RangesWithPartOfSpeech(kDecomposedId, kPosGuid, "old"); + // We upgraded, so our export normalizes the id. + var ours = RangesWithPartOfSpeech(kComposedId, kPosGuid, "old"); + // They did not upgrade, and they edited the abbreviation. + var theirs = RangesWithPartOfSpeech(kDecomposedId, kPosGuid, "new"); + + var result = DoMerge(common, ours, theirs, 0, 2); + + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + string.Format("//range-element[@guid='{0}' and @id='{1}']", kPosGuid, kComposedId), 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element/abbrev/form/text[text()='new']", 1); + } + + /// + /// @guid is optional in LIFT, so a file written without one must still merge on its id. + /// + [Test] + public void RangeElementWithoutAGuidStillMergesOnItsId() + { + var common = RangesWithPartOfSpeech("Noun", null, "old"); + var ours = RangesWithPartOfSpeech("Noun", null, "old"); + var theirs = RangesWithPartOfSpeech("Noun", null, "new"); + + var result = DoMerge(common, ours, theirs, 0, 0); + + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element/abbrev/form/text[text()='new']", 1); + } + + /// + /// The finder branches on a value, not on the attribute, so @guid='' counts as no guid at + /// all: such an element is matched on its @id, and matches one that omits @guid outright, + /// rather than reading as a deletion plus an addition. + /// + [Test] + public void AnEmptyGuidCountsAsNoGuidAtAll() + { + var common = RangesWithPartOfSpeech("Noun", null, "old"); + // Our writer emits the attribute but leaves it empty. + var ours = RangesWithPartOfSpeech("Noun", "", "old"); + var theirs = RangesWithPartOfSpeech("Noun", null, "new"); + + // Two changes for our added attribute, as in the respelled-id case; none for theirs. + var result = DoMerge(common, ours, theirs, 0, 2); + + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element/abbrev/form/text[text()='new']", 1); + } + + /// + /// An element that names a guid is matched only against elements that likewise name + /// one, so a writer that drops the guid a previous writer wrote loses the object's + /// identity: the element reads as a deletion and its guid-less replacement as an + /// addition. That is the cost of keeping the two sets apart, and a project whose + /// writers disagree over the guid pays it on every merge between them. + /// + [Test] + public void DroppingTheGuidReadsAsADeletionPlusAnAddition() + { + var common = RangesWithPartOfSpeech("Noun", kPosGuid, "old"); + // Our writer omits the guid the ancestor names. + var ours = RangesWithPartOfSpeech("Noun", null, "old"); + var theirs = RangesWithPartOfSpeech("Noun", kPosGuid, "new"); + + var result = DoMerge(common, ours, theirs, 1, 1); + + _eventListener.AssertFirstConflictType(); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 2); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element[not(@guid)]/abbrev/form/text[text()='old']", 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath(string.Format( + "//range-element[@guid='{0}']/abbrev/form/text[text()='new']", kPosGuid), 1); + } + + /// + /// Two possibilities that happen to share a name are still two possibilities. + /// Matching them on the id would merge them into one and lose a guid. + /// + [Test] + public void RangeElementsWithDifferentGuidsAreNotMatched() + { + const string theirGuid = "9c4d3e2b-77a6-4b1e-8d55-6a0f2c3e14bb"; + const string common = +@" + + +"; + var ours = RangesWithPartOfSpeech("Noun", kPosGuid, "n"); + var theirs = RangesWithPartOfSpeech("Noun", theirGuid, "n"); + + var result = DoMerge(common, ours, theirs, 0, 2); + + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 2); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + string.Format("//range-element[@guid='{0}']", kPosGuid), 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + string.Format("//range-element[@guid='{0}']", theirGuid), 1); + } + + /// + /// A sibling that still carries the old id, written by a tool that omits guids, must + /// not be taken as the partner just because it comes first in the file. + /// + [Test] + public void GuidMatchIsPreferredOverAnEarlierSiblingMatchingOnTheOldId() + { + var common = RangesWithPartOfSpeech(kDecomposedId, kPosGuid, "old"); + // A guid-less element carrying the old id sorts ahead of our respelled one. + var ours = string.Format( +@" + + + +unrelated + + +
old
 + + +", kDecomposedId, kComposedId, kPosGuid); + var theirs = RangesWithPartOfSpeech(kDecomposedId, kPosGuid, "new"); + + // No conflict: the guid-less sibling is matched only against other guid-less + // elements, so it cannot also claim the element the guid already speaks for. + var result = DoMerge(common, ours, theirs, 0, 3); + + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 2); + // Their edit belongs to the element sharing the guid, not the one sharing the old id. + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath(string.Format( + "//range-element[@guid='{0}']/abbrev/form/text[text()='new']", kPosGuid), 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element[not(@guid)]/abbrev/form/text[text()='unrelated']", 1); + } + + /// + /// A merge done before @guid was preferred could leave one possibility standing as + /// two range-elements, spelled differently. Matching on the guid value makes them + /// ambiguous siblings, which the merger collapses back to one. The warning must name + /// @guid as the attribute whose values are the same, and quote the shared value, + /// since the differing ids are not what made them indistinguishable. + /// + /// + /// The collapse keeps the first of the pair in document order and discards the other + /// outright, so whatever the earlier merge left on the loser goes with it. The two are + /// given different content here to pin that cost rather than hide it behind a pair that + /// happens to agree. There is no better winner available: this is a sanitizing pass over + /// each revision as it is read, with no view of which side wrote which. + /// + [Test] + public void RangeElementsDuplicatedByAnEarlierMergeCollapseToOne() + { + var duplicated = string.Format( +@" + + + +
kept
 + + +
dropped
 + + + +", kDecomposedId, kComposedId, kPosGuid); + + var result = DoMerge(duplicated, duplicated, duplicated, 0, 0); + + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 1); + // The loser's content is discarded, not merged into the survivor. + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element/abbrev/form/text[text()='kept']", 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element/abbrev/form/text[text()='dropped']", 0); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range-element/label", 0); + + Assert.That(_eventListener.Warnings, Is.Not.Empty, "the dropped duplicate should be reported"); + var warning = _eventListener.Warnings[0].GetFullHumanReadableDescription(); + Assert.That(warning, Does.Contain("'guid'").And.Contain(kPosGuid), warning); + Assert.That(warning, Does.Not.Contain("'id'"), warning); + } + + // The two @id values a part of speech named "Compléments" is exported under. Built + // rather than written out, since the two spellings are indistinguishable in source + // and an editor or a text filter that normalizes on save would silently make them + // the same string. + private static readonly string kDecomposedId = "Compléments".Normalize(NormalizationForm.FormD); + private static readonly string kComposedId = "Compléments".Normalize(NormalizationForm.FormC); + private const string kPosGuid = "e8c4b4b0-1a2f-4f9e-9f39-3f2b0d8f7a11"; + + /// + /// A null leaves @guid off the element altogether. + /// + private static string RangesWithPartOfSpeech(string posId, string posGuidOrNull, string abbrevText) + { + return string.Format( +@" + + + +
{2}
 + + +", posId, posGuidOrNull == null ? string.Empty : string.Format(" guid='{0}'", posGuidOrNull), abbrevText); + } + private string DoMerge(string commonAncestor, string ourContent, string theirContent, int expectedConflictCount, int expectedChangesCount) {