Skip to content

Commit c37f70d

Browse files
joelverhagenjoelverhagen
authored
Revert "Reduce database calls for Microsoft profile page from 21.056 to 2 (#10411)" (#10433)
This reverts commit 24d69cd.
1 parent e5ee7c9 commit c37f70d

8 files changed

Lines changed: 116 additions & 199 deletions

File tree

‎src/NuGetGallery.Services/PackageManagement/IPackageService.cs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,6 @@ Package FilterLatestPackageBySuffix(IReadOnlyCollection<Package> packages,
103103

104104
IEnumerable<Package> FindPackagesByOwner(User user, bool includeUnlisted, bool includeVersions = false);
105105

106-
(IReadOnlyCollection<Package> Packages, long TotalDownloadCount, int PackageCount) FindPackagesByProfile(User user, int page, int pageSize);
107-
108106
IEnumerable<Package> FindPackagesByAnyMatchingOwner(User user, bool includeUnlisted, bool includeVersions = false);
109107

110108
IQueryable<PackageRegistration> FindPackageRegistrationsByOwner(User user);

‎src/NuGetGallery.Services/PackageManagement/PackageService.cs‎

Lines changed: 14 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -428,53 +428,6 @@ private static Package FilterLatestPackageHelper(
428428
return package;
429429
}
430430

431-
public (IReadOnlyCollection<Package> Packages, long TotalDownloadCount, int PackageCount) FindPackagesByProfile(
432-
User user,
433-
int page,
434-
int pageSize)
435-
{
436-
if (user == null)
437-
{
438-
throw new ArgumentNullException(nameof(user));
439-
}
440-
441-
IQueryable<Package> packages = _packageRepository.GetAll()
442-
.Where(p => p.PackageRegistration.Owners.Any(o => o.Key == user.Key));
443-
444-
var packageSummary = packages
445-
.Where(p => p.IsLatestSemVer2)
446-
.Include(p => p.PackageRegistration)
447-
.GroupBy(r => 1)
448-
.Select(g => new
449-
{
450-
PackageCount = g.Sum(x => 1),
451-
DownloadCount = g.Sum(x => x.PackageRegistration.DownloadCount)
452-
})
453-
.FirstOrDefault();
454-
455-
var packageCount = packageSummary != null ? packageSummary.PackageCount : 0;
456-
var downloadCount = packageSummary != null ? packageSummary.DownloadCount : 0;
457-
458-
packages = packages
459-
.Where(p => p.Listed
460-
&& p.PackageStatusKey == PackageStatus.Available);
461-
462-
packages = GetLatestVersion(packages);
463-
464-
return (packages
465-
.OrderByDescending(p => p.PackageRegistration.DownloadCount)
466-
.ThenBy(p => p.PackageRegistration.Id)
467-
.Skip((page-1) * pageSize)
468-
.Take(pageSize)
469-
.Include(p => p.PackageRegistration.Owners)
470-
.Include(p => p.PackageRegistration.RequiredSigners)
471-
.Include(p => p.PackageRegistration.Packages.Select(p => p.SupportedFrameworks))
472-
.Include(p => p.PackageRegistration.Packages.Select(p => p.Deprecations))
473-
.ToList(),
474-
downloadCount,
475-
packageCount);
476-
}
477-
478431
public IEnumerable<Package> FindPackagesByOwner(User user, bool includeUnlisted, bool includeVersions = false)
479432
{
480433
return GetPackagesForOwners(new[] { user.Key }, includeUnlisted, includeVersions);
@@ -506,7 +459,20 @@ private IEnumerable<Package> GetPackagesForOwners(IEnumerable<int> ownerKeys, bo
506459

507460
if (!includeVersions)
508461
{
509-
packages = GetLatestVersion(packages);
462+
// Do a best effort of retrieving the latest versions. Note that UpdateIsLatest has had concurrency issues
463+
// where sometimes packages no rows with IsLatest set. In this case, we'll just select the last inserted
464+
// row (descending [Key]) as opposed to reading all rows into memory and sorting on NuGetVersion.
465+
packages = packages
466+
.GroupBy(p => p.PackageRegistrationKey)
467+
.Select(g => g
468+
// order booleans desc so that true (1) comes first
469+
.OrderByDescending(p => p.IsLatestStableSemVer2)
470+
.ThenByDescending(p => p.IsLatestStable)
471+
.ThenByDescending(p => p.IsLatestSemVer2)
472+
.ThenByDescending(p => p.IsLatest)
473+
.ThenByDescending(p => p.Listed)
474+
.ThenByDescending(p => p.Key)
475+
.FirstOrDefault());
510476
}
511477

512478
return packages
@@ -516,25 +482,6 @@ private IEnumerable<Package> GetPackagesForOwners(IEnumerable<int> ownerKeys, bo
516482
.ToList();
517483
}
518484

519-
private static IQueryable<Package> GetLatestVersion(IQueryable<Package> packages)
520-
{
521-
// Do a best effort of retrieving the latest versions. Note that UpdateIsLatest has had concurrency issues
522-
// where sometimes packages no rows with IsLatest set. In this case, we'll just select the last inserted
523-
// row (descending [Key]) as opposed to reading all rows into memory and sorting on NuGetVersion.
524-
packages = packages
525-
.GroupBy(p => p.PackageRegistrationKey)
526-
.Select(g => g
527-
// order booleans desc so that true (1) comes first
528-
.OrderByDescending(p => p.IsLatestStableSemVer2)
529-
.ThenByDescending(p => p.IsLatestStable)
530-
.ThenByDescending(p => p.IsLatestSemVer2)
531-
.ThenByDescending(p => p.IsLatest)
532-
.ThenByDescending(p => p.Listed)
533-
.ThenByDescending(p => p.Key)
534-
.FirstOrDefault());
535-
return packages;
536-
}
537-
538485
public IQueryable<PackageRegistration> FindPackageRegistrationsByOwner(User user)
539486
{
540487
return _packageRegistrationRepository.GetAll().Where(p => p.Owners.Any(o => o.Key == user.Key));

‎src/NuGetGallery/Controllers/UsersController.cs‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -751,25 +751,17 @@ public virtual ActionResult Profiles(string username, int page = 1)
751751
return HttpNotFound();
752752
}
753753

754-
var packageResult = PackageService.FindPackagesByProfile(user, page, GalleryConstants.DefaultPackageListPageSize);
755-
756-
var packages = packageResult.Packages
754+
var packages = PackageService.FindPackagesByOwner(user, includeUnlisted: false)
755+
.Where(p => p.PackageStatusKey == PackageStatus.Available)
756+
.OrderByDescending(p => p.PackageRegistration.DownloadCount)
757757
.Select(p =>
758758
{
759759
var viewModel = _listPackageItemViewModelFactory.Create(p, currentUser, false);
760760
viewModel.DownloadCount = p.PackageRegistration.DownloadCount;
761761
return viewModel;
762762
}).ToList();
763763

764-
var model = new UserProfileModel(
765-
user,
766-
currentUser,
767-
packages,
768-
page - 1,
769-
GalleryConstants.DefaultPackageListPageSize,
770-
Url,
771-
packageResult.TotalDownloadCount,
772-
packageResult.PackageCount);
764+
var model = new UserProfileModel(user, currentUser, packages, page - 1, GalleryConstants.DefaultPackageListPageSize, Url);
773765

774766
return View(model);
775767
}

‎src/NuGetGallery/ViewModels/UserProfileModel.cs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,27 +10,28 @@ namespace NuGetGallery
1010
{
1111
public class UserProfileModel
1212
{
13-
public UserProfileModel(User user, User currentUser, List<ListPackageItemViewModel> allPackages, int pageIndex, int pageSize, UrlHelper url, long totalDownloadCount, int packageCount)
13+
public UserProfileModel(User user, User currentUser, List<ListPackageItemViewModel> allPackages, int pageIndex, int pageSize, UrlHelper url)
1414
{
1515
User = user;
1616
Username = user.Username;
1717
EmailAddress = user.EmailAddress;
1818
UnconfirmedEmailAddress = user.UnconfirmedEmailAddress;
1919
IsLocked = user.IsLocked;
2020
AllPackages = allPackages;
21-
TotalPackages = packageCount;
21+
TotalPackages = allPackages.Count;
2222
PackagePage = pageIndex;
2323
PackagePageSize = pageSize;
2424

25-
TotalPackageDownloadCount = totalDownloadCount;
25+
TotalPackageDownloadCount = AllPackages.Sum(p => p.TotalDownloadCount);
2626

2727
PackagePageTotalCount = (TotalPackages + PackagePageSize - 1) / PackagePageSize;
2828

2929
var pager = new PreviousNextPagerViewModel<ListPackageItemViewModel>(allPackages, pageIndex, PackagePageTotalCount,
3030
page => url.User(user, page));
3131

3232
Pager = pager;
33-
PagedPackages = AllPackages;
33+
PagedPackages = AllPackages.Skip(PackagePageSize * pageIndex)
34+
.Take(PackagePageSize).ToList();
3435

3536
CanManageAccount = ActionsRequiringPermissions.ManageAccount.CheckPermissions(currentUser, user) == PermissionsCheckResult.Allowed;
3637
CanViewAccount = ActionsRequiringPermissions.ViewAccount.CheckPermissions(currentUser, user) == PermissionsCheckResult.Allowed;

‎src/NuGetGallery/Views/Users/Profiles.cshtml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@
2525
responsive: true)
2626
<div class="statistics">
2727
<div class="statistic">
28-
<div class="value">@Model.TotalPackages.ToNuGetNumberString()</div>
29-
<div class="description">@(Model.TotalPackages == 1 ? "Package" : "Packages")</div>
28+
<div class="value">@Model.AllPackages.Count.ToNuGetNumberString()</div>
29+
<div class="description">@(Model.AllPackages.Count == 1 ? "Package" : "Packages")</div>
3030
</div>
3131
<div class="statistic">
3232
<div class="value">@Model.TotalPackageDownloadCount.ToNuGetNumberString()</div>

‎tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs‎

Lines changed: 87 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1162,6 +1162,89 @@ public static IEnumerable<object[]> PossibleOwnershipScenarios_Data
11621162
}
11631163
}
11641164

1165+
[Theory]
1166+
[MemberData(nameof(PossibleOwnershipScenarios_Data))]
1167+
public void ReturnsSinglePackageAsExpected(User currentUser, User owner)
1168+
{
1169+
// Arrange
1170+
var username = "test";
1171+
1172+
var package = new Package
1173+
{
1174+
Version = "1.1.1",
1175+
1176+
PackageRegistration = new PackageRegistration
1177+
{
1178+
Id = "package",
1179+
Owners = new[] { owner },
1180+
DownloadCount = 150
1181+
},
1182+
1183+
DownloadCount = 100,
1184+
PackageStatusKey = PackageStatus.Available
1185+
};
1186+
var invalidatedPackage = new Package
1187+
{
1188+
Version = "1.0.0",
1189+
1190+
PackageRegistration = new PackageRegistration
1191+
{
1192+
Id = "packageFailedValidation",
1193+
Owners = new[] { owner },
1194+
DownloadCount = 0
1195+
},
1196+
1197+
DownloadCount = 0,
1198+
PackageStatusKey = PackageStatus.FailedValidation
1199+
};
1200+
var validatingPackage = new Package
1201+
{
1202+
Version = "1.0.0",
1203+
1204+
PackageRegistration = new PackageRegistration
1205+
{
1206+
Id = "packageValidating",
1207+
Owners = new[] { owner },
1208+
DownloadCount = 0
1209+
},
1210+
1211+
DownloadCount = 0,
1212+
PackageStatusKey = PackageStatus.Validating
1213+
};
1214+
var deletedPackage = new Package
1215+
{
1216+
Version = "1.0.0",
1217+
1218+
PackageRegistration = new PackageRegistration
1219+
{
1220+
Id = "packageDeleted",
1221+
Owners = new[] { owner },
1222+
DownloadCount = 0
1223+
},
1224+
1225+
DownloadCount = 0,
1226+
PackageStatusKey = PackageStatus.Deleted
1227+
};
1228+
1229+
GetMock<IUserService>()
1230+
.Setup(x => x.FindByUsername(username, false))
1231+
.Returns(owner);
1232+
1233+
GetMock<IPackageService>()
1234+
.Setup(x => x.FindPackagesByOwner(owner, false, false))
1235+
.Returns(new[] { package, invalidatedPackage, validatingPackage, deletedPackage });
1236+
1237+
var controller = GetController<UsersController>();
1238+
controller.SetCurrentUser(currentUser);
1239+
1240+
// Act
1241+
var result = controller.Profiles(username);
1242+
1243+
// Assert
1244+
var model = ResultAssert.IsView<UserProfileModel>(result);
1245+
AssertUserProfileModel(model, currentUser, owner, package);
1246+
}
1247+
11651248
[Theory]
11661249
[MemberData(nameof(PossibleOwnershipScenarios_Data))]
11671250
public void SortsPackagesByDownloadCount(User currentUser, User owner)
@@ -1201,11 +1284,9 @@ public void SortsPackagesByDownloadCount(User currentUser, User owner)
12011284
.Setup(x => x.FindByUsername(username, false))
12021285
.Returns(owner);
12031286

1204-
var packages = new List<Package> { package2, package1 };
1205-
12061287
GetMock<IPackageService>()
1207-
.Setup(x => x.FindPackagesByProfile(owner, 1, GalleryConstants.DefaultPackageListPageSize))
1208-
.Returns((packages.AsReadOnly(), 350, 2));
1288+
.Setup(x => x.FindPackagesByOwner(owner, false, false))
1289+
.Returns(new[] { package1, package2 });
12091290

12101291
var controller = GetController<UsersController>();
12111292
controller.SetCurrentUser(currentUser);
@@ -1246,8 +1327,8 @@ public void UsesProperIconUrl(User currentUser, User owner)
12461327
.Setup(x => x.FindByUsername(username, false))
12471328
.Returns(owner);
12481329
GetMock<IPackageService>()
1249-
.Setup(x => x.FindPackagesByProfile(owner, 1, GalleryConstants.DefaultPackageListPageSize))
1250-
.Returns((userPackages.AsReadOnly(), 0, 0 ));
1330+
.Setup(x => x.FindPackagesByOwner(owner, false, false))
1331+
.Returns(new[] { userPackage });
12511332

12521333
var controller = GetController<UsersController>();
12531334
controller.SetCurrentUser(currentUser);

0 commit comments

Comments
 (0)