Skip to content

fix(versioning): block manual commit of a stale change request #8219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bardock-2393 wants to merge 1 commit into
base: main
Choose a base branch
from
+212 −4
Open

fix(versioning): block manual commit of a stale change request #8219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion api/core/workflows_services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
from features.versioning.models import EnvironmentFeatureVersion
from features.versioning.signals import environment_feature_version_published
from features.versioning.tasks import trigger_update_version_webhooks
from features.workflows.core.exceptions import ChangeRequestNotApprovedError
from features.workflows.core.exceptions import (
ChangeRequestNotApprovedError,
ChangeRequestStaleError,
)

if TYPE_CHECKING:
from features.workflows.core.models import ChangeRequest
Expand All @@ -27,6 +30,8 @@ def commit(self, committed_by: "FFAdminUser") -> None:
"Change request has not been approved by all required approvers."
)

self._raise_if_stale()

self._publish_feature_states()
self._publish_environment_feature_versions(committed_by)
self._publish_change_sets(committed_by)
Expand All @@ -45,6 +50,24 @@ def commit(self, committed_by: "FFAdminUser") -> None:

self.change_request.save()

def _raise_if_stale(self) -> None:
# Mirror the conflict check already performed for scheduled change
# sets (see `publish_version_change_set`) so that a manual commit
# can't silently overwrite overrides published by another change
# request since this one was created.
if self.change_request.ignore_conflicts:
return

for change_set in self.change_request.change_sets.all():
if change_set.get_conflicts():
logger.warning(
"change_request.stale",
organisation__id=self.change_request.project.organisation_id,
environment__id=self.change_request.environment_id,
change_request__id=self.change_request.id,
)
raise ChangeRequestStaleError()

def _publish_feature_states(self) -> None:
now = timezone.now()

Expand Down
8 changes: 8 additions & 0 deletions api/features/workflows/core/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@ class ChangeRequestNotApprovedError(FeatureWorkflowError):
status_code = status.HTTP_400_BAD_REQUEST # type: ignore[assignment]


class ChangeRequestStaleError(FeatureWorkflowError):
status_code = status.HTTP_400_BAD_REQUEST # type: ignore[assignment]
default_detail = (
"This change request is out of date with changes published since it "
"was created. Please refresh and reapply your changes."
)


class CannotApproveOwnChangeRequest(FeatureWorkflowError):
status_code = status.HTTP_400_BAD_REQUEST # type: ignore[assignment]

Expand Down
167 changes: 167 additions & 0 deletions api/tests/unit/features/workflows/core/test_unit_workflows_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from core.helpers import get_current_site_url
from environments.models import Environment
from features.models import Feature, FeatureSegment, FeatureState
from features.value_types import STRING
from features.versioning.models import (
EnvironmentFeatureVersion,
VersionChangeSet,
Expand All @@ -33,6 +34,7 @@
CannotApproveOwnChangeRequest,
ChangeRequestDeletionError,
ChangeRequestNotApprovedError,
ChangeRequestStaleError,
)
from features.workflows.core.models import (
ChangeRequest,
Expand Down Expand Up @@ -231,6 +233,171 @@ def test_change_request_commit__valid_request__emits_structlog_event(
} in log.events


def test_change_request_commit__stale_change_set__raises_exception_and_does_not_revert_conflicting_change(
environment_v2_versioning: Environment,
feature: Feature,
segment: Segment,
admin_user: FFAdminUser,
) -> None:
# Given
# An existing, published segment override on the feature.
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,
)

# CR A captures the full state of that override (e.g., as part of
# reordering overrides on the feature) when it is created.
change_request_a = ChangeRequest.objects.create(
environment=environment_v2_versioning, title="CR A", user=admin_user
)
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",
},
}
]
),
)

# And CR B changes the value of that same override, and is published
# first.
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)

# When / Then
# Committing CR A should now be blocked, since it is stale: its
# captured override state conflicts with CR B's published change.
with pytest.raises(ChangeRequestStaleError):
change_request_a.commit(admin_user)

# and CR B's change has not been silently reverted.
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 True
assert override.get_feature_state_value() == "concurrent value"
assert change_request_a.committed_at is None


def test_change_request_commit__stale_change_set_but_ignore_conflicts__commits_and_reverts_change(
environment_v2_versioning: Environment,
feature: Feature,
segment: Segment,
admin_user: FFAdminUser,
) -> None:
# 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)
Comment on lines +328 to +390

@coderabbitai coderabbitai Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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
Comment on lines +392 to +398

@coderabbitai coderabbitai Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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.

Suggested change
# 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"



def test_change_request_create__valid_environment__creates_audit_log( # type: ignore[no-untyped-def]
environment, admin_user
):
Expand Down
16 changes: 13 additions & 3 deletions docs/docs/deployment-self-hosting/observability/_events-catalogue.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,25 +755,35 @@ Attributes:
### `workflows.change_request.committed`

Logged at `info` from:
- `api/core/workflows_services.py:39`
- `api/core/workflows_services.py:44`

Attributes:
- `environment.id`
- `feature_states.count`
- `organisation.id`

### `workflows.change_request.stale`

Logged at `warning` from:
- `api/core/workflows_services.py:63`

Attributes:
- `change_request.id`
- `environment.id`
- `organisation.id`

### `workflows.missing_live_segment`

Logged at `warning` from:
- `api/core/workflows_services.py:114`
- `api/core/workflows_services.py:137`

Attributes:
- `draft_segment`

### `workflows.segment_revision_created`

Logged at `info` from:
- `api/core/workflows_services.py:119`
- `api/core/workflows_services.py:142`

Attributes:
- `revision_id`
Expand Down
Loading