Fixing the vulnerability order inside the dictionary cache

[sipmann]

Fixes #8703
I'm not sure how much this will impact the performance. Do we have an environment to do any kind of performance test?

I've tried to put the order inside the query but without success.
Accept any tips on this ❤

[loic-sharma]

Thanks for starting this! The PackageVulnerabilitiesCacheService loads all vulnerabilities into memory using a massive Entity Framework query. This query is already known to be quite slow, hence the caching. It may be better to avoid further complicating this query to avoid performance regressions.

This vulnerabilities information is later passed by the PackagesController to the factory that creates the view model. What do you think of sorting the vulnerabilities inside of this view model factory? The drawback I see is that we will do redundant sorting, however, this should be acceptable as most packages don't have vulnerabilities.

[sipmann]

Sounds good to me. I'll do the change ;)

[sipmann]

Do you think this needs a test @loic-sharma ?

[loic-sharma]

Yup please add unit tests. It looks like we have a test gap currently, but you should be able to mirror the test on the deprecations here:

NuGetGallery/tests/NuGetGallery.Facts/ViewModels/DisplayPackageViewModelFacts.cs

Lines 843 to 925 in 1433c81

	[Theory]
	[MemberData(nameof(DeprecationFieldsAreSetAsExpected_Data))]
	public void DeprecationFieldsAreSetAsExpected(
	PackageDeprecationStatus status,
	bool hasAlternateRegistration,
	bool hasAlternatePackage)
	{
	// Arrange
	var deprecation = new PackageDeprecation
	{
	Status = status,
	CustomMessage = "hello",
	};
	var alternateRegistrationId = "alternateRegistrationId";
	if (hasAlternateRegistration)
	{
	var registration = new PackageRegistration
	{
	Id = alternateRegistrationId
	};
	deprecation.AlternatePackageRegistration = registration;
	}
	var alternatePackageRegistrationId = "alternatePackageRegistration";
	var alternatePackageVersion = "1.0.0-alt";
	if (hasAlternatePackage)
	{
	var alternatePackageRegistration = new PackageRegistration
	{
	Id = alternatePackageRegistrationId
	};
	var alternatePackage = new Package
	{
	Version = alternatePackageVersion,
	PackageRegistration = alternatePackageRegistration
	};
	deprecation.AlternatePackage = alternatePackage;
	}
	var package = CreateTestPackage("1.0.0");
	var packageKeyToDeprecation = new Dictionary<int, PackageDeprecation>
	{
	{ 123, deprecation }
	};
	// Act
	var model = CreateDisplayPackageViewModel(
	package,
	currentUser: null,
	packageKeyToDeprecation: packageKeyToDeprecation,
	readmeHtml: null);
	// Assert
	Assert.Equal(status, model.DeprecationStatus);
	Assert.Equal(deprecation.CustomMessage, model.CustomMessage);
	if (hasAlternatePackage)
	{
	Assert.Equal(alternatePackageRegistrationId, model.AlternatePackageId);
	Assert.Equal(alternatePackageVersion, model.AlternatePackageVersion);
	}
	else if (hasAlternateRegistration)
	{
	Assert.Equal(alternateRegistrationId, model.AlternatePackageId);
	Assert.Null(model.AlternatePackageVersion);
	}
	else
	{
	Assert.Null(model.AlternatePackageId);
	Assert.Null(model.AlternatePackageVersion);
	}
	var versionModel = model.PackageVersions.Single();
	Assert.Equal(status, versionModel.DeprecationStatus);
	Assert.Null(versionModel.AlternatePackageId);
	Assert.Null(versionModel.AlternatePackageVersion);
	Assert.Null(versionModel.CustomMessage);
	}

A test like "arrange 3 vulnerabilities with different severities out-of-order and assert the resulting model's vulnerabilities are in order" should be perfect. Let us know if you have questions!

[dnfgituser]

All CLA requirements met.

[loic-sharma]

I'm worried someone could break this logic if they re-order the PackageVulnerabilitySeverity enum. To prevent this mistake, we could manually verify the order here:

Suggested change

	Assert.Equal(model.Vulnerabilities, packageKeyToVulnerabilities[package.Key].OrderByDescending(p => p.Severity).ToList().AsReadOnly());
	Assert.Equal(PackageVulnerabilitySeverity.Critical, model.Vulnerabilities[0].Severity);
	Assert.Equal(PackageVulnerabilitySeverity.High, model.Vulnerabilities[1].Severity);
	Assert.Equal(PackageVulnerabilitySeverity.Low, model.Vulnerabilities[2].Severity);

[sipmann]

For sure :)

[joelverhagen]

@agr (on-call) or @loic-sharma (point of contact), could you shepherd this PR into a merged state?

[loic-sharma]

@sipmann Thanks for the contribution! This will be included in our next nuget.org deployment. You can track our progress using this work item: #8703

@joelverhagen Yup can do.