Refactor the validator so it doesn't get stuck in a weird state

NuGet · Feb 21, 2018 · a7bd6d9 · a7bd6d9
1 parent 6da8b32
commit a7bd6d9
Show file tree

Hide file tree

Showing 8 changed files with 44 additions and 101 deletions.
diff --git a/src/NuGet.Services.Validation.Orchestrator/Job.cs b/src/NuGet.Services.Validation.Orchestrator/Job.cs
@@ -3,10 +3,7 @@
 
 using System;
 using System.Collections.Generic;
-using System.Linq;
 using System.Net;
-using System.Net.Http;
-using System.Reflection;
 using System.Threading.Tasks;
 using AnglicanGeek.MarkdownMailer;
 using Autofac;
@@ -20,6 +17,7 @@
 using Microsoft.WindowsAzure.Storage;
 using NuGet.Jobs;
 using NuGet.Jobs.Configuration;
+using NuGet.Jobs.Validation;
 using NuGet.Jobs.Validation.Common;
 using NuGet.Jobs.Validation.PackageSigning.Messages;
 using NuGet.Jobs.Validation.PackageSigning.Storage;
@@ -91,7 +89,7 @@ public override async Task Run()
                 return;
             }
 
-            var runner = GetRequiredService<OrchestrationRunner>();            
+            var runner = GetRequiredService<OrchestrationRunner>();
             await runner.RunOrchestrationAsync();
         }
 
@@ -217,6 +215,7 @@ private void ConfigureJobServices(IServiceCollection services, IConfigurationRoo
             services.AddTransient<ICoreMessageService, CoreMessageService>();
             services.AddTransient<IMessageService, MessageService>();
             services.AddTransient<ITelemetryService, TelemetryService>();
+            services.AddTransient<IPackageDownloader, PackageDownloader>();
             services.AddSingleton(new TelemetryClient());
         }
 
@@ -354,7 +353,6 @@ private static void ConfigurePackageSigningValidator(ContainerBuilder builder)
 
         private static void ConfigurePackageCompatibilityValidator(ContainerBuilder builder)
         {
-
             // Configure the validator state service for the package compatibility validator.
             builder
                 .RegisterType<ValidatorStateService>()
@@ -373,7 +371,6 @@ private static void ConfigurePackageCompatibilityValidator(ContainerBuilder buil
                 .RegisterType<PackageCompatibilityValidator>()
                 .WithKeyedParameter(typeof(IValidatorStateService), PackageCompatibilityBindingKey)
                 .As<PackageCompatibilityValidator>();
-
         }
 
         private static void ConfigurePackageCertificatesValidator(ContainerBuilder builder)

diff --git a/...et.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs b/...et.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs
@@ -46,7 +46,6 @@ public async Task<IValidationResult> GetResultAsync(IValidationRequest request)
 
         public async Task<IValidationResult> StartValidationAsync(IValidationRequest request)
         {
-            // Try check the whole thing
             try
             {
                 var validatorStatus = await _validatorStateService.GetStatusAsync(request);
@@ -62,34 +61,33 @@ public async Task<IValidationResult> StartValidationAsync(IValidationRequest req
                     return validatorStatus.ToValidationResult();
                 }
 
-                // Add the status, so subsequent calls don't try to reevaluate the same thing
-                await _validatorStateService.TryAddValidatorStatusAsync(request, validatorStatus, ValidationStatus.Incomplete);
-
-                var message = new PackageCompatibilityValidationMessage(
-                    request.PackageId,
-                    request.PackageVersion,
-                    new Uri(request.NupkgUrl),
-                    request.ValidationId
-                    );
-
-                // Do validation
-                await Validate(message, CancellationToken.None);
+                try
+                {
+                    await Validate(request, CancellationToken.None);
+                }
+                catch (Exception e)
+                {
+                    _logger.LogWarning(0, e, "Validation failed in the validator for the following request {request}", request.ValidationId);
+                }
 
+                // Treat every validation as succeeded, as we don't want to block 
                 validatorStatus.State = ValidationStatus.Succeeded;
 
                 await _validatorStateService.SaveStatusAsync(validatorStatus);
+
                 return validatorStatus.ToValidationResult();
             }
             catch (Exception e)
             {
-                _logger.LogWarning($"Validation failed for the validation request {0}.{Environment.NewLine}Exception: {1}", request.ToString(), e.ToString());
+                _logger.LogWarning(0, e, "Validation failed for the following request {request}", request.ValidationId);
             }
-            return null;
+
+            return new ValidationResult(ValidationStatus.Succeeded);
         }
 
-        private async Task Validate(PackageCompatibilityValidationMessage message, CancellationToken cancellationToken)
+        private async Task Validate(IValidationRequest request, CancellationToken cancellationToken)
         {
-            using (var packageStream = await _packageDownloader.DownloadAsync(message.NupkgUri, cancellationToken))
+            using (var packageStream = await _packageDownloader.DownloadAsync(new Uri(request.NupkgUrl), cancellationToken))
             using (var package = new Packaging.PackageArchiveReader(packageStream))
             {
                 var warnings = new List<PackLogMessage>();
@@ -99,7 +97,7 @@ private async Task Validate(PackageCompatibilityValidationMessage message, Cance
                     warnings.AddRange(rule.Validate(package));
                 }
 
-                await _packageCompatibilityService.SetPackageCompatibilityState(message.ValidationId, warnings);
+                await _packageCompatibilityService.SetPackageCompatibilityState(request.ValidationId, warnings);
             }
         }
     }

diff --git a/src/NuGet.Services.Validation.Orchestrator/settings.json b/src/NuGet.Services.Validation.Orchestrator/settings.json
@@ -42,9 +42,9 @@
   },
   "PackageCompatibility": {
     "ServiceBus": {
-      "ConnectionString": "servicebus string",
-      "TopicPath": "validate-compatibility",
-      "SubscriptionName": "validate-compatibility"
+      "ConnectionString": "",
+      "TopicPath": "",
+      "SubscriptionName": ""
     }
   },
   "RunnerConfiguration": {

diff --git a/src/Validation.PackageCompatibility.Core/Messages/PackageCompatibilityValidationMessage.cs b/src/Validation.PackageCompatibility.Core/Messages/PackageCompatibilityValidationMessage.cs
@@ -9,10 +9,14 @@ public class PackageCompatibilityValidationMessage
     {
         public PackageCompatibilityValidationMessage(string packageId, string packageVersion, Uri nupkgUri, Guid validationId)
         {
-            PackageId = packageId;
-            PackageVersion = packageVersion;
-            NupkgUri = nupkgUri;
+            if (validationId == Guid.Empty)
+            {
+                throw new ArgumentOutOfRangeException(nameof(validationId));
+            }
             ValidationId = validationId;
+            PackageId = packageId ?? throw new ArgumentNullException(nameof(packageId));
+            PackageVersion = packageVersion ?? throw new ArgumentNullException(nameof(packageVersion));
+            NupkgUri = nupkgUri ?? throw new ArgumentNullException(nameof(nupkgUri));
         }
 
         public string PackageId { get; }

diff --git a/...ion.PackageCompatibility.Core/Messages/PackageCompatibilityValidationMessageSerializer.cs b/...ion.PackageCompatibility.Core/Messages/PackageCompatibilityValidationMessageSerializer.cs
diff --git a/src/Validation.PackageCompatibility.Core/PackageCompatibilityService.cs b/src/Validation.PackageCompatibility.Core/PackageCompatibilityService.cs
@@ -5,7 +5,6 @@
 using NuGet.Services.Validation;
 using System;
 using System.Collections.Generic;
-using System.Data.Entity.Infrastructure;
 using System.Threading.Tasks;
 
 namespace Validation.PackageCompatibility.Core.Storage
@@ -38,14 +37,7 @@ public class PackageCompatibilityService : IPackageCompatibilityService
                               }
                           );
             }
-            try
-            {
-                await _validationContext.SaveChangesAsync();
-            }
-            catch (DbUpdateConcurrencyException e)
-            {
-                _logger.LogWarning("trouble saving changes async - {0}", e.Message);
-            }
+            await _validationContext.SaveChangesAsync();
         }
     }
 }
diff --git a/src/Validation.PackageCompatibility.Core/Properties/AssemblyInfo.cs b/src/Validation.PackageCompatibility.Core/Properties/AssemblyInfo.cs
@@ -8,9 +8,9 @@
 [assembly: AssemblyTitle("Validation.PackageCompatibility.Core")]
 [assembly: AssemblyDescription("")]
 [assembly: AssemblyConfiguration("")]
-[assembly: AssemblyCompany("HP Inc.")]
+[assembly: AssemblyCompany(".NET Foundation")]
 [assembly: AssemblyProduct("Validation.PackageCompatibility.Core")]
-[assembly: AssemblyCopyright("Copyright © HP Inc. 2018")]
+[assembly: AssemblyCopyright("Copyright © .NET Foundation 2017")]
 [assembly: AssemblyTrademark("")]
 [assembly: AssemblyCulture("")]
 

diff --git a/src/Validation.PackageCompatibility.Core/Validation.PackageCompatibility.Core.csproj b/src/Validation.PackageCompatibility.Core/Validation.PackageCompatibility.Core.csproj
@@ -41,27 +41,26 @@
   </ItemGroup>
   <ItemGroup>
     <Compile Include="IPackageCompatibilityService.cs" />
-    <Compile Include="Messages\PackageCompatibilityValidationMessageSerializer.cs" />
     <Compile Include="Messages\PackageCompatibilityValidationMessage.cs" />
     <Compile Include="PackageCompatibilityService.cs" />
     <Compile Include="Properties\AssemblyInfo.cs" />
   </ItemGroup>
   <ItemGroup>
-    <PackageReference Include="NuGet.Services.ServiceBus">
-      <Version>2.3.0</Version>
-    </PackageReference>
-    <PackageReference Include="NuGet.Services.Storage">
-      <Version>2.3.0</Version>
-    </PackageReference>
-    <PackageReference Include="NuGet.Services.Validation">
-      <Version>2.3.0</Version>
-    </PackageReference>
-    <PackageReference Include="WindowsAzure.Storage">
-      <Version>7.1.2</Version>
-    </PackageReference>
+    <ProjectReference Include="..\Validation.Common.Job\Validation.Common.Job.csproj">
+      <Project>{fa87d075-a934-4443-8d0b-5db32640b6d7}</Project>
+      <Name>Validation.Common.Job</Name>
+    </ProjectReference>
+  </ItemGroup>
+  <ItemGroup>
     <PackageReference Include="NuGet.Packaging">
       <Version>4.7.0-preview1-4929</Version>
     </PackageReference>
   </ItemGroup>
   <Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
+  <PropertyGroup>
+    <SignPath>..\..\build</SignPath>
+    <SignPath Condition="'$(BUILD_SOURCESDIRECTORY)' != ''">$(BUILD_SOURCESDIRECTORY)\build</SignPath>
+    <SignPath Condition="'$(NuGetBuildPath)' != ''">$(NuGetBuildPath)</SignPath>
+  </PropertyGroup>
+  <Import Project="$(SignPath)\sign.targets" Condition="Exists('$(SignPath)\sign.targets')" />
 </Project>