diff --git a/src/NuGetGallery.Core/Entities/EntitiesConfiguration.cs b/src/NuGetGallery.Core/Entities/EntitiesConfiguration.cs index ce91bafd46..5fbf802e01 100644 --- a/src/NuGetGallery.Core/Entities/EntitiesConfiguration.cs +++ b/src/NuGetGallery.Core/Entities/EntitiesConfiguration.cs @@ -1,6 +1,7 @@ // 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; @@ -15,19 +16,41 @@ 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", () => SuspendExecutionStrategy - ? (IDbExecutionStrategy)new DefaultExecutionStrategy() : new SqlAzureExecutionStrategy()); + SetExecutionStrategy("System.Data.SqlClient", () => UseRetriableExecutionStrategy + ? new SqlAzureExecutionStrategy() : (IDbExecutionStrategy)new DefaultExecutionStrategy()); } - public static bool SuspendExecutionStrategy + private static bool UseRetriableExecutionStrategy { get { - return (bool?)CallContext.LogicalGetData("SuspendExecutionStrategy") ?? false; + return (bool?)CallContext.LogicalGetData(nameof(UseRetriableExecutionStrategy)) ?? true; } set { - CallContext.LogicalSetData("SuspendExecutionStrategy", value); + 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; } } } diff --git a/src/NuGetGallery.Core/Entities/EntitiesContext.cs b/src/NuGetGallery.Core/Entities/EntitiesContext.cs index fd3d3aca74..9e65195623 100644 --- a/src/NuGetGallery.Core/Entities/EntitiesContext.cs +++ b/src/NuGetGallery.Core/Entities/EntitiesContext.cs @@ -2,6 +2,7 @@ // 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 @@ -68,6 +69,11 @@ 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 20c3d645c0..04790a988f 100644 --- a/src/NuGetGallery.Core/Entities/IEntitiesContext.cs +++ b/src/NuGetGallery.Core/Entities/IEntitiesContext.cs @@ -1,12 +1,15 @@ // 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; } @@ -20,6 +23,8 @@ 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 5d18b9d432..76a45104f3 100644 --- a/src/NuGetGallery/Controllers/ApiController.cs +++ b/src/NuGetGallery/Controllers/ApiController.cs @@ -393,6 +393,9 @@ 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 @@ -470,6 +473,13 @@ 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(); } @@ -503,6 +513,10 @@ 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 55eabb2fb3..26f1485638 100644 --- a/src/NuGetGallery/Controllers/PackagesController.cs +++ b/src/NuGetGallery/Controllers/PackagesController.cs @@ -996,11 +996,20 @@ 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, @@ -1192,6 +1201,9 @@ 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 ad6f3bfb10..0b3214cc45 100644 --- a/src/NuGetGallery/Services/IPackageService.cs +++ b/src/NuGetGallery/Services/IPackageService.cs @@ -16,13 +16,23 @@ public interface IPackageService IEnumerable FindPackageRegistrationsByOwner(User user); IEnumerable FindDependentPackages(Package package); - Task UpdateIsLatestAsync(PackageRegistration packageRegistration, bool commitChanges = true); + /// + /// 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); /// /// 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. @@ -33,10 +43,49 @@ 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 6fac60f2fa..d028e58fe6 100644 --- a/src/NuGetGallery/Services/PackageDeleteService.cs +++ b/src/NuGetGallery/Services/PackageDeleteService.cs @@ -56,52 +56,55 @@ public PackageDeleteService( public async Task SoftDeletePackagesAsync(IEnumerable packages, User deletedBy, string reason, string signature) { - EntitiesConfiguration.SuspendExecutionStrategy = true; - using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) - { - // Increase command timeout - _entitiesContext.SetCommandTimeout(seconds: 300); + List packageRegistrations; - // Keep package registrations - var packageRegistrations = packages - .GroupBy(p => p.PackageRegistration) - .Select(g => g.First().PackageRegistration) - .ToList(); + // Must suspend the retry execution strategy in order to use transactions. + using (EntitiesConfiguration.SuspendRetriableExecutionStrategy()) + { + using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) + { + // Increase command timeout + _entitiesContext.SetCommandTimeout(seconds: 300); - // 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); + // Keep package registrations + packageRegistrations = packages + .GroupBy(p => p.PackageRegistration) + .Select(g => g.First().PackageRegistration) + .ToList(); - // Store the soft delete in the database - var packageDelete = new PackageDelete - { - DeletedOn = DateTime.UtcNow, - DeletedBy = deletedBy, - Reason = reason, - Signature = signature - }; + // 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); - foreach (var package in packages) - { - package.Listed = false; - package.Deleted = true; - packageDelete.Packages.Add(package); + // Store the soft delete in the database + var packageDelete = new PackageDelete + { + DeletedOn = DateTime.UtcNow, + DeletedBy = deletedBy, + Reason = reason, + Signature = signature + }; - await _auditingService.SaveAuditRecordAsync(CreateAuditRecord(package, package.PackageRegistration, AuditedPackageAction.SoftDelete, reason)); - } + foreach (var package in packages) + { + package.Listed = false; + package.Deleted = true; + packageDelete.Packages.Add(package); - _packageDeletesRepository.InsertOnCommit(packageDelete); + await _auditingService.SaveAuditRecordAsync(CreateAuditRecord(package, package.PackageRegistration, AuditedPackageAction.SoftDelete, reason)); + } - // Update latest versions - await UpdateIsLatestAsync(packageRegistrations); + _packageDeletesRepository.InsertOnCommit(packageDelete); - // Commit changes - await _packageRepository.CommitChangesAsync(); - await _packageDeletesRepository.CommitChangesAsync(); - transaction.Commit(); + // 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(); @@ -109,54 +112,58 @@ public async Task SoftDeletePackagesAsync(IEnumerable packages, User de public async Task HardDeletePackagesAsync(IEnumerable packages, User deletedBy, string reason, string signature, bool deleteEmptyPackageRegistration) { - EntitiesConfiguration.SuspendExecutionStrategy = true; - using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) - { - // Increase command timeout - _entitiesContext.SetCommandTimeout(seconds: 300); - - // 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); + List packageRegistrations; - // Remove the package and related entities from the database - foreach (var package in packages) + // Must suspend the retry execution strategy in order to use transactions. + using (EntitiesConfiguration.SuspendRetriableExecutionStrategy()) + { + using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) { - 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)); + // Increase command timeout + _entitiesContext.SetCommandTimeout(seconds: 300); - await _auditingService.SaveAuditRecordAsync(CreateAuditRecord(package, package.PackageRegistration, AuditedPackageAction.Delete, reason)); + // Keep package registrations + packageRegistrations = packages.GroupBy(p => p.PackageRegistration).Select(g => g.First().PackageRegistration).ToList(); - package.PackageRegistration.Packages.Remove(package); - _packageRepository.DeleteOnCommit(package); - } + // 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); - // Update latest versions - await UpdateIsLatestAsync(packageRegistrations); + // 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); + } - // Commit changes to package repository - await _packageRepository.CommitChangesAsync(); + // Commit changes to package repository + await _packageRepository.CommitChangesAsync(); - // Remove package registrations that have no more packages? - if (deleteEmptyPackageRegistration) - { - await RemovePackageRegistrationsWithoutPackages(packageRegistrations); - } + // Remove package registrations that have no more packages? + if (deleteEmptyPackageRegistration) + { + await RemovePackageRegistrationsWithoutPackages(packageRegistrations); + } - // Commit transaction - transaction.Commit(); + // Commit transaction + transaction.Commit(); + } } - EntitiesConfiguration.SuspendExecutionStrategy = false; + + // handle in separate transaction because of concurrency check with retry + await UpdateIsLatestAsync(packageRegistrations); // Force refresh the index UpdateSearchIndex(); @@ -169,10 +176,9 @@ 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, false); + await _packageService.UpdateIsLatestAsync(packageRegistration); } } diff --git a/src/NuGetGallery/Services/PackageService.cs b/src/NuGetGallery/Services/PackageService.cs index 95e986e4a8..87b43441f2 100644 --- a/src/NuGetGallery/Services/PackageService.cs +++ b/src/NuGetGallery/Services/PackageService.cs @@ -3,30 +3,41 @@ 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.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 IEntityRepository _packageOwnerRequestRepository; private readonly IEntityRepository _packageRegistrationRepository; private readonly IEntityRepository _packageRepository; private readonly IPackageNamingConflictValidator _packageNamingConflictValidator; private readonly AuditingService _auditingService; + private readonly IDiagnosticsSource _trace; public PackageService( IEntityRepository packageRegistrationRepository, IEntityRepository packageRepository, IEntityRepository packageOwnerRequestRepository, + IEntitiesContext entitiesContext, + IDiagnosticsService diagnostics, IIndexingService indexingService, IPackageNamingConflictValidator packageNamingConflictValidator, AuditingService auditingService) @@ -64,9 +75,12 @@ public PackageService( _packageRegistrationRepository = packageRegistrationRepository; _packageRepository = packageRepository; _packageOwnerRequestRepository = packageOwnerRequestRepository; + _entitiesContext = entitiesContext; _indexingService = indexingService; _packageNamingConflictValidator = packageNamingConflictValidator; _auditingService = auditingService; + + _trace = diagnostics.SafeGetSource("PackageService"); } public void EnsureValid(PackageArchiveReader packageArchiveReader) @@ -83,7 +97,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()); @@ -96,7 +110,6 @@ public async Task CreatePackageAsync(PackageArchiveReader nugetPackage, var package = CreatePackageFromNuGetPackage(packageRegistration, nugetPackage, packageMetadata, packageStreamMetadata, user); packageRegistration.Packages.Add(package); - await UpdateIsLatestAsync(packageRegistration, false); if (commitChanges) { @@ -241,7 +254,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); @@ -253,7 +266,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) @@ -264,8 +277,6 @@ 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(); @@ -309,7 +320,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) @@ -335,8 +346,6 @@ 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)); @@ -345,7 +354,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) @@ -362,11 +371,6 @@ 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) @@ -678,51 +682,143 @@ private void ValidatePackageTitle(PackageMetadata packageMetadata) } } - public async Task UpdateIsLatestAsync(PackageRegistration packageRegistration, bool commitChanges = true) + protected internal async virtual Task TryUpdateIsLatestInDatabase(IEntitiesContext context) { - if (!packageRegistration.Packages.Any()) - { - return; + // 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} AND [IsLatest] = {originalIsLatest} AND [IsLatestStable] = {originalIsLatestStable}"); + query.AppendLine($"SET @rowCount = @rowCount + @@ROWCOUNT"); + } + query.AppendLine("SELECT @rowCount"); + + using (var transaction = context.GetDatabase().BeginTransaction(IsolationLevel.ReadCommitted)) + { + 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; + } } - - // TODO: improve setting the latest bit; this is horrible. Trigger maybe? - foreach (var pv in packageRegistration.Packages.Where(p => p.IsLatest || p.IsLatestStable)) + } + + private Task TryUpdateIsLatestAsync(IEntitiesContext context, PackageRegistration packageRegistration) + { + if (packageRegistration.Packages.Any()) { - pv.IsLatest = false; - pv.IsLatestStable = false; - pv.LastUpdated = DateTime.UtcNow; - } - - // 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); + // 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 (latestPackage != null) - { - latestPackage.IsLatest = true; - latestPackage.LastUpdated = DateTime.UtcNow; + // 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.IsPrerelease) + if (latestPackage != 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 IsLatest. - var latestReleasePackage = FindPackage(packageRegistration.Packages.Where(p => !p.IsPrerelease && !p.Deleted && p.Listed)); - if (latestReleasePackage != null) + latestPackage.IsLatest = true; + latestPackage.IsLatestStable = !latestPackage.IsPrerelease; + + if (latestPackage.IsPrerelease) { - // We could have no release packages - latestReleasePackage.IsLatestStable = true; - latestReleasePackage.LastUpdated = DateTime.UtcNow; + // 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; + } } } - else + // 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(); + } + + 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)) { - // Only release versions are marked as IsLatestStable. - latestPackage.IsLatestStable = true; + return; } - } - if (commitChanges) - { - await _packageRepository.CommitChangesAsync(); + // 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); } } diff --git a/src/NuGetGallery/Services/ReflowPackageService.cs b/src/NuGetGallery/Services/ReflowPackageService.cs index 6787353359..285f40c584 100644 --- a/src/NuGetGallery/Services/ReflowPackageService.cs +++ b/src/NuGetGallery/Services/ReflowPackageService.cs @@ -34,51 +34,53 @@ public async Task ReflowAsync(string id, string version) return null; } - EntitiesConfiguration.SuspendExecutionStrategy = true; - using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) + // Must suspend the retry execution strategy in order to use transactions. + using (EntitiesConfiguration.SuspendRetriableExecutionStrategy()) { - // 1) Download package binary to memory - using (var packageStream = await _packageFileService.DownloadPackageFileAsync(package)) + using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) { - using (var packageArchive = new PackageArchiveReader(packageStream, leaveStreamOpen: false)) + // 1) Download package binary to memory + using (var packageStream = await _packageFileService.DownloadPackageFileAsync(package)) { - // 2) Determine package metadata from binary - var packageStreamMetadata = new PackageStreamMetadata + using (var packageArchive = new PackageArchiveReader(packageStream, leaveStreamOpen: false)) { - 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(); + // 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(); + } } - } - // Commit transaction - transaction.Commit(); + // 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 53bce61197..a39a3eefac 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 { @@ -50,9 +50,9 @@ private static PackagesController CreateController( if (uploadFileService == null) { uploadFileService = new Mock(); - uploadFileService.Setup(x => x.DeleteUploadFileAsync(It.IsAny())).Returns(Task.FromResult(0)); + uploadFileService.Setup(x => x.DeleteUploadFileAsync(It.IsAny())).Returns(Task.CompletedTask); uploadFileService.Setup(x => x.GetUploadFileAsync(42)).Returns(Task.FromResult(null)); - uploadFileService.Setup(x => x.SaveUploadFileAsync(42, It.IsAny())).Returns(Task.FromResult(0)); + uploadFileService.Setup(x => x.SaveUploadFileAsync(42, It.IsAny())).Returns(Task.CompletedTask); } messageService = messageService ?? new Mock(); searchService = searchService ?? CreateSearchService(); @@ -62,7 +62,7 @@ private static PackagesController CreateController( if (packageFileService == null) { packageFileService = new Mock(); - packageFileService.Setup(p => p.SavePackageFileAsync(It.IsAny(), It.IsAny())).Returns(Task.FromResult(0)); + packageFileService.Setup(p => p.SavePackageFileAsync(It.IsAny(), It.IsAny())).Returns(Task.CompletedTask); } entitiesContext = entitiesContext ?? new Mock(); @@ -133,7 +133,7 @@ public class TheCancelVerifyPackageAction public async Task DeletesTheInProgressPackageUpload() { var fakeUploadFileService = new Mock(); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(42)).Returns(Task.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(42)).Returns(Task.CompletedTask); var controller = CreateController( uploadFileService: fakeUploadFileService); controller.SetCurrentUser(TestUtility.FakeUser); @@ -147,7 +147,7 @@ public async Task DeletesTheInProgressPackageUpload() public async Task RedirectsToUploadPageAfterDelete() { var fakeUploadFileService = new Mock(); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(42)).Returns(Task.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(42)).Returns(Task.CompletedTask); var controller = CreateController( uploadFileService: fakeUploadFileService); controller.SetCurrentUser(TestUtility.FakeUser); @@ -541,9 +541,11 @@ 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.FromResult(0)).Verifiable(); + .Returns(Task.CompletedTask).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(); @@ -575,11 +577,13 @@ public async Task UpdatesUnlistedIfNotSelected() var packageService = new Mock(MockBehavior.Strict); packageService.Setup(svc => svc.MarkPackageListedAsync(It.IsAny(), It.IsAny())) - .Returns(Task.FromResult(0)).Verifiable(); + .Returns(Task.CompletedTask).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(); @@ -1039,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.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(null)); - fakeUploadFileService.Setup(x => x.SaveUploadFileAsync(TestUtility.FakeUser.Key, It.IsAny())).Returns(Task.FromResult(0)); + fakeUploadFileService.Setup(x => x.SaveUploadFileAsync(TestUtility.FakeUser.Key, It.IsAny())).Returns(Task.CompletedTask); var controller = CreateController( uploadFileService: fakeUploadFileService, fakeNuGetPackage: fakeFileStream); @@ -1061,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.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(null)); - fakeUploadFileService.Setup(x => x.SaveUploadFileAsync(TestUtility.FakeUser.Key, It.IsAny())).Returns(Task.FromResult(0)); + fakeUploadFileService.Setup(x => x.SaveUploadFileAsync(TestUtility.FakeUser.Key, It.IsAny())).Returns(Task.CompletedTask); var controller = CreateController( uploadFileService: fakeUploadFileService); controller.SetCurrentUser(TestUtility.FakeUser); @@ -1082,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.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(null)); var controller = CreateController( uploadFileService: fakeUploadFileService); @@ -1383,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.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); 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" })); @@ -1409,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.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); 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())) @@ -1417,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.FromResult(0)).Verifiable(); + fakePackageFileService.Setup(x => x.SavePackageFileAsync(fakePackage, It.IsAny())).Returns(Task.CompletedTask).Verifiable(); var controller = CreateController( packageService: fakePackageService, @@ -1445,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.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); 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())) @@ -1484,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.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); 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())) @@ -1523,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.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); 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.FromResult(0)).Verifiable(); + fakePackageFileService.Setup(x => x.SavePackageFileAsync(fakePackage, It.IsAny())).Returns(Task.CompletedTask).Verifiable(); var fakeIndexingService = new Mock(MockBehavior.Strict); fakeIndexingService.Setup(f => f.UpdateIndex()).Verifiable(); @@ -1606,6 +1610,8 @@ 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( @@ -1629,7 +1635,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.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); 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())) @@ -1654,7 +1660,7 @@ public async Task WillMarkThePackageUnlistedWhenListedArgumentIsFalse() var fakeUploadFileService = new Mock(); using (var fakeFileStream = new MemoryStream()) { - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); 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())) @@ -1683,7 +1689,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.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); 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" })); @@ -1705,7 +1711,7 @@ public async Task WillNotMarkThePackageUnlistedWhenListedArgumentIsNullorTrue(bo public async Task WillDeleteTheUploadFile() { var fakeUploadFileService = new Mock(); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)).Verifiable(); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask).Verifiable(); using (var fakeFileStream = new MemoryStream()) { fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); @@ -1733,8 +1739,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.FromResult(0)); - fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); + fakeUploadFileService.Setup(x => x.SaveUploadFileAsync(TestUtility.FakeUser.Key, It.IsAny())).Returns(Task.CompletedTask); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); 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" })); @@ -1759,7 +1765,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.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); 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" })); @@ -1785,7 +1791,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.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); 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())) @@ -1814,7 +1820,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.FromResult(0)); + fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.CompletedTask); 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 fff0185bb5..5d0b610b77 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, false)); + packageService.Verify(x => x.UpdateIsLatestAsync(packageRegistration)); } [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, false)); + packageService.Verify(x => x.UpdateIsLatestAsync(packageRegistration)); } [Fact] diff --git a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs index bde83bc809..a17cdf6a53 100644 --- a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs @@ -3,14 +3,15 @@ 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.Diagnostics; using NuGetGallery.Framework; using NuGetGallery.Packaging; using Xunit; @@ -98,6 +99,31 @@ private static IPackageService CreateService( Mock> packageRegistrationRepository = null, Mock> packageRepository = null, Mock> packageOwnerRequestRepo = null, + Mock entitiesContext = null, + Mock diagnosticsService = null, + Mock indexingService = null, + IPackageNamingConflictValidator packageNamingConflictValidator = null, + AuditingService auditingService = null, + Action> setup = null) + { + return CreateServiceMock( + packageRegistrationRepository, + packageRepository, + packageOwnerRequestRepo, + entitiesContext, + diagnosticsService, + indexingService, + packageNamingConflictValidator, + auditingService, + setup).Object; + } + + private static Mock CreateServiceMock( + Mock> packageRegistrationRepository = null, + Mock> packageRepository = null, + Mock> packageOwnerRequestRepo = null, + Mock entitiesContext = null, + Mock diagnosticsService = null, Mock indexingService = null, IPackageNamingConflictValidator packageNamingConflictValidator = null, AuditingService auditingService = null, @@ -106,6 +132,13 @@ private static IPackageService CreateService( 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); + + diagnosticsService = diagnosticsService ?? new Mock(); indexingService = indexingService ?? new Mock(); auditingService = auditingService ?? new TestAuditingService(); @@ -120,10 +153,16 @@ private static IPackageService CreateService( packageRegistrationRepository.Object, packageRepository.Object, packageOwnerRequestRepo.Object, + entitiesContext.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) @@ -131,7 +170,7 @@ private static IPackageService CreateService( setup(packageService); } - return packageService.Object; + return packageService; } public class TheAddPackageOwnerMethod @@ -577,19 +616,6 @@ 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() { @@ -1013,45 +1039,26 @@ public async Task ReturnsExistingMatchingPackageOwnerRequest() public class TheUpdateIsLatestMethod { [Fact] - public async Task DoNotCommitIfCommitChangesIsFalse() + public async Task AlwaysCommitChanges() { // 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.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: + + var service = CreateServiceMock(packageRepository: packageRepository, setup: mockService => { mockService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package); }); // Act - await service.UpdateIsLatestAsync(packageRegistration, true); + await service.Object.UpdateIsLatestAsync(packageRegistration); // Assert - packageRepository.Verify(r => r.CommitChangesAsync(), Times.Once()); + service.Verify(s => s.TryUpdateIsLatestInDatabase(It.IsAny()), Times.Once); } [Fact] - public async Task WillUpdateIsLatest1() + public async Task UpdateIsLatestAsync_DifferentLatestAndLatestStableVersions() { // Arrange var packages = new HashSet(); @@ -1061,24 +1068,23 @@ public async Task WillUpdateIsLatest1() var package09 = new Package { PackageRegistration = packageRegistration, Version = "0.9.0" }; packages.Add(package09); var packageRepository = new Mock>(MockBehavior.Strict); - packageRepository.Setup(r => r.CommitChangesAsync()) - .Returns(Task.CompletedTask).Verifiable(); - var service = CreateService(packageRepository: packageRepository, setup: + var service = CreateServiceMock(packageRepository: packageRepository, setup: mockService => { mockService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package10A); }); // Act - await service.UpdateIsLatestAsync(packageRegistration, true); + await service.Object.UpdateIsLatestAsync(packageRegistration); // Assert Assert.True(package10A.IsLatest); Assert.False(package10A.IsLatestStable); Assert.False(package09.IsLatest); Assert.True(package09.IsLatestStable); - packageRepository.Verify(); + + service.Verify(s => s.TryUpdateIsLatestInDatabase(It.IsAny()), Times.Once); } [Fact] - public async Task WillUpdateIsLatest2() + public async Task UpdateIsLatestAsync_SameLatestAndLatestStableVersions() { // Arrange var packages = new HashSet(); @@ -1090,13 +1096,11 @@ public async Task WillUpdateIsLatest2() var package09 = new Package { PackageRegistration = packageRegistration, Version = "0.9.0" }; packages.Add(package09); var packageRepository = new Mock>(MockBehavior.Strict); - packageRepository.Setup(r => r.CommitChangesAsync()) - .Returns(Task.CompletedTask).Verifiable(); - var service = CreateService(packageRepository: packageRepository, setup: + var service = CreateServiceMock(packageRepository: packageRepository, setup: mockService => { mockService.Setup(x => x.FindPackageByIdAndVersion(It.IsAny(), It.IsAny(), true)).Returns(package100); }); // Act - await service.UpdateIsLatestAsync(packageRegistration, true); + await service.Object.UpdateIsLatestAsync(packageRegistration); // Assert Assert.True(package100.IsLatest); @@ -1105,7 +1109,42 @@ public async Task WillUpdateIsLatest2() Assert.False(package10A.IsLatestStable); Assert.False(package09.IsLatest); Assert.False(package09.IsLatestStable); - packageRepository.Verify(); + + 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); } } @@ -1404,6 +1443,7 @@ 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); @@ -1509,6 +1549,7 @@ 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); @@ -1526,6 +1567,7 @@ 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"); @@ -1621,6 +1663,7 @@ 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); } @@ -1644,6 +1687,7 @@ 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); } @@ -1673,6 +1717,7 @@ 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); } @@ -1704,6 +1749,8 @@ 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); @@ -1737,6 +1784,7 @@ 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); @@ -1772,6 +1820,8 @@ 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); @@ -1806,6 +1856,8 @@ 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 3fe39f40f3..dfadde62c7 100644 --- a/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs @@ -10,6 +10,7 @@ using System.Threading.Tasks; using Moq; using NuGet.Packaging; +using NuGetGallery.Diagnostics; using NuGetGallery.Framework; using NuGetGallery.Packaging; using Xunit; @@ -282,6 +283,8 @@ 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 indexingService = new Mock(); var packageNamingConflictValidator = new PackageNamingConflictValidator( packageRegistrationRepository.Object, @@ -292,6 +295,8 @@ private static Mock SetupPackageService(Package package) packageRegistrationRepository.Object, packageRepository.Object, packageOwnerRequestRepo.Object, + entitiesContext.Object, + diagnosticsService.Object, indexingService.Object, packageNamingConflictValidator, auditingService); diff --git a/tests/NuGetGallery.Facts/TestUtils/FakeEntitiesContext.cs b/tests/NuGetGallery.Facts/TestUtils/FakeEntitiesContext.cs index 969583c167..1927e7bbd7 100644 --- a/tests/NuGetGallery.Facts/TestUtils/FakeEntitiesContext.cs +++ b/tests/NuGetGallery.Facts/TestUtils/FakeEntitiesContext.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Data.Entity; +using System.Data.Entity.Infrastructure; using System.Threading.Tasks; using Xunit; @@ -130,10 +131,19 @@ 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 8babf385c0..4fffadbf36 100644 --- a/tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs +++ b/tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs @@ -2,22 +2,28 @@ // 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) { @@ -59,55 +65,38 @@ public async Task TryDownloadPackageFromFeed(string packageId, string ve } } - public async Task GetTimestampOfPackageFromResponse(string url, string propertyName, string packageId, string version = "1.0.0") + private object GetPackageProperty(string propertyType, string propertyValue) { - 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) + switch (propertyType) { - WriteLine($"Package data does not contain '{propertyName}' timestamp!"); - return null; + case "Edm.DateTime": + return DateTime.Parse(propertyValue); + case "Edm.Boolean": + return Boolean.Parse(propertyValue); + default: + return propertyValue; } - - 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 GetPackageDataInResponse(string url, string packageId, string version = "1.0.0") + public async Task> GetPackagePropertiesFromResponse(string url, string packageId, string version = "1.0.0") { - WriteLine($"Getting data for package '{packageId}' with version '{version}'."); + WriteLine($"Getting properties for package '{packageId}' with version '{version}'."); var responseText = await GetResponseText(url); - var packageString = @"" + UrlHelper.V2FeedRootUrl + @"Packages(Id='" + packageId + @"',Version='" + (string.IsNullOrEmpty(version) ? "" : version + "')"); - var endEntryTag = ""; + var packagesXml = XDocument.Parse(responseText); - var startingIndex = responseText.IndexOf(packageString); + 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"); - if (startingIndex < 0) + var properties = new Dictionary(); + foreach (var prop in packageProperties.Elements()) { - WriteLine("Package not found in response text!"); - return null; + var propType = prop.Attribute(FeedAdoNS + "type")?.Value; + properties[prop.Name.LocalName] = GetPackageProperty(propType, prop.Value); } - - var endingIndex = responseText.IndexOf(endEntryTag, startingIndex); - - return responseText.Substring(startingIndex, endingIndex - startingIndex); + return properties; } 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 6a93865033..a5b56ef1eb 100644 --- a/tests/NuGetGallery.FunctionalTests.Core/NuGetGallery.FunctionalTests.Core.csproj +++ b/tests/NuGetGallery.FunctionalTests.Core/NuGetGallery.FunctionalTests.Core.csproj @@ -48,6 +48,8 @@ + + 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 62646d7f6c..6b84b8de73 100644 --- a/tests/NuGetGallery.FunctionalTests/ODataFeeds/V2FeedExtendedTests.cs +++ b/tests/NuGetGallery.FunctionalTests/ODataFeeds/V2FeedExtendedTests.cs @@ -91,6 +91,60 @@ 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 latestAll = "7.0.2-abc"; + var latestStableAll = "7.0.1"; + var latestFirstHalf = "3.0.2-abc"; + var latestStableFirstHalf = "3.0.1"; + + 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", + "6.0.0-a", "6.0.0-b", "6.0.0", "6.0.1", "6.0.2-abc", + "7.0.0-a", "7.0.0-b", "7.0.0", "7.0.1", "7.0.2-abc" + }; + + // push all and verify; ~15-20 concurrency conflicts seen in testing + var concurrentTasks = new Task[packageVersions.Count]; + for (int i = 0; i < concurrentTasks.Length; i++) + { + var packageVersion = packageVersions[i]; + concurrentTasks[i] = Task.Run(() => _clientSdkHelper.UploadNewPackage(packageId, version: packageVersion)); + } + Task.WaitAll(concurrentTasks); + + await CheckPackageLatestVersions(packageId, packageVersions, latestAll, latestStableAll); + + // unlist last half and verify; ~1-2 concurrency conflicts seen in testing + for (int i = concurrentTasks.Length - 1; i >= 15; i--) + { + var packageVersion = packageVersions[i]; + concurrentTasks[i] = Task.Run(() => _clientSdkHelper.UnlistPackage(packageId, version: packageVersion)); + } + Task.WaitAll(concurrentTasks); + + await CheckPackageLatestVersions(packageId, packageVersions, latestFirstHalf, latestStableFirstHalf); + + // unlist remaining and verify; ~1-2 concurrency conflicts seen in testing + for (int i = 14; i >= 0; i--) + { + var packageVersion = packageVersions[i]; + concurrentTasks[i] = Task.Run(() => _clientSdkHelper.UnlistPackage(packageId, version: packageVersion)); + } + Task.WaitAll(concurrentTasks); + + await CheckPackageLatestVersions(packageId, packageVersions, string.Empty, string.Empty); + } + private static string GetPackagesAppearInFeedInOrderPackageId(DateTime startingTime, int i) { return $"TestV2FeedPackagesAppearInFeedInOrderTest.{startingTime.Ticks}.{i}"; @@ -98,7 +152,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}"; + return $"{UrlHelper.V2FeedRootUrl}/Packages?$filter={timestamp} gt DateTime'{time:o}'&$orderby={timestamp} desc&$select={timestamp},IsLatestVersion,IsAbsoluteLatestVersion,Published"; } /// @@ -107,6 +161,7 @@ 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) { @@ -116,17 +171,54 @@ 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 newTimestamp = - await - _odataHelper.GetTimestampOfPackageFromResponse( - GetPackagesAppearInFeedInOrderUrl(operationStartTimestamp, timestampPropertyName), - timestampPropertyName, - packageId); + var feedUrl = GetPackagesAppearInFeedInOrderUrl(operationStartTimestamp, timestampPropertyName); + var packageDetails = await _odataHelper.GetPackagePropertiesFromResponse(feedUrl, packageId); + Assert.NotNull(packageDetails); + + var newTimestamp = (DateTime?)(packageDetails.ContainsKey(timestampPropertyName) + ? packageDetails[timestampPropertyName] + : null); 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}'" + + "&$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)); } }