From c5472572ec0561dacc28e572e5c9a17f4ff9b5a5 Mon Sep 17 00:00:00 2001 From: Suejung Shin Date: Mon, 21 Oct 2024 15:53:58 -0700 Subject: [PATCH 1/3] fix: Improve branch search speed --- .../branch/interactors/fetch_branches.py | 6 ++++- .../interactors/tests/test_fetch_branches.py | 27 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/core/commands/branch/interactors/fetch_branches.py b/core/commands/branch/interactors/fetch_branches.py index de94aadace..755943574d 100644 --- a/core/commands/branch/interactors/fetch_branches.py +++ b/core/commands/branch/interactors/fetch_branches.py @@ -13,7 +13,11 @@ def execute(self, repository, filters): filters = filters or {} search_value = filters.get("search_value") if search_value: - queryset = queryset.filter(name__icontains=search_value) + # force use of ILIKE to optimize search; django icontains doesn't work + # see https://github.com/codecov/engineering-team/issues/2537 + queryset = queryset.extra( + where=['"branches"."branch" ILIKE %s'], params=[f"%{search_value}%"] + ) merged = filters.get("merged_branches", False) if not merged: diff --git a/core/commands/branch/interactors/tests/test_fetch_branches.py b/core/commands/branch/interactors/tests/test_fetch_branches.py index 2d0a57d4c1..c0ae6ff6c3 100644 --- a/core/commands/branch/interactors/tests/test_fetch_branches.py +++ b/core/commands/branch/interactors/tests/test_fetch_branches.py @@ -50,3 +50,30 @@ def test_fetch_branches_unmerged(self): ) ] assert "merged" in branches + + def test_fetch_branches_filtered_by_name(self): + repository = self.repo + filters = {"search_value": "test", "merged_branches": True} + branches = async_to_sync(self.execute)(None, repository, filters) + assert not any(branch.name == "main" for branch in branches) + assert any(branch.name == "test1" for branch in branches) + assert any(branch.name == "test2" for branch in branches) + assert len(branches) == 2 + + def test_fetch_branches_filtered_by_name_no_sql_injection(self): + repository = self.repo + malicious_filters = { + "search_value": "'; DROP TABLE branches; --", + "merged_branches": True, + } + find_branches_sql_injection_attempt = async_to_sync(self.execute)( + None, repository, malicious_filters + ) + assert ( + # assert no branches found with that branch name + len(find_branches_sql_injection_attempt) == 0 + ) + + # confirm data is unaltered after sql injection attempt + find_branches = async_to_sync(self.execute)(None, repository, {}) + assert len(find_branches) == 3 From 41200045907f426e0fd8e5904fac045ad8599f68 Mon Sep 17 00:00:00 2001 From: Suejung Shin Date: Mon, 21 Oct 2024 16:12:46 -0700 Subject: [PATCH 2/3] add case insensitive test --- core/commands/branch/interactors/tests/test_fetch_branches.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/commands/branch/interactors/tests/test_fetch_branches.py b/core/commands/branch/interactors/tests/test_fetch_branches.py index c0ae6ff6c3..338c68e7a5 100644 --- a/core/commands/branch/interactors/tests/test_fetch_branches.py +++ b/core/commands/branch/interactors/tests/test_fetch_branches.py @@ -53,7 +53,7 @@ def test_fetch_branches_unmerged(self): def test_fetch_branches_filtered_by_name(self): repository = self.repo - filters = {"search_value": "test", "merged_branches": True} + filters = {"search_value": "tESt", "merged_branches": True} branches = async_to_sync(self.execute)(None, repository, filters) assert not any(branch.name == "main" for branch in branches) assert any(branch.name == "test1" for branch in branches) From 3a6c251b9178af76ca6719179ee055956e7953f0 Mon Sep 17 00:00:00 2001 From: Suejung Shin Date: Mon, 21 Oct 2024 16:24:29 -0700 Subject: [PATCH 3/3] fix types --- .../branch/interactors/fetch_branches.py | 7 +++++-- .../interactors/tests/test_fetch_branches.py | 16 +++++++++------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/core/commands/branch/interactors/fetch_branches.py b/core/commands/branch/interactors/fetch_branches.py index 755943574d..2d6976729e 100644 --- a/core/commands/branch/interactors/fetch_branches.py +++ b/core/commands/branch/interactors/fetch_branches.py @@ -1,4 +1,7 @@ -from django.db.models import OuterRef, Q, Subquery +from typing import Any + +from django.db.models import OuterRef, Q, QuerySet, Subquery +from shared.django_apps.core.models import Repository from codecov.commands.base import BaseInteractor from codecov.db import sync_to_async @@ -7,7 +10,7 @@ class FetchRepoBranchesInteractor(BaseInteractor): @sync_to_async - def execute(self, repository, filters): + def execute(self, repository: Repository, filters: dict[str, Any]) -> QuerySet: queryset = repository.branches.all() filters = filters or {} diff --git a/core/commands/branch/interactors/tests/test_fetch_branches.py b/core/commands/branch/interactors/tests/test_fetch_branches.py index 338c68e7a5..c66fbb0c91 100644 --- a/core/commands/branch/interactors/tests/test_fetch_branches.py +++ b/core/commands/branch/interactors/tests/test_fetch_branches.py @@ -1,3 +1,5 @@ +from typing import Any + from asgiref.sync import async_to_sync from django.test import TransactionTestCase from shared.django_apps.core.tests.factories import ( @@ -11,7 +13,7 @@ class FetchRepoBranchesInteractorTest(TransactionTestCase): - def setUp(self): + def setUp(self) -> None: self.org = OwnerFactory(username="codecov") self.repo = RepositoryFactory( author=self.org, name="gazebo", private=False, branch="main" @@ -23,20 +25,20 @@ def setUp(self): BranchFactory(repository=self.repo, head=self.head.commitid, name="test2"), ] - def execute(self, owner, repository, filters): + def execute(self, owner, repository, filters) -> FetchRepoBranchesInteractor: service = owner.service if owner else "github" return FetchRepoBranchesInteractor(owner, service).execute(repository, filters) - def test_fetch_branches(self): + def test_fetch_branches(self) -> None: repository = self.repo - filters = {} + filters: dict[str, Any] = {} branches = async_to_sync(self.execute)(None, repository, filters) assert any(branch.name == "main" for branch in branches) assert any(branch.name == "test1" for branch in branches) assert any(branch.name == "test2" for branch in branches) assert len(branches) == 3 - def test_fetch_branches_unmerged(self): + def test_fetch_branches_unmerged(self) -> None: merged = CommitFactory(repository=self.repo, merged=True) BranchFactory(repository=self.repo, head=merged.commitid, name="merged") branches = [ @@ -51,7 +53,7 @@ def test_fetch_branches_unmerged(self): ] assert "merged" in branches - def test_fetch_branches_filtered_by_name(self): + def test_fetch_branches_filtered_by_name(self) -> None: repository = self.repo filters = {"search_value": "tESt", "merged_branches": True} branches = async_to_sync(self.execute)(None, repository, filters) @@ -60,7 +62,7 @@ def test_fetch_branches_filtered_by_name(self): assert any(branch.name == "test2" for branch in branches) assert len(branches) == 2 - def test_fetch_branches_filtered_by_name_no_sql_injection(self): + def test_fetch_branches_filtered_by_name_no_sql_injection(self) -> None: repository = self.repo malicious_filters = { "search_value": "'; DROP TABLE branches; --",