[NuGet.org Bug]: DownloadCountObjectMaterializedInterceptor modifies packages in EF context

[joelverhagen]

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 DownloadCountObjectMaterializedInterceptor enabled, potentially unexpected entity changes can be staged in the EF context by the interceptor.

NuGetGallery/src/NuGetGallery/Services/DownloadCountObjectMaterializedInterceptor.cs

Line 46 in c92f5fd

package.DownloadCount = downloadCount;

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

1. Enable downloads.v1.json integration in gallery (via Gallery.AzureStorage.Statistics.ConnectionString)
2. Make the DB value different from the downloads.v1.json value for a package
3. Deprecate that package
4. Deprecate that package again with the same settings (should no-op)

Actual behavior: HTTP 500 due to no packages being passed into this line:

NuGetGallery/src/NuGetGallery/Services/PackageDeprecationService.cs

Line 161 in c92f5fd

await _packageUpdateService.UpdatePackagesAsync(changedPackages);

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