From b9f8645adb361f228aa3f4fd085470face140c2c Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 1 Apr 2016 13:37:28 -0700 Subject: [PATCH 01/11] Don't hold onto raw Json.net objects in `Npm.Spi.PackageJson` **Bug** A rather silly amount of memory is currently being used to hold onto raw Json.net objects. **Fix** In `Npm.Spi.PackageJson`, don't hold onto the `dynamic` package.json Json object, instead, extract all the properties that are needed and then release it. Had to convert a few properties to be eagerly computed, but already compute 90% of the properties eagerly right now. --- Nodejs/Product/Npm/SPI/PackageJson.cs | 185 ++++++++++---------------- 1 file changed, 69 insertions(+), 116 deletions(-) diff --git a/Nodejs/Product/Npm/SPI/PackageJson.cs b/Nodejs/Product/Npm/SPI/PackageJson.cs index 10f6f8082..a87df4494 100644 --- a/Nodejs/Product/Npm/SPI/PackageJson.cs +++ b/Nodejs/Product/Npm/SPI/PackageJson.cs @@ -21,31 +21,39 @@ namespace Microsoft.NodejsTools.Npm.SPI { internal class PackageJson : IPackageJson { - private dynamic _package; - private Scripts _scripts; - private Bugs _bugs; - public PackageJson(dynamic package) { - _package = package; - - InitKeywords(); - InitHomepages(); - InitLicenses(); - InitFiles(); - InitMan(); - InitDependencies(); - InitDevDependencies(); - InitBundledDependencies(); - InitOptionalDependencies(); - InitAllDependencies(); - InitRequiredBy(); + Name = package.name == null ? null : package.name.ToString(); + Version = package.version == null ? new SemverVersion() : SemverVersion.Parse(package.version.ToString()); + Description = package.description == null ? null : package.description.ToString(); + Author = package.author == null ? null : Person.CreateFromJsonSource(package.author.ToString()); + + Keywords = LoadKeywords(package); + Homepages = LoadHomepages(package); + Licenses = LoadLicenses(package); + Files = LoadFiles(package); + Man = LoadMan(package); + Dependencies = LoadDependencies(package); + DevDependencies = LoadDevDependencies(package); + BundledDependencies = LoadBundledDependencies(package); + OptionalDependencies = LoadOptionalDependencies(package); + AllDependencies = LoadAllDependencies(package); + RequiredBy = LoadRequiredBy(package); + + dynamic scriptsJson = package.scripts; + if (null == scriptsJson) { + scriptsJson = new JObject(); + package.scripts = scriptsJson; + } + Scripts = new Scripts(scriptsJson); + + if (package.bugs != null) { + Bugs = new Bugs(package); + } } - private void WrapRuntimeBinderExceptionAndRethrow( - string errorProperty, - RuntimeBinderException rbe) { - throw new PackageJsonException( - string.Format(@"Exception occurred retrieving {0} from package.json. The file may be invalid: you should edit it to correct an errors. + private static PackageJsonException WrapRuntimeBinderException(string errorProperty, RuntimeBinderException rbe) { + return new PackageJsonException( + string.Format(@"Exception occurred retrieving {0} from package.json. The file may be invalid: you should edit it to correct an errors. The following error occurred: @@ -54,166 +62,111 @@ private void WrapRuntimeBinderExceptionAndRethrow( rbe)); } - private void InitKeywords() { + private static IKeywords LoadKeywords(dynamic package) { try { - Keywords = new Keywords(_package); + return new Keywords(package); } catch (RuntimeBinderException rbe) { - WrapRuntimeBinderExceptionAndRethrow( - "keywords", - rbe); + throw WrapRuntimeBinderException("keywords", rbe); } } - private void InitHomepages() { + private static IHomepages LoadHomepages(dynamic package) { try { - Homepages = new Homepages(_package); + return new Homepages(package); } catch (RuntimeBinderException rbe) { - WrapRuntimeBinderExceptionAndRethrow( - "homepage", - rbe); + throw WrapRuntimeBinderException("homepage", rbe); } } - private void InitFiles() { + private static IFiles LoadFiles(dynamic package) { try { - Files = new PkgFiles(_package); + return new PkgFiles(package); } catch (RuntimeBinderException rbe) { - WrapRuntimeBinderExceptionAndRethrow( - "files", - rbe); + throw WrapRuntimeBinderException("files", rbe); } } - private void InitLicenses() { + private static ILicenses LoadLicenses(dynamic package) { try { - Licenses = new Licenses(_package); + return new Licenses(package); } catch (RuntimeBinderException rbe) { - WrapRuntimeBinderExceptionAndRethrow( - "licenses", - rbe); + throw WrapRuntimeBinderException("licenses",rbe); } } - private void InitMan() { + private static IMan LoadMan(dynamic package) { try { - Man = new Man(_package); + return new Man(package); } catch (RuntimeBinderException rbe) { - WrapRuntimeBinderExceptionAndRethrow( - "man", - rbe); + throw WrapRuntimeBinderException("man", rbe); } } - private void InitDependencies() { + private static IDependencies LoadDependencies(dynamic package) { try { - Dependencies = new Dependencies(_package, "dependencies"); + return new Dependencies(package, "dependencies"); } catch (RuntimeBinderException rbe) { - WrapRuntimeBinderExceptionAndRethrow( - "dependencies", - rbe); + throw WrapRuntimeBinderException("dependencies", rbe); } } - private void InitDevDependencies() { + private static IDependencies LoadDevDependencies(dynamic package) { try { - DevDependencies = new Dependencies(_package, "devDependencies"); + return new Dependencies(package, "devDependencies"); } catch (RuntimeBinderException rbe) { - WrapRuntimeBinderExceptionAndRethrow( - "dev dependencies", - rbe); + throw WrapRuntimeBinderException("dev dependencies", rbe); } } - private void InitBundledDependencies() { + private static IBundledDependencies LoadBundledDependencies(dynamic package) { try { - BundledDependencies = new BundledDependencies(_package); + return new BundledDependencies(package); } catch (RuntimeBinderException rbe) { - WrapRuntimeBinderExceptionAndRethrow( - "bundled dependencies", - rbe); + throw WrapRuntimeBinderException("bundled dependencies", rbe); } } - private void InitOptionalDependencies() { + private static IDependencies LoadOptionalDependencies(dynamic package) { try { - OptionalDependencies = new Dependencies(_package, "optionalDependencies"); + return new Dependencies(package, "optionalDependencies"); } catch (RuntimeBinderException rbe) { - WrapRuntimeBinderExceptionAndRethrow( - "optional dependencies", - rbe); + throw WrapRuntimeBinderException("optional dependencies", rbe); } } - private void InitAllDependencies() { + private static IDependencies LoadAllDependencies(dynamic package) { try { - AllDependencies = new Dependencies(_package, "dependencies", "devDependencies", "optionalDependencies"); + return new Dependencies(package, "dependencies", "devDependencies", "optionalDependencies"); } catch (RuntimeBinderException rbe) { - WrapRuntimeBinderExceptionAndRethrow( - "all dependencies", - rbe); + throw WrapRuntimeBinderException("all dependencies", rbe); } } - private void InitRequiredBy() { + private static IEnumerable LoadRequiredBy(dynamic package) { try { - RequiredBy = (_package["_requiredBy"] as IEnumerable ?? Enumerable.Empty()).Values(); + return (package["_requiredBy"] as IEnumerable ?? Enumerable.Empty()).Values(); } catch (RuntimeBinderException rbe) { System.Diagnostics.Debug.WriteLine(rbe); - WrapRuntimeBinderExceptionAndRethrow( - "required by", - rbe); + throw WrapRuntimeBinderException("required by", rbe); } } - public string Name { - get { return null == _package.name ? null : _package.name.ToString(); } - } + public string Name { get; private set; } - public SemverVersion Version { - get { - return null == _package.version ? new SemverVersion() : SemverVersion.Parse(_package.version.ToString()); - } - } + public SemverVersion Version { get; private set; } - public IScripts Scripts { - get { - if (null == _scripts) { - dynamic scriptsJson = _package.scripts; - if (null == scriptsJson) { - scriptsJson = new JObject(); - _package.scripts = scriptsJson; - } - _scripts = new Scripts(scriptsJson); - } - - return _scripts; - } - } + public IScripts Scripts { get; private set; } - public IPerson Author { - get { - var author = _package.author; - return null == author ? null : Person.CreateFromJsonSource(author.ToString()); - } - } + public IPerson Author { get; private set; } - public string Description { - get { return null == _package.description ? null : _package.description.ToString(); } - } + public string Description { get; private set; } public IKeywords Keywords { get; private set; } public IHomepages Homepages { get; private set; } - public IBugs Bugs { - get { - if (null == _bugs && null != _package.bugs) { - _bugs = new Bugs(_package); - } - return _bugs; - } - } + public IBugs Bugs { get; private set; } public ILicenses Licenses { get; private set; } From aa251b1ece1abcf5bb2cc934ed4b7a30554ec085 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 1 Apr 2016 13:59:30 -0700 Subject: [PATCH 02/11] Don't hold onto entire _package for sub objects --- Nodejs/Product/Npm/SPI/Bugs.cs | 19 ++++++++----------- Nodejs/Product/Npm/SPI/Licenses.cs | 21 +++++++++------------ 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/Nodejs/Product/Npm/SPI/Bugs.cs b/Nodejs/Product/Npm/SPI/Bugs.cs index 29f86d84c..d03bba64a 100644 --- a/Nodejs/Product/Npm/SPI/Bugs.cs +++ b/Nodejs/Product/Npm/SPI/Bugs.cs @@ -18,21 +18,19 @@ namespace Microsoft.NodejsTools.Npm.SPI { internal class Bugs : IBugs { - private readonly dynamic _package; + private readonly dynamic _json; public Bugs(dynamic package) { - _package = package; + _json = package.bugs; } - public string Url { get { string url = null; - var bugs = _package.bugs; - if (null != bugs) { - var token = bugs as JToken; + if (null != _json) { + var token = _json as JToken; if (token.Type == JTokenType.Object) { - var temp = bugs.url ?? bugs.web; + var temp = _json.url ?? _json.web; if (null != temp) { url = temp.ToString(); } @@ -47,11 +45,10 @@ public string Url { public string Email { get { string email = null; - var bugs = _package.bugs; - if (null != bugs) { - var token = bugs as JToken; + if (null != _json) { + var token = _json as JToken; if (token.Type == JTokenType.Object) { - var temp = bugs.email ?? bugs.mail; + var temp = _json.email ?? _json.mail; if (null != temp) { email = temp.ToString(); } diff --git a/Nodejs/Product/Npm/SPI/Licenses.cs b/Nodejs/Product/Npm/SPI/Licenses.cs index 59197b57d..e8f3c4975 100644 --- a/Nodejs/Product/Npm/SPI/Licenses.cs +++ b/Nodejs/Product/Npm/SPI/Licenses.cs @@ -19,25 +19,23 @@ namespace Microsoft.NodejsTools.Npm.SPI { internal class Licenses : ILicenses { - private dynamic _package; + private dynamic _json; public Licenses(dynamic package) { - _package = package; + _json = package.license; } public int Count { get { - if (_package.license != null) { + if (_json != null) { return 1; } - var json = _package.licenses; - if (null == json) { + if (null == _json) { return 0; } - JArray array = json; - return array.Count; + return (_json as JArray).Count; } } @@ -47,16 +45,15 @@ public ILicense this[int index] { throw new IndexOutOfRangeException(Resources.CannotRetrieveLicenseInvalidIndex); } - if (index == 0 && _package.license != null) { - return new License(_package.license.ToString()); + if (index == 0 && _json != null) { + return new License(_json.ToString()); } - var json = _package.licenses; - if (null == json) { + if (null == _json) { throw new IndexOutOfRangeException(Resources.CannotRetrieveLicenseEmptyCollection); } - var lic = json[index]; + var lic = _json[index]; return new License(lic.type.ToString(), lic.url.ToString()); } } From 02fd81063f27ac8f65b81e51a19790b6592549b0 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 1 Apr 2016 14:07:35 -0700 Subject: [PATCH 03/11] Delete unused properties from package.json --- Nodejs/Product/Npm/IBugs.cs | 22 -------- Nodejs/Product/Npm/IPackageJson.cs | 2 - Nodejs/Product/Npm/IScript.cs | 22 -------- Nodejs/Product/Npm/IScripts.cs | 22 -------- Nodejs/Product/Npm/Npm.csproj | 8 +-- Nodejs/Product/Npm/SPI/Bugs.cs | 61 ---------------------- Nodejs/Product/Npm/SPI/PackageJson.cs | 15 ------ Nodejs/Product/Npm/SPI/Script.cs | 32 ------------ Nodejs/Product/Npm/SPI/Scripts.cs | 45 ---------------- Nodejs/Tests/NpmTests/PackageJsonTests.cs | 62 ----------------------- 10 files changed, 1 insertion(+), 290 deletions(-) delete mode 100644 Nodejs/Product/Npm/IBugs.cs delete mode 100644 Nodejs/Product/Npm/IScript.cs delete mode 100644 Nodejs/Product/Npm/IScripts.cs delete mode 100644 Nodejs/Product/Npm/SPI/Bugs.cs delete mode 100644 Nodejs/Product/Npm/SPI/Script.cs delete mode 100644 Nodejs/Product/Npm/SPI/Scripts.cs diff --git a/Nodejs/Product/Npm/IBugs.cs b/Nodejs/Product/Npm/IBugs.cs deleted file mode 100644 index be80b3586..000000000 --- a/Nodejs/Product/Npm/IBugs.cs +++ /dev/null @@ -1,22 +0,0 @@ -//*********************************************************// -// 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. -// -//*********************************************************// - -namespace Microsoft.NodejsTools.Npm { - public interface IBugs { - string Url { get; } - string Email { get; } - } -} \ No newline at end of file diff --git a/Nodejs/Product/Npm/IPackageJson.cs b/Nodejs/Product/Npm/IPackageJson.cs index 1adddeabb..e93474794 100644 --- a/Nodejs/Product/Npm/IPackageJson.cs +++ b/Nodejs/Product/Npm/IPackageJson.cs @@ -20,12 +20,10 @@ namespace Microsoft.NodejsTools.Npm { public interface IPackageJson { string Name { get; } SemverVersion Version { get; } - IScripts Scripts { get; } IPerson Author { get; } string Description { get; } IKeywords Keywords { get; } IHomepages Homepages { get; } - IBugs Bugs { get; } ILicenses Licenses { get; } IFiles Files { get; } IMan Man { get; } diff --git a/Nodejs/Product/Npm/IScript.cs b/Nodejs/Product/Npm/IScript.cs deleted file mode 100644 index 4a7ca8814..000000000 --- a/Nodejs/Product/Npm/IScript.cs +++ /dev/null @@ -1,22 +0,0 @@ -//*********************************************************// -// 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. -// -//*********************************************************// - -namespace Microsoft.NodejsTools.Npm { - public interface IScript { - string Name { get; } - string Code { get; } - } -} \ No newline at end of file diff --git a/Nodejs/Product/Npm/IScripts.cs b/Nodejs/Product/Npm/IScripts.cs deleted file mode 100644 index 6b24bf73e..000000000 --- a/Nodejs/Product/Npm/IScripts.cs +++ /dev/null @@ -1,22 +0,0 @@ -//*********************************************************// -// 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. -// -//*********************************************************// - -namespace Microsoft.NodejsTools.Npm { - public interface IScripts { - int Count { get; } - IScript this[string name] { get; } - } -} \ No newline at end of file diff --git a/Nodejs/Product/Npm/Npm.csproj b/Nodejs/Product/Npm/Npm.csproj index 95fb24a20..8afd77a65 100644 --- a/Nodejs/Product/Npm/Npm.csproj +++ b/Nodejs/Product/Npm/Npm.csproj @@ -1,4 +1,4 @@ - + @@ -114,7 +114,6 @@ - @@ -137,8 +136,6 @@ - - Nodejs.cs @@ -173,7 +170,6 @@ - @@ -200,8 +196,6 @@ - - diff --git a/Nodejs/Product/Npm/SPI/Bugs.cs b/Nodejs/Product/Npm/SPI/Bugs.cs deleted file mode 100644 index d03bba64a..000000000 --- a/Nodejs/Product/Npm/SPI/Bugs.cs +++ /dev/null @@ -1,61 +0,0 @@ -//*********************************************************// -// 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 Newtonsoft.Json.Linq; - -namespace Microsoft.NodejsTools.Npm.SPI { - internal class Bugs : IBugs { - private readonly dynamic _json; - - public Bugs(dynamic package) { - _json = package.bugs; - } - - public string Url { - get { - string url = null; - if (null != _json) { - var token = _json as JToken; - if (token.Type == JTokenType.Object) { - var temp = _json.url ?? _json.web; - if (null != temp) { - url = temp.ToString(); - } - } else { - url = token.Value(); - } - } - return url; - } - } - - public string Email { - get { - string email = null; - if (null != _json) { - var token = _json as JToken; - if (token.Type == JTokenType.Object) { - var temp = _json.email ?? _json.mail; - if (null != temp) { - email = temp.ToString(); - } - } - } - return email; - } - } - } -} \ No newline at end of file diff --git a/Nodejs/Product/Npm/SPI/PackageJson.cs b/Nodejs/Product/Npm/SPI/PackageJson.cs index a87df4494..cf4c6b2fb 100644 --- a/Nodejs/Product/Npm/SPI/PackageJson.cs +++ b/Nodejs/Product/Npm/SPI/PackageJson.cs @@ -38,17 +38,6 @@ public PackageJson(dynamic package) { OptionalDependencies = LoadOptionalDependencies(package); AllDependencies = LoadAllDependencies(package); RequiredBy = LoadRequiredBy(package); - - dynamic scriptsJson = package.scripts; - if (null == scriptsJson) { - scriptsJson = new JObject(); - package.scripts = scriptsJson; - } - Scripts = new Scripts(scriptsJson); - - if (package.bugs != null) { - Bugs = new Bugs(package); - } } private static PackageJsonException WrapRuntimeBinderException(string errorProperty, RuntimeBinderException rbe) { @@ -156,8 +145,6 @@ private static IEnumerable LoadRequiredBy(dynamic package) { public SemverVersion Version { get; private set; } - public IScripts Scripts { get; private set; } - public IPerson Author { get; private set; } public string Description { get; private set; } @@ -166,8 +153,6 @@ private static IEnumerable LoadRequiredBy(dynamic package) { public IHomepages Homepages { get; private set; } - public IBugs Bugs { get; private set; } - public ILicenses Licenses { get; private set; } public IFiles Files { get; private set; } diff --git a/Nodejs/Product/Npm/SPI/Script.cs b/Nodejs/Product/Npm/SPI/Script.cs deleted file mode 100644 index c16234d87..000000000 --- a/Nodejs/Product/Npm/SPI/Script.cs +++ /dev/null @@ -1,32 +0,0 @@ -//*********************************************************// -// 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. -// -//*********************************************************// - -namespace Microsoft.NodejsTools.Npm.SPI { - internal class Script : IScript { - private dynamic _code; - - public Script(string name, dynamic code) { - Name = name; - _code = code; - } - - public string Name { get; private set; } - - public string Code { - get { return _code.ToString(); } - } - } -} \ No newline at end of file diff --git a/Nodejs/Product/Npm/SPI/Scripts.cs b/Nodejs/Product/Npm/SPI/Scripts.cs deleted file mode 100644 index e43dbf8f9..000000000 --- a/Nodejs/Product/Npm/SPI/Scripts.cs +++ /dev/null @@ -1,45 +0,0 @@ -//*********************************************************// -// 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 Newtonsoft.Json.Linq; - -namespace Microsoft.NodejsTools.Npm.SPI { - internal class Scripts : IScripts { - private dynamic _scripts; - - public Scripts(dynamic scripts) { - _scripts = scripts; - } - - public int Count { - get { - JObject temp = _scripts; - return null == temp ? 0 : temp.Count; - } - } - - public IScript this[string name] { - get { - IScript script = null; - dynamic json = _scripts[name]; - if (null != json) { - script = new Script(name, json); - } - return script; - } - } - } -} \ No newline at end of file diff --git a/Nodejs/Tests/NpmTests/PackageJsonTests.cs b/Nodejs/Tests/NpmTests/PackageJsonTests.cs index 3c30dfb78..39ea06fb7 100644 --- a/Nodejs/Tests/NpmTests/PackageJsonTests.cs +++ b/Nodejs/Tests/NpmTests/PackageJsonTests.cs @@ -225,35 +225,6 @@ public void TestGetEmptyScripts() { Assert.AreEqual(0, scripts.Count, "Shouldn't find any scripts."); } - [TestMethod, Priority(0)] - public void TestReadSingleStartScript() { - var pkg = LoadFrom(PkgStartScript); - var scripts = pkg.Scripts; - Assert.AreEqual(1, scripts.Count, "Should be a single script."); - IScript start = scripts[ScriptName.Start]; - Assert.IsNotNull(start, "Start script should not be null."); - Assert.AreEqual(ScriptName.Start, start.Name, "Script name mismatch."); - Assert.AreEqual("node server.js", start.Code, "Script code mismatch."); - } - - [TestMethod, Priority(0)] - public void TestReadNonExistentScriptsNull() { - var pkg = LoadFrom(PkgStartScript); - var scripts = pkg.Scripts; - - foreach (var name in new[]{ - ScriptName.Install, - ScriptName.Postinstall, - ScriptName.Postpublish, - ScriptName.Postrestart, - ScriptName.Poststart, - ScriptName.Poststop, - ScriptName.Posttest - }) { - Assert.IsNull(scripts[name], string.Format("Script '{0}' should be null.", name)); - } - } - [TestMethod, Priority(0)] public void TestReadNoDescriptionNull() { var pkg = LoadFrom(PkgEmpty); @@ -306,39 +277,6 @@ public void TestReadHomepageNonCompliant() { new[] { "http://www.mypackagehomepage.com/" }); } - [TestMethod, Priority(0)] - public void TestReadNoBugsNull() { - var pkg = LoadFrom(PkgSimple); - Assert.IsNull(pkg.Bugs, "Bugs should be null."); - } - - [TestMethod, Priority(0)] - public void TestReadBugsUrlOnly() { - var pkg = LoadFrom(PkgSimpleBugs); - var bugs = pkg.Bugs; - Assert.IsNotNull(bugs, "Bugs should not be null."); - Assert.AreEqual("http://www.mybugtracker.com/", bugs.Url, "Bugs URL mismatch."); - Assert.IsNull(bugs.Email, "Bugs email should be null."); - } - - private void TestReadBugsUrlAndEmail(string json) { - var pkg = LoadFrom(json); - var bugs = pkg.Bugs; - Assert.IsNotNull(bugs, "Bugs should not be null."); - Assert.AreEqual("http://www.example.com/bugs", bugs.Url, "Bugs URL mismatch."); - Assert.AreEqual("dev@example.com", bugs.Email, "Bugs email mismatch."); - } - - [TestMethod, Priority(0)] - public void TestReadBugsUrlAndEmailCompliant() { - TestReadBugsUrlAndEmail(PkgLargeCompliant); - } - - [TestMethod, Priority(0)] - public void TestReadBugsUrlAndEmailNonCompliant() { - TestReadBugsUrlAndEmail(PkgLargeNonCompliant); - } - [TestMethod, Priority(0)] public void TestReadNoLicensesEmpty() { var pkg = LoadFrom(PkgSimpleBugs); From c2d9093393787f0a122015461232c682c60d6fed Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 1 Apr 2016 14:25:30 -0700 Subject: [PATCH 04/11] Don't hold onto json in pkgstrarray --- Nodejs/Product/Npm/SPI/PkgStringArray.cs | 77 +++++++++--------------- 1 file changed, 30 insertions(+), 47 deletions(-) diff --git a/Nodejs/Product/Npm/SPI/PkgStringArray.cs b/Nodejs/Product/Npm/SPI/PkgStringArray.cs index 0eef8c0a7..4fca546c7 100644 --- a/Nodejs/Product/Npm/SPI/PkgStringArray.cs +++ b/Nodejs/Product/Npm/SPI/PkgStringArray.cs @@ -22,17 +22,33 @@ namespace Microsoft.NodejsTools.Npm.SPI { internal abstract class PkgStringArray : IPkgStringArray { - private readonly JObject _package; - private readonly string[] arrayPropertyNames; + private IList elements; protected PkgStringArray(JObject package, params string[] arrayPropertyNames) { - _package = package; - this.arrayPropertyNames = arrayPropertyNames; + + var token = GetArrayProperty(package, arrayPropertyNames); + if (token == null) { + elements = new List(); + } else { + switch (token.Type) { + case JTokenType.String: + elements = new[] { token.Value() }; + break; + + case JTokenType.Array: + elements = (token as JArray).Select(value => value.Value()).ToList(); + break; + + default: + elements = new List(); + break; + } + } } - private JToken GetArrayProperty() { + private static JToken GetArrayProperty(JObject package, string[] arrayPropertyNames) { foreach (var name in arrayPropertyNames) { - var array = _package[name] as JToken; + var array = package[name] as JToken; if (null != array) { return array; } @@ -42,21 +58,7 @@ private JToken GetArrayProperty() { public int Count { get { - var token = GetArrayProperty(); - if (null == token) { - return 0; - } - - switch (token.Type) { - case JTokenType.String: - return 1; - - case JTokenType.Array: - return (token as JArray).Count; - - default: - return 0; - } + return elements.Count; } } @@ -67,37 +69,18 @@ public string this[int index] { "Cannot retrieve item from empty package.json string array."); } - var token = GetArrayProperty(); - switch (token.Type) { - case JTokenType.String: - if (index != 0) { - throw new IndexOutOfRangeException( - string.Format( - "Cannot retrieve value from index '{0}' in a package.json string array containing only 1 item.", - index)); - } - return token.Value(); - - default: // Can only be an array at this point, since Count has been called. - return (token as JArray)[index].Value(); + if (index > Count) { + throw new IndexOutOfRangeException( + string.Format( + "Cannot retrieve value from index '{0}' in a package.json string array containing only 1 item.", + index)); } + return elements[index]; } } public IEnumerator GetEnumerator() { - switch (Count) { - case 0: - return new List().GetEnumerator(); - - case 1: - return new List { this[0] }.GetEnumerator(); - - default: - return - (GetArrayProperty() as JArray).Select(value => value.Value()) - .ToList() - .GetEnumerator(); - } + return elements.GetEnumerator(); } IEnumerator IEnumerable.GetEnumerator() { From 5c752081f7cfd9492d15d66fade4f6141a84a669 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 1 Apr 2016 14:35:38 -0700 Subject: [PATCH 05/11] Pre load deps list --- Nodejs/Product/Npm/SPI/Dependencies.cs | 30 ++++++++++---------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/Nodejs/Product/Npm/SPI/Dependencies.cs b/Nodejs/Product/Npm/SPI/Dependencies.cs index b3bf42a6b..d2336ccca 100644 --- a/Nodejs/Product/Npm/SPI/Dependencies.cs +++ b/Nodejs/Product/Npm/SPI/Dependencies.cs @@ -21,29 +21,21 @@ namespace Microsoft.NodejsTools.Npm.SPI { internal class Dependencies : IDependencies { - private JObject _package; - private string[] _dependencyPropertyNames; + private IList _dependencyProperties; public Dependencies(JObject package, params string[] dependencyPropertyNames) { - _package = package; - _dependencyPropertyNames = dependencyPropertyNames; - } - - private IEnumerable GetDependenciesProperties() { - foreach (var propertyName in _dependencyPropertyNames) { - var property = _package[propertyName] as JObject; - if (null != property) { - yield return property; - } - } + _dependencyProperties = dependencyPropertyNames + .Select(propertyName => package[propertyName] as JObject) + .Where(x => x != null) + .ToList(); } public IEnumerator GetEnumerator() { - var dependencyProps = GetDependenciesProperties(); - foreach (var dependencies in dependencyProps) { - var properties = null == dependencies ? new List() : dependencies.Properties(); - foreach (var property in properties) { - yield return new Dependency(property.Name, property.Value.Value()); + foreach (var dependencies in _dependencyProperties) { + if (dependencies != null) { + foreach (var property in dependencies.Properties()) { + yield return new Dependency(property.Name, property.Value.Value()); + } } } } @@ -58,7 +50,7 @@ public int Count { public IDependency this[string name] { get { - foreach (var dependencies in GetDependenciesProperties()) { + foreach (var dependencies in _dependencyProperties) { var property = dependencies[name]; if (null != property) { return new Dependency(name, property.Value()); From 12af3823303d265729095a16ebbe721f50d7def3 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 1 Apr 2016 14:58:13 -0700 Subject: [PATCH 06/11] Finish removing cycles. --- Nodejs/Product/Npm/IPackageJson.cs | 1 - Nodejs/Product/Npm/Npm.csproj | 2 - Nodejs/Product/Npm/SPI/Dependencies.cs | 18 +++----- Nodejs/Product/Npm/SPI/License.cs | 31 ------------- Nodejs/Product/Npm/SPI/Licenses.cs | 61 -------------------------- Nodejs/Product/Npm/SPI/PackageJson.cs | 11 +---- 6 files changed, 7 insertions(+), 117 deletions(-) delete mode 100644 Nodejs/Product/Npm/SPI/License.cs delete mode 100644 Nodejs/Product/Npm/SPI/Licenses.cs diff --git a/Nodejs/Product/Npm/IPackageJson.cs b/Nodejs/Product/Npm/IPackageJson.cs index e93474794..e28c00b20 100644 --- a/Nodejs/Product/Npm/IPackageJson.cs +++ b/Nodejs/Product/Npm/IPackageJson.cs @@ -24,7 +24,6 @@ public interface IPackageJson { string Description { get; } IKeywords Keywords { get; } IHomepages Homepages { get; } - ILicenses Licenses { get; } IFiles Files { get; } IMan Man { get; } IDependencies Dependencies { get; } diff --git a/Nodejs/Product/Npm/Npm.csproj b/Nodejs/Product/Npm/Npm.csproj index 8afd77a65..9dcdb7ba1 100644 --- a/Nodejs/Product/Npm/Npm.csproj +++ b/Nodejs/Product/Npm/Npm.csproj @@ -176,8 +176,6 @@ - - diff --git a/Nodejs/Product/Npm/SPI/Dependencies.cs b/Nodejs/Product/Npm/SPI/Dependencies.cs index d2336ccca..f2c49e7e5 100644 --- a/Nodejs/Product/Npm/SPI/Dependencies.cs +++ b/Nodejs/Product/Npm/SPI/Dependencies.cs @@ -21,23 +21,18 @@ namespace Microsoft.NodejsTools.Npm.SPI { internal class Dependencies : IDependencies { - private IList _dependencyProperties; + private IList _dependencyProperties; public Dependencies(JObject package, params string[] dependencyPropertyNames) { _dependencyProperties = dependencyPropertyNames .Select(propertyName => package[propertyName] as JObject) .Where(x => x != null) + .SelectMany(x => x.Properties().Select(property => new Dependency(property.Name, property.Value.Value()))) .ToList(); } public IEnumerator GetEnumerator() { - foreach (var dependencies in _dependencyProperties) { - if (dependencies != null) { - foreach (var property in dependencies.Properties()) { - yield return new Dependency(property.Name, property.Value.Value()); - } - } - } + return _dependencyProperties.GetEnumerator(); } IEnumerator IEnumerable.GetEnumerator() { @@ -50,10 +45,9 @@ public int Count { public IDependency this[string name] { get { - foreach (var dependencies in _dependencyProperties) { - var property = dependencies[name]; - if (null != property) { - return new Dependency(name, property.Value()); + foreach (var dependeny in _dependencyProperties) { + if (dependeny.Name == name) { + return dependeny; } } return null; diff --git a/Nodejs/Product/Npm/SPI/License.cs b/Nodejs/Product/Npm/SPI/License.cs deleted file mode 100644 index 71423f8dc..000000000 --- a/Nodejs/Product/Npm/SPI/License.cs +++ /dev/null @@ -1,31 +0,0 @@ -//*********************************************************// -// 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. -// -//*********************************************************// - -namespace Microsoft.NodejsTools.Npm.SPI { - internal class License : ILicense { - public License(string type) { - Type = type; - } - - public License(string type, string url) - : this(type) { - Url = url; - } - - public string Type { get; private set; } - public string Url { get; private set; } - } -} \ No newline at end of file diff --git a/Nodejs/Product/Npm/SPI/Licenses.cs b/Nodejs/Product/Npm/SPI/Licenses.cs deleted file mode 100644 index e8f3c4975..000000000 --- a/Nodejs/Product/Npm/SPI/Licenses.cs +++ /dev/null @@ -1,61 +0,0 @@ -//*********************************************************// -// 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 System; -using Newtonsoft.Json.Linq; - -namespace Microsoft.NodejsTools.Npm.SPI { - internal class Licenses : ILicenses { - private dynamic _json; - - public Licenses(dynamic package) { - _json = package.license; - } - - public int Count { - get { - if (_json != null) { - return 1; - } - - if (null == _json) { - return 0; - } - - return (_json as JArray).Count; - } - } - - public ILicense this[int index] { - get { - if (index < 0) { - throw new IndexOutOfRangeException(Resources.CannotRetrieveLicenseInvalidIndex); - } - - if (index == 0 && _json != null) { - return new License(_json.ToString()); - } - - if (null == _json) { - throw new IndexOutOfRangeException(Resources.CannotRetrieveLicenseEmptyCollection); - } - - var lic = _json[index]; - return new License(lic.type.ToString(), lic.url.ToString()); - } - } - } -} \ No newline at end of file diff --git a/Nodejs/Product/Npm/SPI/PackageJson.cs b/Nodejs/Product/Npm/SPI/PackageJson.cs index cf4c6b2fb..66ffe8e09 100644 --- a/Nodejs/Product/Npm/SPI/PackageJson.cs +++ b/Nodejs/Product/Npm/SPI/PackageJson.cs @@ -29,7 +29,6 @@ public PackageJson(dynamic package) { Keywords = LoadKeywords(package); Homepages = LoadHomepages(package); - Licenses = LoadLicenses(package); Files = LoadFiles(package); Man = LoadMan(package); Dependencies = LoadDependencies(package); @@ -76,14 +75,6 @@ private static IFiles LoadFiles(dynamic package) { } } - private static ILicenses LoadLicenses(dynamic package) { - try { - return new Licenses(package); - } catch (RuntimeBinderException rbe) { - throw WrapRuntimeBinderException("licenses",rbe); - } - } - private static IMan LoadMan(dynamic package) { try { return new Man(package); @@ -134,7 +125,7 @@ private static IDependencies LoadAllDependencies(dynamic package) { private static IEnumerable LoadRequiredBy(dynamic package) { try { - return (package["_requiredBy"] as IEnumerable ?? Enumerable.Empty()).Values(); + return (package["_requiredBy"] as IEnumerable ?? Enumerable.Empty()).Values().ToList(); } catch (RuntimeBinderException rbe) { System.Diagnostics.Debug.WriteLine(rbe); throw WrapRuntimeBinderException("required by", rbe); From b3a70fc3a8a8560c858fa3d52f05e436141cd40b Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 1 Apr 2016 15:01:28 -0700 Subject: [PATCH 07/11] Fix test reference --- Nodejs/Tests/NpmTests/PackageJsonTests.cs | 37 ----------------------- 1 file changed, 37 deletions(-) diff --git a/Nodejs/Tests/NpmTests/PackageJsonTests.cs b/Nodejs/Tests/NpmTests/PackageJsonTests.cs index 39ea06fb7..c57c3f3f1 100644 --- a/Nodejs/Tests/NpmTests/PackageJsonTests.cs +++ b/Nodejs/Tests/NpmTests/PackageJsonTests.cs @@ -217,14 +217,6 @@ public void TestReadNameAndVersion() { SemverVersionTestHelper.AssertVersionsEqual(0, 1, 0, null, null, pkgJson.Version); } - [TestMethod, Priority(0)] - public void TestGetEmptyScripts() { - var pkg = LoadFrom(PkgSimple); - var scripts = pkg.Scripts; - Assert.IsNotNull(scripts, "Scripts collection should not be null."); - Assert.AreEqual(0, scripts.Count, "Shouldn't find any scripts."); - } - [TestMethod, Priority(0)] public void TestReadNoDescriptionNull() { var pkg = LoadFrom(PkgEmpty); @@ -277,35 +269,6 @@ public void TestReadHomepageNonCompliant() { new[] { "http://www.mypackagehomepage.com/" }); } - [TestMethod, Priority(0)] - public void TestReadNoLicensesEmpty() { - var pkg = LoadFrom(PkgSimpleBugs); - var licenses = pkg.Licenses; - Assert.IsNotNull(licenses, "Licenses should not be null."); - Assert.AreEqual(0, licenses.Count, "Should not be any licenses."); - } - - [TestMethod, Priority(0)] - public void TestReadLicensesTypeOnly() { - var pkg = LoadFrom(PkgSingleLicenseType); - var licenses = pkg.Licenses; - Assert.AreEqual(1, licenses.Count, "License count mismatch."); - var license = licenses[0]; - Assert.IsNotNull(license, "License should not be null."); - Assert.AreEqual("BSD", license.Type, "License type mismatch."); - } - - [TestMethod, Priority(0)] - public void ReadLicensesTypeAndUrl() { - var pkg = LoadFrom(PkgLargeCompliant); - var licenses = pkg.Licenses; - Assert.AreEqual(1, licenses.Count, "License count mismatch."); - var license = licenses[0]; - Assert.IsNotNull(license, "License should not be null."); - Assert.AreEqual("GPLv2", license.Type, "License type mismatch."); - Assert.AreEqual("http://www.example.org/licenses/gpl.html", license.Url, "License URL mismatch."); - } - [TestMethod, Priority(0)] public void TestReadEmptyFilesEmpty() { CheckEmptyArray(LoadFrom(PkgSimple).Files); From 6115fc9f2a88da24d815a7f3b8878478be02f523 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 1 Apr 2016 15:58:21 -0700 Subject: [PATCH 08/11] Fix unit tests failing after a bit too much linq hijinks --- Nodejs/Product/Npm/SPI/Dependencies.cs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/Nodejs/Product/Npm/SPI/Dependencies.cs b/Nodejs/Product/Npm/SPI/Dependencies.cs index f2c49e7e5..7956f0d34 100644 --- a/Nodejs/Product/Npm/SPI/Dependencies.cs +++ b/Nodejs/Product/Npm/SPI/Dependencies.cs @@ -24,11 +24,15 @@ internal class Dependencies : IDependencies { private IList _dependencyProperties; public Dependencies(JObject package, params string[] dependencyPropertyNames) { - _dependencyProperties = dependencyPropertyNames - .Select(propertyName => package[propertyName] as JObject) - .Where(x => x != null) - .SelectMany(x => x.Properties().Select(property => new Dependency(property.Name, property.Value.Value()))) - .ToList(); + _dependencyProperties = new List(); + foreach (var propertyName in dependencyPropertyNames) { + var dependencies = package[propertyName] as JObject; + if (dependencies != null) { + foreach (var property in dependencies.Properties()) { + _dependencyProperties.Add(new Dependency(property.Name, property.Value.Value())); + } + } + } } public IEnumerator GetEnumerator() { From e7b3e84cfc78f6779b6c3c478b23acc6e3bffaef Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 1 Apr 2016 17:00:12 -0700 Subject: [PATCH 09/11] Really fix cast --- Nodejs/Product/Npm/SPI/Dependencies.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Nodejs/Product/Npm/SPI/Dependencies.cs b/Nodejs/Product/Npm/SPI/Dependencies.cs index 7956f0d34..9ac67c644 100644 --- a/Nodejs/Product/Npm/SPI/Dependencies.cs +++ b/Nodejs/Product/Npm/SPI/Dependencies.cs @@ -29,7 +29,9 @@ public Dependencies(JObject package, params string[] dependencyPropertyNames) { var dependencies = package[propertyName] as JObject; if (dependencies != null) { foreach (var property in dependencies.Properties()) { - _dependencyProperties.Add(new Dependency(property.Name, property.Value.Value())); + if (property.Value.Type == JTokenType.String) { + _dependencyProperties.Add(new Dependency(property.Name, property.Value.Value())); + } } } } From 7ad74af1b5ca288653858d2ec90be95f2b35360e Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Mon, 4 Apr 2016 11:25:59 -0700 Subject: [PATCH 10/11] Throw correct exception type for tests --- Nodejs/Product/Npm/SPI/PackageJson.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Nodejs/Product/Npm/SPI/PackageJson.cs b/Nodejs/Product/Npm/SPI/PackageJson.cs index 66ffe8e09..b9f79395e 100644 --- a/Nodejs/Product/Npm/SPI/PackageJson.cs +++ b/Nodejs/Product/Npm/SPI/PackageJson.cs @@ -22,11 +22,6 @@ namespace Microsoft.NodejsTools.Npm.SPI { internal class PackageJson : IPackageJson { public PackageJson(dynamic package) { - Name = package.name == null ? null : package.name.ToString(); - Version = package.version == null ? new SemverVersion() : SemverVersion.Parse(package.version.ToString()); - Description = package.description == null ? null : package.description.ToString(); - Author = package.author == null ? null : Person.CreateFromJsonSource(package.author.ToString()); - Keywords = LoadKeywords(package); Homepages = LoadHomepages(package); Files = LoadFiles(package); @@ -37,6 +32,11 @@ public PackageJson(dynamic package) { OptionalDependencies = LoadOptionalDependencies(package); AllDependencies = LoadAllDependencies(package); RequiredBy = LoadRequiredBy(package); + + Name = package.name == null ? null : package.name.ToString(); + Version = package.version == null ? new SemverVersion() : SemverVersion.Parse(package.version.ToString()); + Description = package.description == null ? null : package.description.ToString(); + Author = package.author == null ? null : Person.CreateFromJsonSource(package.author.ToString()); } private static PackageJsonException WrapRuntimeBinderException(string errorProperty, RuntimeBinderException rbe) { From f4f081480eab8f95caa374765f9135bce253815f Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Mon, 4 Apr 2016 12:26:42 -0700 Subject: [PATCH 11/11] Restore old exception version behavior --- Nodejs/Product/Npm/SPI/PackageJson.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Nodejs/Product/Npm/SPI/PackageJson.cs b/Nodejs/Product/Npm/SPI/PackageJson.cs index b9f79395e..4f78243be 100644 --- a/Nodejs/Product/Npm/SPI/PackageJson.cs +++ b/Nodejs/Product/Npm/SPI/PackageJson.cs @@ -21,6 +21,8 @@ namespace Microsoft.NodejsTools.Npm.SPI { internal class PackageJson : IPackageJson { + private string _versionString; + public PackageJson(dynamic package) { Keywords = LoadKeywords(package); Homepages = LoadHomepages(package); @@ -34,7 +36,7 @@ public PackageJson(dynamic package) { RequiredBy = LoadRequiredBy(package); Name = package.name == null ? null : package.name.ToString(); - Version = package.version == null ? new SemverVersion() : SemverVersion.Parse(package.version.ToString()); + _versionString = package.version; Description = package.description == null ? null : package.description.ToString(); Author = package.author == null ? null : Person.CreateFromJsonSource(package.author.ToString()); } @@ -134,7 +136,11 @@ private static IEnumerable LoadRequiredBy(dynamic package) { public string Name { get; private set; } - public SemverVersion Version { get; private set; } + public SemverVersion Version { + get { + return _versionString == null ? new SemverVersion() : SemverVersion.Parse(_versionString); + } + } public IPerson Author { get; private set; }