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.

Sign up for GitHub

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

Add remoterepo query param #7580

Merged
merged 2 commits into from
Oct 21, 2020
Merged

Add remoterepo query param #7580

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
daa7c54
Add remoterepo query param
agjohnson Oct 19, 2020
74a8430
Update readthedocs/api/v2/views/model_views.py
agjohnson Oct 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions readthedocs/api/v2/views/model_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,9 @@ class RemoteRepositoryViewSet(viewsets.ReadOnlyModelViewSet):

def get_queryset(self):
query = self.model.objects.api(self.request.user)
full_name = self.request.query_params.get('full_name')
if full_name is not None:
query = query.filter(full_name__icontains=full_name)
Copy link
Member

Choose a reason for hiding this comment

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

This query could be super slow. This table is huge.

It seems that we are filtering by user first, and some month ago we created an index on full_name but it would be good to check that this won't take the db down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. I'm not sure of the best way to test this though, other than looking at sql explain output or testing this in production. If you have any ideas to gauge this, it should be easy enough to run some queries directly.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't have a great idea about how to test this. However, a couple of queries on production could give us a good idea about how it will behave.

Also, I think we could enable "search as you type" under a feature flag (as a middle step for now) and if not enabled, it only filter by exact matches. Once we are sure that this is not taking down our DB, we can enable the feature to all the projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, it's integral to the design and is not optional. A feature flag won't work in this case.

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 be fine shipping this to test the performance, and we can roll back if query times look awful on user data. I feel like NR will give us the best data here.

It's also probably only going to be an issue at scale or with high throughput. It's not documented either, so stakes are pretty low for at least a test.

Seem reasonable?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds risky, 😄 --but we should be fine, or we rollback otherwise. It would be good to have a way to enable/disable this without re-deploying, tho.

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'll make a release note to test this more and watch NR, but i don't expect an issue. Manual queries in prod looked fine. It would be nice to have customer level feature flags, but honestly not much we'd use them for.

If APIv3 affords us any additional features for rate limiting, or token auth provides us with some additional security, we will eventually need this endpoint in APIv3 as well.

org = self.request.query_params.get('org', None)
if org is not None:
query = query.filter(organization__pk=org)
Expand Down