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: use update_or_create instead of get_or_create depending on the webhook response #364

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

ajay-sentry
Copy link
Contributor

@ajay-sentry ajay-sentry commented Sep 19, 2024

This PR aims to fix these related sentry issues:

https://codecov.sentry.io/issues/5334155312
https://codecov.sentry.io/issues/5409774549
https://codecov.sentry.io/issues/5215132965
https://codecov.sentry.io/issues/5820085486
https://codecov.sentry.io/issues/5441235127

That seem to all be related to a unique_constraint error when trying to create this object when a similar one already exists. Thanks @adrian-codecov for talking me through this one

Unique constraint comes from here:

constraints = [
models.UniqueConstraint(fields=["author", "name"], name="repos_slug"),
models.UniqueConstraint(
fields=["author", "service_id"], name="repos_service_ids"
),
]
verbose_name_plural = "Repositories"

Related django documentation: https://docs.djangoproject.com/en/5.1/ref/models/querysets/#update-or-create

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

@@ -284,7 +285,7 @@ def with_oldest_commit_at(self):
def get_or_create_from_git_repo(self, git_repo, owner):
from shared.django_apps.codecov_auth.models import Owner

repo, created = self.get_or_create(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how this fixes it, so the unique constraints are the triplet of service_id, name, and author right? And I'm guessing the initial error happens when get_or_create is called with the same above triplet but different branch or private values. It doesn't recognize these 5 args being in the DB so it tries to create a new one, but failing from the unique constraints. This is what's currently happening right?

Now with update_or_create, similarly it doesn't find these 5 args in the DB so it won't update it would do create leading to the same error no?

Copy link
Contributor

@JerrySentry JerrySentry Sep 20, 2024

Choose a reason for hiding this comment

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

I'm thinking the way to do it is one of these 2:

  1. Ignore changes on subsequent changes to branch and private columns
repo, created = self.get_or_create(
    author=owner,
    service_id=git_repo.get("service_id") or git_repo.get("id"),
    name=git_repo["name"],
    defaults={
        "branch": git_repo.get("branch") or git_repo.get("default_branch") or "main",
        "private": git_repo["private"],
    }
)
  1. Updates changes on subsequent changes to branch and private columns
repo, created = self.update_or_create(
    author=owner,
    service_id=git_repo.get("service_id") or git_repo.get("id"),
    name=git_repo["name"],
    defaults={
        "branch": git_repo.get("branch") or git_repo.get("default_branch") or "main",
        "private": git_repo["private"],
    }
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually that's not gonna work because the unique constraints are 2 pairs and get_or_update is 1 triplet. So if you had in the DB (author=codecov, service_id=1, name=gazebo)
and call with params:
(author=codecov, service_id=2, name=gazebo)
a unique constraint error would occur as well because (author, name) constraint fail.

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.12%. Comparing base (c9d8b87) to head (2492c71).
Report is 1 commits behind head on main.

Current head 2492c71 differs from pull request most recent head ac60e53

Please upload reports for the commit ac60e53 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #364      +/-   ##
==========================================
- Coverage   89.75%   89.12%   -0.64%     
==========================================
  Files         376      324      -52     
  Lines       11660     9697    -1963     
  Branches     2065     1735     -330     
==========================================
- Hits        10465     8642    -1823     
+ Misses       1106      991     -115     
+ Partials       89       64      -25     
Flag Coverage Δ
shared-docker-uploader ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JerrySentry
JerrySentry previously approved these changes Sep 23, 2024
@ajay-sentry ajay-sentry added this pull request to the merge queue Sep 23, 2024
Merged via the queue into main with commit 6742804 Sep 23, 2024
6 checks passed
@ajay-sentry ajay-sentry deleted the Ajay/fix-integrity-error-git-repos branch September 23, 2024 20:56
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