Skip to content
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

Apply search filters on database search results #418

Merged
merged 5 commits into from
Jan 19, 2020

Conversation

tiksn
Copy link
Contributor

@tiksn tiksn commented Nov 8, 2019

Summary of the changes (in less than 80 chars)

  • separated query composition into local function
  • calling that function for initial query creation and after query changes

Addresses #417

@@ -178,32 +178,39 @@ public Task IndexAsync(Package package, CancellationToken cancellationToken)
var frameworks = GetCompatibleFrameworksOrNull(framework);
var search = (IQueryable<Package>)_context.Packages.Where(p => p.Listed);

if (!string.IsNullOrEmpty(query))
IQueryable<Package> ComposePackageQuery(IQueryable<Package> packageQuery)
{
Copy link
Owner

@loic-sharma loic-sharma Nov 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add packageQuery = packageQuery.Where(p => p.Listed); here.

This lets you simplify line 179 to:

var search = AddSearchFilters(_context.Packages);

And lines 237-239 to:

var results = await AddSearchFilters(search).ToListAsync(cancellationToken);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to change/update search before using it. Maybe someone later will add some more lines of code, and I would like to have this filtering in that search already. Please let me know if you agree with it.

if (!string.IsNullOrEmpty(query))
{
query = query.ToLower();
packageQuery = packageQuery.Where(p => p.Id.ToLower().Contains(query));
Copy link
Owner

@loic-sharma loic-sharma Nov 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need to filter on query once, could you move lines 183-187 out of ComposePackageQuery? I would make line 179 something like:

var search = AddSearchFilters(_context.Packages);
if (!string.IsNullOrEmpty(query))
{
  query = query.ToLower();
  search = search.Where(p => p.Id.ToLower().Contains(query));
}

var packageIds = ...

@loic-sharma
Copy link
Owner

Thank you for opening this! I left some minor suggestions, I'll merge this in once you've replied 😊

@loic-sharma
Copy link
Owner

loic-sharma commented Jan 18, 2020

Your changes look good. Before I merge this in, I will run the following test plan:

  1. Create and upload packages:
    1. Create a package with a single prerelease and semver2 version
    2. Create a package with a single prerelease version
    3. Create a package with a single semver2 version
    4. Create a package with a single "normal" version
    5. Create a package with versions:
      • 4.0.0-prerelease+semver2
      • 3.0.0+semver2
      • 2.0.0-prerelease
      • 1.0.0
  2. For each database, check that expected package shows up for combinations of prerelease and semver2 filter:
    • SQLite
    • PostgreSQL
    • MySQL
    • SQL Server
function Push {
    param (
        $PackageId,
        $PackageVersion
    )

    $normalizedVersion = $PackageVersion.Split('+')[0]

    & dotnet pack /p:PackageId=$PackageId /p:PackageVersion=$PackageVersion
    & dotnet nuget push "./bin/Debug/${PackageId}.${normalizedVersion}.nupkg" -s http://localhost:50561/v3/index.json 
}

Push "normal" "1.0.0"
Push "prerelease" "1.0.0-prerelease"
Push "semver2" "1.0.0+semver2"
Push "both" "1.0.0-prerelease+semver2"

Push "all" "1.0.0"
Push "all" "2.0.0-prerelease"
Push "all" "3.0.0+semver2"
Push "all" "4.0.0-prerelease+semver2"

@loic-sharma loic-sharma changed the title when query changes, original parameters need to be reapplied Apply search filters on database search results Jan 19, 2020
@loic-sharma
Copy link
Owner

Thank you for fixing this!

@loic-sharma loic-sharma merged commit dcbd51d into loic-sharma:master Jan 19, 2020
@tiksn tiksn deleted the bugfix/prerelease-search-issue branch March 3, 2020 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants