Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: fetch accurate branch head
Browse files Browse the repository at this point in the history
This commit is a fix for the recent issue where
branch heads were all set to the same value each
time they were updated due to a bug in the Branch
django model.

This fix creates a helper function that checks for the 2
remaining innacurate commit shas that a large number of
branches were set to. If the current branch head is set to
one of those shas, we update the branch head by fetching
the latest commit on that branch from the db.

Note that we must use a raw sql query here to update the
branch object because using the django ORM to update it
would lead to the same issue we are trying to fix.

Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
joseph-sentry committed Jan 23, 2024
1 parent c55a1f4 commit cc2e320
Showing 6 changed files with 78 additions and 7 deletions.
10 changes: 9 additions & 1 deletion api/internal/coverage/views.py
Original file line number Diff line number Diff line change
@@ -8,6 +8,7 @@
from api.shared.permissions import RepositoryArtifactPermissions
from api.shared.report.serializers import TreeSerializer
from services.path import ReportPaths
from utils.temp_branch_fix import get_or_update_branch_head


class CoverageViewSet(viewsets.ViewSet, RepoPropertyMixin):
@@ -23,7 +24,14 @@ def get_object(self):
f"The branch '{branch_name}' in not in our records. Please provide a valid branch name.",
404,
)
commit_sha = branch.head
commit_sha = get_or_update_branch_head(
self.repo.commits, branch, self.repo.repoid
)
if commit_sha is None:
raise NotFound(

Check warning on line 31 in api/internal/coverage/views.py

Codecov / codecov/patch

api/internal/coverage/views.py#L31

Added line #L31 was not covered by tests

Check warning on line 31 in api/internal/coverage/views.py

Codecov Public QA / codecov/patch

api/internal/coverage/views.py#L31

Added line #L31 was not covered by tests

Check warning on line 31 in api/internal/coverage/views.py

Codecov - Staging / codecov/patch

api/internal/coverage/views.py#L31

Added line #L31 was not covered by tests

Check warning on line 31 in api/internal/coverage/views.py

Codecov - QA / codecov/patch

api/internal/coverage/views.py#L31

Added line #L31 was not covered by tests
f"The head of this branch '{branch_name}' is not in our records. Please specify a valid branch name.",
404,
)

commit = self.repo.commits.filter(commitid=commit_sha).first()
if commit is None:
3 changes: 3 additions & 0 deletions api/public/v2/branch/serializers.py
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@

from api.public.v2.commit.serializers import CommitDetailSerializer
from core.models import Branch, Commit
from utils.temp_branch_fix import get_or_update_branch_head


class BranchSerializer(serializers.ModelSerializer):
@@ -19,6 +20,8 @@ class BranchDetailSerializer(BranchSerializer):
)

def get_head_commit(self, branch: Branch) -> CommitDetailSerializer:
_ = get_or_update_branch_head(Commit.objects, branch, branch.repository_id)

