Changed the way Validators are referred by their "names" (#396)

agr · web-flow · commit b349b718d98b · 2018-04-03T15:24:45.000-07:00
* Changed the way Validators are referred by their "names". Now "names" are explicitly specified through the ValidatorAliasAttribute instead of the class name.

Renamed the `ValidatorUtil`

* Period

* `ValidatorAlias*` -&gt; `ValidatorName*`

* Proper file name for the ValidatorName class
diff --git a/src/NuGet.Services.Validation.Orchestrator/Job.cs b/src/NuGet.Services.Validation.Orchestrator/Job.cs
@@ -337,8 +337,8 @@ private static void ConfigurePackageSigningValidator(ContainerBuilder builder)
             builder
                 .RegisterType<ValidatorStateService>()
                 .WithParameter(
-                    (pi, ctx) => pi.ParameterType == typeof(Type),
-                    (pi, ctx) => typeof(PackageSigningValidator))
+                    (pi, ctx) => pi.ParameterType == typeof(string),
+                    (pi, ctx) => ValidatorName.PackageSigning)
                 .Keyed<IValidatorStateService>(PackageSigningBindingKey);
 
             // Configure the package signature verification enqueuer.
@@ -369,8 +369,8 @@ private static void ConfigurePackageCertificatesValidator(ContainerBuilder build
             builder
                 .RegisterType<ValidatorStateService>()
                 .WithParameter(
-                    (pi, ctx) => pi.ParameterType == typeof(Type),
-                    (pi, ctx) => typeof(PackageCertificatesValidator))
+                    (pi, ctx) => pi.ParameterType == typeof(string),
+                    (pi, ctx) => ValidatorName.PackageCertificate)
                 .Keyed<IValidatorStateService>(PackageCertificatesBindingKey);
 
             // Configure the certificate verification enqueuer.
diff --git a/src/NuGet.Services.Validation.Orchestrator/NuGet.Services.Validation.Orchestrator.csproj b/src/NuGet.Services.Validation.Orchestrator/NuGet.Services.Validation.Orchestrator.csproj
@@ -58,7 +58,6 @@
     <Compile Include="IValidationSetProcessor.cs" />
     <Compile Include="IValidationSetProvider.cs" />
     <Compile Include="IValidationStorageService.cs" />
-    <Compile Include="IValidatorProvider.cs" />
     <Compile Include="Job.cs" />
     <Compile Include="MessageService.cs" />
     <Compile Include="OrchestrationRunner.cs" />
@@ -136,4 +135,4 @@
     <SignPath Condition="'$(NuGetBuildPath)' != ''">$(NuGetBuildPath)</SignPath>
   </PropertyGroup>
   <Import Project="$(SignPath)\sign.targets" Condition="Exists('$(SignPath)\sign.targets')" />
-</Project>
+</Project>
diff --git a/src/NuGet.Services.Validation.Orchestrator/PackageSigning/ProcessSignature/PackageSigningValidator.cs b/src/NuGet.Services.Validation.Orchestrator/PackageSigning/ProcessSignature/PackageSigningValidator.cs
@@ -5,12 +5,14 @@
 using System.Diagnostics;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
