Fix XML queries

src/WinSW.Core/Configuration/XmlServiceConfig.cs

@@ -20,6 +20,7 @@ namespace WinSW
     {
         protected readonly XmlDocument dom = new XmlDocument();
 
+        private readonly XmlNode root;
         private readonly Dictionary<string, string> environmentVariables;
 
         internal static XmlServiceConfig? TestConfig;
@@ -63,6 +64,8 @@ namespace WinSW
                 throw new InvalidDataException(e.Message, e);
             }
 
+            this.root = this.dom.SelectSingleNode(Names.Service) ?? throw new InvalidDataException("<" + Names.Service + "> is missing in configuration XML");
+
             // register the base directory as environment variable so that future expansions can refer to this.
             Environment.SetEnvironmentVariable("BASE", baseDir);
 
@@ -86,7 +89,7 @@ namespace WinSW
 #pragma warning restore CS8618 // Non-nullable field is uninitialized. Consider declaring as nullable.
         {
             this.dom = dom;
-
+            this.root = this.dom.SelectSingleNode(Names.Service) ?? throw new InvalidDataException("<" + Names.Service + "> is missing in configuration XML");
             this.environmentVariables = this.LoadEnvironmentVariables();
         }
 
@@ -120,30 +123,25 @@ namespace WinSW
 
         private string SingleElement(string tagName)
         {
-            return this.SingleElement(tagName, false)!;
+            return this.SingleElementOrNull(tagName) ?? throw new InvalidDataException("<" + tagName + "> is missing in configuration XML");
         }
 
-        private string? SingleElement(string tagName, bool optional)
+        private string? SingleElementOrNull(string tagName)
         {
-            XmlNode? n = this.dom.SelectSingleNode("//" + tagName);
-            if (n is null && !optional)
-            {
-                throw new InvalidDataException("<" + tagName + "> is missing in configuration XML");
-            }
-
+            XmlNode? n = this.root.SelectSingleNode(tagName);
             return n is null ? null : Environment.ExpandEnvironmentVariables(n.InnerText);
         }
 
-        private bool SingleBoolElement(string tagName, bool defaultValue)
+        private bool SingleBoolElementOrDefault(string tagName, bool defaultValue)
         {
-            XmlNode? e = this.dom.SelectSingleNode("//" + tagName);
+            XmlNode? e = this.root.SelectSingleNode(tagName);
 
             return e is null ? defaultValue : bool.Parse(e.InnerText);
         }
 
         private TimeSpan SingleTimeSpanElement(XmlNode parent, string tagName, TimeSpan defaultValue)
         {
-            string? value = this.SingleElement(tagName, true);
+            string? value = this.SingleElementOrNull(tagName);
             return value is null ? defaultValue : ParseTimeSpan(value);
         }
 
@@ -167,48 +165,27 @@ namespace WinSW
         /// </summary>
         public override string Executable => this.SingleElement("executable");
 
-        public override bool HideWindow => this.SingleBoolElement("hidewindow", base.HideWindow);
+        public override bool HideWindow => this.SingleBoolElementOrDefault("hidewindow", base.HideWindow);
 
         /// <summary>
         /// Optionally specify a different Path to an executable to shutdown the service.
         /// </summary>
-        public override string? StopExecutable => this.SingleElement("stopexecutable", true);
+        public override string? StopExecutable => this.SingleElementOrNull("stopexecutable");
 
         /// <summary>
         /// The <c>arguments</c> element.
         /// </summary>
-        public override string Arguments
-        {
-            get
-            {
-                XmlNode? argumentsNode = this.dom.SelectSingleNode("//arguments");
-                return argumentsNode is null ? base.Arguments : Environment.ExpandEnvironmentVariables(argumentsNode.InnerText);
-            }
-        }
+        public override string Arguments => this.SingleElementOrNull("arguments") ?? base.Arguments;
 
         /// <summary>
         /// The <c>startarguments</c> element.
         /// </summary>
-        public override string? StartArguments
-        {
-            get
-            {
-                XmlNode? startArgumentsNode = this.dom.SelectSingleNode("//startarguments");
-                return startArgumentsNode is null ? null : Environment.ExpandEnvironmentVariables(startArgumentsNode.InnerText);
-            }
-        }
+        public override string? StartArguments => this.SingleElementOrNull("startarguments");
 
         /// <summary>
         /// The <c>stoparguments</c> element.
         /// </summary>
-        public override string? StopArguments
-        {
-            get
-            {
-                XmlNode? stopArgumentsNode = this.dom.SelectSingleNode("//stoparguments");
-                return stopArgumentsNode is null ? null : Environment.ExpandEnvironmentVariables(stopArgumentsNode.InnerText);
-            }
-        }
+        public override string? StopArguments => this.SingleElementOrNull("stoparguments");
 
         public ProcessCommand Prestart => this.GetProcessCommand(Names.Prestart);
 
@@ -222,7 +199,7 @@ namespace WinSW
         {
             get
             {
-                var wd = this.SingleElement("workingdirectory", true);
+                string? wd = this.SingleElementOrNull("workingdirectory");
                 return string.IsNullOrEmpty(wd) ? base.WorkingDirectory : wd!;
             }
         }
@@ -248,64 +225,12 @@ namespace WinSW
             }
         }
 
-        public override XmlNode? ExtensionsConfiguration => this.dom.SelectSingleNode("//extensions");
-
-        /// <summary>
-        /// Combines the contents of all the elements of the given name,
-        /// or return null if no element exists. Handles whitespace quotation.
-        /// </summary>
-        private string? AppendTags(string tagName, string? defaultValue = null)
-        {
-            XmlNode? argumentNode = this.dom.SelectSingleNode("//" + tagName);
-            if (argumentNode is null)
-            {
-                return defaultValue;
-            }
-
-            StringBuilder arguments = new StringBuilder();
-
-            XmlNodeList argumentNodeList = this.dom.SelectNodes("//" + tagName)!;
-            for (int i = 0; i < argumentNodeList.Count; i++)
-            {
-                arguments.Append(' ');
-
-                string token = Environment.ExpandEnvironmentVariables(argumentNodeList[i]!.InnerText);
-
-                if (token.StartsWith("\"") && token.EndsWith("\""))
-                {
-                    // for backward compatibility, if the argument is already quoted, leave it as is.
-                    // in earlier versions we didn't handle quotation, so the user might have worked
-                    // around it by themselves
-                }
-                else
-                {
-                    if (token.Contains(" "))
-                    {
-                        arguments.Append('"').Append(token).Append('"');
-                        continue;
-                    }
-                }
-
-                arguments.Append(token);
-            }
-
-            return arguments.ToString();
-        }
+        public override XmlNode? ExtensionsConfiguration => this.root.SelectSingleNode("extensions");
 
         /// <summary>
         /// LogDirectory is the service wrapper executable directory or the optionally specified logpath element.
         /// </summary>
-        public override string LogDirectory
-        {
-            get
-            {
-                XmlNode? loggingNode = this.dom.SelectSingleNode("//logpath");
-
-                return loggingNode is null
-                    ? base.LogDirectory
-                    : Environment.ExpandEnvironmentVariables(loggingNode.InnerText);
-            }
-        }
+        public override string LogDirectory => this.SingleElementOrNull("logpath") ?? base.LogDirectory;
 
         public override string LogMode
         {
@@ -314,7 +239,7 @@ namespace WinSW
                 string? mode = null;
 
                 // first, backward compatibility with older configuration
-                XmlElement? e = (XmlElement?)this.dom.SelectSingleNode("//logmode");
+                XmlElement? e = (XmlElement?)this.root.SelectSingleNode("logmode");
                 if (e != null)
                 {
                     mode = e.InnerText;
@@ -322,7 +247,7 @@ namespace WinSW
                 else
                 {
                     // this is more modern way, to support nested elements as configuration
-                    e = (XmlElement?)this.dom.SelectSingleNode("//log");
+                    e = (XmlElement?)this.root.SelectSingleNode("log");
                     if (e != null)
                     {
                         mode = e.GetAttribute("mode");
@@ -333,48 +258,24 @@ namespace WinSW
             }
         }
 
-        public string LogName
-        {
-            get
-            {
-                XmlNode? loggingName = this.dom.SelectSingleNode("//logname");
-
-                return loggingName is null ? this.BaseName : Environment.ExpandEnvironmentVariables(loggingName.InnerText);
-            }
-        }
+        public string LogName => this.SingleElementOrNull("logname") ?? this.BaseName;
 
-        public override bool OutFileDisabled => this.SingleBoolElement("outfiledisabled", base.OutFileDisabled);
-
-        public override bool ErrFileDisabled => this.SingleBoolElement("errfiledisabled", base.ErrFileDisabled);
-
-        public override string OutFilePattern
-        {
-            get
-            {
-                XmlNode? loggingName = this.dom.SelectSingleNode("//outfilepattern");
+        public override bool OutFileDisabled => this.SingleBoolElementOrDefault("outfiledisabled", base.OutFileDisabled);
 
-                return loggingName is null ? base.OutFilePattern : Environment.ExpandEnvironmentVariables(loggingName.InnerText);
-            }
-        }
+        public override bool ErrFileDisabled => this.SingleBoolElementOrDefault("errfiledisabled", base.ErrFileDisabled);
 
-        public override string ErrFilePattern
-        {
-            get
-            {
-                XmlNode? loggingName = this.dom.SelectSingleNode("//errfilepattern");
+        public override string OutFilePattern => this.SingleElementOrNull("outfilepattern") ?? base.OutFilePattern;
 
-                return loggingName is null ? base.ErrFilePattern : Environment.ExpandEnvironmentVariables(loggingName.InnerText);
-            }
-        }
+        public override string ErrFilePattern => this.SingleElementOrNull("errfilepattern") ?? base.ErrFilePattern;
 
         public LogHandler LogHandler
         {
             get
             {
-                XmlElement? e = (XmlElement?)this.dom.SelectSingleNode("//logmode");
+                XmlElement? e = (XmlElement?)this.root.SelectSingleNode("logmode");
 
                 // this is more modern way, to support nested elements as configuration
-                e ??= (XmlElement?)this.dom.SelectSingleNode("//log")!; // WARNING: NRE
+                e ??= (XmlElement?)this.root.SelectSingleNode("log")!; // WARNING: NRE
 
                 int sizeThreshold;
                 switch (this.LogMode)
@@ -462,7 +363,7 @@ namespace WinSW
         {
             get
             {
-                XmlNodeList? nodeList = this.dom.SelectNodes("//depend");
+                XmlNodeList? nodeList = this.root.SelectNodes("depend");
                 if (nodeList is null)
                 {
                     return base.ServiceDependencies;
@@ -480,9 +381,9 @@ namespace WinSW
 
         public override string Name => this.SingleElement("id");
 
-        public override string DisplayName => this.SingleElement("name", true) ?? base.DisplayName;
+        public override string DisplayName => this.SingleElementOrNull("name") ?? base.DisplayName;
 
-        public override string Description => this.SingleElement("description", true) ?? base.Description;
+        public override string Description => this.SingleElementOrNull("description") ?? base.Description;
 
         /// <summary>
         /// Start mode of the Service
@@ -491,7 +392,7 @@ namespace WinSW
         {
             get
             {
-                string? p = this.SingleElement("startmode", true);
+                string? p = this.SingleElementOrNull("startmode");
                 if (p is null)
                 {
                     return base.StartMode;
@@ -519,15 +420,15 @@ namespace WinSW
         /// True if the service should be installed with the DelayedAutoStart flag.
         /// This setting will be applyed only during the install command and only when the Automatic start mode is configured.
         /// </summary>
-        public override bool DelayedAutoStart => this.SingleBoolElement("delayedAutoStart", base.DelayedAutoStart);
+        public override bool DelayedAutoStart => this.SingleBoolElementOrDefault("delayedAutoStart", base.DelayedAutoStart);
 
-        public override bool Preshutdown => this.SingleBoolElement("preshutdown", base.Preshutdown);
+        public override bool Preshutdown => this.SingleBoolElementOrDefault("preshutdown", base.Preshutdown);
 
         public TimeSpan? PreshutdownTimeout
         {
             get
             {
-                string? value = this.SingleElement("preshutdownTimeout", true);
+                string? value = this.SingleElementOrNull("preshutdownTimeout");
                 return value is null ? default : ParseTimeSpan(value);
             }
         }
@@ -536,12 +437,12 @@ namespace WinSW
         /// True if the service should beep when finished on shutdown.
         /// This doesn't work on some OSes. See http://msdn.microsoft.com/en-us/library/ms679277%28VS.85%29.aspx
         /// </summary>
-        public override bool BeepOnShutdown => this.SingleBoolElement("beeponshutdown", base.DelayedAutoStart);
+        public override bool BeepOnShutdown => this.SingleBoolElementOrDefault("beeponshutdown", base.DelayedAutoStart);
 
         /// <summary>
         /// True if the service can interact with the desktop.
         /// </summary>
-        public override bool Interactive => this.SingleBoolElement("interactive", base.DelayedAutoStart);
+        public override bool Interactive => this.SingleBoolElementOrDefault("interactive", base.DelayedAutoStart);
 
         /// <summary>
         /// Environment variable overrides
@@ -556,7 +457,7 @@ namespace WinSW
         {
             get
             {
-                XmlNodeList? nodeList = this.dom.SelectNodes("//download");
+                XmlNodeList? nodeList = this.root.SelectNodes("download");
                 if (nodeList is null)
                 {
                     return base.Downloads;
@@ -579,7 +480,7 @@ namespace WinSW
         {
             get
             {
-                XmlNodeList? childNodes = this.dom.SelectNodes("//onfailure");
+                XmlNodeList? childNodes = this.root.SelectNodes("onfailure");
                 if (childNodes is null)
                 {
                     return Array.Empty<SC_ACTION>();
@@ -605,11 +506,11 @@ namespace WinSW
             }
         }
 
-        public override TimeSpan ResetFailureAfter => this.SingleTimeSpanElement(this.dom, "resetfailure", base.ResetFailureAfter);
+        public override TimeSpan ResetFailureAfter => this.SingleTimeSpanElement(this.root, "resetfailure", base.ResetFailureAfter);
 
         protected string? GetServiceAccountPart(string subNodeName)
         {
-            XmlNode? node = this.dom.SelectSingleNode("//serviceaccount");
+            XmlNode? node = this.root.SelectSingleNode("serviceaccount");
 
             if (node != null)
             {
@@ -633,7 +534,7 @@ namespace WinSW
 
         public bool HasServiceAccount()
         {
-            return this.dom.SelectSingleNode("//serviceaccount") != null;
+            return this.root.SelectSingleNode("serviceaccount") != null;
         }
 
         public override bool AllowServiceAcountLogonRight
@@ -655,7 +556,7 @@ namespace WinSW
         /// <summary>
         /// Time to wait for the service to gracefully shutdown the executable before we forcibly kill it
         /// </summary>
-        public override TimeSpan StopTimeout => this.SingleTimeSpanElement(this.dom, "stoptimeout", base.StopTimeout);
+        public override TimeSpan StopTimeout => this.SingleTimeSpanElement(this.root, "stoptimeout", base.StopTimeout);
 
         public int StopTimeoutInMs => (int)this.StopTimeout.TotalMilliseconds;
 
@@ -666,7 +567,7 @@ namespace WinSW
         {
             get
             {
-                string? p = this.SingleElement("priority", true);
+                string? p = this.SingleElementOrNull("priority");
                 if (p is null)
                 {
                     return base.Priority;
@@ -676,13 +577,13 @@ namespace WinSW
             }
         }
 
-        public string? SecurityDescriptor => this.SingleElement("securityDescriptor", true);
+        public string? SecurityDescriptor => this.SingleElementOrNull("securityDescriptor");
 
-        public bool AutoRefresh => this.SingleBoolElement("autoRefresh", true);
+        public bool AutoRefresh => this.SingleBoolElementOrDefault("autoRefresh", true);
 
         private Dictionary<string, string> LoadEnvironmentVariables()
         {
-            XmlNodeList nodeList = this.dom.SelectNodes("//env")!;
+            XmlNodeList nodeList = this.root.SelectNodes("env")!;
             Dictionary<string, string> environment = new Dictionary<string, string>(nodeList.Count);
             for (int i = 0; i < nodeList.Count; i++)
             {
@@ -699,7 +600,7 @@ namespace WinSW
 
         private ProcessCommand GetProcessCommand(string name)
         {
-            XmlNode? node = this.dom.SelectSingleNode(Names.Service)?.SelectSingleNode(name);
+            XmlNode? node = this.root.SelectSingleNode(name);
             return node is null ? default : new ProcessCommand
             {
                 Executable = GetInnerText(Names.Executable),

src/WinSW.Tests/ServiceConfigTests.cs

@@ -355,24 +355,24 @@ $@"<service>
             };
             var poststart = new ProcessCommand
             {
-                Executable = "a1",
-                Arguments = "a2",
-                StdoutPath = "a3",
-                StderrPath = "a4",
+                Executable = "b1",
+                Arguments = "b2",
+                StdoutPath = "b3",
+                StderrPath = "b4",
             };
             var prestop = new ProcessCommand
             {
-                Executable = "a1",
-                Arguments = "a2",
-                StdoutPath = "a3",
-                StderrPath = "a4",
+                Executable = "c1",
+                Arguments = "c2",
+                StdoutPath = "c3",
+                StderrPath = "c4",
             };
             var poststop = new ProcessCommand
             {
-                Executable = "a1",
-                Arguments = "a2",
-                StdoutPath = "a3",
-                StderrPath = "a4",
+                Executable = "d1",
+                Arguments = "d2",
+                StdoutPath = "d3",
+                StderrPath = "d4",
             };
 
             string seedXml =
@@ -401,6 +401,8 @@ $@"<service>
     <stdoutPath>{poststop.StdoutPath}</stdoutPath>
     <stderrPath>{poststop.StderrPath}</stderrPath>
   </poststop>
+  <executable>executable</executable>
+  <arguments>arguments</arguments>
 </service>";
 
             XmlServiceConfig config = XmlServiceConfig.FromXml(seedXml);