diff --git a/src/NuGetGallery.Core/Entities/EntitiesConfiguration.cs b/src/NuGetGallery.Core/Entities/EntitiesConfiguration.cs index 5fbf802e01..ce91bafd46 100644 --- a/src/NuGetGallery.Core/Entities/EntitiesConfiguration.cs +++ b/src/NuGetGallery.Core/Entities/EntitiesConfiguration.cs @@ -1,7 +1,6 @@ // 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.Data.Entity; using System.Data.Entity.Infrastructure; using System.Data.Entity.SqlServer; @@ -16,41 +15,19 @@ public EntitiesConfiguration() { // Configure Connection Resiliency / Retry Logic // See https://msdn.microsoft.com/en-us/data/dn456835.aspx and msdn.microsoft.com/en-us/data/dn307226 - SetExecutionStrategy("System.Data.SqlClient", () => UseRetriableExecutionStrategy - ? new SqlAzureExecutionStrategy() : (IDbExecutionStrategy)new DefaultExecutionStrategy()); + SetExecutionStrategy("System.Data.SqlClient", () => SuspendExecutionStrategy + ? (IDbExecutionStrategy)new DefaultExecutionStrategy() : new SqlAzureExecutionStrategy()); } - private static bool UseRetriableExecutionStrategy + public static bool SuspendExecutionStrategy { get { - return (bool?)CallContext.LogicalGetData(nameof(UseRetriableExecutionStrategy)) ?? true; + return (bool?)CallContext.LogicalGetData("SuspendExecutionStrategy") ?? false; } set { - CallContext.LogicalSetData(nameof(UseRetriableExecutionStrategy), value); - } - } - - public static IDisposable SuspendRetriableExecutionStrategy() - { - return new RetriableExecutionStrategySuspension(); - } - - private class RetriableExecutionStrategySuspension : IDisposable - { - private readonly bool _originalValue; - - internal RetriableExecutionStrategySuspension() - { - _originalValue = UseRetriableExecutionStrategy; - - UseRetriableExecutionStrategy = false; - } - - public void Dispose() - { - UseRetriableExecutionStrategy = _originalValue; + CallContext.LogicalSetData("SuspendExecutionStrategy", value); } } } diff --git a/src/NuGetGallery.Core/Entities/EntitiesContext.cs b/src/NuGetGallery.Core/Entities/EntitiesContext.cs index 9e65195623..fd3d3aca74 100644 --- a/src/NuGetGallery.Core/Entities/EntitiesContext.cs +++ b/src/NuGetGallery.Core/Entities/EntitiesContext.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Data.Entity; -using System.Data.Entity.Infrastructure; using System.Threading.Tasks; namespace NuGetGallery @@ -69,11 +68,6 @@ public void SetCommandTimeout(int? seconds) ObjectContext.CommandTimeout = seconds; } - public DbChangeTracker GetChangeTracker() - { - return ChangeTracker; - } - public Database GetDatabase() { return Database; diff --git a/src/NuGetGallery.Core/Entities/IEntitiesContext.cs b/src/NuGetGallery.Core/Entities/IEntitiesContext.cs index 04790a988f..20c3d645c0 100644 --- a/src/NuGetGallery.Core/Entities/IEntitiesContext.cs +++ b/src/NuGetGallery.Core/Entities/IEntitiesContext.cs @@ -1,15 +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. -using System; using System.Data.Entity; -using System.Data.Entity.Infrastructure; using System.Threading.Tasks; namespace NuGetGallery { public interface IEntitiesContext - : IDisposable { IDbSet CuratedFeeds { get; set; } IDbSet CuratedPackages { get; set; } @@ -23,8 +20,6 @@ public interface IEntitiesContext IDbSet Set() where T : class; void DeleteOnCommit(T entity) where T : class; void SetCommandTimeout(int? seconds); - - DbChangeTracker GetChangeTracker(); Database GetDatabase(); } } \ No newline at end of file diff --git a/src/NuGetGallery/Controllers/ApiController.cs b/src/NuGetGallery/Controllers/ApiController.cs index fb716a9262..1862446d1c 100644 --- a/src/NuGetGallery/Controllers/ApiController.cs +++ b/src/NuGetGallery/Controllers/ApiController.cs @@ -473,9 +473,6 @@ await AuditingService.SaveAuditRecordAsync( throw; } - // Handle in separate transaction because of concurrency check with retry - await PackageService.UpdateIsLatestAsync(package.PackageRegistration); - IndexingService.UpdatePackage(package); // Write an audit record @@ -555,13 +552,6 @@ public virtual async Task DeletePackage(string id, string version) } await PackageService.MarkPackageUnlistedAsync(package); - - // Handle in separate transaction because of concurrency check with retry. Due to using - // separate transactions, we must always call UpdateIsLatest on delete/unlist. This is - // because a concurrent thread could be marking the package as latest before this thread - // is able to commit the delete /unlist. - await PackageService.UpdateIsLatestAsync(package.PackageRegistration); - IndexingService.UpdatePackage(package); return new EmptyResult(); } @@ -595,10 +585,6 @@ public virtual async Task PublishPackage(string id, string version } await PackageService.MarkPackageListedAsync(package); - - // handle in separate transaction because of concurrency check with retry - await PackageService.UpdateIsLatestAsync(package.PackageRegistration); - IndexingService.UpdatePackage(package); return new EmptyResult(); } diff --git a/src/NuGetGallery/Controllers/PackagesController.cs b/src/NuGetGallery/Controllers/PackagesController.cs index 3e07e669a3..9d96e9b9db 100644 --- a/src/NuGetGallery/Controllers/PackagesController.cs +++ b/src/NuGetGallery/Controllers/PackagesController.cs @@ -999,20 +999,11 @@ internal virtual async Task Edit(string id, string version, bool? { action = "unlisted"; await _packageService.MarkPackageUnlistedAsync(package); - - // Handle in separate transaction because of concurrency check with retry. Due to using - // separate transactions, we must always call UpdateIsLatest on delete/unlist. This is - // because a concurrent thread could be marking the package as latest before this thread - // is able to commit the delete /unlist. - await _packageService.UpdateIsLatestAsync(package.PackageRegistration); } else { action = "listed"; await _packageService.MarkPackageListedAsync(package); - - // handle in separate transaction because of concurrency check with retry - await _packageService.UpdateIsLatestAsync(package.PackageRegistration); } TempData["Message"] = String.Format( CultureInfo.CurrentCulture, @@ -1204,9 +1195,6 @@ public virtual async Task VerifyPackage(VerifyPackageRequest formD throw; } - // handle in separate transaction because of concurrency check with retry - await _packageService.UpdateIsLatestAsync(package.PackageRegistration); - // tell Lucene to update index for the new package _indexingService.UpdateIndex(); diff --git a/src/NuGetGallery/Services/IPackageService.cs b/src/NuGetGallery/Services/IPackageService.cs index 0b3214cc45..ad6f3bfb10 100644 --- a/src/NuGetGallery/Services/IPackageService.cs +++ b/src/NuGetGallery/Services/IPackageService.cs @@ -16,23 +16,13 @@ public interface IPackageService IEnumerable FindPackageRegistrationsByOwner(User user); IEnumerable FindDependentPackages(Package package); - /// - /// Updates IsLatest/IsLatestStable flags after a package create, update or delete operation. - /// - /// Due to the manual optimistic concurrency check added to the underlying implementation, - /// IsLatest/IsLatestStable changes will be applied in memory and should not be committed with EF. - /// Callers should ensure that all other commits are complete before calling UpdateIsLatestAsync. - /// - /// - /// - Task UpdateIsLatestAsync(PackageRegistration packageRegistration); + Task UpdateIsLatestAsync(PackageRegistration packageRegistration, bool commitChanges = true); /// /// Populate the related database tables to create the specified package for the specified user. /// /// /// This method doesn't upload the package binary to the blob storage. The caller must do it after this call. - /// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call. /// /// The package to be created. /// The package stream's metadata. @@ -43,49 +33,10 @@ public interface IPackageService Package EnrichPackageFromNuGetPackage(Package package, PackageArchiveReader packageArchive, PackageMetadata packageMetadata, PackageStreamMetadata packageStreamMetadata, User user); - /// - /// Publishes a package by listing it. - /// - /// - /// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call. - /// - /// ID for the package to publish - /// Package version - /// Whether to commit changes to the database. - /// Task PublishPackageAsync(string id, string version, bool commitChanges = true); - - /// - /// Publishes a package by listing it. - /// - /// - /// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call. - /// - /// The package to publish - /// Whether to commit changes to the database. - /// Task PublishPackageAsync(Package package, bool commitChanges = true); - /// - /// Mark a package as unlisted. - /// - /// - /// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call. - /// - /// The package to unlist - /// Whether to commit changes to the database. - /// Task MarkPackageUnlistedAsync(Package package, bool commitChanges = true); - - /// - /// Mark a package as listed. - /// - /// - /// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call. - /// - /// The package to list. - /// Whether to commit changes to the database. - /// Task MarkPackageListedAsync(Package package, bool commitChanges = true); Task CreatePackageOwnerRequestAsync(PackageRegistration package, User currentOwner, User newOwner); diff --git a/src/NuGetGallery/Services/PackageDeleteService.cs b/src/NuGetGallery/Services/PackageDeleteService.cs index 3b1a97da0d..fa7ec2dfdf 100644 --- a/src/NuGetGallery/Services/PackageDeleteService.cs +++ b/src/NuGetGallery/Services/PackageDeleteService.cs @@ -56,55 +56,52 @@ public PackageDeleteService( public async Task SoftDeletePackagesAsync(IEnumerable packages, User deletedBy, string reason, string signature) { - List packageRegistrations; - - // Must suspend the retry execution strategy in order to use transactions. - using (EntitiesConfiguration.SuspendRetriableExecutionStrategy()) + EntitiesConfiguration.SuspendExecutionStrategy = true; + using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) { - using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) - { - // Increase command timeout - _entitiesContext.SetCommandTimeout(seconds: 300); + // Increase command timeout + _entitiesContext.SetCommandTimeout(seconds: 300); - // Keep package registrations - packageRegistrations = packages - .GroupBy(p => p.PackageRegistration) - .Select(g => g.First().PackageRegistration) - .ToList(); + // Keep package registrations + var packageRegistrations = packages + .GroupBy(p => p.PackageRegistration) + .Select(g => g.First().PackageRegistration) + .ToList(); - // Backup the package binaries and remove from main storage - // We're doing this early in the process as we need the metadata to still exist in the DB. - await BackupPackageBinaries(packages); + // Backup the package binaries and remove from main storage + // We're doing this early in the process as we need the metadata to still exist in the DB. + await BackupPackageBinaries(packages); - // Store the soft delete in the database - var packageDelete = new PackageDelete - { - DeletedOn = DateTime.UtcNow, - DeletedBy = deletedBy, - Reason = reason, - Signature = signature - }; + // Store the soft delete in the database + var packageDelete = new PackageDelete + { + DeletedOn = DateTime.UtcNow, + DeletedBy = deletedBy, + Reason = reason, + Signature = signature + }; - foreach (var package in packages) - { - package.Listed = false; - package.Deleted = true; - packageDelete.Packages.Add(package); + foreach (var package in packages) + { + package.Listed = false; + package.Deleted = true; + packageDelete.Packages.Add(package); - await _auditingService.SaveAuditRecordAsync(CreateAuditRecord(package, package.PackageRegistration, AuditedPackageAction.SoftDelete, reason)); - } + await _auditingService.SaveAuditRecordAsync(CreateAuditRecord(package, package.PackageRegistration, AuditedPackageAction.SoftDelete, reason)); + } - _packageDeletesRepository.InsertOnCommit(packageDelete); + _packageDeletesRepository.InsertOnCommit(packageDelete); - // Commit changes - await _packageRepository.CommitChangesAsync(); - await _packageDeletesRepository.CommitChangesAsync(); - transaction.Commit(); - } + // Update latest versions + await UpdateIsLatestAsync(packageRegistrations); + + // Commit changes + await _packageRepository.CommitChangesAsync(); + await _packageDeletesRepository.CommitChangesAsync(); + transaction.Commit(); } + EntitiesConfiguration.SuspendExecutionStrategy = false; - // handle in separate transaction because of concurrency check with retry - await UpdateIsLatestAsync(packageRegistrations); // Force refresh the index UpdateSearchIndex(); @@ -112,58 +109,54 @@ public async Task SoftDeletePackagesAsync(IEnumerable packages, User de public async Task HardDeletePackagesAsync(IEnumerable packages, User deletedBy, string reason, string signature, bool deleteEmptyPackageRegistration) { - List packageRegistrations; - - // Must suspend the retry execution strategy in order to use transactions. - using (EntitiesConfiguration.SuspendRetriableExecutionStrategy()) + EntitiesConfiguration.SuspendExecutionStrategy = true; + using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) { - using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) - { - // Increase command timeout - _entitiesContext.SetCommandTimeout(seconds: 300); + // Increase command timeout + _entitiesContext.SetCommandTimeout(seconds: 300); - // Keep package registrations - packageRegistrations = packages.GroupBy(p => p.PackageRegistration).Select(g => g.First().PackageRegistration).ToList(); + // Keep package registrations + var packageRegistrations = packages.GroupBy(p => p.PackageRegistration).Select(g => g.First().PackageRegistration).ToList(); - // Backup the package binaries and remove from main storage - // We're doing this early in the process as we need the metadata to still exist in the DB. - await BackupPackageBinaries(packages); + // Backup the package binaries and remove from main storage + // We're doing this early in the process as we need the metadata to still exist in the DB. + await BackupPackageBinaries(packages); - // Remove the package and related entities from the database - foreach (var package in packages) - { - await ExecuteSqlCommandAsync(_entitiesContext.GetDatabase(), - "DELETE pa FROM PackageAuthors pa JOIN Packages p ON p.[Key] = pa.PackageKey WHERE p.[Key] = @key", - new SqlParameter("@key", package.Key)); - await ExecuteSqlCommandAsync(_entitiesContext.GetDatabase(), - "DELETE pd FROM PackageDependencies pd JOIN Packages p ON p.[Key] = pd.PackageKey WHERE p.[Key] = @key", - new SqlParameter("@key", package.Key)); - await ExecuteSqlCommandAsync(_entitiesContext.GetDatabase(), - "DELETE pf FROM PackageFrameworks pf JOIN Packages p ON p.[Key] = pf.Package_Key WHERE p.[Key] = @key", - new SqlParameter("@key", package.Key)); - - await _auditingService.SaveAuditRecordAsync(CreateAuditRecord(package, package.PackageRegistration, AuditedPackageAction.Delete, reason)); - - package.PackageRegistration.Packages.Remove(package); - _packageRepository.DeleteOnCommit(package); - } + // Remove the package and related entities from the database + foreach (var package in packages) + { + await ExecuteSqlCommandAsync(_entitiesContext.GetDatabase(), + "DELETE pa FROM PackageAuthors pa JOIN Packages p ON p.[Key] = pa.PackageKey WHERE p.[Key] = @key", + new SqlParameter("@key", package.Key)); + await ExecuteSqlCommandAsync(_entitiesContext.GetDatabase(), + "DELETE pd FROM PackageDependencies pd JOIN Packages p ON p.[Key] = pd.PackageKey WHERE p.[Key] = @key", + new SqlParameter("@key", package.Key)); + await ExecuteSqlCommandAsync(_entitiesContext.GetDatabase(), + "DELETE pf FROM PackageFrameworks pf JOIN Packages p ON p.[Key] = pf.Package_Key WHERE p.[Key] = @key", + new SqlParameter("@key", package.Key)); - // Commit changes to package repository - await _packageRepository.CommitChangesAsync(); + await _auditingService.SaveAuditRecordAsync(CreateAuditRecord(package, package.PackageRegistration, AuditedPackageAction.Delete, reason)); - // Remove package registrations that have no more packages? - if (deleteEmptyPackageRegistration) - { - await RemovePackageRegistrationsWithoutPackages(packageRegistrations); - } + package.PackageRegistration.Packages.Remove(package); + _packageRepository.DeleteOnCommit(package); + } + + // Update latest versions + await UpdateIsLatestAsync(packageRegistrations); + + // Commit changes to package repository + await _packageRepository.CommitChangesAsync(); - // Commit transaction - transaction.Commit(); + // Remove package registrations that have no more packages? + if (deleteEmptyPackageRegistration) + { + await RemovePackageRegistrationsWithoutPackages(packageRegistrations); } - } - // handle in separate transaction because of concurrency check with retry - await UpdateIsLatestAsync(packageRegistrations); + // Commit transaction + transaction.Commit(); + } + EntitiesConfiguration.SuspendExecutionStrategy = false; // Force refresh the index UpdateSearchIndex(); @@ -176,9 +169,10 @@ protected virtual async Task ExecuteSqlCommandAsync(Database database, string sq private async Task UpdateIsLatestAsync(IEnumerable packageRegistrations) { + // Update latest versions foreach (var packageRegistration in packageRegistrations) { - await _packageService.UpdateIsLatestAsync(packageRegistration); + await _packageService.UpdateIsLatestAsync(packageRegistration, false); } } diff --git a/src/NuGetGallery/Services/PackageService.cs b/src/NuGetGallery/Services/PackageService.cs index 00c0329cd9..0f88e21894 100644 --- a/src/NuGetGallery/Services/PackageService.cs +++ b/src/NuGetGallery/Services/PackageService.cs @@ -3,44 +3,30 @@ using System; using System.Collections.Generic; -using System.Data; using System.Data.Entity; using System.Linq; -using System.Text; using System.Threading.Tasks; using NuGet.Frameworks; using NuGet.Packaging; using NuGet.Versioning; using NuGetGallery.Auditing; -using NuGetGallery.Configuration; -using NuGetGallery.Diagnostics; using NuGetGallery.Packaging; namespace NuGetGallery { public class PackageService : IPackageService { - private const int UpdateIsLatestMaxRetries = 3; - - private static readonly Lazy _randomGenerator = new Lazy(); - private readonly IIndexingService _indexingService; - private readonly IEntitiesContext _entitiesContext; - private readonly IAppConfiguration _configuration; private readonly IEntityRepository _packageOwnerRequestRepository; private readonly IEntityRepository _packageRegistrationRepository; private readonly IEntityRepository _packageRepository; private readonly IPackageNamingConflictValidator _packageNamingConflictValidator; private readonly IAuditingService _auditingService; - private readonly IDiagnosticsSource _trace; public PackageService( IEntityRepository packageRegistrationRepository, IEntityRepository packageRepository, IEntityRepository packageOwnerRequestRepository, - IEntitiesContext entitiesContext, - IAppConfiguration configuration, - IDiagnosticsService diagnostics, IIndexingService indexingService, IPackageNamingConflictValidator packageNamingConflictValidator, IAuditingService auditingService) @@ -60,16 +46,6 @@ public PackageService( throw new ArgumentNullException(nameof(packageOwnerRequestRepository)); } - if (entitiesContext == null) - { - throw new ArgumentNullException(nameof(entitiesContext)); - } - - if (configuration == null) - { - throw new ArgumentNullException(nameof(configuration)); - } - if (indexingService == null) { throw new ArgumentNullException(nameof(indexingService)); @@ -88,13 +64,9 @@ public PackageService( _packageRegistrationRepository = packageRegistrationRepository; _packageRepository = packageRepository; _packageOwnerRequestRepository = packageOwnerRequestRepository; - _entitiesContext = entitiesContext; - _configuration = configuration; _indexingService = indexingService; _packageNamingConflictValidator = packageNamingConflictValidator; _auditingService = auditingService; - - _trace = diagnostics.SafeGetSource("PackageService"); } public void EnsureValid(PackageArchiveReader packageArchiveReader) @@ -111,7 +83,7 @@ public void EnsureValid(PackageArchiveReader packageArchiveReader) ValidateSupportedFrameworks(supportedFrameworks); } } - + public async Task CreatePackageAsync(PackageArchiveReader nugetPackage, PackageStreamMetadata packageStreamMetadata, User user, bool commitChanges = true) { var packageMetadata = PackageMetadata.FromNuspecReader(nugetPackage.GetNuspecReader()); @@ -124,6 +96,7 @@ public async Task CreatePackageAsync(PackageArchiveReader nugetPackage, var package = CreatePackageFromNuGetPackage(packageRegistration, nugetPackage, packageMetadata, packageStreamMetadata, user); packageRegistration.Packages.Add(package); + await UpdateIsLatestAsync(packageRegistration, false); if (commitChanges) { @@ -268,7 +241,7 @@ where VersionRange.Parse(d.VersionSpec).Satisfies(packageVersion) return dependents.Select(d => d.Package); } - + public async Task PublishPackageAsync(string id, string version, bool commitChanges = true) { var package = FindPackageByIdAndVersion(id, version); @@ -280,7 +253,7 @@ public async Task PublishPackageAsync(string id, string version, bool commitChan await PublishPackageAsync(package, commitChanges); } - + public async Task PublishPackageAsync(Package package, bool commitChanges = true) { if (package == null) @@ -291,6 +264,8 @@ public async Task PublishPackageAsync(Package package, bool commitChanges = true package.Published = DateTime.UtcNow; package.Listed = true; + await UpdateIsLatestAsync(package.PackageRegistration, false); + if (commitChanges) { await _packageRepository.CommitChangesAsync(); @@ -334,7 +309,7 @@ public async Task RemovePackageOwnerAsync(PackageRegistration package, User user await _auditingService.SaveAuditRecordAsync( new PackageRegistrationAuditRecord(package, AuditedPackageRegistrationAction.RemoveOwner, user.Username)); } - + public async Task MarkPackageListedAsync(Package package, bool commitChanges = true) { if (package == null) @@ -360,6 +335,8 @@ public async Task MarkPackageListedAsync(Package package, bool commitChanges = t package.LastUpdated = DateTime.UtcNow; // NOTE: LastEdited will be overwritten by a trigger defined in the migration named "AddTriggerForPackagesLastEdited". package.LastEdited = DateTime.UtcNow; + + await UpdateIsLatestAsync(package.PackageRegistration, false); await _auditingService.SaveAuditRecordAsync(new PackageAuditRecord(package, AuditedPackageAction.List)); @@ -368,7 +345,7 @@ public async Task MarkPackageListedAsync(Package package, bool commitChanges = t await _packageRepository.CommitChangesAsync(); } } - + public async Task MarkPackageUnlistedAsync(Package package, bool commitChanges = true) { if (package == null) @@ -385,6 +362,11 @@ public async Task MarkPackageUnlistedAsync(Package package, bool commitChanges = // NOTE: LastEdited will be overwritten by a trigger defined in the migration named "AddTriggerForPackagesLastEdited". package.LastEdited = DateTime.UtcNow; + if (package.IsLatest || package.IsLatestStable) + { + await UpdateIsLatestAsync(package.PackageRegistration, false); + } + await _auditingService.SaveAuditRecordAsync(new PackageAuditRecord(package, AuditedPackageAction.Unlist)); if (commitChanges) @@ -696,150 +678,51 @@ private void ValidatePackageTitle(PackageMetadata packageMetadata) } } - protected internal async virtual Task TryUpdateIsLatestInDatabase(IEntitiesContext context) + public async Task UpdateIsLatestAsync(PackageRegistration packageRegistration, bool commitChanges = true) { - // Use the EF change tracker to identify changes made in TryUpdateIsLatestAsync which - // need to be applied to the database below. - // Note that the change tracker is not mocked which make this method hard to unit test. - var changeTracker = context.GetChangeTracker(); - var modifiedPackages = changeTracker.Entries().Where(p => p.State == EntityState.Modified).ToList(); - if (modifiedPackages.Count == 0) - { - return true; - } - - // Apply changes to the database with an optimistic concurrency check to prevent multiple - // threads (in the same or different gallery instance) from setting IsLatest/IsLatestStable - // flag to true on different package versions. - // To preserve existing behavior, we only want to reject concurrent updates which set the - // IsLatest/IsLatestStable columns. For this reason, we must avoid the EF ConcurrencyCheck - // attribute which could reject any package update or delete. - var query = new StringBuilder("DECLARE @rowCount INT = 0"); - foreach (var packageEntry in modifiedPackages) - { - // Set LastUpdated after all IsLatest/IsLatestStable changes are complete to ensure - // that we don't update rows where IsLatest/IsLatestStable hasn't changed. - packageEntry.Entity.LastUpdated = DateTime.UtcNow; - - var isLatest = packageEntry.Entity.IsLatest ? 1 : 0; - var isLatestStable = packageEntry.Entity.IsLatestStable ? 1 : 0; - var key = packageEntry.Entity.Key; - var originalIsLatest = Boolean.Parse(packageEntry.OriginalValues["IsLatest"].ToString()) ? 1 : 0; - var originalIsLatestStable = Boolean.Parse(packageEntry.OriginalValues["IsLatestStable"].ToString()) ? 1 : 0; - - query.AppendLine($"UPDATE [dbo].[Packages]"); - query.AppendLine($"SET [IsLatest] = {isLatest}, [IsLatestStable] = {isLatestStable}, [LastUpdated] = GETUTCDATE()"); - query.AppendLine($"WHERE [Key] = {key}"); - // optimistic concurrency check to prevent concurrent sets of latest/latestStable - query.AppendLine($" AND [IsLatest] = {originalIsLatest} AND [IsLatestStable] = {originalIsLatestStable}"); - // ensure new latest/latestStable was not concurrently unlisted/deleted - if (packageEntry.Entity.IsLatest || packageEntry.Entity.IsLatestStable) - { - query.AppendLine($" AND [Listed] = 1 AND [Deleted] = 0"); - } - query.AppendLine($"SET @rowCount = @rowCount + @@ROWCOUNT"); + if (!packageRegistration.Packages.Any()) + { + return; } - query.AppendLine("SELECT @rowCount"); - using (var transaction = context.GetDatabase().BeginTransaction(IsolationLevel.ReadCommitted)) + // TODO: improve setting the latest bit; this is horrible. Trigger maybe? + foreach (var pv in packageRegistration.Packages.Where(p => p.IsLatest || p.IsLatestStable)) { - var rowCount = await context.GetDatabase().ExecuteSqlCommandAsync(query.ToString()); - if (rowCount == modifiedPackages.Count) - { - transaction.Commit(); - return true; - } - else - { - // RowCount will not match if one or more updates failed the concurrency check. This - // likely means another thread is trying to clear the current IsLatest/IsLatestStable. - transaction.Rollback(); - return false; - } + pv.IsLatest = false; + pv.IsLatestStable = false; + pv.LastUpdated = DateTime.UtcNow; } - } - - private Task TryUpdateIsLatestAsync(IEntitiesContext context, PackageRegistration packageRegistration) - { - if (packageRegistration.Packages.Any()) - { - // Update in memory first to avoid putting request entities in a bad state should a concurrency - // conflict occur. - foreach (var pv in packageRegistration.Packages.Where(p => p.IsLatest || p.IsLatestStable)) - { - pv.IsLatest = false; - pv.IsLatestStable = false; - } - // If the last listed package was just unlisted, then we won't find another one. - var latestPackage = FindPackage(packageRegistration.Packages, p => !p.Deleted && p.Listed); + // If the last listed package was just unlisted, then we won't find another one + var latestPackage = FindPackage(packageRegistration.Packages, p => !p.Deleted && p.Listed); - if (latestPackage != null) - { - latestPackage.IsLatest = true; - latestPackage.IsLatestStable = !latestPackage.IsPrerelease; + if (latestPackage != null) + { + latestPackage.IsLatest = true; + latestPackage.LastUpdated = DateTime.UtcNow; - if (latestPackage.IsPrerelease) + if (latestPackage.IsPrerelease) + { + // If the newest uploaded package is a prerelease package, we need to find an older package that is + // a release version and set it to IsLatest. + var latestReleasePackage = FindPackage(packageRegistration.Packages.Where(p => !p.IsPrerelease && !p.Deleted && p.Listed)); + if (latestReleasePackage != null) { - // If the newest uploaded package is a prerelease package, we need to find an older package - // that is a release version and set it to IsLatestStable. - var latestReleasePackage = FindPackage(packageRegistration.Packages.Where(p => !p.IsPrerelease && !p.Deleted && p.Listed)); - if (latestReleasePackage != null) - { - latestReleasePackage.IsLatest = false; - latestReleasePackage.IsLatestStable = true; - } + // We could have no release packages + latestReleasePackage.IsLatestStable = true; + latestReleasePackage.LastUpdated = DateTime.UtcNow; } } - // Now try to apply the changes to the database. If this fails, we still keep the in-memory changes - // for the current executing request. More than likely the concurrent thread is just making the - // same changes and the in-memory changes will be correct. - return TryUpdateIsLatestInDatabase(context); - } - return Task.FromResult(true); - } - - protected internal virtual IEntitiesContext CreateNewEntitiesContext() - { - return new EntitiesContext(_configuration.SqlConnectionString, readOnly: false); - } - - public async Task UpdateIsLatestAsync(PackageRegistration packageRegistration) - { - // Must suspend the retry execution strategy in order to use transactions. - using (EntitiesConfiguration.SuspendRetriableExecutionStrategy()) - { - if (await TryUpdateIsLatestAsync(_entitiesContext, packageRegistration)) + else { - return; + // Only release versions are marked as IsLatestStable. + latestPackage.IsLatestStable = true; } + } - // Retry the update in case a concurrency conflict was detected on the first attempt. - int retryCount = 1; - do - { - await Task.Delay(_randomGenerator.Value.Next(0, 1000)); - - _trace.Information(String.Format("Retrying {0} for package '{1}' ({2}/{3})", - nameof(UpdateIsLatestAsync), packageRegistration.Id, retryCount, UpdateIsLatestMaxRetries)); - - // Since EF contexts are short-lived and do not really support refresh, we will use a - // different context than the request on retry to avoid putting the request context in - // a bad state. More than likely the retry will detect that the concurrent update has - // already made the right updates and no changes will be necessary. - using (var detachedRetryContext = CreateNewEntitiesContext()) - { - var detachedPackageRegistration = detachedRetryContext.PackageRegistrations.SingleOrDefault( - pr => pr.Id == packageRegistration.Id); - - if (await TryUpdateIsLatestAsync(detachedRetryContext, detachedPackageRegistration)) - { - return; - } - } - retryCount++; - } - while (retryCount <= UpdateIsLatestMaxRetries); + if (commitChanges) + { + await _packageRepository.CommitChangesAsync(); } } diff --git a/src/NuGetGallery/Services/ReflowPackageService.cs b/src/NuGetGallery/Services/ReflowPackageService.cs index 285f40c584..6787353359 100644 --- a/src/NuGetGallery/Services/ReflowPackageService.cs +++ b/src/NuGetGallery/Services/ReflowPackageService.cs @@ -34,53 +34,51 @@ public async Task ReflowAsync(string id, string version) return null; } - // Must suspend the retry execution strategy in order to use transactions. - using (EntitiesConfiguration.SuspendRetriableExecutionStrategy()) + EntitiesConfiguration.SuspendExecutionStrategy = true; + using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) { - using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) + // 1) Download package binary to memory + using (var packageStream = await _packageFileService.DownloadPackageFileAsync(package)) { - // 1) Download package binary to memory - using (var packageStream = await _packageFileService.DownloadPackageFileAsync(package)) + using (var packageArchive = new PackageArchiveReader(packageStream, leaveStreamOpen: false)) { - using (var packageArchive = new PackageArchiveReader(packageStream, leaveStreamOpen: false)) + // 2) Determine package metadata from binary + var packageStreamMetadata = new PackageStreamMetadata { - // 2) Determine package metadata from binary - var packageStreamMetadata = new PackageStreamMetadata - { - HashAlgorithm = Constants.Sha512HashAlgorithmId, - Hash = CryptographyService.GenerateHash(packageStream.AsSeekableStream()), - Size = packageStream.Length, - }; - - var packageMetadata = PackageMetadata.FromNuspecReader(packageArchive.GetNuspecReader()); - - // 3) Clear referenced objects that will be reflowed - ClearSupportedFrameworks(package); - ClearAuthors(package); - ClearDependencies(package); - - // 4) Reflow the package - var listed = package.Listed; - - package = _packageService.EnrichPackageFromNuGetPackage( - package, - packageArchive, - packageMetadata, - packageStreamMetadata, - package.User); - - package.LastEdited = DateTime.UtcNow; - package.Listed = listed; - - // 5) Save and profit - await _entitiesContext.SaveChangesAsync(); - } - } + HashAlgorithm = Constants.Sha512HashAlgorithmId, + Hash = CryptographyService.GenerateHash(packageStream.AsSeekableStream()), + Size = packageStream.Length, + }; + + var packageMetadata = PackageMetadata.FromNuspecReader(packageArchive.GetNuspecReader()); + + // 3) Clear referenced objects that will be reflowed + ClearSupportedFrameworks(package); + ClearAuthors(package); + ClearDependencies(package); + + // 4) Reflow the package + var listed = package.Listed; - // Commit transaction - transaction.Commit(); + package = _packageService.EnrichPackageFromNuGetPackage( + package, + packageArchive, + packageMetadata, + packageStreamMetadata, + package.User); + + package.LastEdited = DateTime.UtcNow; + package.Listed = listed; + + // 5) Save and profit + await _entitiesContext.SaveChangesAsync(); + } } + + // Commit transaction + transaction.Commit(); } + EntitiesConfiguration.SuspendExecutionStrategy = false; return package; } diff --git a/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs index 353adab7a4..a639f9cf83 100644 --- a/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs @@ -10,18 +10,18 @@ using System.Web; using System.Web.Mvc; using System.Web.Routing; -using Xunit; using Moq; using NuGet.Frameworks; using NuGet.Packaging; using NuGet.Versioning; -using NuGetGallery.Areas.Admin; using NuGetGallery.AsyncFileUpload; -using NuGetGallery.Auditing; using NuGetGallery.Configuration; using NuGetGallery.Framework; using NuGetGallery.Helpers; using NuGetGallery.Packaging; +using NuGetGallery.Areas.Admin; +using NuGetGallery.Auditing; +using Xunit; namespace NuGetGallery { @@ -51,9 +51,9 @@ private static PackagesController CreateController( if (uploadFileService == null) { uploadFileService = new Mock(); - uploadFileService.Setup(x => x.DeleteUploadFileAsync(It.IsAny())).Returns(Task.CompletedTask); + uploadFileService.Setup(x => x.DeleteUploadFileAsync(It.IsAny())).Returns(Task.FromResult(0)); uploadFileService.Setup(x => x.GetUploadFileAsync(42)).Returns(Task.FromResult(null)); - uploadFileService.Setup(x => x.SaveUploadFileAsync(42, It.IsAny())).Returns(Task.CompletedTask); + uploadFileService.Setup(x => x.SaveUploadFileAsync(42, It.IsAny())).Returns(Task.FromResult(0)); } messageService = messageService ?? new Mock(); searchService = searchService ?? CreateSearchService(); @@ -63,7 +63,7 @@ private static PackagesController CreateController( if (packageFileService == null) { packageFileService = new Mock(); - packageFileService.Setup(p => p.SavePackageFileAsync(It.IsAny(), It.IsAny())).Returns(Task.CompletedTask); + packageFileService.Setup(p => p.SavePackageFileAsync(It.IsAny(), It.IsAny())).Returns(Task.FromResult(0)); } entitiesContext = entitiesContext ?? new Mock(); @@ -137,7 +137,7 @@ public class TheCancelVerifyPackageAction public async Task DeletesTheInProgressPackageUpload() { var fakeUploadFileService = new Mock(); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(42)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(42)).Returns(Task.FromResult(0)); var controller = CreateController( uploadFileService: fakeUploadFileService); controller.SetCurrentUser(TestUtility.FakeUser); @@ -151,7 +151,7 @@ public async Task DeletesTheInProgressPackageUpload() public async Task RedirectsToUploadPageAfterDelete() { var fakeUploadFileService = new Mock(); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(42)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(42)).Returns(Task.FromResult(0)); var controller = CreateController( uploadFileService: fakeUploadFileService); controller.SetCurrentUser(TestUtility.FakeUser); @@ -545,11 +545,9 @@ public async Task UpdatesUnlistedIfSelected() packageService.Setup(svc => svc.MarkPackageListedAsync(It.IsAny(), It.IsAny())) .Throws(new Exception("Shouldn't be called")); packageService.Setup(svc => svc.MarkPackageUnlistedAsync(It.IsAny(), It.IsAny())) - .Returns(Task.CompletedTask).Verifiable(); + .Returns(Task.FromResult(0)).Verifiable(); packageService.Setup(svc => svc.FindPackageByIdAndVersion("Foo", "1.0", true)) .Returns(package).Verifiable(); - packageService.Setup(svc => svc.UpdateIsLatestAsync(It.IsAny())) - .Returns(Task.CompletedTask).Verifiable(); var indexingService = new Mock(); @@ -581,13 +579,11 @@ public async Task UpdatesUnlistedIfNotSelected() var packageService = new Mock(MockBehavior.Strict); packageService.Setup(svc => svc.MarkPackageListedAsync(It.IsAny(), It.IsAny())) - .Returns(Task.CompletedTask).Verifiable(); + .Returns(Task.FromResult(0)).Verifiable(); packageService.Setup(svc => svc.MarkPackageUnlistedAsync(It.IsAny(), It.IsAny())) .Throws(new Exception("Shouldn't be called")); packageService.Setup(svc => svc.FindPackageByIdAndVersion("Foo", "1.0", true)) .Returns(package).Verifiable(); - packageService.Setup(svc => svc.UpdateIsLatestAsync(It.IsAny())) - .Returns(Task.CompletedTask).Verifiable(); var indexingService = new Mock(); @@ -1047,9 +1043,9 @@ public async Task WillSaveTheUploadFile() fakeUploadedFile.Setup(x => x.InputStream).Returns(fakeFileStream); var fakeUploadFileService = new Mock(); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(null)); - fakeUploadFileService.Setup(x => x.SaveUploadFileAsync(TestUtility.FakeUser.Key, It.IsAny())).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.SaveUploadFileAsync(TestUtility.FakeUser.Key, It.IsAny())).Returns(Task.FromResult(0)); var controller = CreateController( uploadFileService: fakeUploadFileService, fakeNuGetPackage: fakeFileStream); @@ -1069,9 +1065,9 @@ public async Task WillRedirectToVerifyPackageActionAfterSaving() var fakeFileStream = TestPackage.CreateTestPackageStream("thePackageId", "1.0.0"); fakeUploadedFile.Setup(x => x.InputStream).Returns(fakeFileStream); var fakeUploadFileService = new Mock(); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(null)); - fakeUploadFileService.Setup(x => x.SaveUploadFileAsync(TestUtility.FakeUser.Key, It.IsAny())).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.SaveUploadFileAsync(TestUtility.FakeUser.Key, It.IsAny())).Returns(Task.FromResult(0)); var controller = CreateController( uploadFileService: fakeUploadFileService); controller.SetCurrentUser(TestUtility.FakeUser); @@ -1090,7 +1086,7 @@ public async Task WillRedirectToUploadPackagePageWhenThereIsNoUploadInProgress() { var fakeUploadFileService = new Mock(); fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(null); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(null)); var controller = CreateController( uploadFileService: fakeUploadFileService); @@ -1391,7 +1387,7 @@ public async Task WillCreateThePackage() using (var fakeFileStream = new MemoryStream()) { fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); var fakePackageService = new Mock(); fakePackageService.Setup(x => x.CreatePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())).Returns( Task.FromResult(new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" })); @@ -1417,7 +1413,7 @@ public async Task WillSavePackageToFileStorage() using (var fakeFileStream = new MemoryStream()) { fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); var fakePackageService = new Mock(); var fakePackage = new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" }; fakePackageService.Setup(x => x.CreatePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) @@ -1425,7 +1421,7 @@ public async Task WillSavePackageToFileStorage() var fakeNuGetPackage = TestPackage.CreateTestPackageStream("theId", "1.0.0"); var fakePackageFileService = new Mock(); - fakePackageFileService.Setup(x => x.SavePackageFileAsync(fakePackage, It.IsAny())).Returns(Task.CompletedTask).Verifiable(); + fakePackageFileService.Setup(x => x.SavePackageFileAsync(fakePackage, It.IsAny())).Returns(Task.FromResult(0)).Verifiable(); var controller = CreateController( packageService: fakePackageService, @@ -1453,7 +1449,7 @@ public async Task WillDeletePackageFileFromBlobStorageIfSavingDbChangesFails() using (var fakeFileStream = new MemoryStream()) { fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); var fakePackageService = new Mock(); var fakePackage = new Package { PackageRegistration = new PackageRegistration { Id = packageId }, Version = packageVersion }; fakePackageService.Setup(x => x.CreatePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) @@ -1492,7 +1488,7 @@ public async Task WillShowViewWithMessageIfSavingPackageBlobFails() using (var fakeFileStream = new MemoryStream()) { fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); var fakePackageService = new Mock(); var fakePackage = new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" }; fakePackageService.Setup(x => x.CreatePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) @@ -1531,14 +1527,14 @@ public async Task WillUpdateIndexingService() using (var fakeFileStream = new MemoryStream()) { fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); var fakePackageService = new Mock(); var fakePackage = new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" }; fakePackageService.Setup(x => x.CreatePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) .Returns(Task.FromResult(fakePackage)); var fakeNuGetPackage = TestPackage.CreateTestPackageStream("theId", "1.0.0"); var fakePackageFileService = new Mock(); - fakePackageFileService.Setup(x => x.SavePackageFileAsync(fakePackage, It.IsAny())).Returns(Task.CompletedTask).Verifiable(); + fakePackageFileService.Setup(x => x.SavePackageFileAsync(fakePackage, It.IsAny())).Returns(Task.FromResult(0)).Verifiable(); var fakeIndexingService = new Mock(MockBehavior.Strict); fakeIndexingService.Setup(f => f.UpdateIndex()).Verifiable(); @@ -1614,8 +1610,6 @@ public async Task WillNotCommitChangesToPackageService() .Returns(Task.CompletedTask); fakePackageService.Setup(x => x.MarkPackageUnlistedAsync(fakePackage, false)) .Returns(Task.CompletedTask); - fakePackageService.Setup(x => x.UpdateIsLatestAsync(It.IsAny())) - .Returns(Task.CompletedTask); var fakeNuGetPackage = TestPackage.CreateTestPackageStream("theId", "1.0.0"); var controller = CreateController( @@ -1639,7 +1633,7 @@ public async Task WillPublishThePackage() using (var fakeFileStream = new MemoryStream()) { fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); var fakePackage = new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" }; var fakePackageService = new Mock(); fakePackageService.Setup(x => x.CreatePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) @@ -1664,7 +1658,7 @@ public async Task WillMarkThePackageUnlistedWhenListedArgumentIsFalse() var fakeUploadFileService = new Mock(); using (var fakeFileStream = new MemoryStream()) { - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); var fakePackageService = new Mock(); fakePackageService.Setup(x => x.CreatePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) @@ -1693,7 +1687,7 @@ public async Task WillNotMarkThePackageUnlistedWhenListedArgumentIsNullorTrue(bo using (var fakeFileStream = new MemoryStream()) { fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); var fakePackageService = new Mock(); fakePackageService.Setup(x => x.CreatePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) .Returns(Task.FromResult(new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" })); @@ -1715,7 +1709,7 @@ public async Task WillNotMarkThePackageUnlistedWhenListedArgumentIsNullorTrue(bo public async Task WillDeleteTheUploadFile() { var fakeUploadFileService = new Mock(); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask).Verifiable(); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)).Verifiable(); using (var fakeFileStream = new MemoryStream()) { fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); @@ -1743,8 +1737,8 @@ public async Task WillSetAFlashMessage() using (var fakeFileStream = new MemoryStream()) { fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); - fakeUploadFileService.Setup(x => x.SaveUploadFileAsync(TestUtility.FakeUser.Key, It.IsAny())).Returns(Task.CompletedTask); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.SaveUploadFileAsync(TestUtility.FakeUser.Key, It.IsAny())).Returns(Task.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); var fakePackageService = new Mock(); fakePackageService.Setup(x => x.CreatePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) .Returns(Task.FromResult(new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" })); @@ -1769,7 +1763,7 @@ public async Task WillRedirectToPackagePage() using (var fakeFileStream = new MemoryStream()) { fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); var fakePackageService = new Mock(); fakePackageService.Setup(x => x.CreatePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) .Returns(Task.FromResult(new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" })); @@ -1795,7 +1789,7 @@ public async Task WillCurateThePackage() using (var fakeFileStream = new MemoryStream()) { fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); var fakePackageService = new Mock(); var fakePackage = new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" }; fakePackageService.Setup(x => x.CreatePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) @@ -1824,7 +1818,7 @@ public async Task WritesAnAuditRecord() using (var fakeFileStream = new MemoryStream()) { fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); var fakePackageService = new Mock(); var fakePackage = new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" }; fakePackageService.Setup(x => x.CreatePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) diff --git a/tests/NuGetGallery.Facts/Services/PackageDeleteServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageDeleteServiceFacts.cs index d6dd9d1ed6..61eca071d9 100644 --- a/tests/NuGetGallery.Facts/Services/PackageDeleteServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageDeleteServiceFacts.cs @@ -173,7 +173,7 @@ public async Task WillUpdatePackageLatestVersions() await service.SoftDeletePackagesAsync(new[] { package }, user, string.Empty, string.Empty); - packageService.Verify(x => x.UpdateIsLatestAsync(packageRegistration)); + packageService.Verify(x => x.UpdateIsLatestAsync(packageRegistration, false)); } [Fact] @@ -351,7 +351,7 @@ public async Task WillUpdatePackageLatestVersions() await service.HardDeletePackagesAsync(new[] { package }, user, string.Empty, string.Empty, false); - packageService.Verify(x => x.UpdateIsLatestAsync(packageRegistration)); + packageService.Verify(x => x.UpdateIsLatestAsync(packageRegistration, false)); } [Fact] diff --git a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs index d566a73102..cf54495582 100644 --- a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs @@ -3,16 +3,14 @@ using System; using System.Collections.Generic; -using System.Data.Entity; using System.Linq; using System.Threading.Tasks; using Moq; using NuGet.Frameworks; using NuGet.Packaging; +using NuGet.Packaging.Core; using NuGet.Versioning; using NuGetGallery.Auditing; -using NuGetGallery.Configuration; -using NuGetGallery.Diagnostics; using NuGetGallery.Framework; using NuGetGallery.Packaging; using Xunit; @@ -100,34 +98,6 @@ private static IPackageService CreateService( Mock> packageRegistrationRepository = null, Mock> packageRepository = null, Mock> packageOwnerRequestRepo = null, - Mock entitiesContext = null, - Mock configuration = null, - Mock diagnosticsService = null, - Mock indexingService = null, - IPackageNamingConflictValidator packageNamingConflictValidator = null, - IAuditingService auditingService = null, - Action> setup = null) - { - return CreateServiceMock( - packageRegistrationRepository, - packageRepository, - packageOwnerRequestRepo, - entitiesContext, - configuration, - diagnosticsService, - indexingService, - packageNamingConflictValidator, - auditingService, - setup).Object; - } - - private static Mock CreateServiceMock( - Mock> packageRegistrationRepository = null, - Mock> packageRepository = null, - Mock> packageOwnerRequestRepo = null, - Mock entitiesContext = null, - Mock configuration = null, - Mock diagnosticsService = null, Mock indexingService = null, IPackageNamingConflictValidator packageNamingConflictValidator = null, IAuditingService auditingService = null, @@ -136,15 +106,6 @@ private static Mock CreateServiceMock( packageRegistrationRepository = packageRegistrationRepository ?? new Mock>(); packageRepository = packageRepository ?? new Mock>(); packageOwnerRequestRepo = packageOwnerRequestRepo ?? new Mock>(); - - var dbContext = new Mock(); - entitiesContext = entitiesContext ?? new Mock(); - entitiesContext.Setup(m => m.GetDatabase()).Returns(dbContext.Object.Database); - entitiesContext.Setup(m => m.GetChangeTracker()).Returns(dbContext.Object.ChangeTracker); - - configuration = configuration ?? new Mock(); - - diagnosticsService = diagnosticsService ?? new Mock(); indexingService = indexingService ?? new Mock(); auditingService = auditingService ?? new TestAuditingService(); @@ -159,17 +120,10 @@ private static Mock CreateServiceMock( packageRegistrationRepository.Object, packageRepository.Object, packageOwnerRequestRepo.Object, - entitiesContext.Object, - configuration.Object, - diagnosticsService.Object, indexingService.Object, packageNamingConflictValidator, auditingService); - packageService.Setup(s => s.TryUpdateIsLatestInDatabase(It.IsAny())) - .Returns(Task.FromResult(true)); - packageService.Setup(s => s.CreateNewEntitiesContext()).Returns(entitiesContext.Object); - packageService.CallBase = true; if (setup != null) @@ -177,7 +131,7 @@ private static Mock CreateServiceMock( setup(packageService); } - return packageService; + return packageService.Object; } public class TheAddPackageOwnerMethod @@ -623,6 +577,19 @@ public async Task WillNotSetTheNewPackagesCreatedAndLastUpdatedTimesAsTheDatabas Assert.Equal(DateTime.MinValue, package.Created); } + [Fact] + public async Task WillSetTheNewPackagesLastUpdatedTimes() + { + var service = CreateService(setup: + mockPackageService => { mockPackageService.Setup(x => x.FindPackageRegistrationById(It.IsAny())).Returns((PackageRegistration)null); }); + var nugetPackage = CreateNuGetPackage(); + var currentUser = new User(); + + var package = await service.CreatePackageAsync(nugetPackage.Object, new PackageStreamMetadata(), currentUser); + + Assert.NotEqual(DateTime.MinValue, package.LastUpdated); + } + [Fact] public async Task WillSaveThePackageFileAndSetThePackageFileSize() { @@ -1046,26 +1013,45 @@ public async Task ReturnsExistingMatchingPackageOwnerRequest() public class TheUpdateIsLatestMethod { [Fact] - public async Task AlwaysCommitChanges() + public async Task DoNotCommitIfCommitChangesIsFalse() { // Arrange var packageRegistration = new PackageRegistration(); var package = new Package { PackageRegistration = packageRegistration, Version = "1.0.0" }; packageRegistration.Packages.Add(package); var packageRepository = new Mock>(); - - var service = CreateServiceMock(packageRepository: packageRepository, setup: + + var service = CreateService(packageRepository: packageRepository, setup: + mockService => { mockService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package); }); + + // Act + await service.UpdateIsLatestAsync(packageRegistration, commitChanges: false); + + // Assert + packageRepository.Verify(r => r.CommitChangesAsync(), Times.Never()); + } + + [Fact] + public async Task CommitIfCommitChangesIsTrue() + { + // Arrange + var packageRegistration = new PackageRegistration(); + var package = new Package { PackageRegistration = packageRegistration, Version = "1.0.0" }; + packageRegistration.Packages.Add(package); + var packageRepository = new Mock>(); + + var service = CreateService(packageRepository: packageRepository, setup: mockService => { mockService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package); }); // Act - await service.Object.UpdateIsLatestAsync(packageRegistration); + await service.UpdateIsLatestAsync(packageRegistration, true); // Assert - service.Verify(s => s.TryUpdateIsLatestInDatabase(It.IsAny()), Times.Once); + packageRepository.Verify(r => r.CommitChangesAsync(), Times.Once()); } [Fact] - public async Task UpdateIsLatestAsync_DifferentLatestAndLatestStableVersions() + public async Task WillUpdateIsLatest1() { // Arrange var packages = new HashSet(); @@ -1075,23 +1061,24 @@ public async Task UpdateIsLatestAsync_DifferentLatestAndLatestStableVersions() var package09 = new Package { PackageRegistration = packageRegistration, Version = "0.9.0" }; packages.Add(package09); var packageRepository = new Mock>(MockBehavior.Strict); - var service = CreateServiceMock(packageRepository: packageRepository, setup: + packageRepository.Setup(r => r.CommitChangesAsync()) + .Returns(Task.CompletedTask).Verifiable(); + var service = CreateService(packageRepository: packageRepository, setup: mockService => { mockService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package10A); }); // Act - await service.Object.UpdateIsLatestAsync(packageRegistration); + await service.UpdateIsLatestAsync(packageRegistration, true); // Assert Assert.True(package10A.IsLatest); Assert.False(package10A.IsLatestStable); Assert.False(package09.IsLatest); Assert.True(package09.IsLatestStable); - - service.Verify(s => s.TryUpdateIsLatestInDatabase(It.IsAny()), Times.Once); + packageRepository.Verify(); } [Fact] - public async Task UpdateIsLatestAsync_SameLatestAndLatestStableVersions() + public async Task WillUpdateIsLatest2() { // Arrange var packages = new HashSet(); @@ -1103,11 +1090,13 @@ public async Task UpdateIsLatestAsync_SameLatestAndLatestStableVersions() var package09 = new Package { PackageRegistration = packageRegistration, Version = "0.9.0" }; packages.Add(package09); var packageRepository = new Mock>(MockBehavior.Strict); - var service = CreateServiceMock(packageRepository: packageRepository, setup: + packageRepository.Setup(r => r.CommitChangesAsync()) + .Returns(Task.CompletedTask).Verifiable(); + var service = CreateService(packageRepository: packageRepository, setup: mockService => { mockService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package100); }); // Act - await service.Object.UpdateIsLatestAsync(packageRegistration); + await service.UpdateIsLatestAsync(packageRegistration, true); // Assert Assert.True(package100.IsLatest); @@ -1116,42 +1105,7 @@ public async Task UpdateIsLatestAsync_SameLatestAndLatestStableVersions() Assert.False(package10A.IsLatestStable); Assert.False(package09.IsLatest); Assert.False(package09.IsLatestStable); - - service.Verify(s => s.TryUpdateIsLatestInDatabase(It.IsAny()), Times.Once); - } - - [Fact] - public async Task UpdateIsLatestAsync_SameNewLatestAndLatestStableVersions() - { - // Arrange - var packages = new HashSet(); - var packageRegistration = new PackageRegistration { Packages = packages }; - var package10A = new Package { PackageRegistration = packageRegistration, Version = "1.0.0-a", IsPrerelease = true, IsLatest = true }; - packages.Add(package10A); - var package10 = new Package { PackageRegistration = packageRegistration, Version = "1.0.0" }; - packages.Add(package10); - var package09 = new Package { PackageRegistration = packageRegistration, Version = "0.9.0", IsLatestStable = true }; - packages.Add(package09); - var package20 = new Package { PackageRegistration = packageRegistration, Version = "2.0.0" }; - packages.Add(package20); - var packageRepository = new Mock>(MockBehavior.Strict); - var service = CreateServiceMock(packageRepository: packageRepository, setup: - mockService => { mockService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package10A); }); - - // Act - await service.Object.UpdateIsLatestAsync(packageRegistration); - - // Assert - Assert.True(package20.IsLatest); - Assert.True(package20.IsLatestStable); - Assert.False(package10A.IsLatest); - Assert.False(package10A.IsLatestStable); - Assert.False(package10.IsLatest); - Assert.False(package10.IsLatestStable); - Assert.False(package09.IsLatest); - Assert.False(package09.IsLatestStable); - - service.Verify(s => s.TryUpdateIsLatestInDatabase(It.IsAny()), Times.Once); + packageRepository.Verify(); } } @@ -1450,7 +1404,6 @@ public async Task OnPackageVersionHigherThanLatestSetsItToLatestVersion() var service = CreateService(packageRepository: packageRepository); await service.MarkPackageListedAsync(packages[0]); - await service.UpdateIsLatestAsync(packageRegistration); Assert.True(packageRegistration.Packages.ElementAt(0).IsLatest); Assert.True(packageRegistration.Packages.ElementAt(0).IsLatestStable); @@ -1556,7 +1509,6 @@ public async Task OnLatestPackageVersionSetsPreviousToLatestVersion() var service = CreateService(packageRepository: packageRepository); await service.MarkPackageUnlistedAsync(packages[0]); - await service.UpdateIsLatestAsync(packages[0].PackageRegistration); Assert.False(packageRegistration.Packages.ElementAt(0).IsLatest); Assert.False(packageRegistration.Packages.ElementAt(0).IsLatestStable); @@ -1574,7 +1526,6 @@ public async Task OnOnlyListedPackageSetsNoPackageToLatestVersion() var service = CreateService(packageRepository: packageRepository); await service.MarkPackageUnlistedAsync(package); - await service.UpdateIsLatestAsync(package.PackageRegistration); Assert.False(package.IsLatest, "IsLatest"); Assert.False(package.IsLatestStable, "IsLatestStable"); @@ -1670,7 +1621,6 @@ public async Task WillSetUpdateIsLatestStableOnThePackageWhenItIsTheLatestVersio mockPackageService => { mockPackageService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package); }); await service.PublishPackageAsync(package); - await service.UpdateIsLatestAsync(package.PackageRegistration); Assert.True(package.IsLatestStable); } @@ -1694,7 +1644,6 @@ public async Task WillSetUpdateIsLatestStableOnThePackageWhenItIsTheLatestVersio mockPackageService => { mockPackageService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package); }); await service.PublishPackageAsync("theId", "1.0.42"); - await service.UpdateIsLatestAsync(package.PackageRegistration); Assert.True(package.IsLatestStable); } @@ -1724,7 +1673,6 @@ public async Task WillNotSetUpdateIsLatestStableOnThePackageWhenItIsNotTheLatest mockPackageService => { mockPackageService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package); }); await service.PublishPackageAsync(package); - await service.UpdateIsLatestAsync(package.PackageRegistration); Assert.False(package.IsLatestStable); } @@ -1756,8 +1704,6 @@ public async Task PublishPackageUpdatesIsAbsoluteLatestForPrereleasePackage() mockPackageService => { mockPackageService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package); }); await service.PublishPackageAsync("theId", "1.0.42-alpha"); - await service.UpdateIsLatestAsync(package.PackageRegistration); - Assert.True(package39.IsLatestStable); Assert.False(package39.IsLatest); Assert.False(package.IsLatestStable); @@ -1791,7 +1737,6 @@ public async Task PublishPackageUpdatesIsAbsoluteLatestForPrereleasePackageWithO mockPackageService => { mockPackageService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package); }); await service.PublishPackageAsync(package); - await service.UpdateIsLatestAsync(package.PackageRegistration); Assert.True(package39.IsLatestStable); Assert.False(package39.IsLatest); @@ -1827,8 +1772,6 @@ public async Task SetUpdateDoesNotSetIsLatestStableForAnyIfAllPackagesArePrerele mockPackageService => { mockPackageService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package); }); await service.PublishPackageAsync("theId", "1.0.42-alpha"); - await service.UpdateIsLatestAsync(package.PackageRegistration); - Assert.False(package39.IsLatestStable); Assert.False(package39.IsLatest); Assert.False(package.IsLatestStable); @@ -1863,8 +1806,6 @@ public async Task SetUpdateDoesNotSetIsLatestStableForAnyIfAllPackagesArePrerele mockPackageService => { mockPackageService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package); }); await service.PublishPackageAsync(package); - await service.UpdateIsLatestAsync(package.PackageRegistration); - Assert.False(package39.IsLatestStable); Assert.False(package39.IsLatest); Assert.False(package.IsLatestStable); diff --git a/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs index f447a68797..3fe39f40f3 100644 --- a/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs @@ -10,8 +10,6 @@ using System.Threading.Tasks; using Moq; using NuGet.Packaging; -using NuGetGallery.Configuration; -using NuGetGallery.Diagnostics; using NuGetGallery.Framework; using NuGetGallery.Packaging; using Xunit; @@ -284,9 +282,6 @@ private static Mock SetupPackageService(Package package) var packageRegistrationRepository = new Mock>(); var packageRepository = new Mock>(); var packageOwnerRequestRepo = new Mock>(); - var diagnosticsService = new Mock(); - var entitiesContext = new Mock(); - var configuration = new Mock(); var indexingService = new Mock(); var packageNamingConflictValidator = new PackageNamingConflictValidator( packageRegistrationRepository.Object, @@ -297,9 +292,6 @@ private static Mock SetupPackageService(Package package) packageRegistrationRepository.Object, packageRepository.Object, packageOwnerRequestRepo.Object, - entitiesContext.Object, - configuration.Object, - diagnosticsService.Object, indexingService.Object, packageNamingConflictValidator, auditingService); diff --git a/tests/NuGetGallery.Facts/TestUtils/FakeEntitiesContext.cs b/tests/NuGetGallery.Facts/TestUtils/FakeEntitiesContext.cs index 1927e7bbd7..969583c167 100644 --- a/tests/NuGetGallery.Facts/TestUtils/FakeEntitiesContext.cs +++ b/tests/NuGetGallery.Facts/TestUtils/FakeEntitiesContext.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using System.Data.Entity; -using System.Data.Entity.Infrastructure; using System.Threading.Tasks; using Xunit; @@ -131,19 +130,10 @@ public void SetCommandTimeout(int? seconds) throw new NotSupportedException(); } - public DbChangeTracker GetChangeTracker() - { - throw new NotSupportedException(); - } - public Database GetDatabase() { throw new NotSupportedException(); } - - public void Dispose() - { - } } } diff --git a/tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs b/tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs index 4fffadbf36..8babf385c0 100644 --- a/tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs +++ b/tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs @@ -2,28 +2,22 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Generic; using System.IO; using System.Linq; using System.Net; using System.Net.Http; using System.Threading.Tasks; -using System.Xml.Linq; using Xunit; using Xunit.Abstractions; namespace NuGetGallery.FunctionalTests { - /// /// This class has the helper methods to do gallery operations via OData. /// public class ODataHelper : HelperBase { - private static readonly XNamespace FeedAtomNS = "http://www.w3.org/2005/Atom"; - private static readonly XNamespace FeedAdoNS = "http://schemas.microsoft.com/ado/2007/08/dataservices/metadata"; - public ODataHelper() : this(ConsoleTestOutputHelper.New) { @@ -65,38 +59,55 @@ public async Task TryDownloadPackageFromFeed(string packageId, string ve } } - private object GetPackageProperty(string propertyType, string propertyValue) + public async Task GetTimestampOfPackageFromResponse(string url, string propertyName, string packageId, string version = "1.0.0") { - switch (propertyType) + WriteLine($"Getting '{propertyName}' timestamp of package '{packageId}' with version '{version}'."); + + var packageResponse = await GetPackageDataInResponse(url, packageId, version); + if (string.IsNullOrEmpty(packageResponse)) + { + return null; + } + + var timestampStartTag = ""; + var timestampEndTag = ""; + + var timestampTagIndex = packageResponse.IndexOf(timestampStartTag); + if (timestampTagIndex < 0) { - case "Edm.DateTime": - return DateTime.Parse(propertyValue); - case "Edm.Boolean": - return Boolean.Parse(propertyValue); - default: - return propertyValue; + WriteLine($"Package data does not contain '{propertyName}' timestamp!"); + return null; } + + var timestampStartIndex = timestampTagIndex + timestampStartTag.Length; + var timestampLength = packageResponse.Substring(timestampStartIndex).IndexOf(timestampEndTag); + + var timestamp = + DateTime.Parse(packageResponse.Substring(timestampStartIndex, timestampLength)); + WriteLine($"'{propertyName}' timestamp of package '{packageId}' with version '{version}' is '{timestamp}'"); + return timestamp; } - public async Task> GetPackagePropertiesFromResponse(string url, string packageId, string version = "1.0.0") + public async Task GetPackageDataInResponse(string url, string packageId, string version = "1.0.0") { - WriteLine($"Getting properties for package '{packageId}' with version '{version}'."); + WriteLine($"Getting data for package '{packageId}' with version '{version}'."); var responseText = await GetResponseText(url); - var packagesXml = XDocument.Parse(responseText); + var packageString = @"" + UrlHelper.V2FeedRootUrl + @"Packages(Id='" + packageId + @"',Version='" + (string.IsNullOrEmpty(version) ? "" : version + "')"); + var endEntryTag = ""; - var entries = packagesXml.Root.Elements(FeedAtomNS + "entry").ToList(); - var packageEntry = entries.First(e => e.Element(FeedAtomNS + "id").Value.Contains($"Id='{packageId}'")); - var packageProperties = packageEntry.Element(FeedAdoNS + "properties"); + var startingIndex = responseText.IndexOf(packageString); - var properties = new Dictionary(); - foreach (var prop in packageProperties.Elements()) + if (startingIndex < 0) { - var propType = prop.Attribute(FeedAdoNS + "type")?.Value; - properties[prop.Name.LocalName] = GetPackageProperty(propType, prop.Value); + WriteLine("Package not found in response text!"); + return null; } - return properties; + + var endingIndex = responseText.IndexOf(endEntryTag, startingIndex); + + return responseText.Substring(startingIndex, endingIndex - startingIndex); } public async Task ContainsResponseText(string url, params string[] expectedTexts) diff --git a/tests/NuGetGallery.FunctionalTests.Core/NuGetGallery.FunctionalTests.Core.csproj b/tests/NuGetGallery.FunctionalTests.Core/NuGetGallery.FunctionalTests.Core.csproj index a5b56ef1eb..6a93865033 100644 --- a/tests/NuGetGallery.FunctionalTests.Core/NuGetGallery.FunctionalTests.Core.csproj +++ b/tests/NuGetGallery.FunctionalTests.Core/NuGetGallery.FunctionalTests.Core.csproj @@ -48,8 +48,6 @@ - - False ..\..\packages\xunit.abstractions.2.0.0\lib\net35\xunit.abstractions.dll diff --git a/tests/NuGetGallery.FunctionalTests/ODataFeeds/V2FeedExtendedTests.cs b/tests/NuGetGallery.FunctionalTests/ODataFeeds/V2FeedExtendedTests.cs index 927a86626c..17a695a630 100644 --- a/tests/NuGetGallery.FunctionalTests/ODataFeeds/V2FeedExtendedTests.cs +++ b/tests/NuGetGallery.FunctionalTests/ODataFeeds/V2FeedExtendedTests.cs @@ -91,68 +91,6 @@ public async Task PackagesAppearInFeedInOrderTest() await CheckPackageTimestampsInOrder(packageIds, "LastEdited", unlistStartTimestamp); } - [Fact] - [Description("Verifies that the IsLatest/IsLatestStable flags are set correctly when different package versions are pushed concurrently")] - [Priority(1)] - [Category("P0Tests")] - public async Task PackageLatestSetCorrectlyOnConcurrentPushes() - { - var packageId = $"TestV2FeedPackageLatestSetCorrectlyOnConcurrentPushes.{DateTime.UtcNow.Ticks}"; - var packageVersions = new List() - { - "1.0.0-a", "1.0.0-b", "1.0.0", "1.0.1", "1.0.2-abc", - "2.0.0-a", "2.0.0-b", "2.0.0", "2.0.1", "2.0.2-abc", - "3.0.0-a", "3.0.0-b", "3.0.0", "3.0.1", "3.0.2-abc", - "4.0.0-a", "4.0.0-b", "4.0.0", "4.0.1", "4.0.2-abc", - "5.0.0-a", "5.0.0-b", "5.0.0", "5.0.1", "5.0.2-abc", - "6.0.0-a", "6.0.0-b", "6.0.0", "6.0.1", "6.0.2-abc" - }; - - // first push should not be concurrent to avoid conflict on creation of package registration. - var concurrentTasks = new Task[packageVersions.Count]; - concurrentTasks[0] = Task.Run(() => _clientSdkHelper.UploadNewPackage(packageId, version: packageVersions[0])); - concurrentTasks[0].Wait(); - - // push remaining packages over period of 5 seconds - // goal is insert of multiple packages that would be marked as new latest/latestStable - var random = new Random(); - for (int i = 1; i < concurrentTasks.Length; i++) - { - var packageVersion = packageVersions[i]; - concurrentTasks[i] = Task.Run(async () => - { - await Task.Delay(random.Next(0, 5000)); - await _clientSdkHelper.UploadNewPackage(packageId, version: packageVersion); - }); - } - Task.WaitAll(concurrentTasks); - - // unlist packages a row at a time - // goal is unlist of package that would be marked latest/latestStable by concurrent UpdateIsLatest - var rowCount = 6; - var columnCount = 5; - var concurrentUnlists = new Task[columnCount]; - for (int r = rowCount; r > 0; r--) - { - // verify current latest/latestStable before unlisting next row - var latestIndex = (r * columnCount) - 1; - var latestStableIndex = (r * columnCount) - 2; - await CheckPackageLatestVersions(packageId, packageVersions, packageVersions[latestIndex], packageVersions[latestStableIndex]); - - // unlist each package in the current row - for (int c = 1; c <= columnCount; c++) - { - var index = (r * columnCount) - c; - var versionToUnlist = packageVersions[index]; - concurrentUnlists[c - 1] = Task.Run(() => _clientSdkHelper.UnlistPackage(packageId, version: versionToUnlist)); - } - Task.WaitAll(concurrentUnlists); - } - - // no latest/latestStable, as all packages have been unlisted - await CheckPackageLatestVersions(packageId, packageVersions, string.Empty, string.Empty); - } - private static string GetPackagesAppearInFeedInOrderPackageId(DateTime startingTime, int i) { return $"TestV2FeedPackagesAppearInFeedInOrderTest.{startingTime.Ticks}.{i}"; @@ -160,7 +98,7 @@ private static string GetPackagesAppearInFeedInOrderPackageId(DateTime startingT private static string GetPackagesAppearInFeedInOrderUrl(DateTime time, string timestamp) { - return $"{UrlHelper.V2FeedRootUrl}/Packages?$filter={timestamp} gt DateTime'{time:o}'&$orderby={timestamp} desc&$select={timestamp},IsLatestVersion,IsAbsoluteLatestVersion,Published"; + return $"{UrlHelper.V2FeedRootUrl}/Packages?$filter={timestamp} gt DateTime'{time:o}'&$orderby={timestamp} desc&$select={timestamp}"; } /// @@ -169,7 +107,6 @@ private static string GetPackagesAppearInFeedInOrderUrl(DateTime time, string ti /// An ordered list of package ids. Each package id in the list must have a timestamp in the feed earlier than all package ids after it. /// The timestamp property to test the ordering of. For example, "Created" or "LastEdited". /// A timestamp that is before all of the timestamps expected to be found in the feed. This is used in a request to the feed. - /// Whether packages are listed, used to verify if latest flags are set properly. private async Task CheckPackageTimestampsInOrder(List packageIds, string timestampPropertyName, DateTime operationStartTimestamp) { @@ -179,55 +116,17 @@ private async Task CheckPackageTimestampsInOrder(List packageIds, string var packageId = packageIds[i]; TestOutputHelper.WriteLine($"Attempting to check order of package #{i} {timestampPropertyName} timestamp in feed."); - var feedUrl = GetPackagesAppearInFeedInOrderUrl(operationStartTimestamp, timestampPropertyName); - var packageDetails = await _odataHelper.GetPackagePropertiesFromResponse(feedUrl, packageId); - Assert.NotNull(packageDetails); - - var newTimestamp = (DateTime?)(packageDetails.ContainsKey(timestampPropertyName) - ? packageDetails[timestampPropertyName] - : null); + var newTimestamp = + await + _odataHelper.GetTimestampOfPackageFromResponse( + GetPackagesAppearInFeedInOrderUrl(operationStartTimestamp, timestampPropertyName), + timestampPropertyName, + packageId); Assert.True(newTimestamp.HasValue); Assert.True(newTimestamp.Value > lastTimestamp, $"Package #{i} was last modified after package #{i - 1} but has an earlier {timestampPropertyName} timestamp ({newTimestamp} should be greater than {lastTimestamp})."); lastTimestamp = newTimestamp.Value; - - var published= packageDetails["Published"] as DateTime?; - var isListed = !published?.Year.Equals(1900); - var isLatest = packageDetails["IsAbsoluteLatestVersion"] as bool?; - var isLatestStable = packageDetails["IsLatestVersion"] as bool?; - - Assert.NotNull(isLatest); - Assert.Equal(isLatest, isListed); - - Assert.NotNull(isLatestStable); - Assert.Equal(isLatestStable, isListed); - } - } - - private async Task CheckPackageLatestVersions(string packageId, List versions, string expectedLatest, string expectedLatestStable) - { - foreach (var version in versions) - { - var feedUrl = $"{UrlHelper.V2FeedRootUrl}/Packages?$filter=Id eq '{packageId}' and Version eq '{version}'" + - "and 1 eq 1" + // Disable search hijacking to get immediate results. - "&$select=Id,Version,IsLatestVersion,IsAbsoluteLatestVersion"; - var packageDetails = await _odataHelper.GetPackagePropertiesFromResponse(feedUrl, packageId, version); - Assert.NotNull(packageDetails); - - var actualId = packageDetails["Id"] as string; - Assert.True(packageId.Equals(actualId, StringComparison.InvariantCultureIgnoreCase)); - - var actualVersion = packageDetails["Version"] as string; - Assert.True(version.Equals(actualVersion, StringComparison.InvariantCultureIgnoreCase)); - - var isLatest = packageDetails["IsAbsoluteLatestVersion"] as bool?; - Assert.NotNull(isLatest); - Assert.Equal(isLatest, expectedLatest.Equals(version, StringComparison.InvariantCultureIgnoreCase)); - - var isLatestStable = packageDetails["IsLatestVersion"] as bool?; - Assert.NotNull(isLatestStable); - Assert.Equal(isLatestStable, expectedLatestStable.Equals(version, StringComparison.InvariantCultureIgnoreCase)); } }