Add package staging schema and status support - #10918
Conversation
2238a54 to
4a74929
Compare
There was a problem hiding this comment.
Pull request overview
This PR lays the schema + entity groundwork for “package staging” in NuGetGallery, introducing new staging tables/entities and wiring a new hidden PackageStatus.Staged through the data model and a few internal code paths, without enabling end-user staging behavior yet.
Changes:
- Add new staging entities (
StagingGroup,StagedPackage,StagingBlobCleanup) plus EF model configuration and a migration (AddPackageStaging). - Introduce
PackageStatus.Staged(andPackageStatusSummary.Staged) and update view-model/status handling and related tests. - Extend account deletion to remove staging-owned data and record promotion identity via
Package.ApproverUserKey.
Show a summary per file
| File | Description |
|---|---|
| tests/NuGetGallery.Facts/ViewModels/PackageViewModelFacts.cs | Adds test coverage for mapping PackageStatus.Staged to a status summary; strengthens “unexpected status” test value. |
| tests/NuGetGallery.Facts/TestUtils/FakeEntitiesContext.cs | Extends the fake EF context with DbSets for new staging entities. |
| tests/NuGetGallery.Facts/Services/DeleteAccountServiceFacts.cs | Adds assertions + test data for staging cleanup and for removing package approver references during deletion. |
| tests/NuGet.Services.Entities.Tests/StagedValidationStatusFacts.cs | New test intended to lock enum integer values for persisted StagedValidationStatus. |
| tests/NuGet.Services.Entities.Tests/PackageStatusFacts.cs | Appends PackageStatus.Staged to the immutable value test map. |
| tests/NuGet.Services.AzureSearch.Tests/DatabaseAuxiliaryDataFetcherFacts.cs | Updates test context stub to satisfy expanded IEntitiesContext contract. |
| src/VerifyMicrosoftPackage/Fakes/FakeEntitiesContext.cs | Updates stub context to satisfy expanded IEntitiesContext contract. |
| src/NuGetGallery/ViewModels/PackageStatusSummary.cs | Adds PackageStatusSummary.Staged enum value with display description. |
| src/NuGetGallery/NuGetGallery.csproj | Adds the new EF migration artifacts to the legacy explicit-compile project file. |
| src/NuGetGallery/Migrations/202607290027439_AddPackageStaging.resx | Adds migration resource payload for EF6 metadata. |
| src/NuGetGallery/Migrations/202607290027439_AddPackageStaging.Designer.cs | Adds EF-generated migration metadata class. |
| src/NuGetGallery/Migrations/202607290027439_AddPackageStaging.cs | Adds migration creating staging tables + new columns/index/foreign key for approver tracking. |
| src/NuGetGallery/Helpers/ViewModelExtensions/PackageViewModelFactory.cs | Maps PackageStatus.Staged to the new PackageStatusSummary.Staged. |
| src/NuGetGallery/Controllers/PackagesController.cs | Excludes staged packages from the symbols-deletion flow’s candidate set. |
| src/NuGetGallery.Services/AccountManagement/DeleteAccountService.cs | Removes staging-owned rows on account deletion; clears package approver references; enqueues staged blobs for deferred cleanup. |
| src/NuGetGallery.Core/Entities/IEntitiesContext.cs | Extends the EF context interface with staging DbSets. |
| src/NuGetGallery.Core/Entities/EntitiesContext.cs | Adds DbSets + EF model configuration for staging entities and Package.ApproverUser relationship. |
| src/NuGet.Services.Entities/User.cs | Adds per-account staged package quota (StagingPackageLimit). |
| src/NuGet.Services.Entities/StagingGroup.cs | New entity defining owner-scoped staging groups. |
| src/NuGet.Services.Entities/StagingBlobCleanup.cs | New table-as-queue entity for deferred deletion of staging blobs. |
| src/NuGet.Services.Entities/StagedValidationStatus.cs | New enum representing validation progress while a package remains in Staged. |
| src/NuGet.Services.Entities/StagedPackage.cs | New entity holding ephemeral staging metadata for a staged package. |
| src/NuGet.Services.Entities/PackageStatus.cs | Adds PackageStatus.Staged with documentation indicating it’s non-public/hidden. |
| src/NuGet.Services.Entities/Package.cs | Adds ApproverUser/ApproverUserKey to record who promoted a staged package. |
Review details
Files not reviewed (1)
- src/NuGetGallery/Migrations/202607290027439_AddPackageStaging.Designer.cs: Generated file
- Files reviewed: 23/24 changed files
- Comments generated: 2
- Review effort level: Lite
| // 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 Xunit; |
| package.ApproverUser = null; | ||
| } |
| @@ -0,0 +1,105 @@ | |||
| namespace NuGetGallery.Migrations | |||
| /// <summary> | ||
| /// Gets or sets the primary key for the entity. | ||
| /// </summary> | ||
| public int Key { get; set; } |
There was a problem hiding this comment.
If we are not going to have multiple entries in this table for an entry in Packages table, can we just use PackageKey here as primary key and drop this column?
Is there a scenario where look up in this table would be done NOT by package key?
There was a problem hiding this comment.
I'm still refining the design so hold off on reviewing until I republish the PR.
| public int PackageKey { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the staged package. |
There was a problem hiding this comment.
Gets or sets? It should be a navigation property value of which defined by PackageKey.
| /// <summary> | ||
| /// Gets or sets the user or organization that owns this staged package. | ||
| /// </summary> | ||
| public virtual User Owner { get; set; } |
There was a problem hiding this comment.
Package ownership right now happens on package registration level and there might be multiple owners. How that property fits into existing ownership setup?
|
|
||
| /// <summary> | ||
| /// Gets or sets the path to the nupkg in the private staging container. The path embeds a GUID so that | ||
| /// deleting and re-uploading the same id and version cannot collide with the outgoing blob. |
There was a problem hiding this comment.
If deleting leaves a chance for the blob to stay, what's the plan to clean up the storage from stale entries?
| /// this mirrors the group's expiration; for ungrouped packages it is set at push time and reset on replace. | ||
| /// The timestamp is in UTC. | ||
| /// </summary> | ||
| public DateTime ExpirationDate { get; set; } |
| /// Gets or sets how far validation has got for this package. A staged package stays in | ||
| /// <see cref="PackageStatus.Staged"/> throughout, so this is the only signal that it is promotable. | ||
| /// </summary> | ||
| public StagedValidationStatus ValidationStatus { get; set; } |
There was a problem hiding this comment.
Don't we need 2 statuses here: one for regular package, another for symbols?
Why not reuse status columns in Packages/SymbolPackages tables? No state duplication, less code changes to make.
| /// Gets or sets the key of the group this package belongs to, or <c>null</c> if the package is ungrouped. | ||
| /// When set, the group's owner must match <see cref="OwnerKey"/>. | ||
| /// </summary> | ||
| public int? StagingGroupKey { get; set; } |
There was a problem hiding this comment.
Can a package be staged to multiple groups at the same time?
| .Index(t => t.PackageKey, unique: true) | ||
| .Index(t => t.OwnerKey) | ||
| .Index(t => t.StagingGroupKey) | ||
| .Index(t => t.ExpirationDate); |
There was a problem hiding this comment.
Single column indexes are not very useful: we are not going to be doing just single value operations with those, right? Additional columns should be included based on data that is actually retrieved using those indexes.
There was a problem hiding this comment.
Depending how heavily the feature is used it might not matter much.
Schema and entity groundwork for package staging. No behavior change.
StagingGroup,StagedPackage, andStagingBlobCleanupentities and DbSets.PackageStatus.Stagedhidden from all public surfaces.Package.ApproverUserKeyto record who promoted a package, separate from who pushed it.User.StagingPackageLimitfor per-account staged package quota.AddPackageStaging: 3 tables, 2 columns, 5 indexes, 5 FKs.