-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
ref(auto_source_config): Multiple calls does not create new repository #87437
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #87437 +/- ##
=======================================
Coverage 87.73% 87.74%
=======================================
Files 9892 9897 +5
Lines 561283 561253 -30
Branches 22125 22112 -13
=======================================
+ Hits 492465 492479 +14
+ Misses 68416 68370 -46
- Partials 402 404 +2 |
3fa19fe
to
43daf9a
Compare
In #86597 I introduced a bug causing many [IntegrityErrors](https://sentry.sentry.io/issues/?environment=prod&groupStatsPeriod=auto&page=0&project=1&query=error.type%3AIntegrityError%20transaction%3Asentry.tasks.auto_source_code_config&referrer=issue-list&statsPeriod=90d) because we called `create` when the code mapping already existed. This change is to have a test to prevent trying to create the repository or code mappings more than once.
43daf9a
to
4e86339
Compare
if not repository: | ||
created = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm refactoring some of the logic here to make sure the metrics are called correctly.
project=project, stack_root=code_mapping.stacktrace_root | ||
) | ||
if existing_code_mappings.exists(): | ||
logger.warning("Investigate.", extra=extra) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass by change.
This has not been happening in production, thus, removing it.
continue | ||
|
||
RepositoryProjectPathConfig.objects.create( | ||
_, created = RepositoryProjectPathConfig.objects.get_or_create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing from create
to get_or_create
.
if not dry_run: | ||
repository = Repository.objects.create( | ||
repository, created = Repository.objects.get_or_create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice I'm changing from create
to get_or_create
.
@@ -91,8 +91,24 @@ def _process_and_assert_code_mapping( | |||
), | |||
patch("sentry.utils.metrics.incr") as mock_incr, | |||
): | |||
repositories_count = Repository.objects.all().count() | |||
code_mappings_count = RepositoryProjectPathConfig.objects.all().count() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two variables are used below to determining if new code mappings and repositories have been created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the one comment in your tests. Otherwise lgtm.
Just curious — were there any lasting consequences here? Or did we just get a ton of Integrity errors because we tried to create repositories that already existed?
We get the errors, but the DB was not changed. |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This fixes a bunch of integrity errors introduced in #87437. From the [docs](https://docs.djangoproject.com/en/5.1/ref/models/querysets/#get-or-create): > Any keyword arguments passed to get_or_create() — except an optional one called defaults — will be used in a get() call. If an object is found, get_or_create() returns a tuple of that object and False.
This depends on #87498 merging first.
In #86597 I introduced a bug causing many IntegrityErrors because we called
create
when the repository already existed.This change is to have a test to prevent trying to create the repository more than once.