commit = (
Commit.objects.filter(
repository_id=branch.repository_id, commitid=branch.head
14 changes: 13 additions & 1 deletion api/shared/mixins.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from typing import Optional

from django.conf import settings
from django.db import connection
from django.http import Http404
from django.shortcuts import get_object_or_404
from django.utils.functional import cached_property
@@ -19,6 +20,7 @@
from codecov_auth.models import Owner, Service
from core.models import Commit, Repository
from utils.services import get_long_service_name
from utils.temp_branch_fix import get_or_update_branch_head


class OwnerPropertyMixin:
@@ -50,7 +52,17 @@ def get_commit(self, commit_sha: Optional[str] = None) -> Commit:
f"The branch '{branch_name}' in not in our records. Please provide a valid branch name.",
404,
)
commit_sha = branch.head

commit_sha = get_or_update_branch_head(
self.repo.commits,
branch,
self.repo.repoid,
)
if commit_sha is None:
raise NotFound(

Check warning on line 62 in api/shared/mixins.py

Codecov / codecov/patch

api/shared/mixins.py#L62

Added line #L62 was not covered by tests

Check warning on line 62 in api/shared/mixins.py

Codecov Public QA / codecov/patch

api/shared/mixins.py#L62

Added line #L62 was not covered by tests

Check warning on line 62 in api/shared/mixins.py

Codecov - Staging / codecov/patch

api/shared/mixins.py#L62

Added line #L62 was not covered by tests

Check warning on line 62 in api/shared/mixins.py

Codecov - QA / codecov/patch

api/shared/mixins.py#L62

Added line #L62 was not covered by tests
f"The head of this branch '{branch_name}' is not in our records. Please specify a valid branch name.",
404,
)

commit = self.repo.commits.filter(commitid=commit_sha).first()
if commit is None:
11 changes: 8 additions & 3 deletions graphql_api/types/branch/branch.py
Original file line number Diff line number Diff line change
@@ -4,17 +4,22 @@

from core.models import Branch, Commit
from graphql_api.dataloader.commit import CommitLoader
from utils.temp_branch_fix import get_or_update_branch_head

branch_bindable = ObjectType("Branch")


@branch_bindable.field("headSha")
def resolve_head_sha(branch: Branch, info) -> str:
return branch.head
head = get_or_update_branch_head(Commit.objects, branch, branch.repository_id)
if head is None:
return ""

Check warning on line 16 in graphql_api/types/branch/branch.py

Codecov / codecov/patch

graphql_api/types/branch/branch.py#L16

Added line #L16 was not covered by tests

Check warning on line 16 in graphql_api/types/branch/branch.py

Codecov Public QA / codecov/patch

graphql_api/types/branch/branch.py#L16

Added line #L16 was not covered by tests

Check warning on line 16 in graphql_api/types/branch/branch.py

Codecov - Staging / codecov/patch

graphql_api/types/branch/branch.py#L16

Added line #L16 was not covered by tests

Check warning on line 16 in graphql_api/types/branch/branch.py

Codecov - QA / codecov/patch

graphql_api/types/branch/branch.py#L16

Added line #L16 was not covered by tests
return head


@branch_bindable.field("head")
def resolve_head_commit(branch: Branch, info) -> Optional[Commit]:
if branch.head:
head = get_or_update_branch_head(Commit.objects, branch, branch.repository_id)
if head:
loader = CommitLoader.loader(info, branch.repository_id)
return loader.load(branch.head)
return loader.load(head)
9 changes: 7 additions & 2 deletions graphs/views.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import logging

from django.core.exceptions import ObjectDoesNotExist
from django.db import connection
from django.http import Http404
from rest_framework import exceptions
from rest_framework.exceptions import NotFound
@@ -13,6 +14,7 @@
from api.shared.mixins import RepoPropertyMixin
from core.models import Branch, Pull
from graphs.settings import settings
from utils.temp_branch_fix import get_or_update_branch_head

from .helpers.badge import format_coverage_precision, get_badge
from .helpers.graphs import icicle, sunburst, tree
@@ -42,7 +44,6 @@ def select_renderer(self, request, renderers, format_suffix):


class BadgeHandler(APIView, RepoPropertyMixin, GraphBadgeAPIMixin):

content_negotiation_class = IgnoreClientContentNegotiation

permission_classes = [AllowAny]
@@ -100,7 +101,8 @@ def get_coverage(self):
)
return None, coverage_range
try:
commit = repo.commits.get(commitid=branch.head)
_ = get_or_update_branch_head(repo.commits, branch, repo.repoid)
commit = repo.commits.filter(commitid=branch.head).first()
except ObjectDoesNotExist:
# if commit does not exist return None coverage
log.warning("Commit not found", extra=dict(commit=branch.head))
@@ -246,6 +248,9 @@ def get_commit(self):
if branch is None:
return None

branch_head = get_or_update_branch_head(repo.commits, branch, repo.repoid)
if branch_head is None:
return None

Check warning on line 253 in graphs/views.py

Codecov / codecov/patch

graphs/views.py#L253

Added line #L253 was not covered by tests

Check warning on line 253 in graphs/views.py

Codecov Public QA / codecov/patch

graphs/views.py#L253

Added line #L253 was not covered by tests

Check warning on line 253 in graphs/views.py

Codecov - Staging / codecov/patch

graphs/views.py#L253

Added line #L253 was not covered by tests

Check warning on line 253 in graphs/views.py

Codecov - QA / codecov/patch

graphs/views.py#L253

Added line #L253 was not covered by tests
commit = repo.commits.filter(commitid=branch.head).first()

