-
Notifications
You must be signed in to change notification settings - Fork 644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SemVer2] Filter SemVer v2.0.0 package versions by default on v1 and v2 OData endpoints #3694
Conversation
7a1e93d
to
50017b4
Compare
5722604
to
ad99c7c
Compare
@joelverhagen @scottbommarito @ryuyu need more squirrels :) |
// Assert | ||
foreach (var feedPackage in resultSet) | ||
{ | ||
// Assert none of the items in the result set are SemVer v.2.0.0 packages (checking on original version is enough in this case) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this code repeated in many test cases. You could easily reduce this by creating a helper function that takes in a TFeedPackage
...
public void AssertResultCorrect(TFeedPackage feedPackage)
{
// Assert none of the items in the result set are SemVer v.2.0.0 packages (checking on original version is enough in this case)
Assert.Empty(SemVer2Packages.Where(p => string.Equals(p.Version, feedPackage.Version)));
// Assert each of the items in the result set is a non-SemVer v2.0.0 package
Assert.Single(NonSemVer2Packages.Where(p =>
string.Equals(p.Version, feedPackage.Version) &&
string.Equals(p.PackageRegistration.Id, feedPackage.Id)));
}
[Fact]
public async Task Get_FiltersSemVerV2PackageVersions()
{
// Act
var resultSet = await GetCollection<V1FeedPackage>(
(controller, options) => controller.Get(options),
"/api/v1/Packages");
// Assert
resultSet.Select(AssertResultCorrect);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, missed that one during clean up :) Addressed with 6be4a72
@@ -50,7 +49,7 @@ public IHttpActionResult Get(ODataQueryOptions<V1FeedPackage> options) | |||
return BadRequest(ODataQueryVerifier.GetValidationFailedMessage(options)); | |||
} | |||
var queryable = _packagesRepository.GetAll() | |||
.Where(p => !p.IsPrerelease && !p.Deleted) | |||
.Where(p => !p.IsPrerelease && !p.Deleted && p.SemVerLevelKey == SemVerLevelKey.Unknown) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish there was an easier way to add conditions here rather than having to change every place where we query the packages repository.
Do you think we could extract these checks out to a function?
public static bool ShouldShowPackage(V1FeedPackage package, bool showUnlisted)
{
return !package.IsPrerelease && !package.Deleted && package.SemVerLevelKey == SemVerLevelKey.Unknown && (showUnlisted || package.IsListed);
}
You could add this as a method in the base class (NuGetODataController
) and use it in ODataV2FeedController
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about consolidating this somehow too, however, the logic is not the same for v1 and v2. E.g. v1 also filters out pre-release packages from the feed.
This PR is NOT about adding support for the new
semVerLevel
parameter... yet! Not on the gallery endpoints, and not on the calls to the search service. That work is tracked by #3557.This PR is about filtering out all SemVer v2.0.0 package versions by default on all requests to v1 and v2 endpoints.
These changes should be all that is needed to ensure default filtering behavior to avoid client-side issues with clients that do not support the NuGet SemVer 2.0.0 protocol (i.e. clients that don't send the
semVerLevel=2.0.0
query string parameter)