diff --git a/src/LogExpert.Core/Classes/Persister/PersisterXML.cs b/src/LogExpert.Core/Classes/Persister/PersisterXML.cs index 674f7159..f27285a7 100644 --- a/src/LogExpert.Core/Classes/Persister/PersisterXML.cs +++ b/src/LogExpert.Core/Classes/Persister/PersisterXML.cs @@ -235,7 +235,13 @@ private static PersistenceData LoadInternal (string fileName) private static PersistenceData ReadPersistenceDataFromNode (XmlNode node) { PersistenceData persistenceData = new(); - var fileElement = node as XmlElement; + if (node is not XmlElement fileElement) + { + // Documented above: a node that is not an element yields defaults. ReadFilterTabs walks every + // child of filterTabs, so a comment or text node in a hand-edited .lxp arrives here. + return persistenceData; + } + persistenceData.BookmarkList = ReadBookmarks(fileElement); persistenceData.RowHeightList = ReadRowHeightList(fileElement); ReadOptions(fileElement, persistenceData); @@ -447,17 +453,59 @@ private static SortedList ReadRowHeightList (XmlElement sta } /// - /// Reads configuration options from the specified XML element and populates the provided object with the extracted settings. + /// Reads the options section of a file element, plus the tab, columnizer and + /// highlightGroup elements that sit beside it, into the provided . /// - /// This method processes various configuration options such as multi-file settings, current line - /// information, filter visibility, and more. It expects the XML structure to contain specific nodes and attributes - /// that define these settings. If certain attributes are missing or invalid, default values are applied. - /// The XML element containing the configuration options to be read. + /// + /// The options section is delegated to and is optional: a hand-edited + /// or very old .lxp may omit it, in which case every setting it carries keeps its + /// default. The three sibling elements are read either way, so a missing + /// options section does not cost the user their tab name, columnizer or highlight group. + /// + /// The file element to read from. /// The object to populate with the settings extracted from the XML element. private static void ReadOptions (XmlElement startNode, PersistenceData persistenceData) { XmlNode optionsNode = startNode.SelectSingleNode("options"); + if (optionsNode != null) + { + ReadOptionsNode(optionsNode, persistenceData); + } + + // tab, columnizer and highlightGroup are siblings of options, not children, so they are read + // even when a hand-edited or very old .lxp carries no options section at all. + XmlNode tabNode = startNode.SelectSingleNode("tab"); + if (tabNode != null) + { + persistenceData.TabName = (tabNode as XmlElement).GetAttribute("name"); + } + + XmlNode columnizerNode = startNode.SelectSingleNode("columnizer"); + if (columnizerNode != null) + { + persistenceData.ColumnizerName = (columnizerNode as XmlElement).GetAttribute("name"); + } + + XmlNode highlightGroupNode = startNode.SelectSingleNode("highlightGroup"); + if (highlightGroupNode != null) + { + persistenceData.HighlightGroupName = (highlightGroupNode as XmlElement).GetAttribute("name"); + } + } + + /// + /// Reads the settings held under the options element into the given . + /// + /// + /// Boolean flags are written unconditionally, so an absent element or attribute reads as false. + /// The remaining settings keep the value they already have. Not calling this at all — which is what + /// does for a file element without an options child — therefore + /// leaves the whole group at its default, FollowTail included. + /// + /// The options element. Must not be null. + /// The object to populate. + private static void ReadOptionsNode (XmlNode optionsNode, PersistenceData persistenceData) + { var value = GetOptionsAttribute(optionsNode, "multifile", "enabled"); persistenceData.MultiFile = value != null && value.Equals("1", StringComparison.OrdinalIgnoreCase); persistenceData.MultiFilePattern = GetOptionsAttribute(optionsNode, "multifile", "pattern"); @@ -530,24 +578,6 @@ FormatException or value = GetOptionsAttribute(optionsNode, "filterSaveList", "visible"); persistenceData.FilterSaveListVisible = value != null && value.Equals("1", StringComparison.OrdinalIgnoreCase); - - XmlNode tabNode = startNode.SelectSingleNode("tab"); - if (tabNode != null) - { - persistenceData.TabName = (tabNode as XmlElement).GetAttribute("name"); - } - - XmlNode columnizerNode = startNode.SelectSingleNode("columnizer"); - if (columnizerNode != null) - { - persistenceData.ColumnizerName = (columnizerNode as XmlElement).GetAttribute("name"); - } - - XmlNode highlightGroupNode = startNode.SelectSingleNode("highlightGroup"); - if (highlightGroupNode != null) - { - persistenceData.HighlightGroupName = (highlightGroupNode as XmlElement).GetAttribute("name"); - } } /// @@ -599,8 +629,19 @@ private static string GetOptionsAttribute (XmlNode optionsNode, string elementNa /// when the file cannot be read or parsed (XML/IO/security related issues are logged). /// /// - /// Only XML format is attempted. Any , , - /// or is caught and logged; in these cases null is returned. + /// Only XML format is attempted. Unreadable files (, + /// , ) and unparseable content are caught + /// and logged; in these cases null is returned. + /// + /// "Unparseable" covers more than malformed markup: this is a read-only fallback for a format LogExpert + /// no longer writes, so the readers below assume a well-formed document and let a hand-edited or truncated + /// one throw (unparseable numbers, invalid Base64), + /// (numbers too large for their field), + /// (missing attributes, duplicate bookmark lines, a filter tab with no filter) or + /// (attribute iteration over a comment or text node that stands where + /// an element was expected). The caller reaches this synchronously from the file-open path, so every one of + /// those has to degrade to "no persistence data" rather than escape. + /// /// public static PersistenceData Load (string fileName) { @@ -611,7 +652,11 @@ public static PersistenceData Load (string fileName) catch (Exception xmlParsingException) when (xmlParsingException is XmlException or UnauthorizedAccessException or IOException or - FileNotFoundException) + FileNotFoundException or + FormatException or + OverflowException or + ArgumentException or + NullReferenceException) { _logger.Error(xmlParsingException, $"Error loading persistence data from {fileName}, unknown format, parsing xml or json was not possible"); return null; diff --git a/src/LogExpert.Persister.Tests/PersisterXmlMalformedTests.cs b/src/LogExpert.Persister.Tests/PersisterXmlMalformedTests.cs new file mode 100644 index 00000000..ee6aaba9 --- /dev/null +++ b/src/LogExpert.Persister.Tests/PersisterXmlMalformedTests.cs @@ -0,0 +1,144 @@ +using LogExpert.Core.Classes.Persister; + +namespace LogExpert.Persister.Tests; + +/// +/// The deprecated XML .lxp format is a read-only fallback: it is only reached when a session file fails +/// to parse as JSON, and that happens on the file-open path, synchronously, with no handler above it. +/// A .lxp that is hand-edited, truncated, or written by a very old version must therefore degrade to +/// "no persistence data" instead of throwing into the open. +/// +[TestFixture] +#pragma warning disable CS0618 // PersisterXML is the deprecated fallback format, and still loads old .lxp files. +public class PersisterXmlMalformedTests +{ + private string _lxpFile = null!; + + [SetUp] + public void Setup () + { + _lxpFile = Path.Join(Path.GetTempPath(), $"{Guid.NewGuid()}.lxp"); + } + + [TearDown] + public void Cleanup () + { + if (File.Exists(_lxpFile)) + { + File.Delete(_lxpFile); + } + } + + [Test] + public void Load_FileElementWithoutOptions_ReturnsDefaults () + { + WriteLxp(""""""); + + var data = PersisterXML.Load(_lxpFile); + + Assert.That(data, Is.Not.Null, "a without must still load"); + Assert.Multiple(() => + { + Assert.That(data.FileName, Is.EqualTo("test.log")); + Assert.That(data.LineCount, Is.EqualTo(1)); + Assert.That(data.MultiFile, Is.False); + Assert.That(data.MultiFilePattern, Is.Null); + Assert.That(data.MultiFileMaxDays, Is.Zero); + Assert.That(data.FilterVisible, Is.False); + + // A missing means "nothing was persisted", so every option keeps its + // PersistenceData default — including the ones whose default is not the zero value. + Assert.That(data.CurrentLine, Is.EqualTo(-1)); + Assert.That(data.FollowTail, Is.True); + }); + } + + [Test] + public void Load_FileElementWithoutOptions_StillReadsSiblingsOfOptions () + { + // tab, columnizer and highlightGroup are children of , not of . A missing + // must not cost the user their columnizer and highlight group. + WriteLxp(""" + + + + + + """); + + var data = PersisterXML.Load(_lxpFile); + + Assert.That(data, Is.Not.Null); + Assert.Multiple(() => + { + Assert.That(data.TabName, Is.EqualTo("My Tab")); + Assert.That(data.ColumnizerName, Is.EqualTo("CSV Columnizer")); + Assert.That(data.HighlightGroupName, Is.EqualTo("My Group")); + }); + } + + /// + /// Load documents "null when the file cannot be read or parsed", and malformed content counts, not + /// just malformed markup. One case per exception the readers let escape, so the widened catch in + /// Load is pinned rather than merely asserted in prose. + /// + [Test] + [TestCase("""""", "FormatException")] + [TestCase("""""", "OverflowException")] + [TestCase("""1234""", "ArgumentException")] + [TestCase("""""", "NullReferenceException")] + public void Load_MalformedContent_ReturnsNull (string fileElement, string escapingException) + { + WriteLxp(fileElement); + + Assert.That(PersisterXML.Load(_lxpFile), Is.Null, $"{escapingException} escaped Load"); + } + + [Test] + public void Load_FilterTabsWithNonElementChild_SkipsIt () + { + WriteLxp(""" + + + + + """); + + var data = PersisterXML.Load(_lxpFile); + + // ReadPersistenceDataFromNode documents defaults for a node that is not an element, so the + // remark costs the reader nothing beyond the tab it is standing in for. + Assert.That(data, Is.Not.Null); + Assert.Multiple(() => + { + Assert.That(data.FileName, Is.EqualTo("test.log")); + Assert.That(data.FilterTabDataList, Is.Empty); + }); + } + + [Test] + public void PersisterLoad_FileElementWithoutOptions_DoesNotThrowOnTheFileOpenPath () + { + // Persister.Load tries JSON first and only falls back to XML from inside the catch block, so + // anything PersisterXML throws replaces the original exception and escapes into AddFileTab. + WriteLxp(""""""); + + var data = Core.Classes.Persister.Persister.Load(_lxpFile); + + Assert.That(data, Is.Not.Null); + Assert.That(data.FileName, Is.EqualTo("test.log")); + } + + private void WriteLxp (string fileElement) + { + File.WriteAllText( + _lxpFile, + $""" + + + {fileElement} + + """); + } +} +#pragma warning restore CS0618 diff --git a/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs b/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs index a2a1e044..a917255f 100644 --- a/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs +++ b/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs @@ -73,7 +73,6 @@ public void Load_UnresolvableEncodingName_FallsBackToDefault () private void WriteLxp (string encodingElement) { - // has to be present: PersisterXML.ReadOptions dereferences it unconditionally. File.WriteAllText( _lxpFile, $"""