Introduce a version-specific ID column for display purposes #8504
Conversation
joelverhagen
requested review from
agr,
dannyjdev,
drewgillies,
loic-sharma,
lyndaidaii,
ryuyu,
shishirx34,
skofman1,
xavierdecoster and
zhhyu
as code owners
| @@ -594,7 +594,7 @@ public async Task WillSaveThePackageFileAndSetThePackageFileSize() | |||
| } | |||
|
|
|||
| [Fact] | |||
| private async Task WillSaveTheCreatedPackageWhenANewPackageRegistrationIsCreated() | |||
| public async Task WillSaveTheCreatedPackageWhenANewPackageRegistrationIsCreated() | |||
There was a problem hiding this comment.
UTs must be public to run.
| @@ -35,7 +35,7 @@ public PackageViewModel Setup(PackageViewModel viewModel, Package package) | |||
|
|
|||
| viewModel.FullVersion = NuGetVersionFormatter.ToFullString(package.Version); | |||
|
|
|||
| viewModel.Id = package.PackageRegistration.Id; | |||
| viewModel.Id = package.Id; | |||
There was a problem hiding this comment.
Thanks to our view model inheritance, this was the model impactful part of the PR and fixed the majority of the cases.
There was a problem hiding this comment.
The only concern I have is in the area of discoverability. Simply calling the column Id seems to indicate to a casual observer that it should have the Id in all rows of the table, and that it's in no way obvious that PackageRegistrations has the true working Id. Could it be something more descriptive like CasedIdForVersion or the like? Not a strong opinion but something to consider. Approving just the same.
|
Well, unfortunately |
| && !string.Equals(latestSemVer2Package.Id, packageRegistration.Id, StringComparison.Ordinal)) | ||
| { | ||
| packageRegistration.Id = latestSemVer2Package.Id; | ||
| } |
There was a problem hiding this comment.
I am not very sure whether it's a good question, but how do we evaluate the risk and influence of this "packageRegistration.Id" change for many other components, including validation, V3, search, and etc.? This seems a very big change here.
There was a problem hiding this comment.
It's a very good question 😄. Let's think about the components you mentioned and split them into two categories:
- Components that call into gallery DB:
- some validation jobs (orchestrator, process signature, scan-and-sign)
- Db2Catalog
- Db2AzureSearch
- Auxiliary2AzureSearch
- Components that do NOT call into gallery DB:
- the rest of V3
- search service
- most other jobs
For components that call into gallery DB, there are further two categories:
- Services that read and write the DB
- Services that just read the DB
For services that read and write the gallery DB, only orchestrator and gallery modify the Packages table. In general scary DB changes would be ones that are not backwards compatible since you may have readers and writers that are out of sync codewise but running at the same time. This is mitigated in this case because new code reading records updated/written by old code will fall back to the previous behavior of using the PackageRegistration.Id.
As mentioned in the PR description there will be another PR in NuGet.Jobs to bring orchestrator up to date with NuGetGallery.Core so that it starts populating the new Id column. Technically Orchestrator does not add new Packages rows so it won't be possible for a new row to be created without the Id column populated since gallery is the only thing that creates these rows, and it will be updated. But even if there was a job in this category, gallery would handle those rows just fine because the null in the Package.Id column will fall back to PackageRegistration.Id.
For services that just read the DB, if they have old code they will ignore the column. If they have new code they will handle the null column case via the null coalescence in the Id property that I added. Raw SQL will not know about this column and it will therefore be ignored taking the pre-existing behavior.
Db2Catalog does not care about package ID casing since the xslt on the package nuspec is what determines the ID casing for V3, and this has had the proper, true casing for ages. This is why the Azure Search index has the correct package ID casing a lot of the time. Packages pushed since the last index rebuild have their ID casing source from the catalog leaf, which is sourced from the .nupkg itself not gallery DB.
For Db2AzureSearch, there will need to be a follow-up change to use the new column but during sprint planning we decided to punt this for now. This is not a huge deal since there will be no regression from current behavior. In fact it will be a bit better automatically since I have modified the upload flow to also try to keep PackageRegistration.Id casing in sync with the latest version.
For Auxiliary2AzureSearch, the package ID casing is irrelevant since it's only used for building data files that are used as look-up hash maps.
I'm not sure if this answers your question directly, but the tldr is that adding this column is backwards compatible with existing services/code.
There was a problem hiding this comment.
If I were to guess, any problems that would come from this change would be app-code problems in Gallery not data issues or app-code problems in any other service that is not yet referencing the column.
In light of this, we can use the normal recovery plan for Gallery in the case of critical bugs which is to roll back to the previous version of the code.
We should not need to roll back the DB change (hopefully these are not famous last works 😄)
|
looking this PR now |
Progress on #3349.
Packagestable. This is nullable and will be filled lazily as new packages are pushed or existing packages are reflowed.PackageRegistrationwhen the latest version is calculated.PackageRegistration.Idalso have an improvement. Examples are the API keys page and the admin package locking UI.A subsequent PR will update the version of NuGetGallery.Core in orchestrator (i.e. propagate change 3 above).
The migration SQL is: