feat(bulk seer settings): Add option to append repos in bulk settings #105261
Conversation
src/sentry/seer/endpoints/organization_autofix_automation_settings.py
Outdated
Show resolved
Hide resolved
src/sentry/seer/endpoints/organization_autofix_automation_settings.py
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #105261 +/- ##
===========================================
+ Coverage 80.56% 80.58% +0.01%
===========================================
Files 9443 9430 -13
Lines 404660 404327 -333
Branches 25723 25652 -71
===========================================
- Hits 326008 325813 -195
+ Misses 78220 78046 -174
- Partials 432 468 +36 |
billyvg
left a comment
There was a problem hiding this comment.
imo, we shouldn't add this right now since there's not an immediate need for it.
| new_repos = [SeerRepoDefinition(**repo_data).dict() for repo_data in repos_data] | ||
| if append_repositories: | ||
| existing_repos = existing_pref.get("repositories") or [] | ||
| pref_update["repositories"] = merge_repositories(existing_repos, new_repos) | ||
| else: | ||
| pref_update["repositories"] = new_repos |
There was a problem hiding this comment.
Bug: When appending repositories, missing organization_id values can cause incorrect deduplication, leading to repositories being silently dropped because the field is not validated as required.
Severity: MEDIUM | Confidence: High
🔍 Detailed Analysis
When the appendRepositories option is used, the endpoint does not validate that incoming repositories have an organization_id. The RepositorySerializer allows this field to be optional, defaulting to None if omitted. The merge_repositories function uses (organization_id, provider, external_id) as a composite key for deduplication. If multiple repositories are processed with organization_id as None but with the same provider and external_id, they will be incorrectly treated as duplicates. This results in legitimate repositories being silently dropped instead of appended, leading to data loss in the configuration.
💡 Suggested Fix
Either make the organization_id field required in the RepositorySerializer or automatically populate the organization_id from the current organization's context if it is not provided in the request payload. This ensures the deduplication key is always complete.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
src/sentry/seer/endpoints/organization_autofix_automation_settings.py#L282-L287
Potential issue: When the `appendRepositories` option is used, the endpoint does not
validate that incoming repositories have an `organization_id`. The
`RepositorySerializer` allows this field to be optional, defaulting to `None` if
omitted. The `merge_repositories` function uses `(organization_id, provider,
external_id)` as a composite key for deduplication. If multiple repositories are
processed with `organization_id` as `None` but with the same `provider` and
`external_id`, they will be incorrectly treated as duplicates. This results in
legitimate repositories being silently dropped instead of appended, leading to data loss
in the configuration.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7764901
PR Details