update XmlPeek and XmlPoke tasks (#9194)

Add `[Required]` to parameters.

- `XmlPeek` Task
  - Change `Query` parameter.
  - Remove redundant `Dispose` that was flagged by the analyzer.
  - Change XmlPeek.XmlInput class from `Internal` to `private sealed` and change access of some members
  - Minor cleanup changes
- `XmlPoke` Task
  - Change `Query` parameter.
  - Change `XmlInputPath` parameter.
  - Minor cleanup changes
- XmlPeek_Tests class
  - Add new `PeekWithNoParameters` test
- XmlPoke_Tests class
  - Remove `PokeMissingParams` test
    - The test was defined as a `[Fact]` and used a for loop to test 4 distinct cases
    - The test expected `ArgumentNullException` to be thrown
  - Add 4 new tests, one for each of the four cases:
    - `PokeWithNoParameters`
    - `PokeWithMissingRequiredQuery`
    - `PokeWithMissingRequiredXmlInputPath`
    - `PokeWithRequiredParameters` (completes the replacement of `PokeMissingParams` but might be a redundant test)

Fixes #9140.

Author: jrdodds

src/Tasks.UnitTests/XmlPeek_Tests.cs

@@ -3,8 +3,13 @@
 
 using System;
 using System.IO;
+
+using Microsoft.Build.Evaluation;
 using Microsoft.Build.Tasks;
 using Microsoft.Build.Utilities;
+
+using Shouldly;
+
 using Xunit;
 
 #nullable disable
@@ -316,6 +321,17 @@ public void PeekWithoutUsingTask()
             logger.AssertLogDoesntContain("MSB4036");
         }
 
+        [Fact]
+        public void PeekWithNoParameters()
+        {
+            MockLogger log = new();
+            Project project = ObjectModelHelpers.CreateInMemoryProject(@"<Project><Target Name=""Test""><XmlPeek /></Target></Project>", log);
+
+            project.Build().ShouldBeFalse();
+            log.AssertLogContains("MSB4044");
+            log.AssertLogContains("\"Query\"");
+        }
+
         private void Prepare(string xmlFile, out string xmlInputPath)
         {
             string dir = Path.Combine(Path.GetTempPath(), DateTime.Now.Ticks.ToString());

src/Tasks.UnitTests/XmlPoke_Tests.cs

@@ -6,9 +6,13 @@
 using System.IO;
 using System.Linq;
 using System.Xml;
+
+using Microsoft.Build.Evaluation;
 using Microsoft.Build.Tasks;
 using Microsoft.Build.Utilities;
+
 using Shouldly;
+
 using Xunit;
 
 #nullable disable
@@ -135,37 +139,55 @@ public void PokeAttributeWithCondition()
         }
 
         [Fact]
-        public void PokeMissingParams()
+        public void PokeWithNoParameters()
         {
-            MockEngine engine = new MockEngine(true);
-            string xmlInputPath;
-            Prepare(_xmlFileNoNs, out xmlInputPath);
+            MockLogger log = new();
+            Project project = ObjectModelHelpers.CreateInMemoryProject(@"<Project><Target Name=""Test""><XmlPoke /></Target></Project>", log);
 
-            for (int i = 0; i < 4; i++)
-            {
-                XmlPoke p = new XmlPoke();
-                p.BuildEngine = engine;
+            project.Build().ShouldBeFalse();
+            log.AssertLogContains("MSB4044");
+        }
 
-                if ((i & 1) == 1)
-                {
-                    p.XmlInputPath = new TaskItem(xmlInputPath);
-                }
+        [Fact]
+        public void PokeWithMissingRequiredQuery()
+        {
+            const string projectContent = @"<Project><Target Name=""Test""><XmlPoke XmlInputPath=""nonesuch"" /></Target></Project>";
 
-                if ((i & 2) == 2)
-                {
-                    p.Query = "//variable/@Name";
-                }
+            MockLogger log = new();
+            Project project = ObjectModelHelpers.CreateInMemoryProject(projectContent, log);
 
-                // "Expecting argumentnullexception for the first 3 tests"
-                if (i < 3)
-                {
-                    Should.Throw<ArgumentNullException>(() => p.Execute());
-                }
-                else
-                {
-                    Should.NotThrow(() => p.Execute());
-                }
-            }
+            project.Build().ShouldBeFalse();
+            log.AssertLogContains("MSB4044");
+            log.AssertLogContains("\"Query\"");
+        }
+
+        [Fact]
+        public void PokeWithMissingRequiredXmlInputPath()
+        {
+            const string projectContent = @"<Project><Target Name=""Test""><XmlPoke Query=""nonesuch"" /></Target></Project>";
+
+            MockLogger log = new();
+            Project project = ObjectModelHelpers.CreateInMemoryProject(projectContent, log);
+
+            project.Build().ShouldBeFalse();
+            log.AssertLogContains("MSB4044");
+            log.AssertLogContains("\"XmlInputPath\"");
+        }
+
+        [Fact]
+        public void PokeWithRequiredParameters()
+        {
+            MockEngine engine = new(true);
+            Prepare(_xmlFileNoNs, out string xmlInputPath);
+
+            XmlPoke task = new()
+            {
+                BuildEngine = engine,
+                XmlInputPath = new TaskItem(xmlInputPath),
+                Query = "//variable/@Name",
+            };
+
+            task.Execute().ShouldBeTrue();
         }
 
         [Fact]

src/Tasks/XmlPeek.cs

@@ -21,19 +21,11 @@ namespace Microsoft.Build.Tasks
     /// </summary>
     public class XmlPeek : TaskExtension
     {
-        #region Members
+        #region Properties
 
         /// <summary>
         /// The XPath Query.
         /// </summary>
-        private string _query;
-
-        #endregion
-
-        #region Properties
-        /// <summary>
-        /// The XML input as a file path.
-        /// </summary>
         public ITaskItem XmlInputPath { get; set; }
 
         /// <summary>
@@ -44,16 +36,8 @@ public class XmlPeek : TaskExtension
         /// <summary>
         /// The XPath Query.
         /// </summary>
-        public string Query
-        {
-            get
-            {
-                ErrorUtilities.VerifyThrowArgumentNull(_query, "Query");
-                return _query;
-            }
-
-            set => _query = value;
-        }
+        [Required]
+        public string Query { get; set; }
 
         /// <summary>
         /// The results returned by this task.
@@ -71,6 +55,7 @@ public string Query
         /// if DTD is present. This was a pre-v15 behavior. By default, a DTD clause if any is ignored.
         /// </summary>
         public bool ProhibitDtd { get; set; }
+
         #endregion
 
         /// <summary>
@@ -80,8 +65,6 @@ public string Query
         public override bool Execute()
         {
             XmlInput xmlinput;
-            ErrorUtilities.VerifyThrowArgumentNull(_query, nameof(Query));
-
             try
             {
                 xmlinput = new XmlInput(XmlInputPath, XmlContent);
@@ -99,7 +82,6 @@ public override bool Execute()
                 using (XmlReader xr = xmlinput.CreateReader(ProhibitDtd))
                 {
                     xpathdoc = new XPathDocument(xr);
-                    xr.Dispose();
                 }
             }
             catch (Exception e) when (!ExceptionHandling.IsCriticalException(e))
@@ -117,11 +99,11 @@ public override bool Execute()
             try
             {
                 // Create the expression from query
-                expr = nav.Compile(_query);
+                expr = nav.Compile(Query);
             }
             catch (Exception e) when (!ExceptionHandling.IsCriticalException(e))
             {
-                Log.LogErrorWithCodeFromResources("XmlPeekPoke.XPathError", _query, e.Message);
+                Log.LogErrorWithCodeFromResources("XmlPeekPoke.XPathError", Query, e.Message);
                 return false;
             }
 
@@ -230,15 +212,15 @@ private static void LoadNamespaces(ref XmlNamespaceManager namespaceManager, str
         /// <summary>
         /// This class prepares XML input from XMLInputPath and XMLContent parameters
         /// </summary>
-        internal class XmlInput
+        private sealed class XmlInput
         {
             /// <summary>
             /// This either contains the raw Xml or the path to Xml file.
             /// </summary>
             private readonly string _data;
 
             /// <summary>
-            /// Filestream used to read XML.
+            /// FileStream used to read XML.
             /// </summary>
             private FileStream _fs;
 
@@ -254,7 +236,8 @@ public XmlInput(ITaskItem xmlInputPath, string xmlContent)
                 {
                     throw new ArgumentException(ResourceUtilities.GetResourceString("XmlPeek.XmlInput.TooMany"));
                 }
-                else if (xmlInputPath == null && xmlContent == null)
+
+                if (xmlInputPath == null && xmlContent == null)
                 {
                     throw new ArgumentException(ResourceUtilities.GetResourceString("XmlPeek.XmlInput.TooFew"));
                 }
@@ -274,7 +257,7 @@ public XmlInput(ITaskItem xmlInputPath, string xmlContent)
             /// <summary>
             /// Possible accepted types of XML input.
             /// </summary>
-            public enum XmlModes
+            private enum XmlModes
             {
                 /// <summary>
                 /// If the mode is a XML file.
@@ -290,7 +273,7 @@ public enum XmlModes
             /// <summary>
             /// Returns the current mode of the XmlInput
             /// </summary>
-            public XmlModes XmlMode { get; }
+            private XmlModes XmlMode { get; }
 
             /// <summary>
             /// Creates correct reader based on the input type.

src/Tasks/XmlPoke.cs

@@ -20,51 +20,23 @@ namespace Microsoft.Build.Tasks
     /// </summary>
     public class XmlPoke : TaskExtension
     {
-        #region Members
-        /// <summary>
-        /// The XML input as file paths.
-        /// </summary>
-        private ITaskItem _xmlInputPath;
-
-        /// <summary>
-        /// The XPath Query.
-        /// </summary>
-        private string _query;
-
-        #endregion
-
         #region Properties
+
         /// <summary>
         /// The XML input as file path.
         /// </summary>
-        public ITaskItem XmlInputPath
-        {
-            get
-            {
-                ErrorUtilities.VerifyThrowArgumentNull(_xmlInputPath, nameof(XmlInputPath));
-                return _xmlInputPath;
-            }
-
-            set => _xmlInputPath = value;
-        }
+        [Required]
+        public ITaskItem XmlInputPath { get; set; }
 
         /// <summary>
         /// The XPath Query.
         /// </summary>
-        public string Query
-        {
-            get
-            {
-                ErrorUtilities.VerifyThrowArgumentNull(_query, nameof(Query));
-                return _query;
-            }
-
-            set => _query = value;
-        }
+        [Required]
+        public string Query { get; set; }
 
         /// <summary>
         /// The value to be inserted into the specified location.
-        /// </summary>        
+        /// </summary>
         public ITaskItem Value { get; set; }
 
         /// <summary>
@@ -77,23 +49,21 @@ public string Query
         /// <summary>
         /// Executes the XMLPoke task.
         /// </summary>
-        /// <returns>true if transformation succeeds.</returns>
+        /// <returns>true if task execution succeeds.</returns>
         public override bool Execute()
         {
-            ErrorUtilities.VerifyThrowArgumentNull(_query, "Query");
-            ErrorUtilities.VerifyThrowArgumentNull(_xmlInputPath, "XmlInputPath");
             if (Value == null)
             {
                 // When Value is null, it means Value is not set or empty. Here we treat them all as empty.
-                Value = new TaskItem(String.Empty);
+                Value = new TaskItem(string.Empty);
             }
 
             // Load the XPath Document
             XmlDocument xmlDoc = new XmlDocument();
 
             try
             {
-                using (FileStream fs = new FileStream(_xmlInputPath.ItemSpec, FileMode.Open, FileAccess.Read, FileShare.ReadWrite))
+                using (FileStream fs = new FileStream(XmlInputPath.ItemSpec, FileMode.Open, FileAccess.Read, FileShare.ReadWrite))
                 {
                     XmlReaderSettings xrs = new XmlReaderSettings { DtdProcessing = DtdProcessing.Ignore };
                     using (XmlReader sr = XmlReader.Create(fs, xrs))
@@ -104,7 +74,7 @@ public override bool Execute()
             }
             catch (Exception e) when (!ExceptionHandling.IsCriticalException(e))
             {
-                Log.LogErrorWithCodeFromResources("XmlPeekPoke.InputFileError", _xmlInputPath.ItemSpec, e.Message);
+                Log.LogErrorWithCodeFromResources("XmlPeekPoke.InputFileError", XmlInputPath.ItemSpec, e.Message);
                 return false;
             }
 
@@ -114,11 +84,11 @@ public override bool Execute()
             try
             {
                 // Create the expression from query
-                expr = nav.Compile(_query);
+                expr = nav.Compile(Query);
             }
             catch (Exception e) when (!ExceptionHandling.IsCriticalException(e))
             {
-                Log.LogErrorWithCodeFromResources("XmlPeekPoke.XPathError", _query, e.Message);
+                Log.LogErrorWithCodeFromResources("XmlPeekPoke.XPathError", Query, e.Message);
                 return false;
             }
 
@@ -169,12 +139,12 @@ public override bool Execute()
             if (count > 0)
             {
 #if RUNTIME_TYPE_NETCORE
-                using (Stream stream = File.Create(_xmlInputPath.ItemSpec))
+                using (Stream stream = File.Create(XmlInputPath.ItemSpec))
                 {
                     xmlDoc.Save(stream);
                 }
 #else
-                xmlDoc.Save(_xmlInputPath.ItemSpec);
+                xmlDoc.Save(XmlInputPath.ItemSpec);
 #endif
             }