Skip to content

fix(code-review): add date_updated to update_fields#107129

Merged
srest2021 merged 1 commit into
masterfrom
srest2021/fix-date-updated-in-bulk-create
Jan 28, 2026
Merged

fix(code-review): add date_updated to update_fields#107129
srest2021 merged 1 commit into
masterfrom
srest2021/fix-date-updated-in-bulk-create

Conversation

@srest2021

Copy link
Copy Markdown
Member

followup to #106977

Add "date_updated" to update_fields when bulk creating repo settings. Otherwise the value will not update.

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 27, 2026
@srest2021 srest2021 marked this pull request as ready for review January 28, 2026 00:17
@srest2021 srest2021 requested a review from a team as a code owner January 28, 2026 00:17
Comment on lines +70 to 73
update_fields = ["date_updated"]
if updated_enabled_code_review is not None:
update_fields.append("enabled_code_review")
if updated_code_review_triggers is not None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: Adding date_updated to update_fields in bulk_create without explicitly setting its value will not update the timestamp due to how Django handles auto_now fields on conflict.
Severity: MEDIUM

Suggested Fix

Before appending each RepositorySettings object to the settings_to_upsert list, explicitly set its date_updated attribute to the current time. For example: setting.date_updated = timezone.now().

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/integrations/api/endpoints/organization_repository_settings.py#L70-L73

Potential issue: The code adds `"date_updated"` to the `update_fields` list for a
`RepositorySettings.objects.bulk_create` call with `update_conflicts=True`. However, it
does not set a value for `date_updated` on the model instances. Django's `auto_now=True`
mechanism on a `DateTimeField` does not automatically update the timestamp during an
`INSERT ... ON CONFLICT DO UPDATE` operation when the field is specified in
`update_fields`. As a result, the `date_updated` field will either be set to `None` for
new records or retain its old value for existing records, instead of being updated to
the current time. This leads to incorrect timestamp data for repository settings
modifications.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's true that doing a bulk create will not hit save() and therefore auto_now on date_updated won't run. However, when we add date_updated to update_fields without manually setting it to anything, it'll get set to db_default=Now() and override the old value thanks to update_conflicts=True.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The issue is well-explained by this comment which I overlooked #106977 (comment)

@srest2021 srest2021 merged commit e62c6ce into master Jan 28, 2026
67 checks passed
@srest2021 srest2021 deleted the srest2021/fix-date-updated-in-bulk-create branch January 28, 2026 01:01
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 12, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants