fix(code-review): add date_updated to update_fields#107129
Conversation
| 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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The issue is well-explained by this comment which I overlooked #106977 (comment)
followup to #106977
Add
"date_updated"toupdate_fieldswhen bulk creating repo settings. Otherwise the value will not update.