fix(versioning): block manual commit of a stale change request - #8219
fix(versioning): block manual commit of a stale change request#8219bardock-2393 wants to merge 3 commits into
Conversation
A Change Request commit skipped the conflict check that scheduled publishes already run, so committing a CR whose captured overrides had since been changed by another published CR would silently overwrite that newer change. Run the same VersionChangeSet conflict check on manual commits and reject them with ChangeRequestStaleError unless ignore_conflicts is set, consistent with scheduled publishes.
|
@bardock-2393 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe commit workflow now detects stale change sets before publishing. It raises Estimated code review effort: 3 (Moderate) | ~20 minutes ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
github-actions
Bot
added
api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 98429da5-3b78-4bdb-864e-9d80314bfbbb
📒 Files selected for processing (4)
api/core/workflows_services.pyapi/features/workflows/core/exceptions.pyapi/tests/unit/features/workflows/core/test_unit_workflows_models.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8219 +/- ##
==========================================
- Coverage 98.73% 98.59% -0.15%
==========================================
Files 1567 1567
Lines 62379 62413 +34
==========================================
- Hits 61591 61535 -56
- Misses 788 878 +90 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…setup Addresses review feedback on the stale change request tests: the `ignore_conflicts` test asserted only that the commit happened, not that CR A's captured state actually overwrote CR B's published change, so a regression that skipped the publish would still pass. The shared setup for both tests is now built by a single helper.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/core/workflows_services.py (1)
37-42: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake conflict detection atomic with publication.
commit()checks conflicts before_publish_change_sets(). No shared transaction or lock covers these operations, and scheduled publication performs its conflict check separately. Concurrent commits can both pass the check and publish overlapping changes, allowing stale state to become current.Use a common lock or an atomic compare-and-set. Perform the conflict check and publication in the same critical section, including
publish_version_change_set()for scheduled changes. Add a concurrent regression test.Source: Learnings
api/features/workflows/core/exceptions.py (1)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
mypysuppression.The
# type: ignore[assignment]code is precise, but the comment does not state why the Django REST framework status constant requires suppression. Add a short reason, or remove the suppression if the repository's current typing accepts the assignment.Based on learnings: API type suppressions should include a short explanation of the external typing limitation.
Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e0e5857e-5461-4483-965e-a8a59f96ea80
📒 Files selected for processing (4)
api/core/workflows_services.pyapi/features/workflows/core/exceptions.pyapi/tests/unit/features/workflows/core/test_unit_workflows_models.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
1 participant
Changes
Manually committing a Change Request skipped the conflict check that scheduled publishes already run, so a CR whose captured overrides had since been changed and published by another CR could still be committed — silently reverting the newer change back to the stale captured values, with only a passive "published since" notice.
This reuses the existing
VersionChangeSet.get_conflicts()check (already used by scheduled publishes) on the manual commit path too. A stale commit is now rejected with a 400 (ChangeRequestStaleError) instead of silently overwriting, unless the CR hasignore_conflictsset — the same opt-out scheduled publishes already respect.Closes #7931
How did you test this code?
Added
test_change_request_commit__stale_change_set__raises_exception_and_does_not_revert_conflicting_changeandtest_change_request_commit__stale_change_set_but_ignore_conflicts__commits_and_reverts_changetotest_unit_workflows_models.py, reproducing the issue's exact scenario. Verified both fail against the old code and pass against the fix. Ran the fulltest_unit_workflows_models.py+features/versioning+coresuites (198 passed). Ran mypy and the project's pre-commit hooks, all clean.Note: the HTTP endpoint that calls
.commit()lives in the closed-sourceworkflows_logicmodule, so this was verified at the service/model layer; I couldn't add an end-to-end API test for it from this repo.Review effort: 3/5