From a6e7e1dc0bb19cec55a510665fc07be8fcb2124e Mon Sep 17 00:00:00 2001 From: Staffan Gustafsson Date: Wed, 8 Aug 2018 11:53:26 +0200 Subject: [PATCH 1/9] Code cleanup and simplification Using pattern matching to simplify code. Removing some redundant initializations. nameof(input) instead of "input" in exception. Simplification of assignment Removing code redundancies --- .../commands/utility/WebCmdlet/JsonObject.cs | 61 ++++++------------- 1 file changed, 18 insertions(+), 43 deletions(-) diff --git a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs index 37ec46f92f4..f0c09fb09d4 100644 --- a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs +++ b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs @@ -10,11 +10,6 @@ using Newtonsoft.Json; using Newtonsoft.Json.Linq; using System.Collections; -using System.Collections.ObjectModel; -using System.IO; -using System.Linq; -using System.Management.Automation.Internal; -using System.Reflection; namespace Microsoft.PowerShell.Commands { @@ -50,11 +45,11 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err { if (input == null) { - throw new ArgumentNullException("input"); + throw new ArgumentNullException(nameof(input)); } error = null; - object obj = null; + object obj; try { // JsonConvert.DeserializeObject does not throw an exception when an invalid Json array is passed. @@ -83,32 +78,20 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err }); // JObject is a IDictionary - var dictionary = obj as JObject; - if (dictionary != null) + if (obj is JObject dictionary) { - if (returnHashTable) - { - obj = PopulateHashTableFromJDictionary(dictionary, out error); - } - else - { - obj = PopulateFromJDictionary(dictionary, out error); - } + obj = returnHashTable ? + PopulateHashTableFromJDictionary(dictionary, out error) : + PopulateFromJDictionary(dictionary, out error); } else { // JArray is a collection - var list = obj as JArray; - if (list != null) + if (obj is JArray list) { - if (returnHashTable) - { - obj = PopulateHashTableFromJArray(list, out error); - } - else - { - obj = PopulateFromJArray(list, out error); - } + obj = returnHashTable ? + PopulateHashTableFromJArray(list, out error) : + PopulateFromJArray(list, out error); } } } @@ -170,9 +153,8 @@ private static PSObject PopulateFromJDictionary(JObject entries, out ErrorRecord } // Array - else if (entry.Value is JArray) + if (entry.Value is JArray list) { - JArray list = entry.Value as JArray; ICollection listResult = PopulateFromJArray(list, out error); if (error != null) { @@ -182,9 +164,8 @@ private static PSObject PopulateFromJDictionary(JObject entries, out ErrorRecord } // Dictionary - else if (entry.Value is JObject) + else if (entry.Value is JObject dic) { - JObject dic = entry.Value as JObject; PSObject dicResult = PopulateFromJDictionary(dic, out error); if (error != null) { @@ -212,9 +193,8 @@ private static ICollection PopulateFromJArray(JArray list, out ErrorReco foreach (var element in list) { // Array - if (element is JArray) + if (element is JArray subList) { - JArray subList = element as JArray; ICollection listResult = PopulateFromJArray(subList, out error); if (error != null) { @@ -224,9 +204,8 @@ private static ICollection PopulateFromJArray(JArray list, out ErrorReco } // Dictionary - else if (element is JObject) + else if (element is JObject dic) { - JObject dic = element as JObject; PSObject dicResult = PopulateFromJDictionary(dic, out error); if (error != null) { @@ -266,9 +245,8 @@ private static Hashtable PopulateHashTableFromJDictionary(JObject entries, out E } // Array - else if (entry.Value is JArray) + if (entry.Value is JArray list) { - JArray list = entry.Value as JArray; ICollection listResult = PopulateHashTableFromJArray(list, out error); if (error != null) { @@ -278,9 +256,8 @@ private static Hashtable PopulateHashTableFromJDictionary(JObject entries, out E } // Dictionary - else if (entry.Value is JObject) + else if (entry.Value is JObject dic) { - JObject dic = entry.Value as JObject; Hashtable dicResult = PopulateHashTableFromJDictionary(dic, out error); if (error != null) { @@ -308,9 +285,8 @@ private static ICollection PopulateHashTableFromJArray(JArray list, out foreach (var element in list) { // Array - if (element is JArray) + if (element is JArray subList) { - JArray subList = element as JArray; ICollection listResult = PopulateHashTableFromJArray(subList, out error); if (error != null) { @@ -320,9 +296,8 @@ private static ICollection PopulateHashTableFromJArray(JArray list, out } // Dictionary - else if (element is JObject) + else if (element is JObject dic) { - JObject dic = element as JObject; Hashtable dicResult = PopulateHashTableFromJDictionary(dic, out error); if (error != null) { From 44a8bc472f5031331c2c5a4d987a44cff3971098 Mon Sep 17 00:00:00 2001 From: Staffan Gustafsson Date: Wed, 8 Aug 2018 12:04:25 +0200 Subject: [PATCH 2/9] Using array directly instead of list + .ToArray() to reduce allocations. --- .../commands/utility/WebCmdlet/JsonObject.cs | 32 ++++++++++++------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs index f0c09fb09d4..1f61ecc864e 100644 --- a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs +++ b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs @@ -188,10 +188,11 @@ private static PSObject PopulateFromJDictionary(JObject entries, out ErrorRecord private static ICollection PopulateFromJArray(JArray list, out ErrorRecord error) { error = null; - List result = new List(); + var result = new object[list.Count]; - foreach (var element in list) + for (var index = 0; index < list.Count; index++) { + var element = list[index]; // Array if (element is JArray subList) { @@ -200,7 +201,8 @@ private static ICollection PopulateFromJArray(JArray list, out ErrorReco { return null; } - result.Add(listResult); + + result[index] = listResult; } // Dictionary @@ -211,16 +213,18 @@ private static ICollection PopulateFromJArray(JArray list, out ErrorReco { return null; } - result.Add(dicResult); + + result[index] = dicResult; } // Value else // (element is JValue) { - result.Add(((JValue)element).Value); + result[index] = ((JValue)element).Value; } } - return result.ToArray(); + + return result; } // This function is a clone of PopulateFromDictionary using JObject as an input. @@ -280,10 +284,11 @@ private static Hashtable PopulateHashTableFromJDictionary(JObject entries, out E private static ICollection PopulateHashTableFromJArray(JArray list, out ErrorRecord error) { error = null; - List result = new List(); + var result = new object[list.Count]; - foreach (var element in list) + for (var index = 0; index < list.Count; index++) { + var element = list[index]; // Array if (element is JArray subList) { @@ -292,7 +297,8 @@ private static ICollection PopulateHashTableFromJArray(JArray list, out { return null; } - result.Add(listResult); + + result[index] = listResult; } // Dictionary @@ -303,16 +309,18 @@ private static ICollection PopulateHashTableFromJArray(JArray list, out { return null; } - result.Add(dicResult); + + result[index] = dicResult; } // Value else // (element is JValue) { - result.Add(((JValue)element).Value); + result[index] = ((JValue)element).Value; } } - return result.ToArray(); + + return result; } } } From ac4061c5266ebf142ada97e62541e5299cebf1ae Mon Sep 17 00:00:00 2001 From: Staffan Gustafsson Date: Wed, 8 Aug 2018 12:10:45 +0200 Subject: [PATCH 3/9] Use a hashset as a faster method of tracking member duplicates. --- .../commands/utility/WebCmdlet/JsonObject.cs | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs index 1f61ecc864e..82b11961284 100644 --- a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs +++ b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs @@ -19,7 +19,12 @@ namespace Microsoft.PowerShell.Commands [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly")] public static class JsonObject { - private const int maxDepthAllowed = 100; + class DuplicateMemberSet : HashSet + { + public DuplicateMemberSet(int capacity) : base(capacity, StringComparer.CurrentCultureIgnoreCase) + { + } + } /// /// Convert a Json string back to an object of type PSObject. @@ -48,6 +53,7 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err throw new ArgumentNullException(nameof(input)); } + var memberTracker = new DuplicateMemberSet(32); error = null; object obj; try @@ -82,7 +88,7 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err { obj = returnHashTable ? PopulateHashTableFromJDictionary(dictionary, out error) : - PopulateFromJDictionary(dictionary, out error); + PopulateFromJDictionary(dictionary, memberTracker, out error); } else { @@ -105,15 +111,15 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err } // This function is a clone of PopulateFromDictionary using JObject as an input. - private static PSObject PopulateFromJDictionary(JObject entries, out ErrorRecord error) + private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMemberSet memberTracker, out ErrorRecord error) { error = null; - PSObject result = new PSObject(); + var result = new PSObject(entries.Count); foreach (var entry in entries) { if (string.IsNullOrEmpty(entry.Key)) { - string errorMsg = string.Format(CultureInfo.InvariantCulture, + var errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.EmptyKeyInJsonString); error = new ErrorRecord( new InvalidOperationException(errorMsg), @@ -125,9 +131,10 @@ private static PSObject PopulateFromJDictionary(JObject entries, out ErrorRecord // Case sensitive duplicates should normally not occur since JsonConvert.DeserializeObject // does not throw when encountering duplicates and just uses the last entry. - if (result.Properties.Any(psPropertyInfo => psPropertyInfo.Name.Equals(entry.Key, StringComparison.InvariantCulture))) + if (memberTracker.TryGetValue(entry.Key, out var maybePropertyName) + && String.Compare(entry.Key, maybePropertyName, StringComparison.CurrentCulture) == 0) { - string errorMsg = string.Format(CultureInfo.InvariantCulture, + var errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.DuplicateKeysInJsonString, entry.Key); error = new ErrorRecord( new InvalidOperationException(errorMsg), @@ -139,11 +146,10 @@ private static PSObject PopulateFromJDictionary(JObject entries, out ErrorRecord // Compare case insensitive to tell the user to use the -AsHashTable option instead. // This is because PSObject cannot have keys with different casing. - PSPropertyInfo property = result.Properties[entry.Key]; - if (property != null) + if (memberTracker.TryGetValue(entry.Key, out var propertyName)) { - string errorMsg = string.Format(CultureInfo.InvariantCulture, - WebCmdletStrings.KeysWithDifferentCasingInJsonString, property.Name, entry.Key); + var errorMsg = string.Format(CultureInfo.InvariantCulture, + WebCmdletStrings.KeysWithDifferentCasingInJsonString, propertyName, entry.Key); error = new ErrorRecord( new InvalidOperationException(errorMsg), "KeysWithDifferentCasingInJsonString", @@ -166,7 +172,7 @@ private static PSObject PopulateFromJDictionary(JObject entries, out ErrorRecord // Dictionary else if (entry.Value is JObject dic) { - PSObject dicResult = PopulateFromJDictionary(dic, out error); + PSObject dicResult = PopulateFromJDictionary(dic, new DuplicateMemberSet(dic.Count), out error); if (error != null) { return null; @@ -180,7 +186,10 @@ private static PSObject PopulateFromJDictionary(JObject entries, out ErrorRecord JValue theValue = entry.Value as JValue; result.Properties.Add(new PSNoteProperty(entry.Key, theValue.Value)); } + + memberTracker.Add(entry.Key); } + return result; } @@ -208,7 +217,7 @@ private static ICollection PopulateFromJArray(JArray list, out ErrorReco // Dictionary else if (element is JObject dic) { - PSObject dicResult = PopulateFromJDictionary(dic, out error); + PSObject dicResult = PopulateFromJDictionary(dic, new DuplicateMemberSet(dic.Count), out error); if (error != null) { return null; From f565dd8e62f0bed4b0464d02bcaf836f65a78186 Mon Sep 17 00:00:00 2001 From: Staffan Gustafsson Date: Wed, 8 Aug 2018 12:55:53 +0200 Subject: [PATCH 4/9] CodeFactor fixes --- .../commands/utility/WebCmdlet/JsonObject.cs | 90 +++++++++---------- 1 file changed, 44 insertions(+), 46 deletions(-) diff --git a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs index 82b11961284..d29e34c77ef 100644 --- a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs +++ b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs @@ -2,14 +2,14 @@ // Licensed under the MIT License. using System; +using System.Collections; using System.Collections.Generic; -using System.Globalization; using System.Diagnostics.CodeAnalysis; +using System.Globalization; using System.Management.Automation; using System.Text.RegularExpressions; using Newtonsoft.Json; using Newtonsoft.Json.Linq; -using System.Collections; namespace Microsoft.PowerShell.Commands { @@ -19,7 +19,9 @@ namespace Microsoft.PowerShell.Commands [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly")] public static class JsonObject { - class DuplicateMemberSet : HashSet + private const int DefaultMemberSetCapacity = 32; + + private class DuplicateMemberSet : HashSet { public DuplicateMemberSet(int capacity) : base(capacity, StringComparer.CurrentCultureIgnoreCase) { @@ -29,22 +31,22 @@ public DuplicateMemberSet(int capacity) : base(capacity, StringComparer.CurrentC /// /// Convert a Json string back to an object of type PSObject. /// - /// - /// + /// The json text to convert. + /// An error record if the conversion failed. /// A PSObject. [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly")] public static object ConvertFromJson(string input, out ErrorRecord error) { - return ConvertFromJson(input, false, out error); + return ConvertFromJson(input, returnHashTable: false, out error); } /// - /// Convert a Json string back to an object of type PSObject or Hashtable depending on parameter . + /// Convert a Json string back to an object of type PSObject or HashTable depending on parameter . /// - /// - /// - /// - /// A PSObject or a Hashtable if the parameter is true. + /// The json text to convert. + /// True if the result should be returned as a HashTable instead of a PSObject. + /// An error record if the conversion failed. + /// A PSObject or a HashTable if the parameter is true. [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly")] public static object ConvertFromJson(string input, bool returnHashTable, out ErrorRecord error) { @@ -53,7 +55,7 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err throw new ArgumentNullException(nameof(input)); } - var memberTracker = new DuplicateMemberSet(32); + var memberTracker = new DuplicateMemberSet(DefaultMemberSetCapacity); error = null; object obj; try @@ -63,7 +65,7 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err // To work around this, we need to identify when input is a Json array, and then try to parse it via JArray.Parse(). // If input starts with '[' (ignoring white spaces). - if ((Regex.Match(input, @"^\s*\[")).Success) + if (Regex.Match(input, @"^\s*\[").Success) { // JArray.Parse() will throw a JsonException if the array is invalid. // This will be caught by the catch block below, and then throw an @@ -104,9 +106,11 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err catch (JsonException je) { var msg = string.Format(CultureInfo.CurrentCulture, WebCmdletStrings.JsonDeserializationFailed, je.Message); + // the same as JavaScriptSerializer does throw new ArgumentException(msg, je); } + return obj; } @@ -119,8 +123,7 @@ private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMember { if (string.IsNullOrEmpty(entry.Key)) { - var errorMsg = string.Format(CultureInfo.InvariantCulture, - WebCmdletStrings.EmptyKeyInJsonString); + var errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.EmptyKeyInJsonString); error = new ErrorRecord( new InvalidOperationException(errorMsg), "EmptyKeyInJsonString", @@ -132,10 +135,9 @@ private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMember // Case sensitive duplicates should normally not occur since JsonConvert.DeserializeObject // does not throw when encountering duplicates and just uses the last entry. if (memberTracker.TryGetValue(entry.Key, out var maybePropertyName) - && String.Compare(entry.Key, maybePropertyName, StringComparison.CurrentCulture) == 0) + && string.Compare(entry.Key, maybePropertyName, StringComparison.CurrentCulture) == 0) { - var errorMsg = string.Format(CultureInfo.InvariantCulture, - WebCmdletStrings.DuplicateKeysInJsonString, entry.Key); + var errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.DuplicateKeysInJsonString, entry.Key); error = new ErrorRecord( new InvalidOperationException(errorMsg), "DuplicateKeysInJsonString", @@ -148,8 +150,7 @@ private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMember // This is because PSObject cannot have keys with different casing. if (memberTracker.TryGetValue(entry.Key, out var propertyName)) { - var errorMsg = string.Format(CultureInfo.InvariantCulture, - WebCmdletStrings.KeysWithDifferentCasingInJsonString, propertyName, entry.Key); + var errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.KeysWithDifferentCasingInJsonString, propertyName, entry.Key); error = new ErrorRecord( new InvalidOperationException(errorMsg), "KeysWithDifferentCasingInJsonString", @@ -166,24 +167,24 @@ private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMember { return null; } + result.Properties.Add(new PSNoteProperty(entry.Key, listResult)); } - - // Dictionary else if (entry.Value is JObject dic) { + // Dictionary PSObject dicResult = PopulateFromJDictionary(dic, new DuplicateMemberSet(dic.Count), out error); if (error != null) { return null; } + result.Properties.Add(new PSNoteProperty(entry.Key, dicResult)); } - - // Value - else // (entry.Value is JValue) + else { - JValue theValue = entry.Value as JValue; + // Value + JValue theValue = (JValue)entry.Value; result.Properties.Add(new PSNoteProperty(entry.Key, theValue.Value)); } @@ -202,9 +203,9 @@ private static ICollection PopulateFromJArray(JArray list, out ErrorReco for (var index = 0; index < list.Count; index++) { var element = list[index]; - // Array if (element is JArray subList) { + // Array ICollection listResult = PopulateFromJArray(subList, out error); if (error != null) { @@ -213,10 +214,9 @@ private static ICollection PopulateFromJArray(JArray list, out ErrorReco result[index] = listResult; } - - // Dictionary else if (element is JObject dic) { + // Dictionary PSObject dicResult = PopulateFromJDictionary(dic, new DuplicateMemberSet(dic.Count), out error); if (error != null) { @@ -225,10 +225,9 @@ private static ICollection PopulateFromJArray(JArray list, out ErrorReco result[index] = dicResult; } - - // Value - else // (element is JValue) + else { + // Value result[index] = ((JValue)element).Value; } } @@ -247,8 +246,7 @@ private static Hashtable PopulateHashTableFromJDictionary(JObject entries, out E // does not throw when encountering duplicates and just uses the last entry. if (result.ContainsKey(entry.Key)) { - string errorMsg = string.Format(CultureInfo.InvariantCulture, - WebCmdletStrings.DuplicateKeysInJsonString, entry.Key); + string errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.DuplicateKeysInJsonString, entry.Key); error = new ErrorRecord( new InvalidOperationException(errorMsg), "DuplicateKeysInJsonString", @@ -257,35 +255,36 @@ private static Hashtable PopulateHashTableFromJDictionary(JObject entries, out E return null; } - // Array if (entry.Value is JArray list) { + // Array ICollection listResult = PopulateHashTableFromJArray(list, out error); if (error != null) { return null; } + result.Add(entry.Key, listResult); } - - // Dictionary else if (entry.Value is JObject dic) { + // Dictionary Hashtable dicResult = PopulateHashTableFromJDictionary(dic, out error); if (error != null) { return null; } + result.Add(entry.Key, dicResult); } - - // Value - else // (entry.Value is JValue) + else { + // Value JValue theValue = entry.Value as JValue; result.Add(entry.Key, theValue.Value); } } + return result; } @@ -298,9 +297,10 @@ private static ICollection PopulateHashTableFromJArray(JArray list, out for (var index = 0; index < list.Count; index++) { var element = list[index]; - // Array + if (element is JArray subList) { + // Array ICollection listResult = PopulateHashTableFromJArray(subList, out error); if (error != null) { @@ -309,10 +309,9 @@ private static ICollection PopulateHashTableFromJArray(JArray list, out result[index] = listResult; } - - // Dictionary else if (element is JObject dic) { + // Dictionary Hashtable dicResult = PopulateHashTableFromJDictionary(dic, out error); if (error != null) { @@ -321,10 +320,9 @@ private static ICollection PopulateHashTableFromJArray(JArray list, out result[index] = dicResult; } - - // Value - else // (element is JValue) + else { + // Value result[index] = ((JValue)element).Value; } } From 58ef7ccc270f0e9a6f43327273ffbc22e0f6897b Mon Sep 17 00:00:00 2001 From: Staffan Gustafsson Date: Wed, 8 Aug 2018 20:04:08 +0200 Subject: [PATCH 5/9] Addressing dongbo's comments. --- .../commands/utility/WebCmdlet/JsonObject.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs index d29e34c77ef..206ed123fd0 100644 --- a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs +++ b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs @@ -19,8 +19,6 @@ namespace Microsoft.PowerShell.Commands [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly")] public static class JsonObject { - private const int DefaultMemberSetCapacity = 32; - private class DuplicateMemberSet : HashSet { public DuplicateMemberSet(int capacity) : base(capacity, StringComparer.CurrentCultureIgnoreCase) @@ -55,7 +53,6 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err throw new ArgumentNullException(nameof(input)); } - var memberTracker = new DuplicateMemberSet(DefaultMemberSetCapacity); error = null; object obj; try @@ -90,7 +87,7 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err { obj = returnHashTable ? PopulateHashTableFromJDictionary(dictionary, out error) : - PopulateFromJDictionary(dictionary, memberTracker, out error); + PopulateFromJDictionary(dictionary, new DuplicateMemberSet(dictionary.Count), out error); } else { @@ -123,7 +120,7 @@ private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMember { if (string.IsNullOrEmpty(entry.Key)) { - var errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.EmptyKeyInJsonString); + var errorMsg = string.Format(CultureInfo.CurrentCulture, WebCmdletStrings.EmptyKeyInJsonString); error = new ErrorRecord( new InvalidOperationException(errorMsg), "EmptyKeyInJsonString", @@ -137,7 +134,7 @@ private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMember if (memberTracker.TryGetValue(entry.Key, out var maybePropertyName) && string.Compare(entry.Key, maybePropertyName, StringComparison.CurrentCulture) == 0) { - var errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.DuplicateKeysInJsonString, entry.Key); + var errorMsg = string.Format(CultureInfo.CurrentCulture, WebCmdletStrings.DuplicateKeysInJsonString, entry.Key); error = new ErrorRecord( new InvalidOperationException(errorMsg), "DuplicateKeysInJsonString", @@ -150,7 +147,7 @@ private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMember // This is because PSObject cannot have keys with different casing. if (memberTracker.TryGetValue(entry.Key, out var propertyName)) { - var errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.KeysWithDifferentCasingInJsonString, propertyName, entry.Key); + var errorMsg = string.Format(CultureInfo.CurrentCulture, WebCmdletStrings.KeysWithDifferentCasingInJsonString, propertyName, entry.Key); error = new ErrorRecord( new InvalidOperationException(errorMsg), "KeysWithDifferentCasingInJsonString", @@ -246,7 +243,7 @@ private static Hashtable PopulateHashTableFromJDictionary(JObject entries, out E // does not throw when encountering duplicates and just uses the last entry. if (result.ContainsKey(entry.Key)) { - string errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.DuplicateKeysInJsonString, entry.Key); + string errorMsg = string.Format(CultureInfo.CurrentCulture, WebCmdletStrings.DuplicateKeysInJsonString, entry.Key); error = new ErrorRecord( new InvalidOperationException(errorMsg), "DuplicateKeysInJsonString", From 64f33ea8ce5c7ecd9f83108aaa5abde53ee366e5 Mon Sep 17 00:00:00 2001 From: Staffan Gustafsson Date: Thu, 9 Aug 2018 00:48:00 +0200 Subject: [PATCH 6/9] addressing dantraMSFT comments - part 2 --- .../commands/utility/WebCmdlet/JsonObject.cs | 31 +++++++------------ 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs index 206ed123fd0..e4bfc4c90ef 100644 --- a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs +++ b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs @@ -54,7 +54,6 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err } error = null; - object obj; try { // JsonConvert.DeserializeObject does not throw an exception when an invalid Json array is passed. @@ -73,7 +72,7 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err // we just continue the deserialization. } - obj = JsonConvert.DeserializeObject( + var obj = JsonConvert.DeserializeObject( input, new JsonSerializerSettings { @@ -82,22 +81,18 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err MaxDepth = 1024 }); - // JObject is a IDictionary - if (obj is JObject dictionary) + switch (obj) { - obj = returnHashTable ? - PopulateHashTableFromJDictionary(dictionary, out error) : - PopulateFromJDictionary(dictionary, new DuplicateMemberSet(dictionary.Count), out error); - } - else - { - // JArray is a collection - if (obj is JArray list) - { - obj = returnHashTable ? - PopulateHashTableFromJArray(list, out error) : - PopulateFromJArray(list, out error); - } + case JObject dictionary: + // JObject is a IDictionary + return returnHashTable + ? PopulateHashTableFromJDictionary(dictionary, out error) + : PopulateFromJDictionary(dictionary, new DuplicateMemberSet(dictionary.Count), out error); + case JArray list: + return returnHashTable + ? PopulateHashTableFromJArray(list, out error) + : PopulateFromJArray(list, out error); + default: return obj; } } catch (JsonException je) @@ -107,8 +102,6 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err // the same as JavaScriptSerializer does throw new ArgumentException(msg, je); } - - return obj; } // This function is a clone of PopulateFromDictionary using JObject as an input. From bb15645c853e95650aff7551a3e0592e8b8c51a8 Mon Sep 17 00:00:00 2001 From: Staffan Gustafsson Date: Fri, 10 Aug 2018 12:07:13 +0200 Subject: [PATCH 7/9] Addressing review comments Using OrdinalIgnoreCase for DuplicateMemberHashSet. Renaming DuplicateMemberSet -> DuplicateMemberHashSet Renaming returnHashTable -> returnHashtable Changing help to not use the HashTable casing. --- .../commands/utility/WebCmdlet/JsonObject.cs | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs index e4bfc4c90ef..813a292b09b 100644 --- a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs +++ b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs @@ -19,9 +19,9 @@ namespace Microsoft.PowerShell.Commands [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly")] public static class JsonObject { - private class DuplicateMemberSet : HashSet + private class DuplicateMemberHashSet : HashSet { - public DuplicateMemberSet(int capacity) : base(capacity, StringComparer.CurrentCultureIgnoreCase) + public DuplicateMemberHashSet(int capacity) : base(capacity, StringComparer.OrdinalIgnoreCase) { } } @@ -35,18 +35,21 @@ public DuplicateMemberSet(int capacity) : base(capacity, StringComparer.CurrentC [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly")] public static object ConvertFromJson(string input, out ErrorRecord error) { - return ConvertFromJson(input, returnHashTable: false, out error); + return ConvertFromJson(input, returnHashtable: false, out error); } /// - /// Convert a Json string back to an object of type PSObject or HashTable depending on parameter . + /// Convert a Json string back to an object of type or + /// depending on parameter . /// /// The json text to convert. - /// True if the result should be returned as a HashTable instead of a PSObject. + /// True if the result should be returned as a + /// instead of a . /// An error record if the conversion failed. - /// A PSObject or a HashTable if the parameter is true. + /// A or a + /// if the parameter is true. [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly")] - public static object ConvertFromJson(string input, bool returnHashTable, out ErrorRecord error) + public static object ConvertFromJson(string input, bool returnHashtable, out ErrorRecord error) { if (input == null) { @@ -85,11 +88,11 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err { case JObject dictionary: // JObject is a IDictionary - return returnHashTable + return returnHashtable ? PopulateHashTableFromJDictionary(dictionary, out error) - : PopulateFromJDictionary(dictionary, new DuplicateMemberSet(dictionary.Count), out error); + : PopulateFromJDictionary(dictionary, new DuplicateMemberHashSet(dictionary.Count), out error); case JArray list: - return returnHashTable + return returnHashtable ? PopulateHashTableFromJArray(list, out error) : PopulateFromJArray(list, out error); default: return obj; @@ -105,7 +108,7 @@ public static object ConvertFromJson(string input, bool returnHashTable, out Err } // This function is a clone of PopulateFromDictionary using JObject as an input. - private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMemberSet memberTracker, out ErrorRecord error) + private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMemberHashSet memberHashTracker, out ErrorRecord error) { error = null; var result = new PSObject(entries.Count); @@ -124,7 +127,7 @@ private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMember // Case sensitive duplicates should normally not occur since JsonConvert.DeserializeObject // does not throw when encountering duplicates and just uses the last entry. - if (memberTracker.TryGetValue(entry.Key, out var maybePropertyName) + if (memberHashTracker.TryGetValue(entry.Key, out var maybePropertyName) && string.Compare(entry.Key, maybePropertyName, StringComparison.CurrentCulture) == 0) { var errorMsg = string.Format(CultureInfo.CurrentCulture, WebCmdletStrings.DuplicateKeysInJsonString, entry.Key); @@ -138,7 +141,7 @@ private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMember // Compare case insensitive to tell the user to use the -AsHashTable option instead. // This is because PSObject cannot have keys with different casing. - if (memberTracker.TryGetValue(entry.Key, out var propertyName)) + if (memberHashTracker.TryGetValue(entry.Key, out var propertyName)) { var errorMsg = string.Format(CultureInfo.CurrentCulture, WebCmdletStrings.KeysWithDifferentCasingInJsonString, propertyName, entry.Key); error = new ErrorRecord( @@ -163,7 +166,7 @@ private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMember else if (entry.Value is JObject dic) { // Dictionary - PSObject dicResult = PopulateFromJDictionary(dic, new DuplicateMemberSet(dic.Count), out error); + PSObject dicResult = PopulateFromJDictionary(dic, new DuplicateMemberHashSet(dic.Count), out error); if (error != null) { return null; @@ -178,7 +181,7 @@ private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMember result.Properties.Add(new PSNoteProperty(entry.Key, theValue.Value)); } - memberTracker.Add(entry.Key); + memberHashTracker.Add(entry.Key); } return result; @@ -207,7 +210,7 @@ private static ICollection PopulateFromJArray(JArray list, out ErrorReco else if (element is JObject dic) { // Dictionary - PSObject dicResult = PopulateFromJDictionary(dic, new DuplicateMemberSet(dic.Count), out error); + PSObject dicResult = PopulateFromJDictionary(dic, new DuplicateMemberHashSet(dic.Count), out error); if (error != null) { return null; From d345979ade326b27a9602f8d56e9e98bdbf953a0 Mon Sep 17 00:00:00 2001 From: Staffan Gustafsson Date: Fri, 10 Aug 2018 13:57:07 +0200 Subject: [PATCH 8/9] Type switch on JArray, JObject and JValue to more clearly specify intent. Also specifying initial capacity on hashtable. --- .../commands/utility/WebCmdlet/JsonObject.cs | 190 ++++++++++-------- 1 file changed, 104 insertions(+), 86 deletions(-) diff --git a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs index 813a292b09b..3a06271e18c 100644 --- a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs +++ b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs @@ -153,32 +153,36 @@ private static PSObject PopulateFromJDictionary(JObject entries, DuplicateMember } // Array - if (entry.Value is JArray list) + switch (entry.Value) { - ICollection listResult = PopulateFromJArray(list, out error); - if (error != null) + case JArray list: { - return null; + var listResult = PopulateFromJArray(list, out error); + if (error != null) + { + return null; + } + + result.Properties.Add(new PSNoteProperty(entry.Key, listResult)); + break; } - - result.Properties.Add(new PSNoteProperty(entry.Key, listResult)); - } - else if (entry.Value is JObject dic) - { - // Dictionary - PSObject dicResult = PopulateFromJDictionary(dic, new DuplicateMemberHashSet(dic.Count), out error); - if (error != null) + case JObject dic: { - return null; + // Dictionary + var dicResult = PopulateFromJDictionary(dic, new DuplicateMemberHashSet(dic.Count), out error); + if (error != null) + { + return null; + } + + result.Properties.Add(new PSNoteProperty(entry.Key, dicResult)); + break; + } + case JValue value: + { + result.Properties.Add(new PSNoteProperty(entry.Key, value.Value)); + break; } - - result.Properties.Add(new PSNoteProperty(entry.Key, dicResult)); - } - else - { - // Value - JValue theValue = (JValue)entry.Value; - result.Properties.Add(new PSNoteProperty(entry.Key, theValue.Value)); } memberHashTracker.Add(entry.Key); @@ -196,32 +200,37 @@ private static ICollection PopulateFromJArray(JArray list, out ErrorReco for (var index = 0; index < list.Count; index++) { var element = list[index]; - if (element is JArray subList) + switch(element) { - // Array - ICollection listResult = PopulateFromJArray(subList, out error); - if (error != null) + case JArray subList: { - return null; + // Array + var listResult = PopulateFromJArray(subList, out error); + if (error != null) + { + return null; + } + + result[index] = listResult; + break; } - - result[index] = listResult; - } - else if (element is JObject dic) - { - // Dictionary - PSObject dicResult = PopulateFromJDictionary(dic, new DuplicateMemberHashSet(dic.Count), out error); - if (error != null) + case JObject dic: { - return null; + // Dictionary + var dicResult = PopulateFromJDictionary(dic, new DuplicateMemberHashSet(dic.Count), out error); + if (error != null) + { + return null; + } + + result[index] = dicResult; + break; + } + case JValue value: + { + result[index] = value.Value; + break; } - - result[index] = dicResult; - } - else - { - // Value - result[index] = ((JValue)element).Value; } } @@ -232,7 +241,7 @@ private static ICollection PopulateFromJArray(JArray list, out ErrorReco private static Hashtable PopulateHashTableFromJDictionary(JObject entries, out ErrorRecord error) { error = null; - Hashtable result = new Hashtable(); + Hashtable result = new Hashtable(entries.Count); foreach (var entry in entries) { // Case sensitive duplicates should normally not occur since JsonConvert.DeserializeObject @@ -248,33 +257,37 @@ private static Hashtable PopulateHashTableFromJDictionary(JObject entries, out E return null; } - if (entry.Value is JArray list) + switch (entry.Value) { - // Array - ICollection listResult = PopulateHashTableFromJArray(list, out error); - if (error != null) + case JArray list: { - return null; + // Array + var listResult = PopulateHashTableFromJArray(list, out error); + if (error != null) + { + return null; + } + + result.Add(entry.Key, listResult); + break; } - - result.Add(entry.Key, listResult); - } - else if (entry.Value is JObject dic) - { - // Dictionary - Hashtable dicResult = PopulateHashTableFromJDictionary(dic, out error); - if (error != null) + case JObject dic: { - return null; + // Dictionary + var dicResult = PopulateHashTableFromJDictionary(dic, out error); + if (error != null) + { + return null; + } + + result.Add(entry.Key, dicResult); + break; + } + case JValue value: + { + result.Add(entry.Key, value.Value); + break; } - - result.Add(entry.Key, dicResult); - } - else - { - // Value - JValue theValue = entry.Value as JValue; - result.Add(entry.Key, theValue.Value); } } @@ -291,32 +304,37 @@ private static ICollection PopulateHashTableFromJArray(JArray list, out { var element = list[index]; - if (element is JArray subList) + switch (element) { - // Array - ICollection listResult = PopulateHashTableFromJArray(subList, out error); - if (error != null) + case JArray array: { - return null; + // Array + var listResult = PopulateHashTableFromJArray(array, out error); + if (error != null) + { + return null; + } + + result[index] = listResult; + break; } - - result[index] = listResult; - } - else if (element is JObject dic) - { - // Dictionary - Hashtable dicResult = PopulateHashTableFromJDictionary(dic, out error); - if (error != null) + case JObject dic: { - return null; + // Dictionary + var dicResult = PopulateHashTableFromJDictionary(dic, out error); + if (error != null) + { + return null; + } + + result[index] = dicResult; + break; + } + case JValue value: + { + result[index] = value.Value; + break; } - - result[index] = dicResult; - } - else - { - // Value - result[index] = ((JValue)element).Value; } } From 9a5dea03399e7a91af32570360f9507b274a8938 Mon Sep 17 00:00:00 2001 From: Staffan Gustafsson Date: Fri, 10 Aug 2018 14:01:24 +0200 Subject: [PATCH 9/9] Spacing around switch. --- .../commands/utility/WebCmdlet/JsonObject.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs index 3a06271e18c..b7c04b1e58b 100644 --- a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs +++ b/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs @@ -200,7 +200,7 @@ private static ICollection PopulateFromJArray(JArray list, out ErrorReco for (var index = 0; index < list.Count; index++) { var element = list[index]; - switch(element) + switch (element) { case JArray subList: {