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

Fix an issue where we were not properly filtering projects #5309

Merged
merged 3 commits into from Feb 19, 2019

Conversation

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Feb 19, 2019

There were some edge cases where the public queryset method on Project’s and Version’s could return an empty list. This lead to not properly filtering the queryset in the docsearch API. Properly raise an exception in this case, so that we don’t return search results that aren’t properly filtered.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Feb 19, 2019

This PR looks to break some tests around search.

@agjohnson agjohnson added the Bug label Feb 19, 2019
Copy link
Member

@humitos humitos left a comment

Changes look good for now.

Although, we will need to find a better fix probably at self.get_all_projects to return the projects we need under different authentication circumstances or filter this list of project from corporate after checking for permissions.

@@ -65,6 +65,10 @@ def get_queryset(self):
kwargs = {'filter_by_user': False}
kwargs['projects_list'] = [p.slug for p in self.get_all_projects()]
kwargs['versions_list'] = self.request.query_params.get('version')
if not kwargs['projects_list']:
Copy link
Member

@humitos humitos Feb 19, 2019

We want two check two things here, 1) if the key exists on the dictionary and 2) if the value of the key contains something. if not kwargs.get('projects_list'): gives us these two checks

Copy link
Member Author

@ericholscher ericholscher Feb 19, 2019

We set the key 2 lines above, I don't know how it wouldn't exist.

Copy link
Member

@humitos humitos Feb 19, 2019

I got confused with the test failing, but it was a typo:

        if not kwargs['projects_list']:
            raise ValidationError("Unable to find a project to search")
>       if not kwargs['version_list']:
E       KeyError: 'version_list'

@@ -81,9 +81,9 @@ def faceted_search(
}

filters = {}
if projects_list:
if projects_list is not None:
Copy link
Member

@humitos humitos Feb 19, 2019

I think that we want to check that the value here is a list (via isinstance).

Copy link
Member Author

@ericholscher ericholscher Feb 19, 2019

I think it may also be a queryset. An empty list doesn't help us here either, so perhaps we just keep this with the existing check?

@humitos
Copy link
Member

@humitos humitos commented Feb 19, 2019

I went ahead an pushed my suggested checkings for the lists. I think that should fix the tests --feel free to revert the commit if you think that the checking is not valid.

@@ -81,9 +81,9 @@ def faceted_search(
}

filters = {}
if projects_list:
if isinstance(projects_list) and projects_list:
Copy link
Member Author

@ericholscher ericholscher Feb 19, 2019

Doesn't isinstance need 2 arguments? What is it checking for an instance of?

Copy link
Member

@humitos humitos Feb 19, 2019

🤦‍♂️

You are right.

@humitos
Copy link
Member

@humitos humitos commented Feb 19, 2019

I QAed this and I get a validation error when using the search,

["Unable to find a project to search"]
  • Corporate:

    • on a Private version accessed by a Shared Link
    • on a Public version
  • Community:

    • on any Public version

Summarizing, with this PR ES search does not work :/

I suppose that get_all_projects returns [] for non logged in users, and that's the problem.

@ericholscher
Copy link
Member Author

@ericholscher ericholscher commented Feb 19, 2019

It works fine for me locally (after fixing the variable name :D)

@@ -65,6 +65,10 @@ def get_queryset(self):
kwargs = {'filter_by_user': False}
kwargs['projects_list'] = [p.slug for p in self.get_all_projects()]
kwargs['versions_list'] = self.request.query_params.get('version')
if not kwargs.get('project_list'):
Copy link
Member Author

@ericholscher ericholscher Feb 19, 2019

Suggested change
if not kwargs.get('project_list'):
if not kwargs.get('projects_list'):

@@ -81,9 +81,9 @@ def faceted_search(
}

filters = {}
if projects_list:
if isinstance(projects_list, list) and projects_list:
Copy link
Member Author

@ericholscher ericholscher Feb 19, 2019

These feel like the wrong solution. I think we should just go back to the original logic. An empty list doesn't work when passed into ES either, so really we want to be checking for truthyness for it to be evaluated in a meaningful way.

Copy link
Member

@humitos humitos Feb 19, 2019

This checks that projects_list is effectively a list and it's not empty (and projects_list).

filters['project'] = projects_list
if versions_list:
if isinstance(versions_list, (list, str)) and versions_list:
Copy link
Member

@humitos humitos Feb 19, 2019

This one was tricky because of the _list suffix. versions_list can be a list or just a latest string, for example. I was expecting it to behave as projects_list that it's a list of one element in that case.

@humitos
Copy link
Member

@humitos humitos commented Feb 19, 2019

OK, I made a lot of mistakes on this PR. I will revert all my changes and just fix the typo that you had originally. I'm sorry for the noise here.

There were some edge cases where the `public` queryset method on Project’s and Version’s could return an empty list. This lead to not properly filtering the queryset in the docsearch API. Properly raise an exception in this case, so that we don’t return search results that aren’t properly filtered.
@humitos humitos force-pushed the fix-empty-project-lists branch from 26fc020 to b000284 Feb 19, 2019
@humitos
Copy link
Member

@humitos humitos commented Feb 19, 2019

QAed this again with the latest push (your changes plus the typo fix) and it works properly in corporate (ES does not run when accessing via Shared Link) and community.

@ericholscher ericholscher merged commit 5978ec9 into master Feb 19, 2019
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the fix-empty-project-lists branch Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants