From 4d015187a6618798f59cd5e9e5b46a110e58badd Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Tue, 1 Mar 2016 16:47:09 -0800 Subject: [PATCH 01/11] Move Npm::Person to use a factory instead of constructor. This class is just a simple structure and its constructors should not be doing all this parsing. --- Nodejs/Product/Npm/NodeModuleBuilder.cs | 2 +- Nodejs/Product/Npm/SPI/PackageJson.cs | 2 +- Nodejs/Product/Npm/SPI/Person.cs | 109 ++++++++++++++++-------- 3 files changed, 76 insertions(+), 37 deletions(-) diff --git a/Nodejs/Product/Npm/NodeModuleBuilder.cs b/Nodejs/Product/Npm/NodeModuleBuilder.cs index c1c3e5de0..cd0118112 100644 --- a/Nodejs/Product/Npm/NodeModuleBuilder.cs +++ b/Nodejs/Product/Npm/NodeModuleBuilder.cs @@ -71,7 +71,7 @@ public void AddAuthor(string text) { public IPerson Author { get { var text = _authorBuff.ToString().Trim(); - return string.IsNullOrEmpty(text) ? null : new Person(text); + return string.IsNullOrEmpty(text) ? null : Person.CreateFromJsonSource(text); } } diff --git a/Nodejs/Product/Npm/SPI/PackageJson.cs b/Nodejs/Product/Npm/SPI/PackageJson.cs index 8f228c707..10f6f8082 100644 --- a/Nodejs/Product/Npm/SPI/PackageJson.cs +++ b/Nodejs/Product/Npm/SPI/PackageJson.cs @@ -194,7 +194,7 @@ public IScripts Scripts { public IPerson Author { get { var author = _package.author; - return null == author ? null : new Person(author.ToString()); + return null == author ? null : Person.CreateFromJsonSource(author.ToString()); } } diff --git a/Nodejs/Product/Npm/SPI/Person.cs b/Nodejs/Product/Npm/SPI/Person.cs index cdbe3f674..50d9df655 100644 --- a/Nodejs/Product/Npm/SPI/Person.cs +++ b/Nodejs/Product/Npm/SPI/Person.cs @@ -29,68 +29,107 @@ internal class Person : IPerson { // We cannot rely on the ordering of any of these fields, // so we should match them separately. - private static readonly Regex RegexPerson = new Regex( - "\"name\":\\s*\"(?[^<]+?)\"" + - "|" + - "\"email\":\\s*\"(?[^<]+?)\"" + - "|" + - "\"url\":\\s*\"(?[^<]+?)\"", - RegexOptions.Singleline); - + private static readonly Regex ObjectPersonRegex = new Regex( + "\"name\":\\s*\"(?[^<]+?)\"" + + "|" + + "\"email\":\\s*\"(?[^<]+?)\"" + + "|" + + "\"url\":\\s*\"(?[^<]+?)\"", + RegexOptions.Singleline); + + private static readonly Regex StringPersonRegex = new Regex( + "^\"([^\"]+)\"$", + RegexOptions.Singleline); + [JsonConstructor] private Person() { // Enables Json deserialization } - public Person(string source) { - InitFromString(source); + private Person(string name, string email = "", string url = "") { + Name = name; + Email = email; + Url = url; } - private void InitFromString(string source) { - if (source == null) { - Name = string.Empty; - return; + public static Person CreateFromJsonSource(string source) { + if (source == null) + return new Person(string.Empty); + + + var objectPerson = TryCreatePersonFromObject(source); + if (objectPerson != null) + return objectPerson; + + var stringPerson = TryCreatePersonFromString(source); + if (stringPerson != null) + return stringPerson; +#if DEBUG + // Verify we are parsing correctly + try { + var jObject = JObject.Parse(source); + var name = (string)jObject["name"]; + Debug.Assert(name != null ? name == Name : Name == source, string.Format("Failed to parse name from {0}", source)); + Debug.Assert((string)jObject["email"] == Email, string.Format("Failed to parse email from {0}", source)); + Debug.Assert((string)jObject["url"] == Url, string.Format("Failed to parse url from {0}", source)); + } catch (Exception) { + Debug.Assert(source == Name); } +#endif + return new Person(source); + } + + /// + /// Try to create a person object from a json object. + /// + /// Json source + private static Person TryCreatePersonFromObject(string source) { + string name = null; + string email = null; + string url = null; // We parse using a regex because JObject.Parse throws exceptions for malformatted json, // and simply handling them causes performance issues. - var matches = RegexPerson.Matches(source); + var matches = ObjectPersonRegex.Matches(source); if (matches.Count >= 1) { foreach (Match match in matches) { var group = match.Groups["name"]; - if (group.Success) { - Name = group.Value; + if (@group.Success) { + name = @group.Value; continue; } - group = match.Groups["email"]; - if (group.Success) { - Email = group.Value; + @group = match.Groups["email"]; + if (@group.Success) { + email = @group.Value; continue; } - group = match.Groups["url"]; - if (group.Success) { - Url = group.Value; + @group = match.Groups["url"]; + if (@group.Success) { + url = @group.Value; continue; } } } else { - Name = source; + return null; } + return new Person(name, email, url); + } -#if DEBUG - // Verify we are parsing correctly - try { - var jObject = JObject.Parse(source); - var name = (string)jObject["name"]; - Debug.Assert(name != null ? name == Name : Name == source, string.Format("Failed to parse name from {0}", source)); - Debug.Assert((string)jObject["email"] == Email, string.Format("Failed to parse email from {0}", source)); - Debug.Assert((string)jObject["url"] == Url, string.Format("Failed to parse url from {0}", source)); - } catch (Exception) { - Debug.Assert(source == Name); + /// + /// Try to create a person object from a json string. + /// + /// TODO: currently does not try to parse the string to extract the email or url. + /// + /// Json source + private static Person TryCreatePersonFromString(string source) { + var matches = StringPersonRegex.Matches(source); + if (matches.Count >= 1) { + var match = matches[0]; + return new Person(match.Value); } -#endif + return null; } [JsonProperty] From 6538704c87502bab87bb6c86088d58802a454a47 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Tue, 1 Mar 2016 16:49:23 -0800 Subject: [PATCH 02/11] Remove debug logger for invalid person parsing This gets hit literally thousands and thousands of times during debugging, making it worthless. Person can be a string too, so the assertion is not even accurate. --- Nodejs/Product/Npm/SPI/Person.cs | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/Nodejs/Product/Npm/SPI/Person.cs b/Nodejs/Product/Npm/SPI/Person.cs index 50d9df655..ee43cecbb 100644 --- a/Nodejs/Product/Npm/SPI/Person.cs +++ b/Nodejs/Product/Npm/SPI/Person.cs @@ -55,7 +55,6 @@ private Person(string name, string email = "", string url = "") { public static Person CreateFromJsonSource(string source) { if (source == null) return new Person(string.Empty); - var objectPerson = TryCreatePersonFromObject(source); if (objectPerson != null) @@ -64,18 +63,7 @@ public static Person CreateFromJsonSource(string source) { var stringPerson = TryCreatePersonFromString(source); if (stringPerson != null) return stringPerson; -#if DEBUG - // Verify we are parsing correctly - try { - var jObject = JObject.Parse(source); - var name = (string)jObject["name"]; - Debug.Assert(name != null ? name == Name : Name == source, string.Format("Failed to parse name from {0}", source)); - Debug.Assert((string)jObject["email"] == Email, string.Format("Failed to parse email from {0}", source)); - Debug.Assert((string)jObject["url"] == Url, string.Format("Failed to parse url from {0}", source)); - } catch (Exception) { - Debug.Assert(source == Name); - } -#endif + return new Person(source); } From c2c0e1714dbfd58f5746cc9893cd19a864be43f5 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Tue, 1 Mar 2016 16:51:38 -0800 Subject: [PATCH 03/11] Added npm link --- Nodejs/Product/Npm/SPI/Person.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Nodejs/Product/Npm/SPI/Person.cs b/Nodejs/Product/Npm/SPI/Person.cs index ee43cecbb..7804b7685 100644 --- a/Nodejs/Product/Npm/SPI/Person.cs +++ b/Nodejs/Product/Npm/SPI/Person.cs @@ -69,6 +69,8 @@ public static Person CreateFromJsonSource(string source) { /// /// Try to create a person object from a json object. + /// + /// This can either be a json object or a string: https://docs.npmjs.com/files/package.json#people-fields-author-contributors /// /// Json source private static Person TryCreatePersonFromObject(string source) { From f6a9d19028eaa35f903321cfec6b7d8114833351 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Tue, 1 Mar 2016 17:34:30 -0800 Subject: [PATCH 04/11] Added a few unit tests for Person that pin the existing behavior and test refactoring --- Nodejs/Product/Npm/SPI/Person.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Nodejs/Product/Npm/SPI/Person.cs b/Nodejs/Product/Npm/SPI/Person.cs index 7804b7685..615550ddb 100644 --- a/Nodejs/Product/Npm/SPI/Person.cs +++ b/Nodejs/Product/Npm/SPI/Person.cs @@ -38,7 +38,7 @@ internal class Person : IPerson { RegexOptions.Singleline); private static readonly Regex StringPersonRegex = new Regex( - "^\"([^\"]+)\"$", + @"^""(?[^""]+)""$", RegexOptions.Singleline); [JsonConstructor] @@ -46,7 +46,7 @@ private Person() { // Enables Json deserialization } - private Person(string name, string email = "", string url = "") { + private Person(string name, string email = null, string url = null) { Name = name; Email = email; Url = url; @@ -115,9 +115,11 @@ private static Person TryCreatePersonFromObject(string source) { /// Json source private static Person TryCreatePersonFromString(string source) { var matches = StringPersonRegex.Matches(source); - if (matches.Count >= 1) { + if (matches.Count == 1) { var match = matches[0]; - return new Person(match.Value); + var group = match.Groups["name"]; + if (group.Success) + return new Person(group.Value); } return null; } From f5f0cef561ff42127d1072220b9aca5e85f51a56 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Tue, 1 Mar 2016 17:36:18 -0800 Subject: [PATCH 05/11] Added person unit test --- Nodejs/Tests/NpmTests/NpmTests.csproj | 1 + Nodejs/Tests/NpmTests/PersonTests.cs | 96 +++++++++++++++++++++++++++ 2 files changed, 97 insertions(+) create mode 100644 Nodejs/Tests/NpmTests/PersonTests.cs diff --git a/Nodejs/Tests/NpmTests/NpmTests.csproj b/Nodejs/Tests/NpmTests/NpmTests.csproj index be54bc4a3..6108b5313 100644 --- a/Nodejs/Tests/NpmTests/NpmTests.csproj +++ b/Nodejs/Tests/NpmTests/NpmTests.csproj @@ -94,6 +94,7 @@ + diff --git a/Nodejs/Tests/NpmTests/PersonTests.cs b/Nodejs/Tests/NpmTests/PersonTests.cs new file mode 100644 index 000000000..8f7cf7558 --- /dev/null +++ b/Nodejs/Tests/NpmTests/PersonTests.cs @@ -0,0 +1,96 @@ +//*********************************************************// +// Copyright (c) Microsoft. All rights reserved. +// +// Apache 2.0 License +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +// implied. See the License for the specific language governing +// permissions and limitations under the License. +// +//*********************************************************// + +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Microsoft.NodejsTools.Npm.SPI; + +namespace NpmTests { + [TestClass] + public class PersonTests { + [TestMethod, TestCategory("UnitTest")] + public void ShouldReturnEmptyPersonForNullOrEmptyInput() { + var sources = new[] { null, "", " ", " " }; + foreach (var emptySource in sources) { + var person = Person.CreateFromJsonSource(null); + Assert.IsNotNull(person); + Assert.AreEqual("", person.Name); + Assert.IsNull(person.Email); + Assert.IsNull(person.Url); + } + } + + [TestMethod, TestCategory("UnitTest")] + public void ShouldReturnNameForObjectWithNameOnly() { + var name = "J Scripter"; + var sources = new[] { + @"{{""name"": ""{0}""}}", + @"{{""name"":""{0}""}}", + @"{{""name"": ""{0}""}}", + @"{{ ""name"": ""{0}"" }}", + }; + foreach (var source in sources) { + var json = string.Format(source, name); + var person = Person.CreateFromJsonSource(json); + Assert.IsNotNull(person); + Assert.AreEqual(name, person.Name); + Assert.IsNull(person.Email); + Assert.IsNull(person.Url); + } + } + + [TestMethod, TestCategory("UnitTest")] + public void ShouldSetNameEmailAndUrlForObjectWithTheseProperties() { + var name = "J Scripter"; + var email = "j@contoso.com"; + var url = "http://contoso.com"; + + var sources = new[] { + @"{{""name"": ""{0}"", ""email"": ""{1}"", ""url"": ""{2}""}}", + @"{{""url"": ""{2}"", ""email"": ""{1}"", ""name"": ""{0}""}}", + // Ignore other properties + @"{{""handle"": ""@code"", ""url"": ""{2}"", ""email"": ""{1}"", ""office"": ""1337"", ""name"": ""{0}""}}", + }; + + foreach (var source in sources) { + var json = string.Format(source, name, email, url); + var person = Person.CreateFromJsonSource(json); + Assert.IsNotNull(person); + Assert.AreEqual(name, person.Name); + Assert.AreEqual(email, person.Email); + Assert.AreEqual(url, person.Url); + } + } + + [TestMethod, TestCategory("UnitTest")] + public void ShouldReturnInputStringAsNameIfInputIsNotJsonStringOrObject() { + var name = "J Scripter"; + var person = Person.CreateFromJsonSource(name); + Assert.IsNotNull(person); + Assert.AreEqual(name, person.Name); + Assert.IsNull(person.Email); + Assert.IsNull(person.Url); + } + + [TestMethod, TestCategory("UnitTest")] + public void ShouldTreatStringAsName() { + var person = Person.CreateFromJsonSource(@"""J Scripter"""); + Assert.IsNotNull(person); + Assert.AreEqual("J Scripter", person.Name); + Assert.IsNull(person.Email); + Assert.IsNull(person.Url); + } + } +} From 22977711cc3d98c6cb2076f3a4812342557f062d Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Tue, 1 Mar 2016 17:50:26 -0800 Subject: [PATCH 06/11] Undo @ prefixed names from resharper extract --- Nodejs/Product/Npm/SPI/Person.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Nodejs/Product/Npm/SPI/Person.cs b/Nodejs/Product/Npm/SPI/Person.cs index 615550ddb..b5da56d08 100644 --- a/Nodejs/Product/Npm/SPI/Person.cs +++ b/Nodejs/Product/Npm/SPI/Person.cs @@ -84,20 +84,20 @@ private static Person TryCreatePersonFromObject(string source) { if (matches.Count >= 1) { foreach (Match match in matches) { var group = match.Groups["name"]; - if (@group.Success) { - name = @group.Value; + if (group.Success) { + name = group.Value; continue; } - @group = match.Groups["email"]; - if (@group.Success) { - email = @group.Value; + group = match.Groups["email"]; + if (group.Success) { + email = group.Value; continue; } - @group = match.Groups["url"]; - if (@group.Success) { - url = @group.Value; + group = match.Groups["url"]; + if (group.Success) { + url = group.Value; continue; } } From 0f016c686d18382f60996f5c1c98e7e205d6d8b4 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Tue, 1 Mar 2016 18:28:56 -0800 Subject: [PATCH 07/11] Cleanup test method name slightly --- Nodejs/Tests/NpmTests/PersonTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Nodejs/Tests/NpmTests/PersonTests.cs b/Nodejs/Tests/NpmTests/PersonTests.cs index 8f7cf7558..86dc8c55c 100644 --- a/Nodejs/Tests/NpmTests/PersonTests.cs +++ b/Nodejs/Tests/NpmTests/PersonTests.cs @@ -75,7 +75,7 @@ public void ShouldSetNameEmailAndUrlForObjectWithTheseProperties() { } [TestMethod, TestCategory("UnitTest")] - public void ShouldReturnInputStringAsNameIfInputIsNotJsonStringOrObject() { + public void ShouldReturnInputAsNameIfInputIsNotJsonStringOrObject() { var name = "J Scripter"; var person = Person.CreateFromJsonSource(name); Assert.IsNotNull(person); From 9455b6c75b0a530455374d5dc5711a0ee6e9b89d Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Mon, 7 Mar 2016 17:16:49 -0800 Subject: [PATCH 08/11] Rebasing person --- Nodejs/Product/Npm/NodeModuleBuilder.cs | 2 +- Nodejs/Product/Npm/SPI/PackageJson.cs | 2 +- Nodejs/Product/Npm/SPI/Person.cs | 95 ++++++++++++++++-------- Nodejs/Tests/NpmTests/NpmTests.csproj | 1 + Nodejs/Tests/NpmTests/PersonTests.cs | 97 +++++++++++++++++++++++++ 5 files changed, 163 insertions(+), 34 deletions(-) create mode 100644 Nodejs/Tests/NpmTests/PersonTests.cs diff --git a/Nodejs/Product/Npm/NodeModuleBuilder.cs b/Nodejs/Product/Npm/NodeModuleBuilder.cs index c1c3e5de0..cd0118112 100644 --- a/Nodejs/Product/Npm/NodeModuleBuilder.cs +++ b/Nodejs/Product/Npm/NodeModuleBuilder.cs @@ -71,7 +71,7 @@ public void AddAuthor(string text) { public IPerson Author { get { var text = _authorBuff.ToString().Trim(); - return string.IsNullOrEmpty(text) ? null : new Person(text); + return string.IsNullOrEmpty(text) ? null : Person.CreateFromJsonSource(text); } } diff --git a/Nodejs/Product/Npm/SPI/PackageJson.cs b/Nodejs/Product/Npm/SPI/PackageJson.cs index 8f228c707..10f6f8082 100644 --- a/Nodejs/Product/Npm/SPI/PackageJson.cs +++ b/Nodejs/Product/Npm/SPI/PackageJson.cs @@ -194,7 +194,7 @@ public IScripts Scripts { public IPerson Author { get { var author = _package.author; - return null == author ? null : new Person(author.ToString()); + return null == author ? null : Person.CreateFromJsonSource(author.ToString()); } } diff --git a/Nodejs/Product/Npm/SPI/Person.cs b/Nodejs/Product/Npm/SPI/Person.cs index cdbe3f674..391cbe811 100644 --- a/Nodejs/Product/Npm/SPI/Person.cs +++ b/Nodejs/Product/Npm/SPI/Person.cs @@ -29,68 +29,99 @@ internal class Person : IPerson { // We cannot rely on the ordering of any of these fields, // so we should match them separately. - private static readonly Regex RegexPerson = new Regex( - "\"name\":\\s*\"(?[^<]+?)\"" + - "|" + - "\"email\":\\s*\"(?[^<]+?)\"" + - "|" + - "\"url\":\\s*\"(?[^<]+?)\"", - RegexOptions.Singleline); - + private static readonly Regex ObjectPersonRegex = new Regex( + "\"name\":\\s*\"(?[^<]+?)\"" + + "|" + + "\"email\":\\s*\"(?[^<]+?)\"" + + "|" + + "\"url\":\\s*\"(?[^<]+?)\"", + RegexOptions.Singleline); + + private static readonly Regex StringPersonRegex = new Regex( + @"^""(?[^""]+)""$", + RegexOptions.Singleline); + [JsonConstructor] private Person() { // Enables Json deserialization } - public Person(string source) { - InitFromString(source); + private Person(string name, string email = null, string url = null) { + Name = name; + Email = email; + Url = url; } - private void InitFromString(string source) { - if (source == null) { - Name = string.Empty; - return; - } + public static Person CreateFromJsonSource(string source) { + if (source == null) + return new Person(string.Empty); + + var objectPerson = TryCreatePersonFromObject(source); + if (objectPerson != null) + return objectPerson; + + var stringPerson = TryCreatePersonFromString(source); + if (stringPerson != null) + return stringPerson; + + return new Person(source); + } + + /// + /// Try to create a person object from a json object. + /// + /// This can either be a json object or a string: https://docs.npmjs.com/files/package.json#people-fields-author-contributors + /// + /// Json source + private static Person TryCreatePersonFromObject(string source) { + string name = null; + string email = null; + string url = null; // We parse using a regex because JObject.Parse throws exceptions for malformatted json, // and simply handling them causes performance issues. - var matches = RegexPerson.Matches(source); + var matches = ObjectPersonRegex.Matches(source); if (matches.Count >= 1) { foreach (Match match in matches) { var group = match.Groups["name"]; if (group.Success) { - Name = group.Value; + name = group.Value; continue; } group = match.Groups["email"]; if (group.Success) { - Email = group.Value; + email = group.Value; continue; } group = match.Groups["url"]; if (group.Success) { - Url = group.Value; + url = group.Value; continue; } } } else { - Name = source; + return null; } + return new Person(name, email, url); + } -#if DEBUG - // Verify we are parsing correctly - try { - var jObject = JObject.Parse(source); - var name = (string)jObject["name"]; - Debug.Assert(name != null ? name == Name : Name == source, string.Format("Failed to parse name from {0}", source)); - Debug.Assert((string)jObject["email"] == Email, string.Format("Failed to parse email from {0}", source)); - Debug.Assert((string)jObject["url"] == Url, string.Format("Failed to parse url from {0}", source)); - } catch (Exception) { - Debug.Assert(source == Name); + /// + /// Try to create a person object from a json string. + /// + /// TODO: currently does not try to parse the string to extract the email or url. + /// + /// Json source + private static Person TryCreatePersonFromString(string source) { + var matches = StringPersonRegex.Matches(source); + if (matches.Count == 1) { + var match = matches[0]; + var group = match.Groups["name"]; + if (group.Success) + return new Person(group.Value); } -#endif + return null; } [JsonProperty] @@ -128,4 +159,4 @@ public override string ToString() { return buff.ToString(); } } -} \ No newline at end of file +} diff --git a/Nodejs/Tests/NpmTests/NpmTests.csproj b/Nodejs/Tests/NpmTests/NpmTests.csproj index a68fda04c..6d5cdb77c 100644 --- a/Nodejs/Tests/NpmTests/NpmTests.csproj +++ b/Nodejs/Tests/NpmTests/NpmTests.csproj @@ -94,6 +94,7 @@ + diff --git a/Nodejs/Tests/NpmTests/PersonTests.cs b/Nodejs/Tests/NpmTests/PersonTests.cs new file mode 100644 index 000000000..c0be25078 --- /dev/null +++ b/Nodejs/Tests/NpmTests/PersonTests.cs @@ -0,0 +1,97 @@ +//*********************************************************// +// Copyright (c) Microsoft. All rights reserved. +// +// Apache 2.0 License +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +// implied. See the License for the specific language governing +// permissions and limitations under the License. +// +//*********************************************************// + +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Microsoft.NodejsTools.Npm.SPI; + +namespace NpmTests { + [TestClass] + public class PersonTests { + [TestMethod, TestCategory("UnitTest")] + public void ShouldReturnEmptyPersonForNullOrEmptyInput() { + var sources = new[] { null, "", " ", " " }; + foreach (var emptySource in sources) { + var person = Person.CreateFromJsonSource(null); + Assert.IsNotNull(person); + Assert.AreEqual("", person.Name); + Assert.IsNull(person.Email); + Assert.IsNull(person.Url); + } + } + + [TestMethod, TestCategory("UnitTest")] + public void ShouldReturnNameForObjectWithNameOnly() { + var name = "J Scripter"; + var sources = new[] { + @"{{""name"": ""{0}""}}", + @"{{""name"":""{0}""}}", + @"{{""name"": ""{0}""}}", + @"{{ ""name"": ""{0}"" }}", + }; + foreach (var source in sources) { + var json = string.Format(source, name); + var person = Person.CreateFromJsonSource(json); + Assert.IsNotNull(person); + Assert.AreEqual(name, person.Name); + Assert.IsNull(person.Email); + Assert.IsNull(person.Url); + } + } + + [TestMethod, TestCategory("UnitTest")] + public void ShouldSetNameEmailAndUrlForObjectWithTheseProperties() { + var name = "J Scripter"; + var email = "j@contoso.com"; + var url = "http://contoso.com"; + + var sources = new[] { + @"{{""name"": ""{0}"", ""email"": ""{1}"", ""url"": ""{2}""}}", + @"{{""url"": ""{2}"", ""email"": ""{1}"", ""name"": ""{0}""}}", + // Ignore other properties + @"{{""handle"": ""@code"", ""url"": ""{2}"", ""email"": ""{1}"", ""office"": ""1337"", ""name"": ""{0}""}}", + }; + + foreach (var source in sources) { + var json = string.Format(source, name, email, url); + var person = Person.CreateFromJsonSource(json); + Assert.IsNotNull(person); + Assert.AreEqual(name, person.Name); + Assert.AreEqual(email, person.Email); + Assert.AreEqual(url, person.Url); + } + } + + [TestMethod, TestCategory("UnitTest")] + public void ShouldReturnInputAsNameIfInputIsNotJsonStringOrObject() { + var name = "J Scripter"; + var person = Person.CreateFromJsonSource(name); + Assert.IsNotNull(person); + Assert.AreEqual(name, person.Name); + Assert.IsNull(person.Email); + Assert.IsNull(person.Url); + } + + [TestMethod, TestCategory("UnitTest")] + public void ShouldTreatStringAsName() { + var person = Person.CreateFromJsonSource(@"""J Scripter"""); + Assert.IsNotNull(person); + Assert.AreEqual("J Scripter", person.Name); + Assert.IsNull(person.Email); + Assert.IsNull(person.Url); + } + } +} + From 573b75fcf718cef40aaea94b11737f172c758393 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Mon, 7 Mar 2016 18:08:53 -0800 Subject: [PATCH 09/11] Removed extra regexp logic --- Nodejs/Product/Npm/SPI/Person.cs | 32 +++++----------------------- Nodejs/Tests/NpmTests/PersonTests.cs | 11 +--------- 2 files changed, 6 insertions(+), 37 deletions(-) diff --git a/Nodejs/Product/Npm/SPI/Person.cs b/Nodejs/Product/Npm/SPI/Person.cs index 391cbe811..8b8f0aab0 100644 --- a/Nodejs/Product/Npm/SPI/Person.cs +++ b/Nodejs/Product/Npm/SPI/Person.cs @@ -37,10 +37,6 @@ internal class Person : IPerson { "\"url\":\\s*\"(?[^<]+?)\"", RegexOptions.Singleline); - private static readonly Regex StringPersonRegex = new Regex( - @"^""(?[^""]+)""$", - RegexOptions.Singleline); - [JsonConstructor] private Person() { // Enables Json deserialization @@ -55,16 +51,7 @@ private Person(string name, string email = null, string url = null) { public static Person CreateFromJsonSource(string source) { if (source == null) return new Person(string.Empty); - - var objectPerson = TryCreatePersonFromObject(source); - if (objectPerson != null) - return objectPerson; - - var stringPerson = TryCreatePersonFromString(source); - if (stringPerson != null) - return stringPerson; - - return new Person(source); + return TryCreatePersonFromObject(source) ?? CreatePersonFromString(source); } /// @@ -101,27 +88,18 @@ private static Person TryCreatePersonFromObject(string source) { continue; } } - } else { - return null; } - return new Person(name, email, url); + return name == null ? null : new Person(name, email, url); } /// - /// Try to create a person object from a json string. + /// Try to create a person object from a string. /// /// TODO: currently does not try to parse the string to extract the email or url. /// /// Json source - private static Person TryCreatePersonFromString(string source) { - var matches = StringPersonRegex.Matches(source); - if (matches.Count == 1) { - var match = matches[0]; - var group = match.Groups["name"]; - if (group.Success) - return new Person(group.Value); - } - return null; + private static Person CreatePersonFromString(string source) { + return new Person(source); } [JsonProperty] diff --git a/Nodejs/Tests/NpmTests/PersonTests.cs b/Nodejs/Tests/NpmTests/PersonTests.cs index 0638721da..c11c2bdc1 100644 --- a/Nodejs/Tests/NpmTests/PersonTests.cs +++ b/Nodejs/Tests/NpmTests/PersonTests.cs @@ -74,7 +74,7 @@ public void ShouldSetNameEmailAndUrlForObjectWithTheseProperties() { } [TestMethod, TestCategory("UnitTest")] - public void ShouldReturnInputAsNameIfInputIsNotJsonStringOrObject() { + public void ShouldReturnInputAsNameIfInputIsObject() { var name = "J Scripter"; var person = Person.CreateFromJsonSource(name); Assert.IsNotNull(person); @@ -82,14 +82,5 @@ public void ShouldReturnInputAsNameIfInputIsNotJsonStringOrObject() { Assert.IsNull(person.Email); Assert.IsNull(person.Url); } - - [TestMethod, TestCategory("UnitTest")] - public void ShouldTreatStringAsName() { - var person = Person.CreateFromJsonSource(@"""J Scripter"""); - Assert.IsNotNull(person); - Assert.AreEqual("J Scripter", person.Name); - Assert.IsNull(person.Email); - Assert.IsNull(person.Url); - } } } From 7f613cc75ab0e1d731b43c4c005117b8cc53541a Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Mon, 7 Mar 2016 18:12:58 -0800 Subject: [PATCH 10/11] Remove now unused import --- Nodejs/Product/Npm/SPI/Person.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Nodejs/Product/Npm/SPI/Person.cs b/Nodejs/Product/Npm/SPI/Person.cs index 8b8f0aab0..e68086879 100644 --- a/Nodejs/Product/Npm/SPI/Person.cs +++ b/Nodejs/Product/Npm/SPI/Person.cs @@ -20,10 +20,6 @@ using System.Text.RegularExpressions; using Newtonsoft.Json; -#if DEBUG -using Newtonsoft.Json.Linq; -#endif - namespace Microsoft.NodejsTools.Npm.SPI { internal class Person : IPerson { From 74088a20a51905b6d4b64529d018dbecbe5be7e0 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Thu, 17 Mar 2016 12:53:40 -0700 Subject: [PATCH 11/11] Added test categories to person tests --- Nodejs/Tests/NpmTests/PersonTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Nodejs/Tests/NpmTests/PersonTests.cs b/Nodejs/Tests/NpmTests/PersonTests.cs index c11c2bdc1..5103388f6 100644 --- a/Nodejs/Tests/NpmTests/PersonTests.cs +++ b/Nodejs/Tests/NpmTests/PersonTests.cs @@ -19,7 +19,7 @@ namespace NpmTests { [TestClass] public class PersonTests { - [TestMethod, TestCategory("UnitTest")] + [TestMethod, TestCategory("NpmIntegration")] public void ShouldReturnEmptyPersonForNullOrEmptyInput() { var sources = new[] { null, "", " ", " " }; foreach (var emptySource in sources) { @@ -31,7 +31,7 @@ public void ShouldReturnEmptyPersonForNullOrEmptyInput() { } } - [TestMethod, TestCategory("UnitTest")] + [TestMethod, TestCategory("NpmIntegration")] public void ShouldReturnNameForObjectWithNameOnly() { var name = "J Scripter"; var sources = new[] { @@ -50,7 +50,7 @@ public void ShouldReturnNameForObjectWithNameOnly() { } } - [TestMethod, TestCategory("UnitTest")] + [TestMethod, TestCategory("NpmIntegration")] public void ShouldSetNameEmailAndUrlForObjectWithTheseProperties() { var name = "J Scripter"; var email = "j@contoso.com"; @@ -73,7 +73,7 @@ public void ShouldSetNameEmailAndUrlForObjectWithTheseProperties() { } } - [TestMethod, TestCategory("UnitTest")] + [TestMethod, TestCategory("NpmIntegration")] public void ShouldReturnInputAsNameIfInputIsObject() { var name = "J Scripter"; var person = Person.CreateFromJsonSource(name);