From 2fcc155be1a1abf29c3b7eb49ab706f1e1f2efca Mon Sep 17 00:00:00 2001 From: shishirh Date: Wed, 22 Mar 2017 17:46:05 -0700 Subject: [PATCH 1/5] Throw for invalid version ranges in strict check mode --- .../NuGet.Packaging/NuspecReader.cs | 180 ++++++++++-------- .../NuGet.Packaging.Test/NuspecReaderTests.cs | 35 ++++ 2 files changed, 135 insertions(+), 80 deletions(-) diff --git a/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs b/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs index 50332b881dc..8eb9442b11c 100644 --- a/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs +++ b/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs @@ -107,81 +107,15 @@ public NuspecReader(XDocument xml, IFrameworkNameProvider frameworkProvider) /// public IEnumerable GetDependencyGroups() { - var ns = MetadataNode.GetDefaultNamespace().NamespaceName; - - var groupFound = false; - - foreach (var depGroup in MetadataNode.Elements(XName.Get(Dependencies, ns)).Elements(XName.Get(Group, ns))) - { - groupFound = true; - - var groupFramework = GetAttributeValue(depGroup, TargetFramework); - - var packages = new HashSet(); - - foreach (var depNode in depGroup.Elements(XName.Get(Dependency, ns))) - { - VersionRange range = null; - - var rangeNode = GetAttributeValue(depNode, Version); - - if (!String.IsNullOrEmpty(rangeNode)) - { - VersionRange.TryParse(rangeNode, out range); - - Debug.Assert(range != null, "Unable to parse range: " + rangeNode); - } - - var includeFlags = GetFlags(GetAttributeValue(depNode, IncludeFlags)); - var excludeFlags = GetFlags(GetAttributeValue(depNode, ExcludeFlags)); - - var dependency = new PackageDependency( - GetAttributeValue(depNode, Id), - range, - includeFlags, - excludeFlags); - - packages.Add(dependency); - } - - var framework = String.IsNullOrEmpty(groupFramework) - ? NuGetFramework.AnyFramework - : NuGetFramework.Parse(groupFramework, _frameworkProvider); - - yield return new PackageDependencyGroup(framework, packages); - } - - // legacy behavior - if (!groupFound) - { - var depNodes = MetadataNode.Elements(XName.Get(Dependencies, ns)) - .Elements(XName.Get(Dependency, ns)); - - var packages = new HashSet(); - - foreach (var depNode in depNodes) - { - VersionRange range = null; - - var rangeNode = GetAttributeValue(depNode, Version); - - if (!String.IsNullOrEmpty(rangeNode)) - { - VersionRange.TryParse(rangeNode, out range); - - Debug.Assert(range != null, "Unable to parse range: " + rangeNode); - } - - packages.Add(new PackageDependency(GetAttributeValue(depNode, Id), range)); - } - - if (packages.Any()) - { - yield return new PackageDependencyGroup(NuGetFramework.AnyFramework, packages); - } - } + return GetAllDependencyGroups(useStrictVersionCheck: false); + } - yield break; + /// + /// Read package dependencies for all frameworks + /// + public IEnumerable GetDependencyGroups(bool useStrictVersionCheck) + { + return GetAllDependencyGroups(useStrictVersionCheck); } /// @@ -199,9 +133,9 @@ public IEnumerable GetReferenceGroups() var groupFramework = GetAttributeValue(group, TargetFramework); - var items = group.Elements(XName.Get(Reference, ns)).Select(n => GetAttributeValue(n, File)).Where(n => !String.IsNullOrEmpty(n)).ToArray(); + var items = group.Elements(XName.Get(Reference, ns)).Select(n => GetAttributeValue(n, File)).Where(n => !string.IsNullOrEmpty(n)).ToArray(); - var framework = String.IsNullOrEmpty(groupFramework) ? NuGetFramework.AnyFramework : NuGetFramework.Parse(groupFramework, _frameworkProvider); + var framework = string.IsNullOrEmpty(groupFramework) ? NuGetFramework.AnyFramework : NuGetFramework.Parse(groupFramework, _frameworkProvider); yield return new FrameworkSpecificGroup(framework, items); } @@ -210,7 +144,7 @@ public IEnumerable GetReferenceGroups() if (!groupFound) { var items = MetadataNode.Elements(XName.Get(References, ns)) - .Elements(XName.Get(Reference, ns)).Select(n => GetAttributeValue(n, File)).Where(n => !String.IsNullOrEmpty(n)).ToArray(); + .Elements(XName.Get(Reference, ns)).Select(n => GetAttributeValue(n, File)).Where(n => !string.IsNullOrEmpty(n)).ToArray(); if (items.Length > 0) { @@ -239,7 +173,7 @@ public IEnumerable GetFrameworkReferenceGroups() var frameworks = new List(); // Empty frameworks go under Any - if (String.IsNullOrEmpty(group.Key)) + if (string.IsNullOrEmpty(group.Key)) { frameworks.Add(NuGetFramework.AnyFramework); } @@ -247,7 +181,7 @@ public IEnumerable GetFrameworkReferenceGroups() { foreach (var fwString in group.Key.Split(CommaArray, StringSplitOptions.RemoveEmptyEntries)) { - if (!String.IsNullOrEmpty(fwString)) + if (!string.IsNullOrEmpty(fwString)) { frameworks.Add(NuGetFramework.Parse(fwString.Trim(), _frameworkProvider)); } @@ -265,7 +199,7 @@ public IEnumerable GetFrameworkReferenceGroups() } // Merge items and ignore duplicates - items.UnionWith(group.Select(item => GetAttributeValue(item, AssemblyName)).Where(item => !String.IsNullOrEmpty(item))); + items.UnionWith(group.Select(item => GetAttributeValue(item, AssemblyName)).Where(item => !string.IsNullOrEmpty(item))); } } @@ -465,6 +399,7 @@ private static string GetAttributeValue(XElement element, string attributeName) } private static readonly List EmptyList = new List(); + private static List GetFlags(string flags) { if (string.IsNullOrEmpty(flags)) @@ -479,5 +414,90 @@ private static List GetFlags(string flags) return set.OrderBy(s => s, StringComparer.OrdinalIgnoreCase).ToList(); } + + private IEnumerable GetAllDependencyGroups(bool useStrictVersionCheck) + { + var ns = MetadataNode.GetDefaultNamespace().NamespaceName; + var dependencyNode = MetadataNode + .Elements(XName.Get(Dependencies, ns)); + + var groupFound = false; + var dependencyGroups = dependencyNode + .Elements(XName.Get(Group, ns)); + + foreach (var depGroup in dependencyGroups) + { + groupFound = true; + + var groupFramework = GetAttributeValue(depGroup, TargetFramework); + + var dependencies = depGroup + .Elements(XName.Get(Dependency, ns)); + + var packages = GetPackageDependencies(dependencies, useStrictVersionCheck); + + var framework = string.IsNullOrEmpty(groupFramework) + ? NuGetFramework.AnyFramework + : NuGetFramework.Parse(groupFramework, _frameworkProvider); + + yield return new PackageDependencyGroup(framework, packages); + } + + // legacy behavior + if (!groupFound) + { + var legacyDependencies = dependencyNode + .Elements(XName.Get(Dependency, ns)); + + var packages = GetPackageDependencies(legacyDependencies, useStrictVersionCheck); + + if (packages.Any()) + { + yield return new PackageDependencyGroup(NuGetFramework.AnyFramework, packages); + } + } + + yield break; + } + + private HashSet GetPackageDependencies(IEnumerable nodes, bool useStrictVersionCheck) + { + var packages = new HashSet(); + + foreach (var depNode in nodes) + { + VersionRange range = null; + + var rangeNode = GetAttributeValue(depNode, Version); + + if (!string.IsNullOrEmpty(rangeNode)) + { + if (useStrictVersionCheck && !VersionRange.TryParse(rangeNode, out range)) + { + // Invalid version + var message = string.Format( + CultureInfo.CurrentCulture, + Strings.ErrorInvalidPackageVersion, + rangeNode, + GetIdentity()); + + throw new PackagingException(message); + } + } + + var includeFlags = GetFlags(GetAttributeValue(depNode, IncludeFlags)); + var excludeFlags = GetFlags(GetAttributeValue(depNode, ExcludeFlags)); + + var dependency = new PackageDependency( + GetAttributeValue(depNode, Id), + range, + includeFlags, + excludeFlags); + + packages.Add(dependency); + } + + return packages; + } } } diff --git a/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs b/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs index 19a56cd6c8d..18a6440ea40 100644 --- a/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs +++ b/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs @@ -255,6 +255,24 @@ public class NuspecReaderTests "; + private const string InvalidVersionRangeInDependency = @" + + + ServiceStack.Extras.Serilog + 2.0.1.0 + ServiceStack.Extras.Serilog + Alexey Zimarev, Nick Van Eeckhout + https://github.com/alexeyzimarev/ServiceStack.Extras.Serilog/blob/master/LICENSE + https://github.com/alexeyzimarev/ServiceStack.Extras.Serilog + Serilog logging adapter for ServiceStack + ServiceStack Serilog + + + + + + "; + [Fact] public void NuspecReaderTests_NamespaceOnMetadata() { @@ -307,6 +325,23 @@ public void NuspecReaderTests_DuplicateDependencyGroups() Assert.Equal(2, dependencies.Count); } + [Fact] + public void NuspecReaderTests_InvalidVersionRangeInDependency() + { + NuspecReader reader = GetReader(InvalidVersionRangeInDependency); + try + { + var dependencies = reader.GetDependencyGroups(useStrictVersion: true).ToList(); + Assert.True(false, "No exception was thrown"); + } + catch(Exception ex) + { + Assert.NotNull(ex); + // check the message contains invalid version range + Assert.NotNull(ex.Message); + } + } + [Fact] public void NuspecReaderTests_FrameworkGroups() { From fcbdc8f647480a44315c8ecfeeaa7ee4ab19575e Mon Sep 17 00:00:00 2001 From: shishirh Date: Thu, 23 Mar 2017 14:14:26 -0700 Subject: [PATCH 2/5] Add test coverage, fix a bug, refactor common code --- .../NuGet.Packaging/NuspecReader.cs | 3 +- .../NuGet.Packaging.Test/NuspecReaderTests.cs | 137 +++++++++++++++--- 2 files changed, 121 insertions(+), 19 deletions(-) diff --git a/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs b/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs index 8eb9442b11c..6b21f7edaf3 100644 --- a/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs +++ b/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs @@ -472,7 +472,8 @@ private HashSet GetPackageDependencies(IEnumerable if (!string.IsNullOrEmpty(rangeNode)) { - if (useStrictVersionCheck && !VersionRange.TryParse(rangeNode, out range)) + var versionParsedSuccessfully = VersionRange.TryParse(rangeNode, out range); + if (!versionParsedSuccessfully && useStrictVersionCheck) { // Invalid version var message = string.Format( diff --git a/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs b/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs index 18a6440ea40..2641622f75d 100644 --- a/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs +++ b/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs @@ -7,6 +7,8 @@ using System.Text; using NuGet.Frameworks; using Xunit; +using NuGet.Packaging.Core; +using System.Collections.Generic; namespace NuGet.Packaging.Test { @@ -255,24 +257,83 @@ public class NuspecReaderTests "; - private const string InvalidVersionRangeInDependency = @" + private const string VersionRangeInDependency = @" - ServiceStack.Extras.Serilog + PackageA 2.0.1.0 - ServiceStack.Extras.Serilog - Alexey Zimarev, Nick Van Eeckhout - https://github.com/alexeyzimarev/ServiceStack.Extras.Serilog/blob/master/LICENSE - https://github.com/alexeyzimarev/ServiceStack.Extras.Serilog - Serilog logging adapter for ServiceStack + Package A + ownera, ownerb + Package A description ServiceStack Serilog - - + "; + public static IEnumerable GetValidVersions() + { + return GetVersionRange(validVersions: true); + } + + public static IEnumerable GetInValidVersions() + { + return GetVersionRange(validVersions: false); + } + + private static IEnumerable GetVersionRange(bool validVersions) + { + IEnumerable range = validVersions + ? ValidVersionRange() + : InvalidVersionRange(); + + foreach (var s in range) + { + yield return new object[] { s }; + } + } + + private static IEnumerable ValidVersionRange() + { + yield return null; + yield return "0.0.0"; + yield return "1.0.0"; + yield return "0.0.1"; + yield return "1.0.0-BETA"; + yield return "1.0.0"; + yield return "1.0"; + yield return "1.0.0.0"; + yield return "1.0.1"; + yield return "1.0.01"; + yield return "00000001.000000000.0000000001"; + yield return "00000001.000000000.0000000001-beta"; + yield return "1.0.01-alpha"; + yield return "1.0.1-alpha.1.2.3"; + yield return "1.0.1-alpha.1.2.3+metadata"; + yield return "1.0.1-alpha.1.2.3+a.b.c.d"; + yield return "1.0.1+metadata"; + yield return "1.0.1+security.fix.ce38429"; + yield return "1.0.1-alpha.10.a"; + yield return "1.0.1--"; + yield return "1.0.1-a.really.long.version.release.label"; + yield return "1238234.198231.2924324.2343432"; + yield return "1238234.198231.2924324.2343432+final"; + yield return "00.00.00.00-alpha"; + yield return "0.0-alpha.1"; + yield return "9.9.9-9"; + } + + private static IEnumerable InvalidVersionRange() + { + yield return "~1"; + yield return "~1.0.0"; + yield return "0.0.0-~4"; + yield return "$version$"; + yield return "Invalid"; + yield return "[15.106.0.preview]"; + } + [Fact] public void NuspecReaderTests_NamespaceOnMetadata() { @@ -325,20 +386,60 @@ public void NuspecReaderTests_DuplicateDependencyGroups() Assert.Equal(2, dependencies.Count); } - [Fact] - public void NuspecReaderTests_InvalidVersionRangeInDependency() + [Theory] + [MemberData("GetValidVersions")] + [MemberData("GetInValidVersions")] + public void NuspecReaderTests_NonStrictCheckInDependencyShouldNotThrowException(string versionRange) + { + var formattedNuspec = string.Format(VersionRangeInDependency, versionRange); + NuspecReader reader = GetReader(formattedNuspec); + try + { + var dependencies = reader.GetDependencyGroups().ToList(); + Assert.Equal(1, dependencies.Count); + } + catch (Exception) + { + Assert.True(false, "Exception should not be thrown for any version range"); + } + } + + [Theory] + [MemberData("GetInValidVersions")] + public void NuspecReaderTests_InvalidVersionRangeInDependencyThrowsExceptionForStrictCheck(string versionRange) + { + var formattedNuspec = string.Format(VersionRangeInDependency, versionRange); + NuspecReader reader = GetReader(formattedNuspec); + try + { + var dependencies = reader.GetDependencyGroups(useStrictVersionCheck: true).ToList(); + Assert.True(false, "Failed to throw exception."); + } + catch (PackagingException pex) + { + Assert.NotNull(pex); + Assert.NotNull(pex.Message); + } + catch (Exception) + { + Assert.True(false, "PackagingException was expected"); + } + } + + [Theory] + [MemberData("GetValidVersions")] + public void NuspecReaderTests_ValidVersionRangeInDependencyReturnsResultForStrictCheck(string versionRange) { - NuspecReader reader = GetReader(InvalidVersionRangeInDependency); + var formattedNuspec = string.Format(VersionRangeInDependency, versionRange); + NuspecReader reader = GetReader(formattedNuspec); try { - var dependencies = reader.GetDependencyGroups(useStrictVersion: true).ToList(); - Assert.True(false, "No exception was thrown"); + var dependencies = reader.GetDependencyGroups(useStrictVersionCheck: true).ToList(); + Assert.Equal(1, dependencies.Count); } - catch(Exception ex) + catch (Exception) { - Assert.NotNull(ex); - // check the message contains invalid version range - Assert.NotNull(ex.Message); + Assert.True(false, "Exception should not be thrown for valid version range"); } } From 959ac9f5f6a215ca5dac52abf36a69a7ce02f860 Mon Sep 17 00:00:00 2001 From: shishirh Date: Thu, 23 Mar 2017 14:48:41 -0700 Subject: [PATCH 3/5] Add message for invalid version in dependency --- src/NuGet.Core/NuGet.Packaging/NuspecReader.cs | 8 +++++--- src/NuGet.Core/NuGet.Packaging/Strings.Designer.cs | 9 +++++++++ src/NuGet.Core/NuGet.Packaging/Strings.resx | 3 +++ 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs b/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs index 6b21f7edaf3..5743cf7d3df 100644 --- a/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs +++ b/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs @@ -476,11 +476,13 @@ private HashSet GetPackageDependencies(IEnumerable if (!versionParsedSuccessfully && useStrictVersionCheck) { // Invalid version + var dependencyId = GetAttributeValue(depNode, Id); var message = string.Format( CultureInfo.CurrentCulture, - Strings.ErrorInvalidPackageVersion, - rangeNode, - GetIdentity()); + Strings.ErrorInvalidPackageVersionForDependency, + dependencyId, + GetIdentity(), + rangeNode); throw new PackagingException(message); } diff --git a/src/NuGet.Core/NuGet.Packaging/Strings.Designer.cs b/src/NuGet.Core/NuGet.Packaging/Strings.Designer.cs index cafdfce809a..855ebfe0da7 100644 --- a/src/NuGet.Core/NuGet.Packaging/Strings.Designer.cs +++ b/src/NuGet.Core/NuGet.Packaging/Strings.Designer.cs @@ -104,6 +104,15 @@ public static string ErrorInvalidPackageVersion { } } + /// + /// Looks up a localized string similar to Invalid package version for a dependency with id '{0}' in package '{1}': '{2}'. + /// + public static string ErrorInvalidPackageVersionForDependency { + get { + return ResourceManager.GetString("ErrorInvalidPackageVersionForDependency", resourceCulture); + } + } + /// /// Looks up a localized string similar to Null or empty package id. /// diff --git a/src/NuGet.Core/NuGet.Packaging/Strings.resx b/src/NuGet.Core/NuGet.Packaging/Strings.resx index ee53f7c37a1..51cc42ff470 100644 --- a/src/NuGet.Core/NuGet.Packaging/Strings.resx +++ b/src/NuGet.Core/NuGet.Packaging/Strings.resx @@ -129,6 +129,9 @@ Invalid package version for package id '{0}': '{1}' + + Invalid package version for a dependency with id '{0}' in package '{1}': '{2}' + Null or empty package id From 51133439887ab49ad5ea0561efb0f343672c1a6c Mon Sep 17 00:00:00 2001 From: Xavier Decoster Date: Mon, 27 Mar 2017 10:23:01 +0200 Subject: [PATCH 4/5] Code review feedback --- src/NuGet.Core/NuGet.Packaging/Strings.resx | 3 + .../NuGet.Packaging.Test/NuspecReaderTests.cs | 108 +++++++++--------- 2 files changed, 55 insertions(+), 56 deletions(-) diff --git a/src/NuGet.Core/NuGet.Packaging/Strings.resx b/src/NuGet.Core/NuGet.Packaging/Strings.resx index 51cc42ff470..64b8f5a77c2 100644 --- a/src/NuGet.Core/NuGet.Packaging/Strings.resx +++ b/src/NuGet.Core/NuGet.Packaging/Strings.resx @@ -131,6 +131,9 @@ Invalid package version for a dependency with id '{0}' in package '{1}': '{2}' + {0} is the dependency's package id. +{1} is the package's identity. +{2} is the dependency's version range. Null or empty package id diff --git a/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs b/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs index 2641622f75d..21476639cac 100644 --- a/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs +++ b/test/NuGet.Core.Tests/NuGet.Packaging.Test/NuspecReaderTests.cs @@ -9,6 +9,7 @@ using Xunit; using NuGet.Packaging.Core; using System.Collections.Generic; +using NuGet.Versioning; namespace NuGet.Packaging.Test { @@ -297,31 +298,20 @@ private static IEnumerable GetVersionRange(bool validVersions) private static IEnumerable ValidVersionRange() { yield return null; + yield return string.Empty; yield return "0.0.0"; - yield return "1.0.0"; - yield return "0.0.1"; - yield return "1.0.0-BETA"; - yield return "1.0.0"; - yield return "1.0"; - yield return "1.0.0.0"; - yield return "1.0.1"; - yield return "1.0.01"; - yield return "00000001.000000000.0000000001"; - yield return "00000001.000000000.0000000001-beta"; - yield return "1.0.01-alpha"; + yield return "1.0.0-beta"; yield return "1.0.1-alpha.1.2.3"; - yield return "1.0.1-alpha.1.2.3+metadata"; yield return "1.0.1-alpha.1.2.3+a.b.c.d"; yield return "1.0.1+metadata"; - yield return "1.0.1+security.fix.ce38429"; - yield return "1.0.1-alpha.10.a"; yield return "1.0.1--"; yield return "1.0.1-a.really.long.version.release.label"; - yield return "1238234.198231.2924324.2343432"; - yield return "1238234.198231.2924324.2343432+final"; yield return "00.00.00.00-alpha"; yield return "0.0-alpha.1"; - yield return "9.9.9-9"; + yield return "(1.0.0-alpha.1, )"; + yield return "[1.0.0-alpha.1+metadata]"; + yield return "[1.0, 2.0.0+metadata)"; + yield return "[1.0+metadata, 2.0.0+metadata)"; } private static IEnumerable InvalidVersionRange() @@ -332,6 +322,7 @@ private static IEnumerable InvalidVersionRange() yield return "$version$"; yield return "Invalid"; yield return "[15.106.0.preview]"; + yield return "15.106.0-preview.01"; // no leading zeros in numeric identifiers of release label } [Fact] @@ -387,60 +378,65 @@ public void NuspecReaderTests_DuplicateDependencyGroups() } [Theory] - [MemberData("GetValidVersions")] - [MemberData("GetInValidVersions")] + [MemberData(nameof(GetValidVersions))] + [MemberData(nameof(GetInValidVersions))] public void NuspecReaderTests_NonStrictCheckInDependencyShouldNotThrowException(string versionRange) { + // Arrange var formattedNuspec = string.Format(VersionRangeInDependency, versionRange); - NuspecReader reader = GetReader(formattedNuspec); - try - { - var dependencies = reader.GetDependencyGroups().ToList(); - Assert.Equal(1, dependencies.Count); - } - catch (Exception) - { - Assert.True(false, "Exception should not be thrown for any version range"); - } + var nuspecReader = GetReader(formattedNuspec); + + // Act + var dependencies = nuspecReader.GetDependencyGroups().ToList(); + + // Assert + Assert.Equal(1, dependencies.Count); + } + + [Theory] + [MemberData(nameof(GetInValidVersions))] + public void NuspecReaderTests_NonStrictCheckInDependencyShouldFallbackToAllRangeForInvalidVersions( + string versionRange) + { + // Arrange + var formattedNuspec = string.Format(VersionRangeInDependency, versionRange); + var nuspecReader = GetReader(formattedNuspec); + + // Act + var dependencies = nuspecReader.GetDependencyGroups().ToList(); + + // Assert + Assert.Equal(VersionRange.All, dependencies.First().Packages.First().VersionRange); } [Theory] - [MemberData("GetInValidVersions")] + [MemberData(nameof(GetInValidVersions))] public void NuspecReaderTests_InvalidVersionRangeInDependencyThrowsExceptionForStrictCheck(string versionRange) { + // Arrange var formattedNuspec = string.Format(VersionRangeInDependency, versionRange); - NuspecReader reader = GetReader(formattedNuspec); - try - { - var dependencies = reader.GetDependencyGroups(useStrictVersionCheck: true).ToList(); - Assert.True(false, "Failed to throw exception."); - } - catch (PackagingException pex) - { - Assert.NotNull(pex); - Assert.NotNull(pex.Message); - } - catch (Exception) - { - Assert.True(false, "PackagingException was expected"); - } + var nuspecReader = GetReader(formattedNuspec); + Action action = () => nuspecReader.GetDependencyGroups(useStrictVersionCheck: true).ToList(); + + // Act & Assert + Assert.Throws(action); } [Theory] - [MemberData("GetValidVersions")] + [MemberData(nameof(GetValidVersions))] public void NuspecReaderTests_ValidVersionRangeInDependencyReturnsResultForStrictCheck(string versionRange) { + // Arrange var formattedNuspec = string.Format(VersionRangeInDependency, versionRange); - NuspecReader reader = GetReader(formattedNuspec); - try - { - var dependencies = reader.GetDependencyGroups(useStrictVersionCheck: true).ToList(); - Assert.Equal(1, dependencies.Count); - } - catch (Exception) - { - Assert.True(false, "Exception should not be thrown for valid version range"); - } + var nuspecReader = GetReader(formattedNuspec); + var expectedVersionRange = string.IsNullOrEmpty(versionRange) ? VersionRange.All : VersionRange.Parse(versionRange); + + // Act + var dependencyGroups = nuspecReader.GetDependencyGroups(useStrictVersionCheck: true).ToList(); + + // Assert + Assert.Equal(1, dependencyGroups.Count); + Assert.Equal(expectedVersionRange, dependencyGroups.First().Packages.First().VersionRange); } [Fact] From 0a35c93c3342ce7527bc5143b635b55f9a57ba28 Mon Sep 17 00:00:00 2001 From: Xavier Decoster Date: Mon, 27 Mar 2017 18:03:59 +0200 Subject: [PATCH 5/5] Code review feedback --- .../NuGet.Packaging/NuspecReader.cs | 88 +++++++++---------- 1 file changed, 40 insertions(+), 48 deletions(-) diff --git a/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs b/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs index 5743cf7d3df..cc442426d48 100644 --- a/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs +++ b/src/NuGet.Core/NuGet.Packaging/NuspecReader.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Diagnostics; using System.Globalization; using System.IO; using System.Linq; @@ -107,7 +106,7 @@ public NuspecReader(XDocument xml, IFrameworkNameProvider frameworkProvider) /// public IEnumerable GetDependencyGroups() { - return GetAllDependencyGroups(useStrictVersionCheck: false); + return GetDependencyGroups(useStrictVersionCheck: false); } /// @@ -115,7 +114,45 @@ public IEnumerable GetDependencyGroups() /// public IEnumerable GetDependencyGroups(bool useStrictVersionCheck) { - return GetAllDependencyGroups(useStrictVersionCheck); + var ns = MetadataNode.GetDefaultNamespace().NamespaceName; + var dependencyNode = MetadataNode + .Elements(XName.Get(Dependencies, ns)); + + var groupFound = false; + var dependencyGroups = dependencyNode + .Elements(XName.Get(Group, ns)); + + foreach (var depGroup in dependencyGroups) + { + groupFound = true; + + var groupFramework = GetAttributeValue(depGroup, TargetFramework); + + var dependencies = depGroup + .Elements(XName.Get(Dependency, ns)); + + var packages = GetPackageDependencies(dependencies, useStrictVersionCheck); + + var framework = string.IsNullOrEmpty(groupFramework) + ? NuGetFramework.AnyFramework + : NuGetFramework.Parse(groupFramework, _frameworkProvider); + + yield return new PackageDependencyGroup(framework, packages); + } + + // legacy behavior + if (!groupFound) + { + var legacyDependencies = dependencyNode + .Elements(XName.Get(Dependency, ns)); + + var packages = GetPackageDependencies(legacyDependencies, useStrictVersionCheck); + + if (packages.Any()) + { + yield return new PackageDependencyGroup(NuGetFramework.AnyFramework, packages); + } + } } /// @@ -415,51 +452,6 @@ private static List GetFlags(string flags) return set.OrderBy(s => s, StringComparer.OrdinalIgnoreCase).ToList(); } - private IEnumerable GetAllDependencyGroups(bool useStrictVersionCheck) - { - var ns = MetadataNode.GetDefaultNamespace().NamespaceName; - var dependencyNode = MetadataNode - .Elements(XName.Get(Dependencies, ns)); - - var groupFound = false; - var dependencyGroups = dependencyNode - .Elements(XName.Get(Group, ns)); - - foreach (var depGroup in dependencyGroups) - { - groupFound = true; - - var groupFramework = GetAttributeValue(depGroup, TargetFramework); - - var dependencies = depGroup - .Elements(XName.Get(Dependency, ns)); - - var packages = GetPackageDependencies(dependencies, useStrictVersionCheck); - - var framework = string.IsNullOrEmpty(groupFramework) - ? NuGetFramework.AnyFramework - : NuGetFramework.Parse(groupFramework, _frameworkProvider); - - yield return new PackageDependencyGroup(framework, packages); - } - - // legacy behavior - if (!groupFound) - { - var legacyDependencies = dependencyNode - .Elements(XName.Get(Dependency, ns)); - - var packages = GetPackageDependencies(legacyDependencies, useStrictVersionCheck); - - if (packages.Any()) - { - yield return new PackageDependencyGroup(NuGetFramework.AnyFramework, packages); - } - } - - yield break; - } - private HashSet GetPackageDependencies(IEnumerable nodes, bool useStrictVersionCheck) { var packages = new HashSet();