[1742] Ensure multiple composite annoations are correctly written. (#2311)

* [1742] Ensure multiple composite annoations are correctly written.

This implementation uses a collector pattern to allow embedded ExecutionContexts to stash Issue objects for later processing by a non-embedded ancestor ExecutionContext.

Also:
 - Provide explicit constructor implementations for ExecutionContext
 - Leverage explicit constructors to solidify immutability of several ExecutionContext class members.
 - Fixed erroneous call to ExecutionContext.Complete in CompositeActionHandler.cs
 - Use a consistent timestamp for FinishTime in ExecutionContext::Complete

* Ensure collected issues are processed only by a non-embedded ExecutionContext.
This was already implicit.  Now, just making it explicit.

* Provide a clear mechanism that allows callers to opt-in/opt-out of ExecutionContext::AddIssue's logging behavior.
* Addressed deserialization inconsistencies in TimelineRecord.cs
* Added TimelineRecord unit tests.
* Refined unit tests related to TimelineRecord::Variables case-insensitivity
* Add a unit test that verifies ExecutionContextLogOptions::LogMessageOverride has the desired effect.
* Responded to PR feedback.
* Don't allow embedded ExecutionContexts to add Issues to a TimelineRecord

src/Sdk/DTWebApi/WebApi/TimelineRecord.cs

@@ -1,8 +1,7 @@
-using System;
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Runtime.Serialization;
-using GitHub.Services.Common;
 
 namespace GitHub.DistributedTask.WebApi
 {
@@ -10,69 +9,78 @@ namespace GitHub.DistributedTask.WebApi
     public sealed class TimelineRecord
     {
         public TimelineRecord()
+            : this(null)
         {
-            this.Attempt = 1;
         }
 
         private TimelineRecord(TimelineRecord recordToBeCloned)
         {
-            this.Attempt = recordToBeCloned.Attempt;
-            this.ChangeId = recordToBeCloned.ChangeId;
-            this.CurrentOperation = recordToBeCloned.CurrentOperation;
-            this.FinishTime = recordToBeCloned.FinishTime;
-            this.Id = recordToBeCloned.Id;
-            this.Identifier = recordToBeCloned.Identifier;
-            this.LastModified = recordToBeCloned.LastModified;
-            this.Location = recordToBeCloned.Location;
-            this.Name = recordToBeCloned.Name;
-            this.Order = recordToBeCloned.Order;
-            this.ParentId = recordToBeCloned.ParentId;
-            this.PercentComplete = recordToBeCloned.PercentComplete;
-            this.RecordType = recordToBeCloned.RecordType;
-            this.Result = recordToBeCloned.Result;
-            this.ResultCode = recordToBeCloned.ResultCode;
-            this.StartTime = recordToBeCloned.StartTime;
-            this.State = recordToBeCloned.State;
-            this.TimelineId = recordToBeCloned.TimelineId;
-            this.WorkerName = recordToBeCloned.WorkerName;
-            this.RefName = recordToBeCloned.RefName;
-            this.ErrorCount = recordToBeCloned.ErrorCount;
-            this.WarningCount = recordToBeCloned.WarningCount;
-            this.NoticeCount = recordToBeCloned.NoticeCount;
-            this.AgentPlatform = recordToBeCloned.AgentPlatform;
+            this.EnsureInitialized();
 
-            if (recordToBeCloned.Log != null)
+            if (recordToBeCloned != null)
             {
-                this.Log = new TaskLogReference
+                this.Attempt = recordToBeCloned.Attempt;
+                this.ChangeId = recordToBeCloned.ChangeId;
+                this.CurrentOperation = recordToBeCloned.CurrentOperation;
+                this.FinishTime = recordToBeCloned.FinishTime;
+                this.Id = recordToBeCloned.Id;
+                this.Identifier = recordToBeCloned.Identifier;
+                this.LastModified = recordToBeCloned.LastModified;
+                this.Location = recordToBeCloned.Location;
+                this.Name = recordToBeCloned.Name;
+                this.Order = recordToBeCloned.Order;
+                this.ParentId = recordToBeCloned.ParentId;
+                this.PercentComplete = recordToBeCloned.PercentComplete;
+                this.RecordType = recordToBeCloned.RecordType;
+                this.Result = recordToBeCloned.Result;
+                this.ResultCode = recordToBeCloned.ResultCode;
+                this.StartTime = recordToBeCloned.StartTime;
+                this.State = recordToBeCloned.State;
+                this.TimelineId = recordToBeCloned.TimelineId;
+                this.WorkerName = recordToBeCloned.WorkerName;
+                this.RefName = recordToBeCloned.RefName;
+                this.ErrorCount = recordToBeCloned.ErrorCount;
+                this.WarningCount = recordToBeCloned.WarningCount;
+                this.NoticeCount = recordToBeCloned.NoticeCount;
+                this.AgentPlatform = recordToBeCloned.AgentPlatform;
+
+                if (recordToBeCloned.Log != null)
                 {
-                    Id = recordToBeCloned.Log.Id,
-                    Location = recordToBeCloned.Log.Location,
-                };
-            }
+                    this.Log = new TaskLogReference
+                    {
+                        Id = recordToBeCloned.Log.Id,
+                        Location = recordToBeCloned.Log.Location,
+                    };
+                }
 
-            if (recordToBeCloned.Details != null)
-            {
-                this.Details = new TimelineReference
+                if (recordToBeCloned.Details != null)
                 {
-                    ChangeId = recordToBeCloned.Details.ChangeId,
-                    Id = recordToBeCloned.Details.Id,
-                    Location = recordToBeCloned.Details.Location,
-                };
-            }
+                    this.Details = new TimelineReference
+                    {
+                        ChangeId = recordToBeCloned.Details.ChangeId,
+                        Id = recordToBeCloned.Details.Id,
+                        Location = recordToBeCloned.Details.Location,
+                    };
+                }
 
-            if (recordToBeCloned.m_issues?.Count> 0)
-            {
-                this.Issues.AddRange(recordToBeCloned.Issues.Select(i => i.Clone()));
-            }
+                if (recordToBeCloned.m_issues?.Count > 0)
+                {
+                    this.Issues.AddRange(recordToBeCloned.Issues.Select(i => i.Clone()));
+                }
 
-            if (recordToBeCloned.m_previousAttempts?.Count > 0)
-            {
-                this.PreviousAttempts.AddRange(recordToBeCloned.PreviousAttempts);
-            }
+                if (recordToBeCloned.m_previousAttempts?.Count > 0)
+                {
+                    this.m_previousAttempts.AddRange(recordToBeCloned.m_previousAttempts);
+                }
 
-            if (recordToBeCloned.m_variables?.Count > 0)
-            {
-                this.m_variables = recordToBeCloned.Variables.ToDictionary(k => k.Key, v => v.Value.Clone());
+                if (recordToBeCloned.m_variables?.Count > 0)
+                {
+                    // Don't pave over the case-insensitive Dictionary we initialized above.
+                    foreach (var kvp in recordToBeCloned.m_variables)
+                    {
+                        m_variables[kvp.Key] = kvp.Value.Clone();
+                    }
+                }
             }
         }
 
@@ -98,14 +106,14 @@ namespace GitHub.DistributedTask.WebApi
         }
 
         [DataMember(Name = "Type", Order = 3)]
-        public String RecordType
+        public string RecordType
         {
             get;
             set;
         }
 
         [DataMember(Order = 4)]
-        public String Name
+        public string Name
         {
             get;
             set;
@@ -126,7 +134,7 @@ namespace GitHub.DistributedTask.WebApi
         }
 
         [DataMember(Order = 7)]
-        public String CurrentOperation
+        public string CurrentOperation
         {
             get;
             set;
@@ -154,7 +162,7 @@ namespace GitHub.DistributedTask.WebApi
         }
 
         [DataMember(Order = 11)]
-        public String ResultCode
+        public string ResultCode
         {
             get;
             set;
@@ -175,7 +183,7 @@ namespace GitHub.DistributedTask.WebApi
         }
 
         [DataMember(Order = 14)]
-        public String WorkerName
+        public string WorkerName
         {
             get;
             set;
@@ -189,7 +197,7 @@ namespace GitHub.DistributedTask.WebApi
         }
 
         [DataMember(Order = 16, EmitDefaultValue = false)]
-        public String RefName
+        public string RefName
         {
             get;
             set;
@@ -209,35 +217,46 @@ namespace GitHub.DistributedTask.WebApi
             set;
         }
 
-        [DataMember(Order = 40)]
-        public Int32? ErrorCount
+        public Int32 ErrorCount
         {
-            get;
-            set;
+            get
+            {
+                return m_errorCount.GetValueOrDefault(0);
+            }
+            set
+            {
+                m_errorCount = value;
+            }
         }
 
-        [DataMember(Order = 50)]
-        public Int32? WarningCount
+        public Int32 WarningCount
         {
-            get;
-            set;
+            get
+            {
+                return m_warningCount.GetValueOrDefault(0);
+            }
+            set
+            {
+                m_warningCount = value;
+            }
         }
 
-        [DataMember(Order = 55)]
-        public Int32? NoticeCount
+        public Int32 NoticeCount
         {
-            get;
-            set;
+            get
+            {
+                return m_noticeCount.GetValueOrDefault(0);
+            }
+            set
+            {
+                m_noticeCount = value;
+            }
         }
 
         public List<Issue> Issues
         {
             get
             {
-                if (m_issues == null)
-                {
-                    m_issues = new List<Issue>();
-                }
                 return m_issues;
             }
         }
@@ -257,7 +276,7 @@ namespace GitHub.DistributedTask.WebApi
         }
 
         [DataMember(Order = 131)]
-        public String Identifier
+        public string Identifier
         {
             get;
             set;
@@ -274,22 +293,14 @@ namespace GitHub.DistributedTask.WebApi
         {
             get
             {
-                if (m_previousAttempts == null)
-                {
-                    m_previousAttempts = new List<TimelineAttempt>();
-                }
                 return m_previousAttempts;
             }
         }
 
-        public IDictionary<String, VariableValue> Variables
+        public IDictionary<string, VariableValue> Variables
         {
             get
             {
-                if (m_variables == null)
-                {
-                    m_variables = new Dictionary<String, VariableValue>(StringComparer.OrdinalIgnoreCase);
-                }
                 return m_variables;
             }
         }
@@ -299,13 +310,53 @@ namespace GitHub.DistributedTask.WebApi
             return new TimelineRecord(this);
         }
 
-        [DataMember(Name = "Issues", EmitDefaultValue = false, Order = 60)]
+        [OnDeserialized]
+        private void OnDeserialized(StreamingContext context)
+        {
+            this.EnsureInitialized();
+        }
+
+
+        /// <summary>
+        /// DataContractSerializer bypasses all constructor logic and inline initialization!
+        /// This method takes the place of a workhorse constructor for baseline initialization.
+        /// The expectation is for this logic to be accessible to constructors and also to the OnDeserialized helper.
+        /// </summary>
+        private void EnsureInitialized()
+        {
+            // Note that ?? is a short-circuiting operator.  (??= would be preferable, but it's not supported in the .NET Framework version currently used by actions/runner.)
+
+            // De-nullify the following historically-nullable ints.
+            // (After several weeks in production, it may be possible to eliminate these nullable backing fields.)
+            m_errorCount = m_errorCount ?? 0;
+            m_warningCount = m_warningCount ?? 0;
+            m_noticeCount = m_noticeCount ?? 0;
+
+            m_issues = m_issues ?? new List<Issue>();
+            m_previousAttempts = m_previousAttempts ?? new List<TimelineAttempt>();
+            this.Attempt = Math.Max(this.Attempt, 1);
+
+            // Ensure whatever content may have been deserialized for m_variables is backed by a case-insensitive Dictionary.
+            var empty = Enumerable.Empty<KeyValuePair<string, VariableValue>>();
+            m_variables = new Dictionary<string, VariableValue>(m_variables ?? empty, StringComparer.OrdinalIgnoreCase);
+        }
+
+        [DataMember(Name = nameof(ErrorCount), Order = 40)]
+        private Int32? m_errorCount;
+
+        [DataMember(Name = nameof(WarningCount), Order = 50)]
+        private Int32? m_warningCount;
+
+        [DataMember(Name = nameof(NoticeCount), Order = 55)]
+        private Int32? m_noticeCount;
+
+        [DataMember(Name = nameof(Issues), EmitDefaultValue = false, Order = 60)]
         private List<Issue> m_issues;
 
-        [DataMember(Name = "Variables", EmitDefaultValue = false, Order = 80)]
-        private Dictionary<String, VariableValue> m_variables;
+        [DataMember(Name = nameof(Variables), EmitDefaultValue = false, Order = 80)]
+        private Dictionary<string, VariableValue> m_variables;
 
-        [DataMember(Name = "PreviousAttempts", EmitDefaultValue = false, Order = 120)]
+        [DataMember(Name = nameof(PreviousAttempts), EmitDefaultValue = false, Order = 120)]
         private List<TimelineAttempt> m_previousAttempts;
     }
 }