When we read download counts from a downloads.v1.json file in Gallery and therefore have DownloadCountObjectMaterializedInterceptor enabled, potentially unexpected entity changes can be staged in the EF context by the interceptor.
This line of code modifies the DownloadCount property on the package meaning and SaveChanges later in the same EF context scope will update this property along with other changes.
It's unclear whether leaking downloads.v1.json values into the DB slowly, and organically along with other package entity changes was intended. But what is does do is case EF context to return HasChanges = true in more cases than the server layer code may expect making no-ops not work properly.
In short, downloads.v1.json values can slowly propagate into the DB as package entities are materialized in gallery and then SaveChanges() is called on the EF context.
The same problem exists for PackageRegistration.
It is quite possible for PackageRegistration total count and Package total counts in the DB to diverge in this way. In fact about 7% of package registration total download counts do not match with the sum of their version.
Repro Steps
Enable downloads.v1.json integration in gallery (via Gallery.AzureStorage.Statistics.ConnectionString)
Make the DB value different from the downloads.v1.json value for a package
Deprecate that package
Deprecate that package again with the same settings (should no-op)
I think the most ideal case would be no modifying the entity at all with the value from the downloads.v1.json and just using it for view purposes. Unfortunately, we have combined our view model and our entity model in many cases meaning it's hard to use a different download count in our app without modifying the entity.
Expected Behavior
Package deprecation should no-op instead of having an HTTP 500.
Impact
It bothers me. A fix would be nice
Describe the bug
When we read download counts from a downloads.v1.json file in Gallery and therefore have
DownloadCountObjectMaterializedInterceptorenabled, potentially unexpected entity changes can be staged in the EF context by the interceptor.https://github.com/NuGet/NuGetGallery/blob/c92f5fdf7b53392d94d967e5c53d842c79afa3e6/src/NuGetGallery/Services/DownloadCountObjectMaterializedInterceptor.cs#L46
This line of code modifies the
DownloadCountproperty on the package meaning andSaveChangeslater in the same EF context scope will update this property along with other changes.It's unclear whether leaking downloads.v1.json values into the DB slowly, and organically along with other package entity changes was intended. But what is does do is case EF context to return
HasChanges = truein more cases than the server layer code may expect making no-ops not work properly.In short, downloads.v1.json values can slowly propagate into the DB as package entities are materialized in gallery and then
SaveChanges()is called on the EF context.The same problem exists for
PackageRegistration.It is quite possible for
PackageRegistrationtotal count andPackagetotal counts in the DB to diverge in this way. In fact about 7% of package registration total download counts do not match with the sum of their version.Repro Steps
Gallery.AzureStorage.Statistics.ConnectionString)Actual behavior: HTTP 500 due to no packages being passed into this line: https://github.com/NuGet/NuGetGallery/blob/c92f5fdf7b53392d94d967e5c53d842c79afa3e6/src/NuGetGallery/Services/PackageDeprecationService.cs#L161
I think the most ideal case would be no modifying the entity at all with the value from the downloads.v1.json and just using it for view purposes. Unfortunately, we have combined our view model and our entity model in many cases meaning it's hard to use a different download count in our app without modifying the entity.
Expected Behavior
Package deprecation should no-op instead of having an HTTP 500.
Screenshots
No response
Additional Context and logs
No response