return commit
38 changes: 38 additions & 0 deletions utils/temp_branch_fix.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
from django.db import connection


def get_or_update_branch_head(
commits,
branch,
repoid,
):
# there was a bug that set a large number of branch heads to these two shas, so we are rectifying that issue here
if (
branch.head == "81c2b4fa3ae9ef615c8f740c5cba95d9851f9ae8s"
or branch.head == "9587100eacc554aa9c03422e28b269c551dc1a72"
):
commit = (

Check warning on line 14 in utils/temp_branch_fix.py

Codecov / codecov/patch

utils/temp_branch_fix.py#L14

Added line #L14 was not covered by tests

Check warning on line 14 in utils/temp_branch_fix.py

Codecov Public QA / codecov/patch

utils/temp_branch_fix.py#L14

Added line #L14 was not covered by tests

Check warning on line 14 in utils/temp_branch_fix.py

Codecov - Staging / codecov/patch

utils/temp_branch_fix.py#L14

Added line #L14 was not covered by tests

Check warning on line 14 in utils/temp_branch_fix.py

Codecov - QA / codecov/patch

utils/temp_branch_fix.py#L14

Added line #L14 was not covered by tests
commits.filter(branch=branch.name, repoid=repoid)
.defer("_report")
.order_by("-updatestamp")
.first()
)

if commit is None:
return None

Check warning on line 22 in utils/temp_branch_fix.py

Codecov / codecov/patch

utils/temp_branch_fix.py#L21-L22

Added lines #L21 - L22 were not covered by tests

Check warning on line 22 in utils/temp_branch_fix.py

Codecov Public QA / codecov/patch

utils/temp_branch_fix.py#L21-L22

Added lines #L21 - L22 were not covered by tests

Check warning on line 22 in utils/temp_branch_fix.py

Codecov - Staging / codecov/patch

utils/temp_branch_fix.py#L21-L22

Added lines #L21 - L22 were not covered by tests

Check warning on line 22 in utils/temp_branch_fix.py

Codecov - QA / codecov/patch

utils/temp_branch_fix.py#L21-L22

Added lines #L21 - L22 were not covered by tests

# using this raw sql because the current branches table does not allow for updating based on repoid
# it only updates based on branch name which means if we were to use the django orm to update this
# branch.head, all branches with the same name as the one we are trying to update would have their head
# updated to the value of this ones head
with connection.cursor() as cursor:
cursor.execute(

Check warning on line 29 in utils/temp_branch_fix.py

Codecov / codecov/patch

utils/temp_branch_fix.py#L28-L29

Added lines #L28 - L29 were not covered by tests

Check warning on line 29 in utils/temp_branch_fix.py

Codecov Public QA / codecov/patch

utils/temp_branch_fix.py#L28-L29

Added lines #L28 - L29 were not covered by tests

Check warning on line 29 in utils/temp_branch_fix.py

Codecov - Staging / codecov/patch

utils/temp_branch_fix.py#L28-L29

Added lines #L28 - L29 were not covered by tests

Check warning on line 29 in utils/temp_branch_fix.py

Codecov - QA / codecov/patch

utils/temp_branch_fix.py#L28-L29

Added lines #L28 - L29 were not covered by tests
"UPDATE branches SET head = %s WHERE branches.repoid = %s AND branches.branch = %s",
[commit.commitid, repoid, branch.name],
)

commit_sha = commit.commitid
return commit_sha

Check warning on line 35 in utils/temp_branch_fix.py

Codecov / codecov/patch

utils/temp_branch_fix.py#L34-L35

Added lines #L34 - L35 were not covered by tests

Check warning on line 35 in utils/temp_branch_fix.py

Codecov Public QA / codecov/patch

utils/temp_branch_fix.py#L34-L35

Added lines #L34 - L35 were not covered by tests

Check warning on line 35 in utils/temp_branch_fix.py

Codecov - Staging / codecov/patch

utils/temp_branch_fix.py#L34-L35

Added lines #L34 - L35 were not covered by tests

Check warning on line 35 in utils/temp_branch_fix.py

Codecov - QA / codecov/patch

utils/temp_branch_fix.py#L34-L35

Added lines #L34 - L35 were not covered by tests

else:
return branch.head

0 comments on commit cc2e320

Please sign in to comment.