From 5fe1ba75e1eb705c87c92b0979e40ccc3fbd27d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Sharma?= Date: Mon, 20 Apr 2020 14:52:06 -0700 Subject: [PATCH] Refactor download overrides (#768) This change introduces a new type, `IDownloadTransferrer`, which determines what downloads should be changed to reflect the latest download overrides. In the future, this type will also apply popularity transfers. Part of https://github.com/nuget/nugetgallery/issues/7898 --- .../UpdateDownloadsCommand.cs | 75 ++++++- .../AuxiliaryFiles/DownloadDataExtensions.cs | 90 -------- .../Db2AzureSearch/Db2AzureSearchCommand.cs | 6 + .../Db2AzureSearch/InitialAuxiliaryData.cs | 5 +- .../NewPackageRegistrationProducer.cs | 60 ++++- .../DependencyInjectionExtensions.cs | 9 + .../DownloadTransferrer.cs | 105 +++++++++ .../IDatabaseAuxiliaryDataFetcher.cs | 1 - .../IDownloadTransferrer.cs | 61 +++++ .../NuGet.Services.AzureSearch.csproj | 3 +- .../UpdateDownloadsCommandFacts.cs | 209 ++++++++--------- .../Db2AzureSearchCommandFacts.cs | 6 +- .../NewPackageRegistrationProducerFacts.cs | 133 ++++++----- .../DownloadTransferrerFacts.cs | 210 ++++++++++++++++++ .../NuGet.Services.AzureSearch.Tests.csproj | 1 + 15 files changed, 702 insertions(+), 272 deletions(-) delete mode 100644 src/NuGet.Services.AzureSearch/AuxiliaryFiles/DownloadDataExtensions.cs create mode 100644 src/NuGet.Services.AzureSearch/DownloadTransferrer.cs create mode 100644 src/NuGet.Services.AzureSearch/IDownloadTransferrer.cs create mode 100644 tests/NuGet.Services.AzureSearch.Tests/DownloadTransferrerFacts.cs diff --git a/src/NuGet.Services.AzureSearch/Auxiliary2AzureSearch/UpdateDownloadsCommand.cs b/src/NuGet.Services.AzureSearch/Auxiliary2AzureSearch/UpdateDownloadsCommand.cs index 52d69c6fb..586d55e3c 100644 --- a/src/NuGet.Services.AzureSearch/Auxiliary2AzureSearch/UpdateDownloadsCommand.cs +++ b/src/NuGet.Services.AzureSearch/Auxiliary2AzureSearch/UpdateDownloadsCommand.cs @@ -28,8 +28,11 @@ public class UpdateDownloadsCommand : IAzureSearchCommand private static readonly int MaxDocumentsPerId = Enum.GetValues(typeof(SearchFilters)).Length; private readonly IAuxiliaryFileClient _auxiliaryFileClient; + private readonly IDatabaseAuxiliaryDataFetcher _databaseFetcher; private readonly IDownloadDataClient _downloadDataClient; private readonly IDownloadSetComparer _downloadSetComparer; + private readonly IDownloadTransferrer _downloadTransferrer; + private readonly IPopularityTransferDataClient _popularityTransferDataClient; private readonly ISearchDocumentBuilder _searchDocumentBuilder; private readonly ISearchIndexActionBuilder _indexActionBuilder; private readonly Func _batchPusherFactory; @@ -41,8 +44,11 @@ public class UpdateDownloadsCommand : IAzureSearchCommand public UpdateDownloadsCommand( IAuxiliaryFileClient auxiliaryFileClient, + IDatabaseAuxiliaryDataFetcher databaseFetcher, IDownloadDataClient downloadDataClient, IDownloadSetComparer downloadSetComparer, + IDownloadTransferrer downloadTransferrer, + IPopularityTransferDataClient popularityTransferDataClient, ISearchDocumentBuilder searchDocumentBuilder, ISearchIndexActionBuilder indexActionBuilder, Func batchPusherFactory, @@ -52,8 +58,11 @@ public UpdateDownloadsCommand( ILogger logger) { _auxiliaryFileClient = auxiliaryFileClient ?? throw new ArgumentException(nameof(auxiliaryFileClient)); + _databaseFetcher = databaseFetcher ?? throw new ArgumentNullException(nameof(databaseFetcher)); _downloadDataClient = downloadDataClient ?? throw new ArgumentNullException(nameof(downloadDataClient)); _downloadSetComparer = downloadSetComparer ?? throw new ArgumentNullException(nameof(downloadSetComparer)); + _downloadTransferrer = downloadTransferrer ?? throw new ArgumentNullException(nameof(downloadTransferrer)); + _popularityTransferDataClient = popularityTransferDataClient ?? throw new ArgumentNullException(nameof(popularityTransferDataClient)); _searchDocumentBuilder = searchDocumentBuilder ?? throw new ArgumentNullException(nameof(searchDocumentBuilder)); _indexActionBuilder = indexActionBuilder ?? throw new ArgumentNullException(nameof(indexActionBuilder)); _batchPusherFactory = batchPusherFactory ?? throw new ArgumentNullException(nameof(batchPusherFactory)); @@ -106,23 +115,38 @@ private async Task PushIndexChangesAsync() _logger.LogInformation("Fetching new download count data from blob storage."); var newData = await _auxiliaryFileClient.LoadDownloadDataAsync(); - _logger.LogInformation("Removing invalid IDs and versions from the old data."); + _logger.LogInformation("Removing invalid IDs and versions from the old downloads data."); CleanDownloadData(oldResult.Data); - _logger.LogInformation("Removing invalid IDs and versions from the new data."); + _logger.LogInformation("Removing invalid IDs and versions from the new downloads data."); CleanDownloadData(newData); - // Fetch the download overrides from the auxiliary file. Note that the overriden downloads are kept - // separate from downloads data as the original data will be persisted to auxiliary data, whereas the - // overriden data will be persisted to Azure Search. - _logger.LogInformation("Overriding download count data."); + _logger.LogInformation("Detecting download count changes."); + var changes = _downloadSetComparer.Compare(oldResult.Data, newData); + _logger.LogInformation("{Count} package IDs have download count changes.", changes.Count); + + // The "old" data is the popularity transfers data that was last indexed by this job (or + // initialized by Db2AzureSearch). + _logger.LogInformation("Fetching old popularity transfer data from blob storage."); + var oldTransfers = await _popularityTransferDataClient.ReadLatestIndexedAsync(); + + // The "new" data is the latest popularity transfers data from the database. + _logger.LogInformation("Fetching new popularity transfer data from database."); + var newTransfers = await _databaseFetcher.GetPackageIdToPopularityTransfersAsync(); + + _logger.LogInformation("Fetching new download overrides from blob storage."); var downloadOverrides = await _auxiliaryFileClient.LoadDownloadOverridesAsync(); - var overridenDownloads = newData.ApplyDownloadOverrides(downloadOverrides, _logger); - _logger.LogInformation("Detecting download count changes."); - var changes = _downloadSetComparer.Compare(oldResult.Data, overridenDownloads); + _logger.LogInformation("Applying download transfers to download changes."); + ApplyDownloadTransfers( + newData, + oldTransfers.Result, + newTransfers, + downloadOverrides, + changes); + var idBag = new ConcurrentBag(changes.Keys); - _logger.LogInformation("{Count} package IDs have download count changes.", idBag.Count); + _logger.LogInformation("{Count} package IDs need to be updated.", idBag.Count); if (!changes.Any()) { @@ -139,9 +163,38 @@ await ParallelAsync.Repeat( _logger.LogInformation("Uploading the new download count data to blob storage."); await _downloadDataClient.ReplaceLatestIndexedAsync(newData, oldResult.Metadata.GetIfMatchCondition()); + + // TODO: Upload the new popularity transfer data to blob storage. + // See: https://github.com/NuGet/NuGetGallery/issues/7898 return true; } + private void ApplyDownloadTransfers( + DownloadData newData, + SortedDictionary> oldTransfers, + SortedDictionary> newTransfers, + IReadOnlyDictionary downloadOverrides, + SortedDictionary downloadChanges) + { + _logger.LogInformation("Finding download changes from popularity transfers and download overrides."); + var transferChanges = _downloadTransferrer.UpdateDownloadTransfers( + newData, + downloadChanges, + oldTransfers, + newTransfers, + downloadOverrides); + + _logger.LogInformation( + "{Count} package IDs have download count changes from popularity transfers and download overrides.", + transferChanges.Count); + + // Apply the transfer changes to the overall download changes. + foreach (var transferChange in transferChanges) + { + downloadChanges[transferChange.Key] = transferChange.Value; + } + } + private async Task WorkAsync(ConcurrentBag idBag, SortedDictionary changes) { // Perform two batching mechanisms: @@ -334,4 +387,4 @@ private void CleanDownloadData(DownloadData data) nonNormalizedVersionCount); } } -} +} \ No newline at end of file diff --git a/src/NuGet.Services.AzureSearch/AuxiliaryFiles/DownloadDataExtensions.cs b/src/NuGet.Services.AzureSearch/AuxiliaryFiles/DownloadDataExtensions.cs deleted file mode 100644 index 144c6733a..000000000 --- a/src/NuGet.Services.AzureSearch/AuxiliaryFiles/DownloadDataExtensions.cs +++ /dev/null @@ -1,90 +0,0 @@ -// 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.Collections.Generic; -using System.Linq; -using Microsoft.Extensions.Logging; - -namespace NuGet.Services.AzureSearch.AuxiliaryFiles -{ - public static class DownloadDataExtensions - { - public static DownloadData ApplyDownloadOverrides( - this DownloadData originalData, - IReadOnlyDictionary downloadOverrides, - ILogger logger) - { - if (originalData == null) - { - throw new ArgumentNullException(nameof(originalData)); - } - - if (downloadOverrides == null) - { - throw new ArgumentNullException(nameof(downloadOverrides)); - } - - if (logger == null) - { - throw new ArgumentNullException(nameof(logger)); - } - - // Create a copy of the original data and apply overrides as we copy. - var result = new DownloadData(); - - foreach (var downloadData in originalData) - { - var packageId = downloadData.Key; - - if (ShouldOverrideDownloads(packageId)) - { - logger.LogInformation( - "Overriding downloads of package {PackageId} from {Downloads} to {DownloadsOverride}", - packageId, - originalData.GetDownloadCount(packageId), - downloadOverrides[packageId]); - - var versions = downloadData.Value.Keys; - - result.SetDownloadCount( - packageId, - versions.First(), - downloadOverrides[packageId]); - } - else - { - foreach (var versionData in downloadData.Value) - { - result.SetDownloadCount(downloadData.Key, versionData.Key, versionData.Value); - } - } - } - - bool ShouldOverrideDownloads(string packageId) - { - if (!downloadOverrides.TryGetValue(packageId, out var downloadOverride)) - { - return false; - } - - // Apply the downloads override only if the package has fewer total downloads. - // In effect, this removes a package's manual boost once its total downloads exceed the override. - if (originalData[packageId].Total >= downloadOverride) - { - logger.LogInformation( - "Skipping download override for package {PackageId} as its downloads of {Downloads} are " + - "greater than its override of {DownloadsOverride}", - packageId, - originalData[packageId].Total, - downloadOverride); - return false; - } - - return true; - } - - return result; - } - } -} diff --git a/src/NuGet.Services.AzureSearch/Db2AzureSearch/Db2AzureSearchCommand.cs b/src/NuGet.Services.AzureSearch/Db2AzureSearch/Db2AzureSearchCommand.cs index c7db8d555..83c7603b7 100644 --- a/src/NuGet.Services.AzureSearch/Db2AzureSearch/Db2AzureSearchCommand.cs +++ b/src/NuGet.Services.AzureSearch/Db2AzureSearch/Db2AzureSearchCommand.cs @@ -30,6 +30,7 @@ public class Db2AzureSearchCommand : IAzureSearchCommand private readonly IOwnerDataClient _ownerDataClient; private readonly IDownloadDataClient _downloadDataClient; private readonly IVerifiedPackagesDataClient _verifiedPackagesDataClient; + private readonly IPopularityTransferDataClient _popularityTransferDataClient; private readonly IOptionsSnapshot _options; private readonly IOptionsSnapshot _developmentOptions; private readonly ILogger _logger; @@ -45,6 +46,7 @@ public Db2AzureSearchCommand( IOwnerDataClient ownerDataClient, IDownloadDataClient downloadDataClient, IVerifiedPackagesDataClient verifiedPackagesDataClient, + IPopularityTransferDataClient popularityTransferDataClient, IOptionsSnapshot options, IOptionsSnapshot developmentOptions, ILogger logger) @@ -59,6 +61,7 @@ public Db2AzureSearchCommand( _ownerDataClient = ownerDataClient ?? throw new ArgumentNullException(nameof(ownerDataClient)); _downloadDataClient = downloadDataClient ?? throw new ArgumentNullException(nameof(downloadDataClient)); _verifiedPackagesDataClient = verifiedPackagesDataClient ?? throw new ArgumentNullException(nameof(verifiedPackagesDataClient)); + _popularityTransferDataClient = popularityTransferDataClient ?? throw new ArgumentNullException(nameof(popularityTransferDataClient)); _options = options ?? throw new ArgumentNullException(nameof(options)); _developmentOptions = developmentOptions ?? throw new ArgumentNullException(nameof(developmentOptions)); _logger = logger ?? throw new ArgumentNullException(nameof(logger)); @@ -114,6 +117,9 @@ private async Task ExecuteAsync(CancellationToken token) // Write the verified packages data file. await WriteVerifiedPackagesDataAsync(initialAuxiliaryData.VerifiedPackages); + // TODO: Write popularity transfers data file. + // See: https://github.com/NuGet/NuGetGallery/issues/7898 + // Write the cursor. _logger.LogInformation("Writing the initial cursor value to be {CursorValue:O}.", initialCursorValue); var frontCursorStorage = _storageFactory.Create(); diff --git a/src/NuGet.Services.AzureSearch/Db2AzureSearch/InitialAuxiliaryData.cs b/src/NuGet.Services.AzureSearch/Db2AzureSearch/InitialAuxiliaryData.cs index 30dcae7b6..dc5475449 100644 --- a/src/NuGet.Services.AzureSearch/Db2AzureSearch/InitialAuxiliaryData.cs +++ b/src/NuGet.Services.AzureSearch/Db2AzureSearch/InitialAuxiliaryData.cs @@ -13,17 +13,20 @@ public InitialAuxiliaryData( SortedDictionary> owners, DownloadData downloads, HashSet excludedPackages, - HashSet verifiedPackages) + HashSet verifiedPackages, + SortedDictionary> popularityTransfers) { Owners = owners ?? throw new ArgumentNullException(nameof(owners)); Downloads = downloads ?? throw new ArgumentNullException(nameof(downloads)); ExcludedPackages = excludedPackages ?? throw new ArgumentNullException(nameof(excludedPackages)); VerifiedPackages = verifiedPackages ?? throw new ArgumentNullException(nameof(verifiedPackages)); + PopularityTransfers = popularityTransfers ?? throw new ArgumentNullException(nameof(popularityTransfers)); } public SortedDictionary> Owners { get; } public DownloadData Downloads { get; } public HashSet ExcludedPackages { get; } public HashSet VerifiedPackages { get; } + public SortedDictionary> PopularityTransfers { get; } } } diff --git a/src/NuGet.Services.AzureSearch/Db2AzureSearch/NewPackageRegistrationProducer.cs b/src/NuGet.Services.AzureSearch/Db2AzureSearch/NewPackageRegistrationProducer.cs index d758f1260..1b847380b 100644 --- a/src/NuGet.Services.AzureSearch/Db2AzureSearch/NewPackageRegistrationProducer.cs +++ b/src/NuGet.Services.AzureSearch/Db2AzureSearch/NewPackageRegistrationProducer.cs @@ -21,6 +21,8 @@ public class NewPackageRegistrationProducer : INewPackageRegistrationProducer { private readonly IEntitiesContextFactory _contextFactory; private readonly IAuxiliaryFileClient _auxiliaryFileClient; + private readonly IDatabaseAuxiliaryDataFetcher _databaseFetcher; + private readonly IDownloadTransferrer _downloadTransferrer; private readonly IOptionsSnapshot _options; private readonly IOptionsSnapshot _developmentOptions; private readonly ILogger _logger; @@ -28,14 +30,18 @@ public class NewPackageRegistrationProducer : INewPackageRegistrationProducer public NewPackageRegistrationProducer( IEntitiesContextFactory contextFactory, IAuxiliaryFileClient auxiliaryFileClient, + IDatabaseAuxiliaryDataFetcher databaseFetcher, + IDownloadTransferrer downloadTransferrer, IOptionsSnapshot options, IOptionsSnapshot developmentOptions, ILogger logger) { _contextFactory = contextFactory ?? throw new ArgumentNullException(nameof(contextFactory)); + _auxiliaryFileClient = auxiliaryFileClient ?? throw new ArgumentNullException(nameof(auxiliaryFileClient)); + _databaseFetcher = databaseFetcher ?? throw new ArgumentNullException(nameof(databaseFetcher)); + _downloadTransferrer = downloadTransferrer ?? throw new ArgumentNullException(nameof(downloadTransferrer)); _options = options ?? throw new ArgumentNullException(nameof(options)); _developmentOptions = developmentOptions ?? throw new ArgumentNullException(nameof(developmentOptions)); - _auxiliaryFileClient = auxiliaryFileClient ?? throw new ArgumentNullException(nameof(auxiliaryFileClient)); _logger = logger ?? throw new ArgumentNullException(nameof(logger)); } @@ -54,15 +60,18 @@ public async Task ProduceWorkAsync( $"Excluded packages HashSet should be using {nameof(StringComparer.OrdinalIgnoreCase)}"); // Fetch the download data from the auxiliary file, since this is what is used for displaying download - // counts in the search service. The gallery DB and the downloads data file have different download count - // numbers we don't use the gallery DB values. + // counts in the search service. We don't use the gallery DB values as they are different from the + // auxiliary file. var downloads = await _auxiliaryFileClient.LoadDownloadDataAsync(); - // Fetch the download overrides from the auxiliary file. Note that the overriden downloads are kept - // separate from downloads data as the original data will be persisted to auxiliary data, whereas the - // overriden data will be persisted to Azure Search. + var popularityTransfers = await _databaseFetcher.GetPackageIdToPopularityTransfersAsync(); var downloadOverrides = await _auxiliaryFileClient.LoadDownloadOverridesAsync(); - var overridenDownloads = downloads.ApplyDownloadOverrides(downloadOverrides, _logger); + + // Apply changes from popularity transfers and download overrides. + var transferredDownloads = GetTransferredDownloads( + downloads, + popularityTransfers, + downloadOverrides); // Build a list of the owners data and verified IDs as we collect package registrations from the database. var ownersBuilder = new PackageIdToOwnersBuilder(_logger); @@ -91,6 +100,11 @@ public async Task ProduceWorkAsync( foreach (var pr in packageRegistrationInfo) { + if (!transferredDownloads.TryGetValue(pr.Id, out var packageDownloads)) + { + packageDownloads = 0; + } + if (!keyToPackages.TryGetValue(pr.Key, out var packages)) { packages = new List(); @@ -100,7 +114,7 @@ public async Task ProduceWorkAsync( allWork.Add(new NewPackageRegistration( pr.Id, - overridenDownloads.GetDownloadCount(pr.Id), + packageDownloads, pr.Owners, packages, isExcludedByDefault)); @@ -120,7 +134,8 @@ public async Task ProduceWorkAsync( ownersBuilder.GetResult(), downloads, excludedPackages, - verifiedPackages); + verifiedPackages, + popularityTransfers); } private bool ShouldWait(ConcurrentBag allWork, bool log) @@ -145,6 +160,31 @@ private bool ShouldWait(ConcurrentBag allWork, bool log) return false; } + private Dictionary GetTransferredDownloads( + DownloadData downloads, + SortedDictionary> popularityTransfers, + IReadOnlyDictionary downloadOverrides) + { + var transferChanges = _downloadTransferrer.InitializeDownloadTransfers( + downloads, + popularityTransfers, + downloadOverrides); + + var result = new Dictionary(StringComparer.OrdinalIgnoreCase); + + foreach (var packageDownload in downloads) + { + result[packageDownload.Key] = packageDownload.Value.Total; + } + + foreach (var transferChange in transferChanges) + { + result[transferChange.Key] = transferChange.Value; + } + + return result; + } + private async Task> GetPackagesAsync(PackageRegistrationRange range) { using (var context = await CreateContextAsync()) @@ -352,4 +392,4 @@ public PackageRegistrationInfo(int key, string id, string[] owners, bool isVerif public bool IsVerified { get; } } } -} +} \ No newline at end of file diff --git a/src/NuGet.Services.AzureSearch/DependencyInjectionExtensions.cs b/src/NuGet.Services.AzureSearch/DependencyInjectionExtensions.cs index cc34e6fcb..839aa9fcb 100644 --- a/src/NuGet.Services.AzureSearch/DependencyInjectionExtensions.cs +++ b/src/NuGet.Services.AzureSearch/DependencyInjectionExtensions.cs @@ -165,6 +165,13 @@ private static void RegisterAzureSearchStorageServices(ContainerBuilder containe c.Resolve(), c.Resolve>())); + containerBuilder + .Register(c => new PopularityTransferDataClient( + c.ResolveKeyed(key), + c.Resolve>(), + c.Resolve(), + c.Resolve>())); + containerBuilder .Register(c => new Catalog2AzureSearchCommand( c.Resolve(), @@ -187,6 +194,7 @@ private static void RegisterAzureSearchStorageServices(ContainerBuilder containe c.Resolve(), c.Resolve(), c.Resolve(), + c.Resolve(), c.Resolve>(), c.Resolve>(), c.Resolve>())); @@ -252,6 +260,7 @@ public static IServiceCollection AddAzureSearch( services.AddTransient(); services.AddTransient(); services.AddTransient(); + services.AddTransient(); services.AddTransient(); services.AddTransient(); services.AddTransient(); diff --git a/src/NuGet.Services.AzureSearch/DownloadTransferrer.cs b/src/NuGet.Services.AzureSearch/DownloadTransferrer.cs new file mode 100644 index 000000000..7bda6c365 --- /dev/null +++ b/src/NuGet.Services.AzureSearch/DownloadTransferrer.cs @@ -0,0 +1,105 @@ +// 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.Collections.Generic; +using System.Linq; +using Microsoft.Extensions.Logging; +using NuGet.Services.AzureSearch.AuxiliaryFiles; + +namespace NuGet.Services.AzureSearch +{ + public class DownloadTransferrer : IDownloadTransferrer + { + private readonly ILogger _logger; + + public DownloadTransferrer(ILogger logger) + { + _logger = logger ?? throw new ArgumentNullException(nameof(logger)); + } + + public SortedDictionary InitializeDownloadTransfers( + DownloadData downloads, + SortedDictionary> outgoingTransfers, + IReadOnlyDictionary downloadOverrides) + { + // TODO: Add download changes due to popularity transfers. + // See: https://github.com/NuGet/NuGetGallery/issues/7898 + var downloadTransfers = new SortedDictionary(StringComparer.OrdinalIgnoreCase); + + // TODO: Remove download overrides. + // See: https://github.com/NuGet/Engineering/issues/3089 + ApplyDownloadOverrides(downloads, downloadOverrides, downloadTransfers); + + return downloadTransfers; + } + + public SortedDictionary UpdateDownloadTransfers( + DownloadData downloads, + SortedDictionary downloadChanges, + SortedDictionary> oldTransfers, + SortedDictionary> newTransfers, + IReadOnlyDictionary downloadOverrides) + { + Guard.Assert( + downloadChanges.Comparer == StringComparer.OrdinalIgnoreCase, + $"Download changes should have comparer {nameof(StringComparer.OrdinalIgnoreCase)}"); + + Guard.Assert( + oldTransfers.Comparer == StringComparer.OrdinalIgnoreCase, + $"Old popularity transfer should have comparer {nameof(StringComparer.OrdinalIgnoreCase)}"); + + Guard.Assert( + downloadChanges.All(x => downloads.GetDownloadCount(x.Key) == x.Value), + "The download changes should match the latest downloads"); + + // TODO: Add download changes due to popularity transfers. + // See: https://github.com/NuGet/NuGetGallery/issues/7898 + var downloadTransfers = new SortedDictionary(StringComparer.OrdinalIgnoreCase); + + // TODO: Remove download overrides. + // See: https://github.com/NuGet/Engineering/issues/3089 + ApplyDownloadOverrides(downloads, downloadOverrides, downloadTransfers); + + return downloadTransfers; + } + + private void ApplyDownloadOverrides( + DownloadData downloads, + IReadOnlyDictionary downloadOverrides, + SortedDictionary transferredDownloads) + { + // TODO: Remove download overrides. + // See: https://github.com/NuGet/Engineering/issues/3089 + foreach (var downloadOverride in downloadOverrides) + { + var packageId = downloadOverride.Key; + var packageDownloads = downloads.GetDownloadCount(packageId); + + if (transferredDownloads.TryGetValue(packageId, out var updatedDownloads)) + { + packageDownloads = updatedDownloads; + } + + if (packageDownloads >= downloadOverride.Value) + { + _logger.LogInformation( + "Skipping download override for package {PackageId} as its downloads of {Downloads} are " + + "greater than its override of {DownloadsOverride}", + packageId, + packageDownloads, + downloadOverride.Value); + continue; + } + + _logger.LogInformation( + "Overriding downloads of package {PackageId} from {Downloads} to {DownloadsOverride}", + packageId, + packageDownloads, + downloadOverride.Value); + + transferredDownloads[packageId] = downloadOverride.Value; + } + } + } +} \ No newline at end of file diff --git a/src/NuGet.Services.AzureSearch/IDatabaseAuxiliaryDataFetcher.cs b/src/NuGet.Services.AzureSearch/IDatabaseAuxiliaryDataFetcher.cs index 48680b76d..0d85f0276 100644 --- a/src/NuGet.Services.AzureSearch/IDatabaseAuxiliaryDataFetcher.cs +++ b/src/NuGet.Services.AzureSearch/IDatabaseAuxiliaryDataFetcher.cs @@ -28,7 +28,6 @@ public interface IDatabaseAuxiliaryDataFetcher /// Fetch a mapping of package IDs to set of replacement package IDs for each renamed packages that transfer /// popularity in the gallery database. /// - /// Task>> GetPackageIdToPopularityTransfersAsync(); /// diff --git a/src/NuGet.Services.AzureSearch/IDownloadTransferrer.cs b/src/NuGet.Services.AzureSearch/IDownloadTransferrer.cs new file mode 100644 index 000000000..dcd52c960 --- /dev/null +++ b/src/NuGet.Services.AzureSearch/IDownloadTransferrer.cs @@ -0,0 +1,61 @@ +// 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.Collections.Generic; +using NuGet.Services.AzureSearch.AuxiliaryFiles; + +namespace NuGet.Services.AzureSearch +{ + /// + /// Determines the downloads that should be changed due to popularity transfers. + /// + public interface IDownloadTransferrer + { + /// + /// Determine changes that should be applied to the initial downloads data due to popularity transfers. + /// + /// The initial downloads data. + /// + /// The initial popularity transfers. Maps packages transferring their popularity + /// away to the list of packages receiving the popularity. + /// + /// + /// The initial download overrides. Maps package that should be overriden to + /// their desired download value. + /// + /// + /// The downloads that are changed due to transfers. Maps package ids to the new download value. + /// + SortedDictionary InitializeDownloadTransfers( + DownloadData downloads, + SortedDictionary> popularityTransfers, + IReadOnlyDictionary downloadOverrides); + + /// + /// Determine changes that should be applied to the latest downloads data due to popularity transfers. + /// + /// The latest downloads data. + /// The downloads that have changed since the last index. + /// + /// The previously indexed popularity transfers. Maps packages transferring their popularity + /// away to the list of packages receiving the popularity. + /// + /// + /// The latest popularity transfers. Maps packages transferring their popularity + /// away to the list of packages receiving the popularity. + /// + /// + /// The latest download overrides. Maps package that should be overriden to + /// their desired download value. + /// + /// + /// The downloads that are changed due to transfers. Maps package ids to the new download value. + /// + SortedDictionary UpdateDownloadTransfers( + DownloadData downloads, + SortedDictionary downloadChanges, + SortedDictionary> oldTransfers, + SortedDictionary> newTransfers, + IReadOnlyDictionary downloadOverrides); + } +} \ No newline at end of file diff --git a/src/NuGet.Services.AzureSearch/NuGet.Services.AzureSearch.csproj b/src/NuGet.Services.AzureSearch/NuGet.Services.AzureSearch.csproj index 61fd4951b..91b53a4bb 100644 --- a/src/NuGet.Services.AzureSearch/NuGet.Services.AzureSearch.csproj +++ b/src/NuGet.Services.AzureSearch/NuGet.Services.AzureSearch.csproj @@ -56,7 +56,6 @@ - @@ -81,10 +80,12 @@ + + diff --git a/tests/NuGet.Services.AzureSearch.Tests/Auxiliary2AzureSearch/UpdateDownloadsCommandFacts.cs b/tests/NuGet.Services.AzureSearch.Tests/Auxiliary2AzureSearch/UpdateDownloadsCommandFacts.cs index 6c0f988da..2f632fdfb 100644 --- a/tests/NuGet.Services.AzureSearch.Tests/Auxiliary2AzureSearch/UpdateDownloadsCommandFacts.cs +++ b/tests/NuGet.Services.AzureSearch.Tests/Auxiliary2AzureSearch/UpdateDownloadsCommandFacts.cs @@ -7,7 +7,6 @@ using System.Linq; using System.Threading.Tasks; using Microsoft.Azure.Search.Models; -using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Moq; using NuGet.Services.AzureSearch.AuxiliaryFiles; @@ -44,6 +43,9 @@ public async Task PushesNothingWhenThereAreNoChanges() DownloadDataClient.Verify( x => x.ReplaceLatestIndexedAsync(It.IsAny(), It.IsAny()), Times.Never); + PopularityTransferDataClient.Verify( + x => x.ReplaceLatestIndexedAsync(It.IsAny>>(), It.IsAny()), + Times.Never); } [Theory] @@ -159,107 +161,72 @@ public async Task RejectsInvalidDataAndNormalizesVersions(string propertyName) } [Fact] - public async Task OverridesDownloadCounts() + public async Task AppliesTransferChanges() { + var downloadChanges = new SortedDictionary(StringComparer.OrdinalIgnoreCase); DownloadSetComparer .Setup(c => c.Compare(It.IsAny(), It.IsAny())) .Returns((oldData, newData) => { - return new SortedDictionary( - newData.ToDictionary(d => d.Key, d => d.Value.Total), - StringComparer.OrdinalIgnoreCase); + return downloadChanges; }); - NewDownloadData.SetDownloadCount("A", "1.0.0", 12); - NewDownloadData.SetDownloadCount("A", "2.0.0", 34); + TransferChanges["Package1"] = 100; + TransferChanges["Package2"] = 200; - NewDownloadData.SetDownloadCount("B", "3.0.0", 5); - NewDownloadData.SetDownloadCount("B", "4.0.0", 4); - - NewDownloadData.SetDownloadCount("C", "5.0.0", 2); - NewDownloadData.SetDownloadCount("C", "6.0.0", 3); - - DownloadOverrides["A"] = 55; - DownloadOverrides["b"] = 66; + NewTransfers["Package1"] = new SortedSet(StringComparer.OrdinalIgnoreCase) + { + "Package2" + }; await Target.ExecuteAsync(); - // Documents should have new data with overriden downloads. - SearchDocumentBuilder + PopularityTransferDataClient .Verify( - b => b.UpdateDownloadCount("A", SearchFilters.IncludePrereleaseAndSemVer2, 55), + c => c.ReadLatestIndexedAsync(), Times.Once); - SearchDocumentBuilder + DatabaseFetcher .Verify( - b => b.UpdateDownloadCount("B", SearchFilters.IncludePrereleaseAndSemVer2, 66), + d => d.GetPackageIdToPopularityTransfersAsync(), Times.Once); - SearchDocumentBuilder + AuxiliaryFileClient .Verify( - b => b.UpdateDownloadCount("C", SearchFilters.IncludePrereleaseAndSemVer2, 5), + a => a.LoadDownloadOverridesAsync(), Times.Once); - // Downloads auxiliary file should have new data without overriden downloads. - DownloadDataClient.Verify( - c => c.ReplaceLatestIndexedAsync( - It.Is(d => - d["A"].Total == 46 && - d["A"]["1.0.0"] == 12 && - d["A"]["2.0.0"] == 34 && - - d["B"].Total == 9 && - d["B"]["3.0.0"] == 5 && - d["B"]["4.0.0"] == 4 && - - d["C"].Total == 5 && - d["C"]["5.0.0"] == 2 && - d["C"]["6.0.0"] == 3), - It.IsAny()), - Times.Once); - } - - [Fact] - public async Task AlwaysAppliesDownloadOverrides() - { - DownloadSetComparer - .Setup(c => c.Compare(It.IsAny(), It.IsAny())) - .Returns((oldData, newData) => - { - var config = new Auxiliary2AzureSearchConfiguration(); - var telemetry = Mock.Of(); - var logger = Mock.Of>(); - var options = new Mock>(); - - options.Setup(o => o.Value).Returns(config); - - return new DownloadSetComparer(telemetry, options.Object, logger) - .Compare(oldData, newData); - }); - - // Download override should be applied even if the package's downloads haven't changed. - OldDownloadData.SetDownloadCount("A", "1.0.0", 1); - NewDownloadData.SetDownloadCount("A", "1.0.0", 1); - DownloadOverrides["A"] = 2; - - await Target.ExecuteAsync(); + DownloadTransferrer + .Verify( + x => x.UpdateDownloadTransfers( + NewDownloadData, + downloadChanges, + OldTransfers, + NewTransfers, + DownloadOverrides), + Times.Once); - // Documents should have new data with overriden downloads. + // Documents should be updated. SearchDocumentBuilder .Verify( - b => b.UpdateDownloadCount("A", SearchFilters.IncludePrereleaseAndSemVer2, 2), + b => b.UpdateDownloadCount("Package1", SearchFilters.IncludePrereleaseAndSemVer2, 100), + Times.Once); + SearchDocumentBuilder + .Verify( + b => b.UpdateDownloadCount("Package2", SearchFilters.IncludePrereleaseAndSemVer2, 200), Times.Once); - // Downloads auxiliary file should have new data without overriden downloads. + // Downloads auxiliary file should not include transfer changes. DownloadDataClient.Verify( c => c.ReplaceLatestIndexedAsync( - It.Is(d => - d["A"].Total == 1 && - d["A"]["1.0.0"] == 1), + It.Is(d => d.Count == 0), It.IsAny()), Times.Once); + + // TODO: Popularity transfers auxiliary file should have new data. + // See: https://github.com/NuGet/NuGetGallery/issues/7898 } [Fact] - public async Task DoesNotOverrideIfDownloadsGreaterOrPackageHasNoDownloads() + public async Task TransferChangesOverideDownloadChanges() { DownloadSetComparer .Setup(c => c.Compare(It.IsAny(), It.IsAny())) @@ -270,57 +237,59 @@ public async Task DoesNotOverrideIfDownloadsGreaterOrPackageHasNoDownloads() StringComparer.OrdinalIgnoreCase); }); - NewDownloadData.SetDownloadCount("A", "1.0.0", 100); - NewDownloadData.SetDownloadCount("A", "2.0.0", 200); + NewDownloadData.SetDownloadCount("A", "1.0.0", 12); + NewDownloadData.SetDownloadCount("A", "2.0.0", 34); NewDownloadData.SetDownloadCount("B", "3.0.0", 5); NewDownloadData.SetDownloadCount("B", "4.0.0", 4); - NewDownloadData.SetDownloadCount("C", "5.0.0", 0); + NewDownloadData.SetDownloadCount("C", "5.0.0", 2); + NewDownloadData.SetDownloadCount("C", "6.0.0", 3); + + TransferChanges["A"] = 55; + TransferChanges["b"] = 66; - DownloadOverrides["A"] = 55; - DownloadOverrides["C"] = 66; - DownloadOverrides["D"] = 77; + NewTransfers["FromPackage"] = new SortedSet(StringComparer.OrdinalIgnoreCase) + { + "ToPackage" + }; await Target.ExecuteAsync(); - // Documents should have new data with overriden downloads. + // Documents should have new data with transfer changes. SearchDocumentBuilder .Verify( - b => b.UpdateDownloadCount("A", SearchFilters.IncludePrereleaseAndSemVer2, 300), + b => b.UpdateDownloadCount("A", SearchFilters.IncludePrereleaseAndSemVer2, 55), Times.Once); SearchDocumentBuilder .Verify( - b => b.UpdateDownloadCount("B", SearchFilters.IncludePrereleaseAndSemVer2, 9), + b => b.UpdateDownloadCount("B", SearchFilters.IncludePrereleaseAndSemVer2, 66), Times.Once); SearchDocumentBuilder .Verify( - b => b.UpdateDownloadCount("B", SearchFilters.IncludePrereleaseAndSemVer2, 9), + b => b.UpdateDownloadCount("C", SearchFilters.IncludePrereleaseAndSemVer2, 5), Times.Once); - SearchDocumentBuilder - .Verify( - b => b.UpdateDownloadCount("C", It.IsAny(), It.IsAny()), - Times.Never); - SearchDocumentBuilder - .Verify( - b => b.UpdateDownloadCount("D", It.IsAny(), It.IsAny()), - Times.Never); - // Downloads auxiliary file should have new data without overriden downloads. + // Downloads auxiliary file should not reflect transfer changes. DownloadDataClient.Verify( c => c.ReplaceLatestIndexedAsync( It.Is(d => - d.Keys.Count() == 2 && - - d["A"].Total == 300 && - d["A"]["1.0.0"] == 100 && - d["A"]["2.0.0"] == 200 && + d["A"].Total == 46 && + d["A"]["1.0.0"] == 12 && + d["A"]["2.0.0"] == 34 && d["B"].Total == 9 && d["B"]["3.0.0"] == 5 && - d["B"]["4.0.0"] == 4), + d["B"]["4.0.0"] == 4 && + + d["C"].Total == 5 && + d["C"]["5.0.0"] == 2 && + d["C"]["6.0.0"] == 3), It.IsAny()), Times.Once); + + // TODO: Popularity transfers auxiliary file should have new data. + // See: https://github.com/NuGet/NuGetGallery/issues/7898 } } @@ -329,8 +298,11 @@ public abstract class Facts public Facts(ITestOutputHelper output) { AuxiliaryFileClient = new Mock(); + DatabaseFetcher = new Mock(); DownloadDataClient = new Mock(); DownloadSetComparer = new Mock(); + DownloadTransferrer = new Mock(); + PopularityTransferDataClient = new Mock(); SearchDocumentBuilder = new Mock(); IndexActionBuilder = new Mock(); BatchPusher = new Mock(); @@ -355,22 +327,45 @@ public Facts(ITestOutputHelper output) .ReturnsAsync(() => OldDownloadResult); NewDownloadData = new DownloadData(); AuxiliaryFileClient.Setup(x => x.LoadDownloadDataAsync()).ReturnsAsync(() => NewDownloadData); - DownloadOverrides = new Dictionary(StringComparer.OrdinalIgnoreCase); - AuxiliaryFileClient - .Setup(x => x.LoadDownloadOverridesAsync()) - .ReturnsAsync(() => DownloadOverrides); Changes = new SortedDictionary(); DownloadSetComparer .Setup(x => x.Compare(It.IsAny(), It.IsAny())) .Returns(() => Changes); + OldTransfers = new SortedDictionary>(StringComparer.OrdinalIgnoreCase); + + OldTransferResult = new ResultAndAccessCondition>>( + OldTransfers, + Mock.Of()); + PopularityTransferDataClient + .Setup(x => x.ReadLatestIndexedAsync()) + .ReturnsAsync(OldTransferResult); + + NewTransfers = new SortedDictionary>(StringComparer.OrdinalIgnoreCase); + DatabaseFetcher + .Setup(x => x.GetPackageIdToPopularityTransfersAsync()) + .ReturnsAsync(NewTransfers); + + DownloadOverrides = new Dictionary(); + AuxiliaryFileClient.Setup(x => x.LoadDownloadOverridesAsync()).ReturnsAsync(() => DownloadOverrides); + + TransferChanges = new SortedDictionary(StringComparer.OrdinalIgnoreCase); + DownloadTransferrer + .Setup(x => x.UpdateDownloadTransfers( + It.IsAny(), + It.IsAny>(), + It.IsAny>>(), + It.IsAny>>(), + It.IsAny>())) + .Returns(TransferChanges); + IndexActions = new IndexActions( new List> { IndexAction.Merge(new KeyedDocument()) }, new List>(), new ResultAndAccessCondition( new VersionListData(new Dictionary()), - new Mock().Object)); + Mock.Of())); ProcessedIds = new ConcurrentBag(); IndexActionBuilder .Setup(x => x.UpdateAsync(It.IsAny(), It.IsAny>())) @@ -403,8 +398,11 @@ public Facts(ITestOutputHelper output) Target = new UpdateDownloadsCommand( AuxiliaryFileClient.Object, + DatabaseFetcher.Object, DownloadDataClient.Object, DownloadSetComparer.Object, + DownloadTransferrer.Object, + PopularityTransferDataClient.Object, SearchDocumentBuilder.Object, IndexActionBuilder.Object, () => BatchPusher.Object, @@ -415,8 +413,11 @@ public Facts(ITestOutputHelper output) } public Mock AuxiliaryFileClient { get; } + public Mock DatabaseFetcher { get; } public Mock DownloadDataClient { get; } public Mock DownloadSetComparer { get; } + public Mock DownloadTransferrer { get; } + public Mock PopularityTransferDataClient { get; } public Mock SearchDocumentBuilder { get; } public Mock IndexActionBuilder { get; } public Mock BatchPusher { get; } @@ -428,8 +429,12 @@ public Facts(ITestOutputHelper output) public DownloadData OldDownloadData { get; } public AuxiliaryFileResult OldDownloadResult { get; } public DownloadData NewDownloadData { get; } + public SortedDictionary> OldTransfers { get; } + public ResultAndAccessCondition>> OldTransferResult { get; } + public SortedDictionary> NewTransfers { get; } public Dictionary DownloadOverrides { get; } public SortedDictionary Changes { get; } + public SortedDictionary TransferChanges { get; } public UpdateDownloadsCommand Target { get; } public IndexActions IndexActions { get; set; } public ConcurrentBag ProcessedIds { get; } diff --git a/tests/NuGet.Services.AzureSearch.Tests/Db2AzureSearch/Db2AzureSearchCommandFacts.cs b/tests/NuGet.Services.AzureSearch.Tests/Db2AzureSearch/Db2AzureSearchCommandFacts.cs index 5628eb38d..4aeba00fa 100644 --- a/tests/NuGet.Services.AzureSearch.Tests/Db2AzureSearch/Db2AzureSearchCommandFacts.cs +++ b/tests/NuGet.Services.AzureSearch.Tests/Db2AzureSearch/Db2AzureSearchCommandFacts.cs @@ -32,6 +32,7 @@ public class Db2AzureSearchCommandFacts private readonly Mock _ownerDataClient; private readonly Mock _downloadDataClient; private readonly Mock _verifiedPackagesDataClient; + private readonly Mock _popularityTransferDataClient; private readonly Mock> _options; private readonly Mock> _developmentOptions; private readonly Db2AzureSearchConfiguration _config; @@ -53,6 +54,7 @@ public Db2AzureSearchCommandFacts(ITestOutputHelper output) _ownerDataClient = new Mock(); _downloadDataClient = new Mock(); _verifiedPackagesDataClient = new Mock(); + _popularityTransferDataClient = new Mock(); _options = new Mock>(); _developmentOptions = new Mock>(); _logger = output.GetLogger(); @@ -68,7 +70,8 @@ public Db2AzureSearchCommandFacts(ITestOutputHelper output) owners: new SortedDictionary>(), downloads: new DownloadData(), excludedPackages: new HashSet(), - verifiedPackages: new HashSet()); + verifiedPackages: new HashSet(), + popularityTransfers: new SortedDictionary>()); _options .Setup(x => x.Value) @@ -108,6 +111,7 @@ public Db2AzureSearchCommandFacts(ITestOutputHelper output) _ownerDataClient.Object, _downloadDataClient.Object, _verifiedPackagesDataClient.Object, + _popularityTransferDataClient.Object, _options.Object, _developmentOptions.Object, _logger); diff --git a/tests/NuGet.Services.AzureSearch.Tests/Db2AzureSearch/NewPackageRegistrationProducerFacts.cs b/tests/NuGet.Services.AzureSearch.Tests/Db2AzureSearch/NewPackageRegistrationProducerFacts.cs index 603126477..d40d22584 100644 --- a/tests/NuGet.Services.AzureSearch.Tests/Db2AzureSearch/NewPackageRegistrationProducerFacts.cs +++ b/tests/NuGet.Services.AzureSearch.Tests/Db2AzureSearch/NewPackageRegistrationProducerFacts.cs @@ -37,8 +37,11 @@ public class ProduceWorkAsync private readonly CancellationToken _token; private readonly NewPackageRegistrationProducer _target; private readonly Mock _auxiliaryFileClient; + private readonly Mock _databaseFetcher; + private readonly Mock _downloadTransferrer; private readonly DownloadData _downloads; - private readonly Dictionary _downloadOverrides; + private readonly SortedDictionary> _popularityTransfers; + private readonly SortedDictionary _transferChanges; private HashSet _excludedPackages; public ProduceWorkAsync(ITestOutputHelper output) @@ -67,10 +70,21 @@ public ProduceWorkAsync(ITestOutputHelper output) _auxiliaryFileClient .Setup(x => x.LoadDownloadDataAsync()) .ReturnsAsync(() => _downloads); - _downloadOverrides = new Dictionary(StringComparer.OrdinalIgnoreCase); - _auxiliaryFileClient - .Setup(x => x.LoadDownloadOverridesAsync()) - .ReturnsAsync(() => _downloadOverrides); + + _popularityTransfers = new SortedDictionary>(); + _databaseFetcher = new Mock(); + _databaseFetcher + .Setup(x => x.GetPackageIdToPopularityTransfersAsync()) + .ReturnsAsync(() => _popularityTransfers); + + _downloadTransferrer = new Mock(); + _transferChanges = new SortedDictionary(StringComparer.OrdinalIgnoreCase); + _downloadTransferrer + .Setup(x => x.InitializeDownloadTransfers( + It.IsAny(), + It.IsAny>>(), + It.IsAny>())) + .Returns(_transferChanges); _entitiesContextFactory .Setup(x => x.CreateAsync(It.IsAny())) @@ -91,6 +105,8 @@ public ProduceWorkAsync(ITestOutputHelper output) _target = new NewPackageRegistrationProducer( _entitiesContextFactory.Object, _auxiliaryFileClient.Object, + _databaseFetcher.Object, + _downloadTransferrer.Object, _options.Object, _developmentOptions.Object, _logger); @@ -270,6 +286,49 @@ public async Task ProducesWorkPerPackageRegistration() Assert.Equal(26, work[3].TotalDownloadCount); } + [Fact] + public async Task DefaultsPackageDownloads() + { + _packageRegistrations.Add(new PackageRegistration + { + Key = 1, + Id = "HasDownloads", + Packages = new[] + { + new Package { Version = "1.0.0" }, + }, + }); + _downloads.SetDownloadCount("HasDownloads", "1.0.0", 100); + _packageRegistrations.Add(new PackageRegistration + { + Key = 2, + Id = "NoDownloads", + Packages = new[] + { + new Package { Version = "1.0.0" }, + }, + }); + + InitializePackagesFromPackageRegistrations(); + + var result = await _target.ProduceWorkAsync(_work, _token); + + // Documents should have overriden downloads. + var work = _work.Reverse().ToList(); + Assert.Equal(2, work.Count); + + Assert.Equal("HasDownloads", work[0].PackageId); + Assert.Equal("1.0.0", work[0].Packages[0].Version); + Assert.Equal(100, work[0].TotalDownloadCount); + Assert.Equal("NoDownloads", work[1].PackageId); + Assert.Equal("1.0.0", work[1].Packages[0].Version); + Assert.Equal(0, work[1].TotalDownloadCount); + + // Downloads auxiliary file should have original downloads. + Assert.Equal(100, result.Downloads["HasDownloads"]["1.0.0"]); + Assert.False(result.Downloads.ContainsKey("NoDownloads")); + } + [Fact] public async Task RetrievesAndUsesExclusionList() { @@ -391,6 +450,7 @@ public async Task ReturnsInitialAuxiliaryData() Assert.Same(_downloads, output.Downloads); Assert.Same(_excludedPackages, output.ExcludedPackages); + Assert.Same(_popularityTransfers, output.PopularityTransfers); Assert.NotNull(output.VerifiedPackages); Assert.Contains("A", output.VerifiedPackages); Assert.NotNull(output.Owners); @@ -417,7 +477,7 @@ public async Task ThrowsWhenExcludedPackagesIsMissing() } [Fact] - public async Task OverridesDownloadCounts() + public async Task AppliesDownloadTransfers() { _packageRegistrations.Add(new PackageRegistration { @@ -458,8 +518,10 @@ public async Task OverridesDownloadCounts() InitializePackagesFromPackageRegistrations(); - _downloadOverrides["A"] = 55; - _downloadOverrides["b"] = 66; + // Transfer changes should be applied to the package registrations. + _transferChanges["A"] = 55; + _transferChanges["b"] = 66; + _transferChanges["C"] = 123; var result = await _target.ProduceWorkAsync(_work, _token); @@ -480,7 +542,7 @@ public async Task OverridesDownloadCounts() Assert.Equal("C", work[2].PackageId); Assert.Equal("5.0.0", work[2].Packages[0].Version); Assert.Equal("6.0.0", work[2].Packages[1].Version); - Assert.Equal(5, work[2].TotalDownloadCount); + Assert.Equal(123, work[2].TotalDownloadCount); // Downloads auxiliary file should have original downloads. Assert.Equal(12, result.Downloads["A"]["1.0.0"]); @@ -492,7 +554,7 @@ public async Task OverridesDownloadCounts() } [Fact] - public async Task DoesNotOverrideIfDownloadsGreaterOrPackageHasNoDownloads() + public async Task IgnoresDownloadTransfersForNonexistentPackages() { _packageRegistrations.Add(new PackageRegistration { @@ -501,67 +563,28 @@ public async Task DoesNotOverrideIfDownloadsGreaterOrPackageHasNoDownloads() Packages = new[] { new Package { Version = "1.0.0" }, - new Package { Version = "2.0.0" }, }, }); _downloads.SetDownloadCount("A", "1.0.0", 100); - _downloads.SetDownloadCount("A", "2.0.0", 200); - _packageRegistrations.Add(new PackageRegistration - { - Key = 2, - Id = "B", - Packages = new[] - { - new Package { Version = "3.0.0" }, - new Package { Version = "4.0.0" }, - }, - }); - _downloads.SetDownloadCount("B", "3.0.0", 5); - _downloads.SetDownloadCount("B", "4.0.0", 4); - _packageRegistrations.Add(new PackageRegistration - { - Key = 3, - Id = "C", - Packages = new[] - { - new Package { Version = "5.0.0" }, - }, - }); - _downloads.SetDownloadCount("C", "5.0.0", 0); InitializePackagesFromPackageRegistrations(); - _downloadOverrides["A"] = 55; - _downloadOverrides["C"] = 66; - _downloadOverrides["D"] = 77; + // Transfer changes should be applied to the package registrations. + // Transfer changes for packages that do not exist should be ignored. + _transferChanges["PackageDoesNotExist"] = 123; var result = await _target.ProduceWorkAsync(_work, _token); // Documents should have overriden downloads. - var work = _work.Reverse().ToList(); - Assert.Equal(3, work.Count); + var work = _work.ToList(); + Assert.Single(work); Assert.Equal("A", work[0].PackageId); Assert.Equal("1.0.0", work[0].Packages[0].Version); - Assert.Equal("2.0.0", work[0].Packages[1].Version); - Assert.Equal(300, work[0].TotalDownloadCount); - - Assert.Equal("B", work[1].PackageId); - Assert.Equal("3.0.0", work[1].Packages[0].Version); - Assert.Equal("4.0.0", work[1].Packages[1].Version); - Assert.Equal(9, work[1].TotalDownloadCount); - - Assert.Equal("C", work[2].PackageId); - Assert.Equal("5.0.0", work[2].Packages[0].Version); - Assert.Equal(0, work[2].TotalDownloadCount); + Assert.Equal(100, work[0].TotalDownloadCount); // Downloads auxiliary file should have original downloads. Assert.Equal(100, result.Downloads["A"]["1.0.0"]); - Assert.Equal(200, result.Downloads["A"]["2.0.0"]); - Assert.Equal(5, result.Downloads["B"]["3.0.0"]); - Assert.Equal(4, result.Downloads["B"]["4.0.0"]); - Assert.DoesNotContain("C", result.Downloads.Keys); - Assert.DoesNotContain("D", result.Downloads.Keys); } private void InitializePackagesFromPackageRegistrations() diff --git a/tests/NuGet.Services.AzureSearch.Tests/DownloadTransferrerFacts.cs b/tests/NuGet.Services.AzureSearch.Tests/DownloadTransferrerFacts.cs new file mode 100644 index 000000000..7b3447d6e --- /dev/null +++ b/tests/NuGet.Services.AzureSearch.Tests/DownloadTransferrerFacts.cs @@ -0,0 +1,210 @@ +// 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.Collections.Generic; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Moq; +using NuGet.Services.AzureSearch.Auxiliary2AzureSearch; +using NuGet.Services.AzureSearch.AuxiliaryFiles; +using Xunit; + +namespace NuGet.Services.AzureSearch +{ + public class DownloadTransferrerFacts + { + public class InitializeDownloadTransfers : Facts + { + [Fact] + public void ReturnsEmptyResult() + { + var result = Target.InitializeDownloadTransfers( + DownloadData, + PopularityTransfers, + DownloadOverrides); + + Assert.Empty(result); + } + + [Fact] + public void AppliesDownloadOverrides() + { + DownloadData.SetDownloadCount("A", "1.0.0", 1); + DownloadData.SetDownloadCount("B", "1.0.0", 2); + + DownloadOverrides["A"] = 1000; + + var result = Target.InitializeDownloadTransfers( + DownloadData, + PopularityTransfers, + DownloadOverrides); + + Assert.Equal(new[] { "A" }, result.Keys); + Assert.Equal(1000, result["A"]); + } + + [Fact] + public void DoesNotOverrideGreaterOrEqualDownloads() + { + DownloadData.SetDownloadCount("A", "1.0.0", 1000); + DownloadData.SetDownloadCount("B", "1.0.0", 1000); + + DownloadOverrides["A"] = 1; + DownloadOverrides["B"] = 1000; + + var result = Target.InitializeDownloadTransfers( + DownloadData, + PopularityTransfers, + DownloadOverrides); + + Assert.Empty(result); + } + } + + public class GetUpdatedTransferChanges : Facts + { + [Fact] + public void RequiresDownloadDataForDownloadChange() + { + DownloadChanges["A"] = 1; + + var ex = Assert.Throws( + () => Target.UpdateDownloadTransfers( + DownloadData, + DownloadChanges, + OldTransfers, + PopularityTransfers, + DownloadOverrides)); + + Assert.Equal("The download changes should match the latest downloads", ex.Message); + } + + [Fact] + public void RequiresDownloadDataAndChangesMatch() + { + DownloadData.SetDownloadCount("A", "1.0.0", 1); + DownloadChanges["A"] = 2; + + var ex = Assert.Throws( + () => Target.UpdateDownloadTransfers( + DownloadData, + DownloadChanges, + OldTransfers, + PopularityTransfers, + DownloadOverrides)); + + Assert.Equal("The download changes should match the latest downloads", ex.Message); + } + + [Fact] + public void ReturnsEmptyResult() + { + var result = Target.UpdateDownloadTransfers( + DownloadData, + DownloadChanges, + OldTransfers, + PopularityTransfers, + DownloadOverrides); + + Assert.Empty(result); + } + + [Fact] + public void AppliesDownloadOverrides() + { + DownloadData.SetDownloadCount("A", "1.0.0", 1); + DownloadData.SetDownloadCount("B", "1.0.0", 2); + DownloadData.SetDownloadCount("C", "1.0.0", 3); + DownloadData.SetDownloadCount("D", "1.0.0", 4); + + DownloadChanges["C"] = 3; + DownloadChanges["D"] = 4; + + DownloadOverrides["A"] = 1000; + DownloadOverrides["C"] = 3000; + + var result = Target.UpdateDownloadTransfers( + DownloadData, + DownloadChanges, + OldTransfers, + PopularityTransfers, + DownloadOverrides); + + Assert.Equal(2, result.Count); + Assert.Equal(new[] { "A", "C" }, result.Keys); + Assert.Equal(1000, result["A"]); + Assert.Equal(3000, result["C"]); + } + + [Fact] + public void DoesNotOverrideGreaterOrEqualDownloads() + { + DownloadData.SetDownloadCount("A", "1.0.0", 1000); + DownloadData.SetDownloadCount("B", "1.0.0", 1000); + DownloadData.SetDownloadCount("C", "1.0.0", 1000); + + DownloadChanges["C"] = 1000; + + DownloadOverrides["A"] = 1; + DownloadOverrides["B"] = 1000; + DownloadOverrides["B"] = 1; + + var result = Target.UpdateDownloadTransfers( + DownloadData, + DownloadChanges, + OldTransfers, + PopularityTransfers, + DownloadOverrides); + + Assert.Empty(result); + } + + public GetUpdatedTransferChanges() + { + DownloadChanges = new SortedDictionary(StringComparer.OrdinalIgnoreCase); + OldTransfers = new SortedDictionary>(StringComparer.OrdinalIgnoreCase); + } + + public SortedDictionary DownloadChanges { get; } + public SortedDictionary> OldTransfers { get; } + } + + public class Facts + { + public Facts() + { + TransferChanges = new SortedDictionary(); + + DownloadOverrides = new Dictionary(); + PopularityTransfers = new SortedDictionary>(StringComparer.OrdinalIgnoreCase); + + DownloadData = new DownloadData(); + + Target = new DownloadTransferrer( + Mock.Of>()); + } + + public Mock DataComparer { get; } + public IDownloadTransferrer Target { get; } + + public DownloadData DownloadData { get; } + public Dictionary DownloadOverrides { get; } + public SortedDictionary> PopularityTransfers { get; } + public SortedDictionary TransferChanges { get; } + public double PopularityTransfer = 0; + + public void AddPopularityTransfer(string fromPackageId, string toPackageId) + { + if (!PopularityTransfers.TryGetValue(fromPackageId, out var toPackageIds)) + { + toPackageIds = new SortedSet(StringComparer.OrdinalIgnoreCase); + PopularityTransfers[fromPackageId] = toPackageIds; + } + + toPackageIds.Add(toPackageId); + } + } + } +} \ No newline at end of file diff --git a/tests/NuGet.Services.AzureSearch.Tests/NuGet.Services.AzureSearch.Tests.csproj b/tests/NuGet.Services.AzureSearch.Tests/NuGet.Services.AzureSearch.Tests.csproj index c3f5d1a9e..aa5a4b381 100644 --- a/tests/NuGet.Services.AzureSearch.Tests/NuGet.Services.AzureSearch.Tests.csproj +++ b/tests/NuGet.Services.AzureSearch.Tests/NuGet.Services.AzureSearch.Tests.csproj @@ -62,6 +62,7 @@ +