fix: Enhance EFV audit logs#7535
Conversation
|
@SahilJat is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
for more information, see https://pre-commit.ci
SahilJat
force-pushed
the
feat/enhanced-audit-logs
branch
from
a776a6c to
5aecaf8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7535 +/- ##
==========================================
- Coverage 98.47% 98.31% -0.16%
==========================================
Files 1400 1400
Lines 53036 52960 -76
==========================================
- Hits 52226 52068 -158
- Misses 810 892 +82 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
SahilJat
force-pushed
the
feat/enhanced-audit-logs
branch
from
5aecaf8 to
3eb06b6
Compare
|
@claude review-once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "environment", "feature" | ||
| ).get(uuid=environment_feature_version_uuid) | ||
|
|
||
| header = ( | ||
| ENVIRONMENT_FEATURE_VERSION_PUBLISHED_MESSAGE | ||
| % environment_feature_version.feature.name | ||
| ) | ||
|
|
||
| changed_states = get_updated_feature_states_for_version(environment_feature_version) | ||
|
|
||
| if changed_states: | ||
| changed_states = list( | ||
| FeatureState.objects.filter( | ||
| id__in=[fs.id for fs in changed_states] | ||
| ).select_related("feature_segment__segment", "identity") | ||
| ) | ||
| change_lines = "\n".join( | ||
| f"- {_build_feature_state_change_summary(fs)}" for fs in changed_states | ||
| ) | ||
| log = f"{header}\n{change_lines}" | ||
| else: | ||
| log = header |
There was a problem hiding this comment.
🔴 The new audit log silently omits removed feature states. get_updated_feature_states_for_version() only iterates the new version's feature states and compares them against the previous version — it never walks the previous version looking for keys that don't appear in the new one. So when a user publishes a version whose only change is dropping a segment override (e.g. via segment_ids_to_delete_overrides), the removed override is never visited and the audit log falls back to just the header ("New version published for feature: X"), contradicting the PR's stated goal of enumerating each changed Feature State's action and scope.
Extended reasoning...
What the bug is
create_environment_feature_version_published_audit_log_task (api/features/versioning/tasks.py:281-302) delegates change detection to get_updated_feature_states_for_version (api/features/versioning/versioning_service.py:479-522). That helper iterates only the new version's feature states:
previous_feature_states_map = (
{get_match_key(fs): fs for fs in previous_version.feature_states.all()}
if previous_version
else {}
)
changed_feature_states = []
for feature_state in version.feature_states.all(): # NEW version only
previous_fs = previous_feature_states_map.get(get_match_key(feature_state))
...It never enumerates the previous map looking for keys absent in the new version, so feature states that existed in the previous version but were removed in the new one are invisible to the audit log.
Why this triggers for deletions
The versioning create serializer (api/features/versioning/serializers.py:312-315) handles segment_ids_to_delete_overrides by hard-deleting the matching feature_segments on the new version:
def _delete_feature_states(self, segment_ids, version):
version.feature_segments.filter(segment_id__in=segment_ids).delete()Because FeatureState.feature_segment cascades, the corresponding FeatureState rows are removed from the new version. So new_version.feature_states.all() simply doesn't contain them, and the loop never visits them. A similar single-override path exists in _delete_segment_override_v2 (versioning_service.py:447-474), which also deletes the FS from the new version after cloning.
Step-by-step proof
- Environment with v2 versioning enabled; feature
Fhas an active segment override on segmentS1in versionV1(live). - User creates a new version
V2whose only change issegment_ids_to_delete_overrides: [S1.id]and publishes it. - During create,
_delete_feature_states([S1.id], V2)is called →V2.feature_segments.filter(segment_id=S1.id).delete()cascades and the cloned segment-override FeatureState inV2is removed. V2.feature_states.all()now contains only the environment-default FS, which is identical toV1's default → it matches by(identity_id=None, segment_id=None), all attributes equal, sochanged_feature_statesis empty.create_environment_feature_version_published_audit_log_tasktherefore takes theelse: log = headerbranch.- Resulting audit log is just
"New version published for feature: F"— no mention of the removedS1override.
Impact
The PR description explicitly promises that the audit log enumerates each changed Feature State's action and scope. A pure-deletion publish produces a log that's indistinguishable from a no-op publish, which is misleading for users auditing what changed. The same blind spot exists in the parallel webhook code (_trigger_feature_state_webhooks_for_version, tasks.py:154-240), so this is a pre-existing limitation of the helper — but this PR is the first to surface it in a user-facing audit message.
How to fix
Either extend get_updated_feature_states_for_version to also yield previous-version FS whose keys are absent in the new version (tagged as removals), or add a parallel removal-detection step inside create_environment_feature_version_published_audit_log_task that diffs previous_version.feature_states against the new version's keys and renders something like - Segment override (S1): removed. The summary builder in this PR (_build_feature_state_change_summary) would need a small extension to render a "removed" state, or a separate helper for removed states.
| def _build_feature_state_change_summary(fs: FeatureState) -> str: | ||
| if fs.identity_id: | ||
| scope = f"Identity override ({fs.identity.identifier})" # type: ignore[union-attr] | ||
| elif fs.feature_segment_id: | ||
| scope = f"Segment override ({fs.feature_segment.segment.name})" # type: ignore[union-attr] | ||
| else: | ||
| scope = "Environment default" | ||
|
|
||
| state = "enabled" if fs.enabled else "disabled" |
There was a problem hiding this comment.
🔴 The audit-log summary only reports each changed feature state's scope and current enabled/disabled flag, but get_updated_feature_states_for_version also flags states whose feature_state_value or multivariate values changed while enabled stayed the same. Such value-only edits produce a line indistinguishable from an enabled-toggle change (e.g. - Environment default: enabled), defeating this PR's stated goal of making EFV audit logs less opaque. Consider including the value/MV diff (or at least marking which attribute changed) in _build_feature_state_change_summary, and add a regression test for a value-only change.
Extended reasoning...
What the bug is
The new _build_feature_state_change_summary (api/features/versioning/tasks.py:264-272) only renders two pieces of information for each changed feature state: its scope (Environment default / Segment override (...) / Identity override (...)) and the current enabled/disabled flag. It never inspects (or surfaces) the feature-state value or the multivariate allocations.
However, get_updated_feature_states_for_version (api/features/versioning/versioning_service.py:510-522) classifies a feature state as "changed" if any of the following differs from the previous version:
if previous_fs is None or (
feature_state.enabled != previous_fs.enabled
or feature_state.get_feature_state_value()
!= previous_fs.get_feature_state_value()
or multivariate_values_changed(feature_state, previous_fs)
):
changed_feature_states.append(feature_state)So a feature state whose value changed (e.g. string_value "v1" → "v2") or whose multivariate allocation changed, with enabled unchanged, will be in the changed list — but the audit log line will only say the current enabled state.
Step-by-step proof
- Start with an EFV where the env-default feature state has
enabled=True,string_value="v1". - Create a new version, copy the previous states, then edit only the env-default value to
"v2"(stillenabled=True). - Publish.
create_environment_feature_version_published_audit_log_taskruns. get_updated_feature_states_for_versionreturns that env-default FS (value differs)._build_feature_state_change_summarybuilds: scope ="Environment default",state = "enabled"(becausefs.enabledis True), output:"Environment default: enabled".- Audit log:
New version published for feature: foo\n- Environment default: enabled. - Now perform a different change on a fresh version: leave value at
"v1"and toggleenabledfromFalsetoTrue. The audit log is identical:- Environment default: enabled.
Two semantically distinct changes (value edit vs. enabled toggle) produce the same audit-log line, and a reader cannot tell what actually changed — exactly the problem this PR was meant to fix (linked issue #7526: "opaque audit logs … completely omitting which feature states were actually changed").
Why existing code does not prevent it
Nothing in the new code path reads feature_state_value or compares against the previous version. The line template is f"{scope}: {state}" with state derived purely from the current fs.enabled. The test suite added in this PR only exercises (a) no changes, (b) an env-default toggle, (c) a brand-new segment override, (d) a brand-new identity override — none cover a value-only or MV-only change against a previous version.
Impact
Quality-of-message regression vs. the PR's stated goal. No crash, audit logs are still created, but they remain misleading/opaque for the very common case of users editing a feature state's value (e.g. config string, JSON payload) without toggling enabled. For multivariate features the line is similarly uninformative.
How to fix
In _build_feature_state_change_summary, accept (or look up) the previous feature state and emit what actually changed — e.g.:
Environment default: value changed ("v1" → "v2")Environment default: enabled (was disabled)Environment default: multivariate allocation changed
Or, less ambitiously, append the current value alongside the enabled flag so at least the value is visible (still not great for MV). Either way, add a regression test that mutates feature_state_value only and asserts the audit log differs from the equivalent enabled-toggle case.
|
For the deleted states — I kept it to just the current version's feature states since the issue didn't explicitly mention deletions, but you're right that a removed segment override is still a change and should show up in the log. |
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #7526
This PR resolves the issue of opaque audit logs for v2 versioning (
EnvironmentFeatureVersion). Previously, publishing a new version only logged a static string ("New version published for feature: %s"), completely omitting which feature states were actually changed.This updates
create_environment_feature_version_published_audit_log_taskto:get_updated_feature_states_for_version(efv).feature_segment__segmentandidentityviaselect_related.How did you test this code?
-I wrote comprehensive unit tests in
api/tests/unit/audit/test_unit_audit_services.pyto cover the different scenarios for the audit log task: