UN-3430 [FIX] Update modified_at field correctly for models#1928
Merged
chandrasekharan-zipstack merged 6 commits intomainfrom Apr 24, 2026
Merged
UN-3430 [FIX] Update modified_at field correctly for models#1928chandrasekharan-zipstack merged 6 commits intomainfrom
chandrasekharan-zipstack merged 6 commits intomainfrom
Conversation
Django's auto_now=True only fires on Model.save(); QuerySet.update() and bulk_update() bypass save(), so BaseModel.modified_at silently stayed at the creation time for every bulk-path write. Audit trail drifted. Introduce BaseModelQuerySet that injects modified_at=timezone.now() into both paths, and expose it via BaseModelManager. Migrate all custom managers on BaseModel subclasses to compose BaseModelManager so their querysets inherit the overrides. Drop the ad-hoc modified_at=now() kwarg in FileHistoryHelper now that the queryset handles it.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a BaseModelQuerySet/Manager that enforces updating/setting modified_at on partial updates/creates; many model managers were switched to inherit BaseModelManager; code that previously set modified_at explicitly in bulk/atomic flows was removed and some bulk upserts no longer include modified_at in conflict update_fields. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chandrasekharan-zipstack
changed the title
chandrasekharan-zipstack
requested review from
Deepak-Kesavan,
muhammad-ali-e and
pk-zipstack
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/utils/tests/test_base_model.py (1)
28-32: Document the intentionally empty__init__to silence SonarCloud.The empty
__init__(and the__new__override) is deliberate — it bypassesmodels.QuerySet.__init__so the overrides can be exercised without a DB-bound queryset. A short comment makes this explicit and clears the static-analysis finding flagged on line 31.📝 Suggested tweak
def __new__(cls): return object.__new__(cls) - def __init__(self): - pass + def __init__(self): + # Intentionally skip models.QuerySet.__init__ — tests only exercise + # update()/bulk_update() overrides and don't need a real query state. + pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/utils/tests/test_base_model.py` around lines 28 - 32, Add a brief explanatory comment to the intentionally empty __init__ in the test class to silence SonarCloud: explain that __new__ is overridden to bypass models.QuerySet.__init__ so the test can exercise overrides without a DB-bound queryset, and that __init__ is left empty on purpose for that reason (place the comment directly above the __init__ method).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/utils/models/base_model.py`:
- Around line 22-29: The bulk_update implementation currently iterates objs
which can exhaust a generator; materialize objs (e.g., convert to tuple or list)
at the start of base_model.bulk_update before modifying objects so the same
sequence is passed into super().bulk_update; keep the existing modified_at
update logic (assign now and append "modified_at" to fields when missing) and
then call super().bulk_update with the materialized objs and fields (do not add
early returns for None/empty fields).
---
Nitpick comments:
In `@backend/utils/tests/test_base_model.py`:
- Around line 28-32: Add a brief explanatory comment to the intentionally empty
__init__ in the test class to silence SonarCloud: explain that __new__ is
overridden to bypass models.QuerySet.__init__ so the test can exercise overrides
without a DB-bound queryset, and that __init__ is left empty on purpose for that
reason (place the comment directly above the __init__ method).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f48acecf-01a3-448c-ae31-66775efa1e8e
📒 Files selected for processing (18)
backend/adapter_processor_v2/models.pybackend/api_v2/models.pybackend/connector_v2/models.pybackend/dashboard_metrics/models.pybackend/pipeline_v2/models.pybackend/prompt_studio/prompt_studio_core_v2/models.pybackend/prompt_studio/prompt_studio_registry_v2/models.pybackend/tags/models.pybackend/tool_instance_v2/models.pybackend/usage_v2/models.pybackend/utils/models/base_model.pybackend/utils/models/org_aware_manager.pybackend/utils/tests/test_base_model.pybackend/workflow_manager/endpoint_v2/models.pybackend/workflow_manager/file_execution/models.pybackend/workflow_manager/workflow_v2/file_history_helper.pybackend/workflow_manager/workflow_v2/models/execution.pybackend/workflow_manager/workflow_v2/models/workflow.py
💤 Files with no reviewable changes (1)
- backend/workflow_manager/workflow_v2/file_history_helper.py
Contributor
|
| Filename | Overview |
|---|---|
| backend/utils/models/base_model.py | Core of this PR — introduces BaseModelQuerySet with update(), bulk_update(), and bulk_create() overrides that inject modified_at, and overrides BaseModel.save() to handle update_fields. Implementation is correct; generator exhaustion is already addressed with objs = list(objs). |
| backend/dashboard_metrics/tasks.py | Removes modified_at from update_fields on three _base_manager.bulk_create(update_conflicts=True) calls. Because _base_manager is a plain models.Manager (not BaseModelQuerySet), the injection override never fires — this is a regression that silently drops modified_at on upsert-on-conflict paths. |
| backend/dashboard_metrics/management/commands/backfill_metrics.py | Same _base_manager.bulk_create regression as tasks.py — modified_at removed from update_fields in all three upsert helpers while _base_manager bypasses BaseModelQuerySet.bulk_create(). |
| backend/utils/models/org_aware_manager.py | Switches OrgAwareManager base from models.Manager to BaseModelManager, correctly propagating BaseModelQuerySet through all OrgAwareManager subclasses. |
| backend/workflow_manager/workflow_v2/file_history_helper.py | Removes redundant modified_at=timezone.now() from FileHistory.objects.filter(...).update(...). Since FileHistory.objects is now a BaseModelQuerySet, the override injects it via setdefault — safe removal. |
| backend/workflow_manager/internal_views.py | Removes execution.modified_at = timezone.now() before execution.save() — correct because auto_now handles it for full saves. |
| backend/workflow_manager/workflow_v2/views.py | Removes the no-op execution.modified_at = timezone.now() assignment before .save() — correct cleanup. |
| backend/dashboard_metrics/models.py | Updates three metric managers to extend BaseModelManager. Correct for objects-accessed paths, but does not affect the _base_manager regression in tasks.py/backfill_metrics.py. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Write path] --> B{Which method?}
B -->|".update(**kwargs)"| C["BaseModelQuerySet.update()\nkwargs.setdefault('modified_at', now())"]
B -->|".bulk_update(objs, fields)"| D{"'modified_at' in fields?"}
D -->|No| E["Materialize objs = list(objs)\nStamp obj.modified_at = now()\nAppend 'modified_at' to fields"]
D -->|Yes| F["Pass through unchanged"]
E --> G["super().bulk_update()"]
F --> G
B -->|"bulk_create(update_conflicts=True)"| H{"update_fields provided?"}
H -->|Yes| I["_with_modified_at(update_fields)\nAppend 'modified_at' if absent"]
H -->|No| J["Pass through unchanged"]
I --> K["super().bulk_create()"]
J --> K
B -->|".save(update_fields=[...])"| L{"update_fields truthy?"}
L -->|Yes| M["_with_modified_at(update_fields)\nAppend 'modified_at' if absent"]
L -->|No / empty list| N["Pass through — Django no-op semantics"]
M --> O["super().save()"]
N --> O
C --> P["super().update() — raw SQL with modified_at"]
style E fill:#d4edda
style I fill:#d4edda
style M fill:#d4edda
style C fill:#d4edda
subgraph bypass["⚠️ Bypasses BaseModelQuerySet"]
Q["Model._base_manager.bulk_create()\n(tasks.py / backfill_metrics.py)"]
R["Plain models.QuerySet — no injection"]
Q --> R
end
Comments Outside Diff (1)
-
backend/dashboard_metrics/tasks.py, line 120-125 (link)_base_managerbypassesBaseModelQuerySet, silently dropsmodified_aton conflictEventMetricsHourly._base_managerresolves to an auto-created plainmodels.Manager— notBaseModelManager. Django's base manager selection falls back to a plain manager whenever the model's only custom manager (EventMetricsHourlyManager) extendsDefaultOrganizationManagerMixin, whoseget_queryset()always calls.filter(organization=organization), making it a filtering manager that Django deliberately excludes from_base_manager.Because
_base_managerreturns a stockmodels.QuerySet, theBaseModelQuerySet.bulk_create()override that injectsmodified_atis never invoked. Removing"modified_at"fromupdate_fieldshere (and in_bulk_upsert_daily,_bulk_upsert_monthly, and all three equivalents inbackfill_metrics.py) is therefore a regression: the conflict-update path will no longer writemodified_at, reverting to the exact behavior this PR set out to fix.The safe fix is to put
"modified_at"back in each of theseupdate_fieldslists:update_fields=["metric_type", "metric_value", "metric_count", "modified_at"],
Prompt To Fix With AI
This is a comment left during a code review. Path: backend/dashboard_metrics/tasks.py Line: 120-125 Comment: **`_base_manager` bypasses `BaseModelQuerySet`, silently drops `modified_at` on conflict** `EventMetricsHourly._base_manager` resolves to an auto-created plain `models.Manager` — not `BaseModelManager`. Django's base manager selection falls back to a plain manager whenever the model's only custom manager (`EventMetricsHourlyManager`) extends `DefaultOrganizationManagerMixin`, whose `get_queryset()` always calls `.filter(organization=organization)`, making it a filtering manager that Django deliberately excludes from `_base_manager`. Because `_base_manager` returns a stock `models.QuerySet`, the `BaseModelQuerySet.bulk_create()` override that injects `modified_at` is never invoked. Removing `"modified_at"` from `update_fields` here (and in `_bulk_upsert_daily`, `_bulk_upsert_monthly`, and all three equivalents in `backfill_metrics.py`) is therefore a regression: the conflict-update path will no longer write `modified_at`, reverting to the exact behavior this PR set out to fix. The safe fix is to put `"modified_at"` back in each of these `update_fields` lists: ```python update_fields=["metric_type", "metric_value", "metric_count", "modified_at"], ``` How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: backend/dashboard_metrics/tasks.py
Line: 120-125
Comment:
**`_base_manager` bypasses `BaseModelQuerySet`, silently drops `modified_at` on conflict**
`EventMetricsHourly._base_manager` resolves to an auto-created plain `models.Manager` — not `BaseModelManager`. Django's base manager selection falls back to a plain manager whenever the model's only custom manager (`EventMetricsHourlyManager`) extends `DefaultOrganizationManagerMixin`, whose `get_queryset()` always calls `.filter(organization=organization)`, making it a filtering manager that Django deliberately excludes from `_base_manager`.
Because `_base_manager` returns a stock `models.QuerySet`, the `BaseModelQuerySet.bulk_create()` override that injects `modified_at` is never invoked. Removing `"modified_at"` from `update_fields` here (and in `_bulk_upsert_daily`, `_bulk_upsert_monthly`, and all three equivalents in `backfill_metrics.py`) is therefore a regression: the conflict-update path will no longer write `modified_at`, reverting to the exact behavior this PR set out to fix.
The safe fix is to put `"modified_at"` back in each of these `update_fields` lists:
```python
update_fields=["metric_type", "metric_value", "metric_count", "modified_at"],
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (6): Last reviewed commit: "Merge branch 'main' into fix/basemodel-m..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
chandrasekharan-zipstack
changed the title
…nerators Addresses PR review: if callers pass a non-rewindable iterable (generator, queryset iterator), the modified_at stamping loop would exhaust it before super().bulk_update() saw it, silently updating zero rows. list(objs) up front keeps generator callers working. Also drop the mock-based unit test — it needed django.setup() at module import which isn't viable without pytest-django, and proper DB-backed coverage is tracked separately.
Django only runs auto_now for fields listed in update_fields, so every save(update_fields=["foo"]) on a BaseModel subclass silently drops the modified_at bump — same family of bug as QuerySet.update/bulk_update. Override BaseModel.save() to add modified_at to update_fields whenever the caller supplies a restricted list without it. Also drop two dead manual-assignment lines (execution.modified_at = timezone.now() before save()) that were redundant with auto_now on a full save().
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/utils/models/base_model.py`:
- Around line 47-53: The save method currently consumes/rewrites iterable
update_fields and turns an empty list (which Django treats as a no-op) into a
write; change it to preserve empty-list behavior and avoid consuming iterables:
if update_fields is None do nothing; if update_fields has a length and
len(update_fields) == 0 call and return super().save(...,
update_fields=update_fields, ...) (preserve the empty-list no-op); otherwise
build a new list from update_fields only when needed and append "modified_at" if
not present, then call super().save with that new list (use the method name
save, the parameter update_fields, the "modified_at" field, and super().save to
locate the change).
- Around line 22-32: The bulk_update method currently mutates an empty fields
list and injects "modified_at", bypassing Django's validation; modify
bulk_update to first materialize objs and fields, then if fields is empty (e.g.
if not fields:), immediately call and return super().bulk_update(objs, fields,
*args, **kwargs) so Django raises ValueError as intended; otherwise proceed with
the existing modified_at injection logic (set obj.modified_at and append
"modified_at") before calling super().bulk_update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82c4aa70-9282-4ec9-9264-863a5816d0cb
📒 Files selected for processing (3)
backend/utils/models/base_model.pybackend/workflow_manager/internal_views.pybackend/workflow_manager/workflow_v2/views.py
💤 Files with no reviewable changes (2)
- backend/workflow_manager/workflow_v2/views.py
- backend/workflow_manager/internal_views.py
QuerySet.bulk_create(update_conflicts=True, update_fields=[...]) runs an UPDATE on conflict with only the listed fields — same auto_now-bypass as save(update_fields=...) and QuerySet.update(). Patch BaseModelQuerySet's bulk_create to inject modified_at into update_fields on upsert. With that in place, the explicit "modified_at" entries in dashboard_metrics upsert callers are redundant. Drop them.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/utils/models/base_model.py (1)
18-20:⚠️ Potential issue | 🟠 MajorPreserve empty/no-field semantics before adding
modified_at.These overrides currently turn no-op/invalid empty writes into
modified_at-only writes, and the membership checks can consume generator-stylefields/update_fieldsbefore conversion. Delegate empty values back to Django unchanged, then appendmodified_atonly for non-empty restricted writes. This also covers the previously flaggedbulk_update()andsave()cases.Proposed fix
def update(self, **kwargs): - kwargs.setdefault("modified_at", timezone.now()) + if kwargs: + kwargs.setdefault("modified_at", timezone.now()) return super().update(**kwargs) def bulk_update(self, objs, fields, *args, **kwargs): # Materialize objs before iterating so we don't exhaust a generator # before Django's own tuple(objs) sees it. objs = list(objs) - fields = list(fields) + fields = list(fields) if fields is not None else fields + if not fields: + return super().bulk_update(objs, fields, *args, **kwargs) if "modified_at" not in fields: now = timezone.now() for obj in objs: obj.modified_at = now fields.append("modified_at") @@ if ( update_conflicts and update_fields is not None - and "modified_at" not in update_fields ): - update_fields = list(update_fields) + ["modified_at"] + update_fields = list(update_fields) + if update_fields and "modified_at" not in update_fields: + update_fields.append("modified_at") return super().bulk_create( objs, *args, @@ def save(self, *args, update_fields=None, **kwargs): # Django only fires auto_now for fields listed in update_fields, so a # partial save() silently drops the modified_at bump. Auto-include it # whenever the caller restricts update_fields. - if update_fields is not None and "modified_at" not in update_fields: - update_fields = list(update_fields) + ["modified_at"] + if update_fields is not None: + update_fields = list(update_fields) + if update_fields and "modified_at" not in update_fields: + update_fields.append("modified_at") return super().save(*args, update_fields=update_fields, **kwargs)Also applies to: 22-32, 40-45, 67-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/utils/models/base_model.py` around lines 18 - 20, The override for update should preserve no-op semantics: if the incoming kwargs are empty (i.e., an actual no-op or invalid empty write) return super().update(**kwargs) unchanged; only append modified_at when there are other fields being written. Also convert any generator-style update_fields/fields to a concrete list/tuple before doing membership checks to avoid consuming them (apply the same pattern to save() and bulk_update() overrides); in short, add an early return when kwargs is empty, only set kwargs.setdefault("modified_at", timezone.now()) for non-empty writes, and normalize update_fields/fields to a list prior to membership testing in update, save, and bulk_update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/utils/models/base_model.py`:
- Around line 18-20: The override for update should preserve no-op semantics: if
the incoming kwargs are empty (i.e., an actual no-op or invalid empty write)
return super().update(**kwargs) unchanged; only append modified_at when there
are other fields being written. Also convert any generator-style
update_fields/fields to a concrete list/tuple before doing membership checks to
avoid consuming them (apply the same pattern to save() and bulk_update()
overrides); in short, add an early return when kwargs is empty, only set
kwargs.setdefault("modified_at", timezone.now()) for non-empty writes, and
normalize update_fields/fields to a list prior to membership testing in update,
save, and bulk_update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9f1550d-4f50-42e2-951e-3010495b1463
📒 Files selected for processing (3)
backend/dashboard_metrics/management/commands/backfill_metrics.pybackend/dashboard_metrics/tasks.pybackend/utils/models/base_model.py
vishnuszipstack
approved these changes
Apr 24, 2026
Contributor
muhammad-ali-e
left a comment
muhammad-ali-e
left a comment
There was a problem hiding this comment.
Automated PR Review (pr-review-toolkit)
Ran six specialized review agents — code-reviewer, code-simplifier, comment-analyzer, pr-test-analyzer, silent-failure-hunter, type-design-analyzer. Consolidated, deduplicated, and prioritized findings below as inline comments.
Overall assessment: Solid structural fix. The BaseModelQuerySet + BaseModelManager + BaseModel.save() triad correctly addresses the three Django paths that skip auto_now. MRO verified: (DefaultOrganizationManagerMixin, BaseModelManager) resolves super().get_queryset() to BaseModelQuerySet as intended. bulk_create(update_conflicts=True) path is correct — Django fires pre_save(add=True) on all objs before the upsert SQL, so EXCLUDED.modified_at carries the fresh timestamp. Managers on non-BaseModel models are correctly left alone.
Priority follow-ups (not blockers):
- Tighten
save(update_fields=[])guard — empty list today silently becomes a real write (medium, inline). - Positional
update_fieldsbypass insave()signature — no repo callers hit it today, but foot-gun for future (medium, inline). - Test coverage is addable without DB — the PR description defers tests on infra grounds, but the four override methods are pure kwargs/list mutation; two existing precedents (
usage_v2/tests/test_helper.py,prompt_studio_core_v2/tests/test_build_index_payload.py) run withoutpytest-django. A ~80-line mock-based test file would lock in every behavior (medium, inline). DefaultOrganizationManagerMixin(models.Manager)inbackend/utils/models/organization_mixin.py:26(not in this diff) should change its base toBaseModelManager. Closes the MRO foot-gun and would let 9 subclass managers drop their explicitBaseModelManagerbase — smallest DRY win with the biggest safety payoff. Recommend filing a follow-up.dashboard_metricsupsert path now depends entirely on the new override (commited5693e1removed explicitmodified_atfromupdate_fieldsin 6 locations). This is the highest-regression-risk consumer; a focused regression test would be valuable (medium, inline).
Docstrings and comments have a few accuracy issues flagged inline. Rest of findings (hardcoded "modified_at" literal, DRY of 3 near-duplicate append blocks, unconditional materialization) are low/nit — mentioned inline for context.
Type-design ratings: encapsulation 3/5, invariant expression 3/5, usefulness 4/5, enforcement 2/5. Enforcement drops because BaseModel subclasses can silently opt out by assigning objects = models.Manager().
Review generated by running pr-review-toolkit agents in parallel over the diff.
- Extract `_with_modified_at` helper; single source of truth for the "inject modified_at into a partial field list" rule across `bulk_update`, `bulk_create` and `BaseModel.save`. - Preserve Django's documented `save(update_fields=[])` no-op (signals-only save, no column writes) instead of rewriting it to `["modified_at"]`. Apply the same guard to `bulk_create(update_conflicts=True, update_fields=[])`. - Match Django's positional `save()` signature (`force_insert`, `force_update`, `using`, `update_fields`) so callers passing flags positionally still hit the auto-bump override. - Skip the per-obj `modified_at` stamp + `objs` materialization in `bulk_update` when the caller already listed `modified_at` — lets the opt-in path stay O(1) before the `super()` delegation. - Docstring corrections: "previous save() timestamp" (not just creation time); manager-level convention note; precise `auto_now` semantics (attribute still updates in-memory, just isn't persisted without `update_fields` inclusion).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/utils/models/base_model.py (1)
20-38: Docstrings capture the manager-level convention well.The caveats about raw SQL,
apps.get_model()in migrations, and subclasses reassigningobjectsset correct expectations. If you want one stricter layer later, a Django system check that warns when aBaseModelsubclass's_default_managerisn't derived fromBaseModelQuerySetwould make the invariant auditable at startup — but that's fine to defer.Also applies to: 77-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/utils/models/base_model.py` around lines 20 - 38, Docstring for BaseModelQuerySet correctly documents manager-level caveats so no functional code change is required; optionally, if you want a stricter startup guarantee later, add a Django system check that iterates subclasses of BaseModel and verifies their _default_manager is a Manager whose get_queryset() returns an instance of BaseModelQuerySet (or otherwise derives from BaseModelQuerySet), emitting a warning/error when the invariant is violated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/utils/models/base_model.py`:
- Around line 20-38: Docstring for BaseModelQuerySet correctly documents
manager-level caveats so no functional code change is required; optionally, if
you want a stricter startup guarantee later, add a Django system check that
iterates subclasses of BaseModel and verifies their _default_manager is a
Manager whose get_queryset() returns an instance of BaseModelQuerySet (or
otherwise derives from BaseModelQuerySet), emitting a warning/error when the
invariant is violated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90c26864-9f5b-43c2-a184-a862c74634b4
📒 Files selected for processing (1)
backend/utils/models/base_model.py
pk-zipstack
approved these changes
Apr 24, 2026
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
BaseModelQuerySetthat mirrorsauto_now=Truesemantics for every bulk write path. Itsupdate()andbulk_update()injectmodified_at = timezone.now()when the caller doesn't pass one explicitly.BaseModel.save()so callers usingsave(update_fields=["foo"])also getmodified_atbumped — Django normally skipsauto_nowfor fields not listed inupdate_fields.BaseModelManager = models.Manager.from_queryset(BaseModelQuerySet)and wire it ontoBaseModel.objects.BaseModelsubclasses to composeBaseModelManagerinstead ofmodels.Managerso their querysets inherit the overrides.modified_at=timezone.now()kwarg inFileHistoryHelper.update_file_history.execution.modified_at = timezone.now()assignments before.save()inworkflow_manager/internal_views.pyandworkflow_manager/workflow_v2/views.py(no-ops —auto_nowalready fired on the save).Why
models.DateTimeField(auto_now=True)only fires onModel.save()for fields that are actually being written. Three common paths silently drop themodified_atbump onBaseModelsubclasses:QuerySet.update(...)— issues raw SQL, bypassessave().QuerySet.bulk_update(...)— same.instance.save(update_fields=[...])— Django skipsauto_nowfor fields not in the list.Rows written through any of these paths kept their original creation timestamp, so audit-trail / "last modified" UI / staleness checks silently drifted. The trap was already known in isolated spots (four places in the codebase manually patched
modified_at=timezone.now()into their.update()call, and two more assigned it before.save()), but ~20+ other call sites dropped it. Fixing it onBaseModelis the correct structural place — one model base replaces ad-hoc per-caller patches.Callers who want to preserve an explicit timestamp can still pass
modified_at=...—setdefaulthonors it on the.update()path, and any value already set on the instance survives.save().How
backend/utils/models/base_model.py:BaseModelQuerySet.update()thatsetdefaultsmodified_at.BaseModelQuerySet.bulk_update()that materializesobjs(so generators aren't silently dropped), appendsmodified_attofields, and stamps it on each obj.BaseModelManager = models.Manager.from_queryset(BaseModelQuerySet); switchBaseModel.objectsto it.BaseModel.save()to injectmodified_atintoupdate_fieldswhen caller restricts the list.BaseModelsubclass now composesBaseModelManagerinstead ofmodels.Manager. ForOrgAwareManagerthis one change covers every subclass transitively.backend/workflow_manager/workflow_v2/file_history_helper.py— remove the redundantmodified_at=timezone.now()kwarg.backend/workflow_manager/internal_views.py+backend/workflow_manager/workflow_v2/views.py— remove the two no-op manual assignments.BaseModelmodels (ConnectorAuth,OrganizationMember,ReviewApiKey,RuleEngine,AutoApprovalSettings,HITLChangeLog,subscription_usage) are intentionally left alone — those tables have nomodified_atcolumn.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Low risk but has behavioral impact on any write touching a
BaseModelrow via.update(),.bulk_update(), or.save(update_fields=[...]):modified_atwhere it previously didn't. For most call sites that's the correct, long-expected behavior.modified_at == created_atafter one of these paths would now fail. A grep didn't find any such assertions.QuerySet.update(modified_at=...)callers relying on their explicit value are unaffected.is_deleted=Truevia.update()) will now also bumpmodified_at— semantically correct.save(update_fields=[...])callers who expected a partial save to leavemodified_atuntouched would see it bumped. No known callers depend on that, and skipping the bump on any write is what caused the bug.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
feat/lookups-v2.Dependencies Versions
Notes on Testing
.update(),.bulk_update(), or.save(update_fields=[...])against aBaseModelrow and verifymodified_atadvances. Good targets: re-running a prompt in prompt studio, file-history update on workflow replay, HITL queue state transitions.pytest-djangoor a tox env wired for backend unit tests. A proper DB-backed test forBaseModelQuerySet+BaseModel.save()would need that infrastructure first, and we don't want to land a test file that hacksdjango.setup()at module scope.Screenshots
N/A — backend-only change.
Checklist
I have read and understood the Contribution Guidelines.