+using NuGet.Jobs.Validation;
 using NuGet.Jobs.Validation.PackageSigning.Storage;
 using NuGet.Jobs.Validation.Storage;
 using NuGet.Services.Validation.Orchestrator.Telemetry;
 
 namespace NuGet.Services.Validation.PackageSigning.ProcessSignature
 {
+    [ValidatorName(ValidatorName.PackageSigning)]
     public class PackageSigningValidator : IProcessor
     {
         private readonly IValidatorStateService _validatorStateService;
diff --git a/src/NuGet.Services.Validation.Orchestrator/PackageSigning/ValidateCertificate/PackageCertificatesValidator.cs b/src/NuGet.Services.Validation.Orchestrator/PackageSigning/ValidateCertificate/PackageCertificatesValidator.cs
@@ -8,12 +8,15 @@
 using System.Linq;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
+using NuGet.Jobs.Validation;
 using NuGet.Jobs.Validation.PackageSigning.Storage;
 using NuGet.Services.Validation.Orchestrator;
 using NuGet.Services.Validation.Orchestrator.Telemetry;
+using Error = NuGet.Services.Validation.Orchestrator.Error;
 
 namespace NuGet.Services.Validation.PackageSigning.ValidateCertificate
 {
+    [ValidatorName(ValidatorName.PackageCertificate)]
     public class PackageCertificatesValidator : BaseValidator, IValidator
     {
         private static readonly TimeSpan DefaultCertificateRevalidationThresholdTime = TimeSpan.FromDays(1);
diff --git a/src/NuGet.Services.Validation.Orchestrator/ValidatorProvider.cs b/src/NuGet.Services.Validation.Orchestrator/ValidatorProvider.cs
@@ -7,6 +7,7 @@
 using System.Reflection;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
+using NuGet.Jobs.Validation;
 
 namespace NuGet.Services.Validation.Orchestrator
 {
@@ -55,13 +56,14 @@ private void InitializeEvaluatedTypes(Assembly callingAssembly)
                     var validatorTypes = candidateTypes
                         .Where(type => typeof(IValidator).IsAssignableFrom(type)
                                && type != typeof(IValidator)
-                               && type != typeof(IProcessor))
-                        .ToDictionary(type => type.Name);
+                               && type != typeof(IProcessor)
+                               && ValidatorUtility.HasValidatorNameAttribute(type))
+                        .ToDictionary(ValidatorUtility.GetValidatorName);
 
                     var processorTypes = validatorTypes
                         .Values
                         .Where(IsProcessorType)
-                        .ToDictionary(type => type.Name);
+                        .ToDictionary(ValidatorUtility.GetValidatorName);
 
                     _logger.LogTrace("After enumeration, got {NumImplementations} implementations: {TypeNames}",
                         validatorTypes.Count,
diff --git a/src/NuGet.Services.Validation.Orchestrator/Vcs/VcsValidator.cs b/src/NuGet.Services.Validation.Orchestrator/Vcs/VcsValidator.cs
@@ -8,16 +8,19 @@
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using Microsoft.WindowsAzure.Storage;
+using NuGet.Jobs.Validation;
 using NuGet.Jobs.Validation.Common;
 using NuGet.Services.Validation.Orchestrator;
 using NuGet.Versioning;
 using NuGetGallery;
+using Error = NuGet.Services.Validation.Orchestrator.Error;
 
 namespace NuGet.Services.Validation.Vcs
 {
+    [ValidatorName(ValidatorName.Vcs)]
     public class VcsValidator : BaseValidator, IValidator
     {
-        private const string ValidatorName = Jobs.Validation.Common.Validators.Vcs.VcsValidator.ValidatorName;
+        private const string InternalValidatorName = Jobs.Validation.Common.Validators.Vcs.VcsValidator.ValidatorName;
 
         private readonly IPackageValidationService _validationService;
         private readonly IPackageValidationAuditor _validationAuditor;
@@ -61,7 +64,7 @@ public async Task<IValidationResult> GetResultAsync(IValidationRequest request)
 
             var validationStatusList = audit
                 .Entries
-                .Where(x => x.ValidatorName == ValidatorName)
+                .Where(x => x.ValidatorName == InternalValidatorName)
                 .Select(x => GetValidationStatus(request, x.EventId))
                 .ToList();
 
@@ -127,8 +130,8 @@ await _validationService.StartValidationProcessAsync(
                         Version = normalizedPackageVerison,
                         DownloadUrl = new Uri(request.NupkgUrl),
                     },
-                    new[] { ValidatorName },
-                    request.ValidationId);
+                    validators: new[] { InternalValidatorName },
+                    validationId: request.ValidationId);
             }
             catch (StorageException e) when (e.RequestInformation?.HttpStatusCode == (int)HttpStatusCode.Conflict
                                              || e.RequestInformation?.HttpStatusCode == (int)HttpStatusCode.PreconditionFailed)
diff --git a/src/Validation.Common.Job/Storage/ValidatorStateService.cs b/src/Validation.Common.Job/Storage/ValidatorStateService.cs
@@ -9,6 +9,7 @@
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
 using NuGet.Services.Validation;
+using NuGet.Services.Validation.Orchestrator;
 
 namespace NuGet.Jobs.Validation.PackageSigning.Storage
 {
@@ -20,23 +21,21 @@ public class ValidatorStateService : IValidatorStateService
 
         public ValidatorStateService(
             IValidationEntitiesContext validationContext,
-            Type validatorType,
+            IValidatorProvider validatorProvider,
+            string validatorName,
             ILogger<ValidatorStateService> logger)
         {
             _validationContext = validationContext ?? throw new ArgumentNullException(nameof(validationContext));
             _logger = logger ?? throw new ArgumentNullException(nameof(logger));
-
-            if (validatorType == null)
+            if (validatorProvider == null)
             {
-                throw new ArgumentNullException(nameof(validatorType));
+                throw new ArgumentNullException(nameof(validatorProvider));
             }
-
-            if (!typeof(IValidator).IsAssignableFrom(validatorType))
+            if (!validatorProvider.IsValidator(validatorName))
             {
-                throw new ArgumentException($"The validator type {validatorType} must implement {nameof(IValidator)}.", nameof(validatorType));
+                throw new ArgumentException($"\"{validatorName}\" is not a proper validator alias.", nameof(validatorName));
             }
-
-            _validatorName = validatorType.Name;
+            _validatorName = validatorName ?? throw new ArgumentNullException(nameof(validatorName));
         }
 
         public async Task<ValidatorStatus> GetStatusAsync(IValidationRequest request)
diff --git a/src/Validation.Common.Job/Validation.Common.Job.csproj b/src/Validation.Common.Job/Validation.Common.Job.csproj
@@ -59,6 +59,10 @@
     <Compile Include="Storage\ValidatorStatusExtensions.cs" />
     <Compile Include="SubcriptionProcessorJob.cs" />
     <Compile Include="JsonConfigurationJob.cs" />
+    <Compile Include="Validation\IValidatorProvider.cs" />
+    <Compile Include="Validation\ValidatorName.cs" />
+    <Compile Include="Validation\ValidatorNameAttribute.cs" />
+    <Compile Include="Validation\ValidatorUtility.cs" />
   </ItemGroup>
   <ItemGroup>
     <PackageReference Include="Autofac">
@@ -114,4 +118,4 @@
     <SignPath Condition="'$(NuGetBuildPath)' != ''">$(NuGetBuildPath)</SignPath>
   </PropertyGroup>
   <Import Project="$(SignPath)\sign.targets" Condition="Exists('$(SignPath)\sign.targets')" />
-</Project>
+</Project>
diff --git a/src/Validation.Common.Job/Validation/IValidatorProvider.cs b/src/Validation.Common.Job/Validation/IValidatorProvider.cs
diff --git a/src/Validation.Common.Job/Validation/ValidatorName.cs b/src/Validation.Common.Job/Validation/ValidatorName.cs
@@ -0,0 +1,12 @@
+﻿// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+namespace NuGet.Jobs.Validation
+{
+    public static class ValidatorName
+    {
+        public const string Vcs = "VcsValidator";
+        public const string PackageCertificate = "PackageCertificatesValidator";
+        public const string PackageSigning = "PackageSigningValidator";
+    }
+}
diff --git a/src/Validation.Common.Job/Validation/ValidatorNameAttribute.cs b/src/Validation.Common.Job/Validation/ValidatorNameAttribute.cs
@@ -0,0 +1,22 @@
+﻿// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+
+namespace NuGet.Jobs.Validation
+{
+    /// <summary>
+    /// Stores the string label for the Validator to be stored in DB and 
+    /// to allow validator instantiation by that label
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Class, Inherited = false, AllowMultiple = false)]
+    public class ValidatorNameAttribute: Attribute
+    {
+        public string Name { get; }
+
+        public ValidatorNameAttribute(string name)
+        {
+            Name = name;
+        }
+    }
+}
diff --git a/src/Validation.Common.Job/Validation/ValidatorUtility.cs b/src/Validation.Common.Job/Validation/ValidatorUtility.cs
@@ -0,0 +1,31 @@
+﻿// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+using System.Linq;
+
+namespace NuGet.Jobs.Validation
+{
+    /// <summary>
+    /// Validator-related utility methods
+    /// </summary>
+    public static class ValidatorUtility
+    {
+        /// <summary>
+        /// Checks whether given type has <see cref="ValidatorNameAttribute"/> attribute.
+        /// </summary>
+        public static bool HasValidatorNameAttribute(Type type)
+            => type.CustomAttributes.Any(a => a.AttributeType == typeof(ValidatorNameAttribute));
+
+        /// <summary>
+        /// Retrieves the value of the <see cref="ValidatorNameAttribute.Name"/> property set 
+        /// for <see cref="ValidatorNameAttribute"/> set on a specified class.
+        /// </summary>
+        public static string GetValidatorName(Type type)
+            => GetCustomAttribute<ValidatorNameAttribute>(type).Name;
+
+        private static T GetCustomAttribute<T>(Type type)
+            where T : Attribute
+            => (T)Attribute.GetCustomAttribute(type, typeof(T));
+    }
+}
diff --git a/tests/NuGet.Services.Validation.Orchestrator.Tests/PackageSigning/ValidateCertificate/PackageCertificatesValidatorFacts.cs b/tests/NuGet.Services.Validation.Orchestrator.Tests/PackageSigning/ValidateCertificate/PackageCertificatesValidatorFacts.cs
@@ -7,7 +7,9 @@
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
 using Moq;
+using NuGet.Jobs.Validation;
 using NuGet.Jobs.Validation.PackageSigning.Storage;
+using NuGet.Services.Validation.Orchestrator;
 using NuGet.Services.Validation.Orchestrator.Telemetry;
 using NuGet.Services.Validation.PackageSigning.ValidateCertificate;
 using Validation.PackageSigning.Helpers;
@@ -1171,6 +1173,7 @@ public async Task RevokedSignaturesAreInvalidated()
         public abstract class FactsBase
         {
             protected readonly Mock<IValidationEntitiesContext> _validationContext;
+            protected readonly Mock<IValidatorProvider> _validatorProvider;
             protected readonly Mock<IValidateCertificateEnqueuer> _certificateVerifier;
             protected readonly Mock<ITelemetryService> _telemetryService;
             protected readonly Mock<ILogger<PackageCertificatesValidator>> _logger;
@@ -1180,10 +1183,14 @@ public abstract class FactsBase
             public FactsBase()
             {
                 _validationContext = new Mock<IValidationEntitiesContext>();
+                _validatorProvider = new Mock<IValidatorProvider>();
                 _certificateVerifier = new Mock<IValidateCertificateEnqueuer>();
                 _telemetryService = new Mock<ITelemetryService>();
                 _logger = new Mock<ILogger<PackageCertificatesValidator>>();
 
+                _validatorProvider.Setup(vp => vp.IsValidator(It.IsAny<string>())).Returns(true);
+                _validatorProvider.Setup(vp => vp.IsProcessor(It.IsAny<string>())).Returns(true);
+
                 _validationRequest = new Mock<IValidationRequest>();
                 _validationRequest.Setup(x => x.NupkgUrl).Returns(NupkgUrl);
                 _validationRequest.Setup(x => x.PackageId).Returns(PackageId);
@@ -1194,7 +1201,8 @@ public FactsBase()
                 var validatorStateServiceLogger = new Mock<ILogger<ValidatorStateService>>();
                 var validatorStateService = new ValidatorStateService(
                     _validationContext.Object,
-                    typeof(PackageCertificatesValidator),
+                    _validatorProvider.Object,
+                    ValidatorName.PackageCertificate,
                     validatorStateServiceLogger.Object);
 
                 _target = new PackageCertificatesValidator(
diff --git a/tests/NuGet.Services.Validation.Orchestrator.Tests/ValidationProviderFacts.cs b/tests/NuGet.Services.Validation.Orchestrator.Tests/ValidationProviderFacts.cs
@@ -7,6 +7,7 @@
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
 using Moq;
+using NuGet.Jobs.Validation;
 using Validation.PackageSigning.Core.Tests.Support;
 using Xunit;
 using Xunit.Abstractions;
@@ -97,32 +98,34 @@ public void KnownValidatorsDoNotChangeNames(string validatorName, bool isProcess
             public void CanGetValidator()
             {
                 var validator = new TestValidator();
+                var validatorType = validator.GetType();
 
                 ServiceProviderMock
-                    .Setup(sp => sp.GetService(validator.GetType()))
+                    .Setup(sp => sp.GetService(validatorType))
                     .Returns(() => validator);
 
-                var result = Target.GetValidator(validator.GetType().Name);
+                var result = Target.GetValidator(ValidatorUtility.GetValidatorName(validatorType));
 
                 ServiceProviderMock
-                    .Verify(sp => sp.GetService(validator.GetType()), Times.Once);
-                Assert.IsType(validator.GetType(), result);
+                    .Verify(sp => sp.GetService(validatorType), Times.Once);
+                Assert.IsType(validatorType, result);
             }
 
             [Fact]
             public void CanGetProcessor()
             {
                 var processor = new TestProcessor();
+                var processorType = processor.GetType();
 
                 ServiceProviderMock
-                    .Setup(sp => sp.GetService(processor.GetType()))
+                    .Setup(sp => sp.GetService(processorType))
                     .Returns(() => processor);
                 
-                var result = Target.GetValidator(processor.GetType().Name);
+                var result = Target.GetValidator(ValidatorUtility.GetValidatorName(processorType));
 
                 ServiceProviderMock
-                    .Verify(sp => sp.GetService(processor.GetType()), Times.Once);
-                Assert.IsType(processor.GetType(), result);
+                    .Verify(sp => sp.GetService(processorType), Times.Once);
+                Assert.IsType(processorType, result);
             }
 
             [Fact]
@@ -156,6 +159,7 @@ public BaseFacts()
             public ValidatorProvider Target { get; }
         }
 
+        [ValidatorName("TestValidator")]
         public class TestValidator : IValidator
         {
             public Task CleanUpAsync(IValidationRequest request)
@@ -174,6 +178,7 @@ public Task<IValidationResult> StartAsync(IValidationRequest request)
             }
         }
 
+        [ValidatorName("TestProcessor")]
         public class TestProcessor : IProcessor
         {
             public Task CleanUpAsync(IValidationRequest request)
diff --git a/tests/NuGet.Services.Validation.Orchestrator.Tests/ValidatorStateServiceFacts.cs b/tests/NuGet.Services.Validation.Orchestrator.Tests/ValidatorStateServiceFacts.cs
diff --git a/tests/Validation.Common.Job.Tests/Storage/ProcessorPackageFileServiceFacts.cs b/tests/Validation.Common.Job.Tests/Storage/ProcessorPackageFileServiceFacts.cs

Original file line number	Diff line number	Diff line change
`@@ -5,12 +5,14 @@`
`5`	`5`	`using System.Diagnostics;`
`6`	`6`	`using System.Threading.Tasks;`
`7`	`7`	`using Microsoft.Extensions.Logging;`
	`8`	`+using NuGet.Jobs.Validation;`
`8`	`9`	`using NuGet.Jobs.Validation.PackageSigning.Storage;`
`9`	`10`	`using NuGet.Jobs.Validation.Storage;`
`10`	`11`	`using NuGet.Services.Validation.Orchestrator.Telemetry;`
`11`	`12`
`12`	`13`	`namespace NuGet.Services.Validation.PackageSigning.ProcessSignature`
`13`	`14`	`{`
	`15`	`+ [ValidatorName(ValidatorName.PackageSigning)]`
`14`	`16`	`public class PackageSigningValidator : IProcessor`
`15`	`17`	`{`
`16`	`18`	`private readonly IValidatorStateService _validatorStateService;`
Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,7 @@`
`9`	`9`	`using System.Threading.Tasks;`
`10`	`10`	`using Microsoft.Extensions.Logging;`
`11`	`11`	`using NuGet.Services.Validation;`
	`12`	`+using NuGet.Services.Validation.Orchestrator;`
`12`	`13`
`13`	`14`	`namespace NuGet.Jobs.Validation.PackageSigning.Storage`
`14`	`15`	`{`
`@@ -20,23 +21,21 @@ public class ValidatorStateService : IValidatorStateService`
`20`	`21`
`21`	`22`	`public ValidatorStateService(`
`22`	`23`	`IValidationEntitiesContext validationContext,`
`23`		`- Type validatorType,`
	`24`	`+ IValidatorProvider validatorProvider,`
	`25`	`+ string validatorName,`
`24`	`26`	`ILogger<ValidatorStateService> logger)`
`25`	`27`	`{`
`26`	`28`	`_validationContext = validationContext ?? throw new ArgumentNullException(nameof(validationContext));`
`27`	`29`	`_logger = logger ?? throw new ArgumentNullException(nameof(logger));`
`28`		`-`
`29`		`- if (validatorType == null)`
	`30`	`+ if (validatorProvider == null)`
`30`	`31`	`{`
`31`		`- throw new ArgumentNullException(nameof(validatorType));`
	`32`	`+ throw new ArgumentNullException(nameof(validatorProvider));`
`32`	`33`	`}`
`33`		`-`
`34`		`- if (!typeof(IValidator).IsAssignableFrom(validatorType))`
	`34`	`+ if (!validatorProvider.IsValidator(validatorName))`
`35`	`35`	`{`
`36`		`- throw new ArgumentException($"The validator type {validatorType} must implement {nameof(IValidator)}.", nameof(validatorType));`
	`36`	`+ throw new ArgumentException($"\"{validatorName}\" is not a proper validator alias.", nameof(validatorName));`
`37`	`37`	`}`
`38`		`-`
`39`		`- _validatorName = validatorType.Name;`
	`38`	`+ _validatorName = validatorName ?? throw new ArgumentNullException(nameof(validatorName));`
`40`	`39`	`}`
`41`	`40`
`42`	`41`	`public async Task<ValidatorStatus> GetStatusAsync(IValidationRequest request)`