[NuGet Symbol Server] Add Symbol push validation support #6276

shishirx34 · 2018-08-07T18:40:07Z

Addresses #6235

Added test coverage, I have verified this locally, will check in DEV. Sending out a separate PR for the new configs for symbol validation service bus and topic.

chenriksson · 2018-08-07T20:09:07Z

src/NuGetGallery.Core/NuGetGallery.Core.csproj

    </PackageReference>
    <PackageReference Include="NuGet.Services.Validation.Issues">
-      <Version>2.26.0-master-33196</Version>
+      <Version>2.28.0-master-37018</Version>


Is there a stable version to update to?

I don't see one in manage nuget packages.

I believe our (undocumented) process for ServerCommon is to kick off a stable build when making changes there. I had similiar feedback in another PR, which is why I mention it.

chenriksson · 2018-08-07T20:16:24Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

+                    .Register(c => {
+                        return new AsynchronousPackageValidationInitiator(
+                            c.ResolveKeyed<IPackageValidationEnqueuer>("PackageValidationEnqueuerBindingKey"),
+                            c.ResolveKeyed<IPackageValidationEnqueuer>("SymbolsPackageValidationEnqueuerBindingKey"),


Should these be constants - to keep this in sync w/ code above?

chenriksson · 2018-08-07T20:19:51Z

src/NuGetGallery/NuGetGallery.csproj

-    </PackageReference>
-    <PackageReference Include="NuGet.Versioning">
-      <Version>4.8.0-preview4.5287</Version>
-    </PackageReference>


Nice! I assume these dependencies are pulled in from Gallery.Core?

chenriksson · 2018-08-07T20:22:58Z

src/NuGetGallery/Services/IPackageValidationInitiator.cs

+        /// <returns>
+        /// The task which signals the completion of the validation initiation. The result of the <see cref="Task"/>
+        /// is the <see cref="PackageStatus"/> that should be applied to the package.</returns>
+        Task<PackageStatus> StartSymbolsPackageValidationAsync(SymbolPackage symbolPackage);


It's a little strange that the initiator has APIs for both nupkg and snupkg packages, but are only configured for one or the other.

It it possible to have a IPackageValidationInitiator<TPackage> so that the initiator can only enqueue based on the configured package type?

chenriksson · 2018-08-07T20:29:31Z

src/NuGetGallery/Services/ValidationService.cs

+            await _symbolPackageService.UpdateStatusAsync(symbolPackage,
+                symbolPackageStatus,
+                commitChanges: false);
+        }


Would having an IPackageEntity interface for use by Package and SymbolsPackage allow more code reuse?

shishirx34 · 2018-08-07T23:57:59Z

@chenriksson - I have managed to refactor the code and setup DI appropriately(thanks @joelverhagen), I tested this in synchronous and blocked async mode, looks good. Please take a look at the refactoring.

agr · 2018-08-08T00:44:22Z

src/NuGetGallery/Services/IPackageValidationInitiator.cs

@@ -8,18 +8,18 @@ namespace NuGetGallery
    /// <summary>
    /// Initiates validation for a specific package.
    /// </summary>
-    public interface IPackageValidationInitiator
+    public interface IPackageValidationInitiator<IPackageEntity>


IPackageEntity [](start = 49, length = 14)

Generic type arguments should start with T. It also could be useful to have where constraint to restrict it to the classes implementing the IPackageEntity

agr · 2018-08-08T00:56:52Z

src/NuGetGallery/Services/SymbolPackageUploadService.cs

@@ -61,6 +58,11 @@ public async Task<PackageCommitResult> CreateAndUploadSymbolsPackage(Package pac
                }
                else if (symbolPackage.StatusKey == PackageStatus.Available)
                {
+                    if (symbolPackage.Published == null)
+                    {


nit: !symbolPackage.Published.HasValue?

agr · 2018-08-08T01:08:02Z

src/NuGetGallery/Services/AsynchronousPackageValidationInitiator.cs

@@ -13,9 +13,9 @@ namespace NuGetGallery
    /// Initiates asynchronous validation on a package by enqueuing a message containing the package identity and a new
    /// <see cref="Guid"/>. The <see cref="Guid"/> represents a unique validation request.
    /// </summary>
-    public class AsynchronousPackageValidationInitiator : IPackageValidationInitiator
+    public class AsynchronousPackageValidationInitiator<TPackage> : IPackageValidationInitiator<TPackage> where TPackage: IPackageEntity
    {


Why does it have to be generic?

My suggestion... see my comment here

My concern was that you could create a snupkg initiator with an (invalid) API to initiate nupkg validation.

I'm not as familiar w/ symbols work though, so feel free to suggest something different. Just thinking this could help with code reuse.

@agr - code reusability for pushing to Package validation topic and symbol validation topic. The DI initializes these generic types to be used in Validation service.

TPackageEntity is not used for anything in this class. IPackageValidationInitiator can be made non-generic with Task<PackageStatus> StartValidationAsync(IPackageEntity package) method instead and then the code would still work unless I am missing something.

Talked and resolved this query offline.

Talked offline: this makes DI setup quite complicated, so disregard.

chenriksson · 2018-08-08T15:37:08Z

src/NuGetGallery/Services/AsynchronousPackageValidationInitiator.cs

@@ -13,9 +13,9 @@ namespace NuGetGallery
    /// Initiates asynchronous validation on a package by enqueuing a message containing the package identity and a new
    /// <see cref="Guid"/>. The <see cref="Guid"/> represents a unique validation request.
    /// </summary>
-    public class AsynchronousPackageValidationInitiator : IPackageValidationInitiator
+    public class AsynchronousPackageValidationInitiator<TPackage> : IPackageValidationInitiator<TPackage> where TPackage: IPackageEntity


Can we wrap this line at where?

chenriksson · 2018-08-08T15:46:11Z

src/NuGetGallery.Core/Entities/IPackageEntity.cs

+
+        string Version { get; }
+
+        ValidatingType Type { get; }


Is ValidatingType an enum that should be added with this PR? Do you need it - why not just check instance type?

Yes it is an enum. This completely removes the dependency of checking the type in initiator. Also, it is going to be specific to each IPackageEntity so it makes sense to put it in here.

chenriksson · 2018-08-08T15:49:45Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

@@ -39,6 +39,14 @@

 namespace NuGetGallery
 {
+    public static class BindingKeys


PackageValidationBindingKeys? I think BindingKeys may be too general given this is nested in the DefaultDependenciesModule class.

I think we should keep it as a general class for binding constants, going forward use it for any DI setup that requires binding. This isn't specific for PackageValidation.

Common coding standard I've seen is to keep 1:1 relation between classes and files, with names matching.

Given this, would consider moving BindingKeys into separate file or nesting under DefaultDependenciesModule class.

chenriksson · 2018-08-08T15:51:28Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

+                            c.ResolveKeyed<IPackageValidationEnqueuer>(BindingKeys.SymbolsPackageValidationEnqueuer),
+                            c.Resolve<IAppConfiguration>(),
+                            c.Resolve<IDiagnosticsService>());
+                    }).As<IPackageValidationInitiator<SymbolPackage>>();


@shishirx34 @joelverhagen

So was ResolveKeyed the solution to get DI registrations to work for the specific generic type args?

I missed instantiating the AsynchronousPackageValidationInitiator and ImmediateValidationInitiator to be Generic which was causing the problem. See the code below of RegisterGenerics which sets up the DI appropriately.

chenriksson · 2018-08-08T15:54:12Z

src/NuGetGallery/Services/AsynchronousPackageValidationInitiator.cs

@@ -13,9 +13,9 @@ namespace NuGetGallery
    /// Initiates asynchronous validation on a package by enqueuing a message containing the package identity and a new
    /// <see cref="Guid"/>. The <see cref="Guid"/> represents a unique validation request.
    /// </summary>
-    public class AsynchronousPackageValidationInitiator : IPackageValidationInitiator
+    public class AsynchronousPackageValidationInitiator<TPackage> : IPackageValidationInitiator<TPackage> where TPackage: IPackageEntity
    {


I'm not as familiar w/ symbols work though, so feel free to suggest something different. Just thinking this could help with code reuse.

chenriksson · 2018-08-08T15:54:58Z

src/NuGetGallery/Services/IPackageValidationInitiator.cs

@@ -8,18 +8,18 @@ namespace NuGetGallery
    /// <summary>
    /// Initiates validation for a specific package.
    /// </summary>
-    public interface IPackageValidationInitiator
+    public interface IPackageValidationInitiator<TEntity> where TEntity: IPackageEntity


nit: would name <TPackageEntity>

chenriksson · 2018-08-08T15:55:39Z

src/NuGetGallery/Services/ImmediatePackageValidator.cs

@@ -9,9 +9,9 @@ namespace NuGetGallery
    /// A validation initiator that immediately marks the package as validated. In other words, no asynchronous
    /// validation is performed by this implementation.
    /// </summary>
-    public class ImmediatePackageValidator : IPackageValidationInitiator
+    public class ImmediatePackageValidator<TPackage> : IPackageValidationInitiator<TPackage> where TPackage: IPackageEntity


nit: TPackageEntity

chenriksson · 2018-08-08T16:14:38Z

src/NuGetGallery/Services/SymbolPackageUploadService.cs

+                    if (!symbolPackage.Published.HasValue)
+                    {
+                        symbolPackage.Published = DateTime.UtcNow;
+                    }


I was comparing this code with nupkg validation initiation... and they look very different.

NuGetGallery/src/NuGetGallery/Services/PackageUploadService.cs

Line 225 in 760a814

public async Task<PackageCommitResult> CommitPackageAsync(Package package, Stream packageFile)

Just wondering, since the design reuses much of the validation infrastructure (even a copy)... why do the code paths look different? Again, feel free to ignore since I don't have as much context here.

I created this file for symbol specific stuff, the validation here isn't a copy though. The Package upload doesn't allow an overwrite, symbols does. It is meant to be different.

By "copy", I meant to say snupkg files share much of the same validation pipeline as nupkg files from what I remember... but Gallery just sends the initiate messages to a different service bus / pipeline instance.

From Gallery's perspective... wouldn't initiating a validation, and receiving validation status look about the same between nupkg and snupkg packages?

e.g., shouldn't Gallery set package.Published = DateTime.UtcNow in roughly the same place for both nupkg and snupkg?

Or are the code paths between the two that different in the Gallery?

talked offline to clarify the differences here.

chenriksson · 2018-08-08T16:16:57Z

src/NuGetGallery/Services/ValidationService.cs

@@ -95,5 +101,13 @@ public IReadOnlyList<ValidationIssue> GetLatestValidationIssues(Package package)

            return issues;
        }
+
+        public async Task StartSymbolsPackageValidationAsync(SymbolPackage symbolPackage)


nit: naming is a little inconsistent with corresponding nupkg API.

Maybe just StartValidationAsync(SymbolPackage) is enough?

skofman1 · 2018-08-08T19:04:53Z

src/NuGetGallery/Services/ValidationService.cs

@@ -95,5 +101,13 @@ public IReadOnlyList<ValidationIssue> GetLatestValidationIssues(Package package)

            return issues;
        }
+
+        public async Task StartValidationAsync(SymbolPackage symbolPackage)


We will need the same validation functionality for symbols and for packages: 1. revalidation, browsing state, monitoring. It's not part of the MVP but is it tracked with issues?

skofman1 · 2018-08-08T19:14:13Z

src/NuGetGallery.Core/Entities/IPackageEntity.cs

+
+        string Version { get; }
+
+        ValidatingType Type { get; }


Consider renaming this to "PackageType". Validation type is not an attribute of the package, it's a behavior derived from the type of the package.

I am reusing this from the Validations side for consistency, we use this to identify the package. While I agree with you, I do not think we should diverge for this enum. I'll alias it here I guess for ease.

chenriksson · 2018-08-08T20:55:27Z

src/NuGetGallery.Core/Entities/IPackageEntity.cs

+
+namespace NuGetGallery
+{
+    public interface IPackageEntity


Consider IPackageEntity : IEntity, to restrict usage of this interface to entity types.

Consider adding doc comments to specify the need for this type -- to reuse code between nupkg and snupkg packages.

chenriksson · 2018-08-08T21:02:21Z

src/NuGetGallery.Core/Entities/Package.cs

+
+        public string Id => PackageRegistration.Id;
+
+        public ValidatingType Type => ValidatingType.Package;


Type could be confused with GetType(). Consider renaming to ValidatingType.

Is this used in this PR? Would remove if not.

removing this.

chenriksson · 2018-08-08T21:05:07Z

src/NuGetGallery.Core/Entities/IPackageEntity.cs

+    {
+        string Id { get; }
+
+        string Version { get; }


Are there any shared code paths as part of this PR that would require NormalizedVersion?

not the code I am touching. I wouldn't refactor much for anything else right now.

chenriksson · 2018-08-08T21:07:46Z

src/NuGetGallery.Core/Entities/SymbolPackage.cs


 namespace NuGetGallery
 {
    public class SymbolPackage
-        : IEntity, IEquatable<SymbolPackage>
+        : IEntity, IPackageEntity, IEquatable<SymbolPackage>


Outside the scope of this PR... but why does SymbolPackage implement IEquatable, and Package does not? Is there functionality only specific to symbols that requires this?

See: https://github.com/NuGet/NuGetGallery/pull/6276/files#diff-c93f5b23258b3e89d85636deb43d0236R70

chenriksson · 2018-08-08T21:09:33Z

src/NuGetGallery.Core/Entities/SymbolPackage.cs

+
+        public string Version => Package?.Version;
+
+        public ValidatingType Type => ValidatingType.SymbolPackage;


See comment above (Package.cs).

This will be deleted. missed it.

chenriksson · 2018-08-08T21:11:21Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

@@ -39,6 +39,14 @@

 namespace NuGetGallery
 {
+    public static class BindingKeys


Common coding standard I've seen is to keep 1:1 relation between classes and files, with names matching.

Given this, would consider moving BindingKeys into separate file or nesting under DefaultDependenciesModule class.

chenriksson · 2018-08-08T21:21:17Z

src/NuGetGallery/Services/AsynchronousPackageValidationInitiator.cs

+            else
+            {
+                throw new ArgumentException($"Unknown IPackageEntity type: {nameof(package)}");
+            }


Why add ValidatingType to Package and SymbolPackage, but not IPackageEntity? Seems like it complicates the code here.

see @skofman1's comment. I think it doesn't make sense to put the ValidatingType on the entity.

chenriksson · 2018-08-08T21:32:30Z

src/NuGetGallery/Services/SymbolPackageUploadService.cs

+                    if (!symbolPackage.Published.HasValue)
+                    {
+                        symbolPackage.Published = DateTime.UtcNow;
+                    }


By "copy", I meant to say snupkg files share much of the same validation pipeline as nupkg files from what I remember... but Gallery just sends the initiate messages to a different service bus / pipeline instance.

From Gallery's perspective... wouldn't initiating a validation, and receiving validation status look about the same between nupkg and snupkg packages?

e.g., shouldn't Gallery set package.Published = DateTime.UtcNow in roughly the same place for both nupkg and snupkg?

Or are the code paths between the two that different in the Gallery?

chenriksson

I may have a few nits remaining, but nothing blocking.

chenriksson · 2018-08-08T22:31:34Z

src/NuGetGallery.Core/NuGetGallery.Core.csproj

    </PackageReference>
    <PackageReference Include="NuGet.Services.Validation.Issues">
-      <Version>2.26.0-master-33196</Version>
+      <Version>2.28.0-master-37018</Version>


I believe our (undocumented) process for ServerCommon is to kick off a stable build when making changes there. I had similiar feedback in another PR, which is why I mention it.

shishirx34 · 2018-08-09T01:06:05Z

I've verified these changes in DEV with the new configs. Looks good.

agr

shishirx34 added 7 commits August 3, 2018 16:43

Add symbol validation service

242351e

Merge remote-tracking branch 'origin/dev' into symbol-push-validation

5004758

Add validation interfaces

517d47a

Update NuGet.Services.Validation with issues and new message data type

20f3d00

Add autofac di magic for symbol validation

0eb9568

nit fix

ade5f9f

Add tests

60e5433

shishirx34 requested review from chenriksson, joelverhagen, ryuyu, agr, loic-sharma, xavierdecoster, zivkan, skofman1, scottbommarito and zhhyu and removed request for chenriksson and joelverhagen August 7, 2018 18:40

chenriksson reviewed Aug 7, 2018

View reviewed changes

Refactor and DI changes

f72c497

shishirx34 force-pushed the symbol-push-validation branch from 72ffa71 to f72c497 Compare August 7, 2018 23:51

agr reviewed Aug 8, 2018

View reviewed changes

Address feedback

743e2ca

agr reviewed Aug 8, 2018

View reviewed changes

chenriksson reviewed Aug 8, 2018

View reviewed changes

Address feedback

a908c03

skofman1 reviewed Aug 8, 2018

View reviewed changes

nit fix and refactor

e04a585

chenriksson reviewed Aug 8, 2018

View reviewed changes

shishirx34 added 2 commits August 8, 2018 14:51

remove validating type

e3bf803

move binding keys

ffb555c

chenriksson approved these changes Aug 8, 2018

View reviewed changes

agr approved these changes Aug 9, 2018

View reviewed changes

shishirx34 merged commit 3805e2e into dev Aug 9, 2018

shishirx34 deleted the symbol-push-validation branch August 9, 2018 01:21

scottbommarito mentioned this pull request Aug 20, 2018

Deployment 2018-08-23 #6316

Closed


		public string Id => PackageRegistration.Id;

		public ValidatingType Type => ValidatingType.Package;


		public string Version => Package?.Version;

		public ValidatingType Type => ValidatingType.SymbolPackage;

[NuGet Symbol Server] Add Symbol push validation support #6276

[NuGet Symbol Server] Add Symbol push validation support #6276

Conversation

shishirx34 commented Aug 7, 2018

Choose a reason for hiding this comment

shishirx34 Aug 7, 2018 • 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

shishirx34 commented Aug 7, 2018

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

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

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

chenriksson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shishirx34 commented Aug 9, 2018

agr left a comment

Choose a reason for hiding this comment

shishirx34 Aug 7, 2018 •

edited

Loading