Update feed2catalog and catalog2registration to properly handle SemVer 2.0.0 #145

joelverhagen · 2017-03-24T23:47:20Z

Write full version to catalog entry JSON
Use normalized version for catalog URLs
Use normalized version for downloading packages from the gallery
Use normalized version for registration URLs and min/max ranges
Write full version to registration entry JSON
Write SemVer 2.0.0 packages to a new registration container

Addresses NuGet/NuGetGallery#3560 and NuGet/NuGetGallery#3680.

joelverhagen · 2017-03-28T01:13:48Z

🔔

joelverhagen · 2017-03-28T23:47:43Z

Will rebase to fix merge conflict. 🔔

joelverhagen · 2017-03-29T00:02:12Z

Rebased. Ready for some 👀

xavierdecoster · 2017-03-29T08:34:47Z

src/Catalog/Helpers/NuGetVersionUtility.cs

+ }
+
+ public static bool IsGraphSemVer2(string version, string resourceUri, IGraph graph)
+ {


May need some argument checks on this utility class.

xavierdecoster · 2017-03-29T08:38:34Z

src/Catalog/Registration/RegistrationCollector.cs

 : base(index, new Uri[] { Schema.DataTypes.PackageDetails, Schema.DataTypes.PackageDelete }, handlerFunc)
 {
- _storageFactory = storageFactory;
+ _semVer1StorageFactory = semVer1StorageFactory;


nit: technically, it is not a SemVer v1.0 storage factory, as it may also contain System.Versioning-style packages. Perhaps rename to legacyStorageFactory, or fallbackStorageFactory, or something along those lines?

Legacy seems right. However I think our the definition in NuGet world is always going to be blurred. For example 1.0.0.1-alpha.1 is considered "SemVer 2.0.0" by NuGet.

…r 2.0.0

…PathProvider

skofman1 · 2017-03-29T17:25:37Z

src/Catalog/Helpers/XsltHelper.cs

@@ -40,22 +42,17 @@ public string LowerCase(string original)

 public string NormalizeVersion(string original)


do we need those thin wrappers? Can use NuGetVersionUtility directly

I chose to wrap thinly here because XsltHelper needs to be called via an instance method. NuGetVersionUtility is static so it's easily consumable by different jobs.

skofman1 · 2017-03-29T17:34:58Z

src/Catalog/Registration/RegistrationCatalogEntry.cs

 using System;
 using System.Collections.Generic;
 using System.Linq;
 using VDS.RDF;

 namespace NuGet.Services.Metadata.Catalog.Registration
 {
+ public delegate bool ShouldIncludeRegistrationPackage(RegistrationEntryKey key, string resourceUri, IGraph graph);


move the delegate inside ShouldIncludeRegistrationPackage. Will make the code more readable.
Also, it's not clear how it's used. You can improve the name or add a comment.

Not sure what you mean by:

move the delegate inside ShouldIncludeRegistrationPackage

The delegate itself is called ShouldIncludeRegistrationPackage. I chose not to put it inside RegistrationCatalogEntry because this delegate is passed from a couple levels up, which do not have knowledge of RegistrationCatalogEntry.

In general, delegates are just types so I see no reason why they should be nested.

I meant to move it inside RegistrationCatalogEntry, but I agree with your logic. However, the name is still confusing and needs a comment/better name.

Latest revision has XML docs now.

skofman1 · 2017-03-29T17:36:21Z

src/Catalog/Registration/RegistrationCollector.cs

 {
- return RegistrationMaker.Process(
+ await RegistrationMaker.Process(


please add named parameters to improve readability

skofman1 · 2017-03-29T17:37:28Z

src/Catalog/Registration/RegistrationCollector.cs

 : base(index, new Uri[] { Schema.DataTypes.PackageDetails, Schema.DataTypes.PackageDelete }, handlerFunc)
 {
- _storageFactory = storageFactory;
+ _semVer1StorageFactory = semVer1StorageFactory;


null checks

skofman1 · 2017-03-29T18:26:55Z

scottbommarito

nitpicks

scottbommarito · 2017-03-29T20:10:04Z

src/Ng/Jobs/Catalog2RegistrationJob.cs

@@ -60,10 +68,11 @@ protected override void Init(IDictionary<string, string> arguments, Cancellation

 var contentBaseAddress = arguments.GetOrDefault<string>(Arguments.ContentBaseAddress);

- StorageFactory storageFactoryToUse;
+ StorageFactory legacyStorageFactory;


I think it may be worthwhile to have a comment here (and potentially in RegistrationCollector as well) about what precisely is "legacy" about the legacyStorageFactory.

As far as I understand, the legacyStorageFactory writes non-gzipped non-semver2 registration blobs and gzipped non-semver2 registration blobs. These are considered legacy because vCurrent nuget.exe always reads the gzipped semver2 registration blobs.

scottbommarito · 2017-03-29T23:47:36Z

tests/CatalogTests/Registration/RegistrationCatalogEntryTests.cs

+ {
+ // Arrange
+ var graph = InitializeGraph(ResourceUri, Id, Version);
+ var isDelegateInvoked = false;


nitpick: use a helper function/class to share the delegate code between tests

private class IsDelegateInvokedHelper { public IsDelegateInvokedHelper() { IsDelegateInvoked = false; ShouldInclude = (k, u, g) => { ... } } public ShouldIncludeRegistrationPackage ShouldInclude { get; private set; } public bool IsDelegateInvoked {get; private set; } }

scottbommarito · 2017-03-29T23:48:12Z

tests/NgTests/Feed2CatalogTests.cs

+ Listed = false,
+
+ Created = new DateTime(2015, 1, 1),
+ LastEdited = new DateTime(2015, 1, 1),


nitpick:

I understand the other ODataPackages do this as well, but I don't think it's possible to have a Created and LastEdited be the exact same. We should probably test realistic values rather than those that cannot happen.

scottbommarito · 2017-03-29T23:53:17Z

tests/NgTests/RegistrationCollectorTests.cs

+ public async Task CreatesRegistrationsWithSemVer2()
+ {
+ // Arrange
+ var catalogStorage = Catalogs.CreateTestCatalogWithSemVer2Package();


nitpick:

a lot of the code here is nearly identical to that in SharedInit, just with the added SemVer2 storage. I would suggest modifying SharedInit to support SemVer2 storage as well (private void SharedInit(bool useSemVer2).

scottbommarito · 2017-03-29T23:54:08Z

tests/NgTests/RegistrationCollectorTests.cs

+ public async Task IgnoresSemVer2Packages()
+ {
+ // Arrange
+ var catalogStorage = Catalogs.CreateTestCatalogWithSemVer2Package();


Use SharedInit?

scottbommarito · 2017-03-29T23:55:54Z

tests/NgTests/StorageFactoryTests.cs

- { Arguments.StorageType, storageType},
- { Arguments.UseCompressedStorage, "false"}
+ { Arguments.StorageType, storageType },
+ { Arguments.UseCompressedStorage, "false" }


typos in method names in this class
AzureCommpressedFactoryNull -> AzureCompressedFactoryNull
AzureCommpressedFactory -> AzureCompressedFactory

scottbommarito · 2017-03-29T23:57:21Z

tests/NgTests/TestableFeed2CatalogJob.cs

@@ -61,12 +61,12 @@ internal class TestLogger : ILogger
 {
 public void Log(LogLevel logLevel, int eventId, object state, Exception exception, Func<object, Exception, string> formatter)
 {
- Console.WriteLine($"{logLevel}: {formatter(state, exception)}");
+ // Console.WriteLine($"{logLevel}: {formatter(state, exception)}");


This was causing tests to have a lot of stdout (locally and on CI). 99% of the time, you only fail about the failure result, not the stdout.

joelverhagen · 2017-03-30T16:22:05Z

Merged into "semver2" feature branch until I do some more validation.

…r 2.0.0 (#145) * Update feed2catalog and catalog2registration to properly handle SemVer 2.0.0 * Add testing for NuGetVersionUtility * Add integration tests for creation of catalog items (feed2catalog) * Add tests for PackageCatalogItem * Add unit tests for RegistrationCatalogEntry and PackagesFolderPackagePathProvider * Only call the SemVer 2.0.0 storage factory if it was provided * Add tests for the SemVer 2.0.0 storage factory * Update Feed2Catalog tests to add a SemVer2 package * Add functional tests for registration collector * Addresses NuGet/NuGetGallery#3560 * Addresses NuGet/NuGetGallery#3680

joelverhagen requested review from xavierdecoster, ryuyu, dtivel and cristinamanum March 24, 2017 23:47

dnfclas added the cla-already-signed label Mar 24, 2017

joelverhagen requested a review from skofman1 March 28, 2017 23:47

xavierdecoster reviewed Mar 29, 2017

View reviewed changes

joelverhagen requested review from chenriksson and scottbommarito March 29, 2017 15:32

joelverhagen added 12 commits March 29, 2017 10:15

Update feed2catalog and catalog2registration to properly handle SemVe…

1f1a3bf

…r 2.0.0

Add testing for NuGetVersionUtility

8e38ffa

Add integration tests for creation of catalog items (feed2catalog)

a6691f7

Add tests for PackageCatalogItem

6e49f1d

Add unit tests for RegistrationCatalogEntry and PackagesFolderPackage…

fa00cbf

…PathProvider

Only call the SemVer 2.0.0 storage factory if it was provided

9b6acf0

Add tests for the SemVer 2.0.0 storage factory

d9e3b5d

Include the new test assembly

43f283c

Update Feed2Catalog tests to add a SemVer2 package

39a869f

Add functional tests for registration collector

ee0f9cc

Fix merge

37c7804

Address comments

dbbb703

skofman1 reviewed Mar 29, 2017

View reviewed changes

Address comments

868e51b

joelverhagen added 2 commits March 29, 2017 12:40

Polish comments

aa89b84

Add null checks

7d19314

xavierdecoster approved these changes Mar 29, 2017

View reviewed changes

joelverhagen mentioned this pull request Mar 29, 2017

Update lightning to handle the new SemVer 2.0.0 container #149

Merged

scottbommarito approved these changes Mar 29, 2017

View reviewed changes

Address comments

0f5e4ce

joelverhagen changed the base branch from dev to semver2 March 30, 2017 16:20

joelverhagen merged commit 47f4f0d into NuGet:semver2 Mar 30, 2017

joelverhagen deleted the semver2-reg branch March 30, 2017 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update feed2catalog and catalog2registration to properly handle SemVer 2.0.0 #145

Update feed2catalog and catalog2registration to properly handle SemVer 2.0.0 #145

joelverhagen commented Mar 24, 2017

joelverhagen commented Mar 28, 2017

joelverhagen commented Mar 28, 2017

joelverhagen commented Mar 29, 2017

xavierdecoster Mar 29, 2017

joelverhagen Mar 29, 2017

xavierdecoster Mar 29, 2017

joelverhagen Mar 29, 2017

joelverhagen Mar 29, 2017

skofman1 Mar 29, 2017

joelverhagen Mar 29, 2017

skofman1 Mar 29, 2017

joelverhagen Mar 29, 2017

skofman1 Mar 29, 2017

joelverhagen Mar 29, 2017

skofman1 Mar 29, 2017

joelverhagen Mar 29, 2017

skofman1 Mar 29, 2017

joelverhagen Mar 29, 2017

skofman1 commented Mar 29, 2017

scottbommarito left a comment

scottbommarito Mar 29, 2017

joelverhagen Mar 30, 2017

scottbommarito Mar 29, 2017 •

edited

Loading

scottbommarito Mar 29, 2017

scottbommarito Mar 29, 2017

scottbommarito Mar 29, 2017

scottbommarito Mar 29, 2017

scottbommarito Mar 29, 2017

joelverhagen Mar 30, 2017

joelverhagen commented Mar 30, 2017

		@@ -40,22 +42,17 @@ public string LowerCase(string original)

		public string NormalizeVersion(string original)

Update feed2catalog and catalog2registration to properly handle SemVer 2.0.0 #145

Update feed2catalog and catalog2registration to properly handle SemVer 2.0.0 #145

Conversation

joelverhagen commented Mar 24, 2017

joelverhagen commented Mar 28, 2017

joelverhagen commented Mar 28, 2017

joelverhagen commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skofman1 commented Mar 29, 2017

scottbommarito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbommarito Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelverhagen commented Mar 30, 2017

scottbommarito Mar 29, 2017 •

edited

Loading