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 1 commit 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. |
📝 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
| # Given | ||
| # Same setup as above, but CR A has `ignore_conflicts` set, which is | ||
| # the existing opt-out already respected by scheduled publishes. | ||
| current_version = EnvironmentFeatureVersion.objects.get_latest_versions_as_queryset( | ||
| environment_v2_versioning.id | ||
| ).get(feature=feature) | ||
| feature_segment = FeatureSegment.objects.create( | ||
| segment=segment, | ||
| feature=feature, | ||
| environment=environment_v2_versioning, | ||
| environment_feature_version=current_version, | ||
| ) | ||
| FeatureState.objects.create( | ||
| environment=environment_v2_versioning, | ||
| feature=feature, | ||
| feature_segment=feature_segment, | ||
| environment_feature_version=current_version, | ||
| enabled=False, | ||
| ) | ||
|
|
||
| change_request_a = ChangeRequest.objects.create( | ||
| environment=environment_v2_versioning, | ||
| title="CR A", | ||
| user=admin_user, | ||
| ignore_conflicts=True, | ||
| ) | ||
| VersionChangeSet.objects.create( | ||
| change_request=change_request_a, | ||
| feature=feature, | ||
| feature_states_to_update=json.dumps( | ||
| [ | ||
| { | ||
| "feature_segment": {"segment": segment.id}, | ||
| "enabled": False, | ||
| "feature_state_value": { | ||
| "type": STRING, | ||
| "string_value": "original value", | ||
| }, | ||
| } | ||
| ] | ||
| ), | ||
| ) | ||
|
|
||
| change_request_b = ChangeRequest.objects.create( | ||
| environment=environment_v2_versioning, title="CR B", user=admin_user | ||
| ) | ||
| VersionChangeSet.objects.create( | ||
| change_request=change_request_b, | ||
| feature=feature, | ||
| feature_states_to_update=json.dumps( | ||
| [ | ||
| { | ||
| "feature_segment": {"segment": segment.id}, | ||
| "enabled": True, | ||
| "feature_state_value": { | ||
| "type": STRING, | ||
| "string_value": "concurrent value", | ||
| }, | ||
| } | ||
| ] | ||
| ), | ||
| ) | ||
| change_request_b.commit(admin_user) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract the duplicated setup into a fixture.
Lines 331-390 repeat lines 244-304 almost exactly. The only difference is ignore_conflicts=True at line 352. Extract the published override, CR A, and CR B setup into a fixture or a helper that accepts ignore_conflicts as a parameter. This keeps the two tests aligned when the change-set payload shape changes.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 356-367: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
[
{
"feature_segment": {"segment": segment.id},
"enabled": False,
"feature_state_value": {
"type": STRING,
"string_value": "original value",
},
}
]
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 376-387: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
[
{
"feature_segment": {"segment": segment.id},
"enabled": True,
"feature_state_value": {
"type": STRING,
"string_value": "concurrent value",
},
}
]
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
| # When | ||
| change_request_a.commit(admin_user) | ||
|
|
||
| # Then | ||
| # commit succeeds, and (as documented by `ignore_conflicts`) CR A's | ||
| # captured state overwrites CR B's published change. | ||
| assert change_request_a.committed_at is not None |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the overwrite that the test name promises.
The test name ends with commits_and_reverts_change, and the comment at lines 396-397 states that CR A's captured state overwrites CR B's published change. The test asserts only committed_at is not None. It does not verify the resulting feature state. A regression that makes ignore_conflicts skip the publish, or that publishes the wrong value, would still pass this test.
Add the same state assertions used in the first test, with the opposite expected values.
💚 Proposed fix to assert the overwritten state
# When
change_request_a.commit(admin_user)
# Then
# commit succeeds, and (as documented by `ignore_conflicts`) CR A's
# captured state overwrites CR B's published change.
assert change_request_a.committed_at is not None
+
+ latest_flags = get_environment_flags_list(
+ environment=environment_v2_versioning, feature_name=feature.name
+ )
+ override = next(fs for fs in latest_flags if fs.feature_segment_id is not None)
+ assert override.enabled is False
+ assert override.get_feature_state_value() == "original value"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # When | |
| change_request_a.commit(admin_user) | |
| # Then | |
| # commit succeeds, and (as documented by `ignore_conflicts`) CR A's | |
| # captured state overwrites CR B's published change. | |
| assert change_request_a.committed_at is not None | |
| # When | |
| change_request_a.commit(admin_user) | |
| # Then | |
| # commit succeeds, and (as documented by `ignore_conflicts`) CR A's | |
| # captured state overwrites CR B's published change. | |
| assert change_request_a.committed_at is not None | |
| latest_flags = get_environment_flags_list( | |
| environment=environment_v2_versioning, feature_name=feature.name | |
| ) | |
| override = next(fs for fs in latest_flags if fs.feature_segment_id is not None) | |
| assert override.enabled is False | |
| assert override.get_feature_state_value() == "original value" |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8219 +/- ##
==========================================
- Coverage 98.71% 98.57% -0.15%
==========================================
Files 1535 1535
Lines 61329 61363 +34
==========================================
- Hits 60541 60488 -53
- Misses 788 875 +87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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