fix(features): inherit mv hashing salt in change requests and scheduled commits#7963
fix(features): inherit mv hashing salt in change requests and scheduled commits#7963Zaimwa9 wants to merge 16 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
github-actions
Bot
added
api
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## fix/7913-mv-hashing-salt #7963 +/- ##
==========================================================
Coverage 98.63% 98.63%
==========================================================
Files 1497 1498 +1
Lines 59132 59256 +124
==========================================================
+ Hits 58326 58450 +124
Misses 806 806 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
fix
docs
Zaimwa9
changed the title
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Skipped testsfirefox › tests/initialise-tests.pw.ts › Signup › Create Organisation and Project @oss
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a33a51d7-3983-4c2c-ba3f-33e68a14e676
📒 Files selected for processing (9)
api/core/workflows_services.pyapi/features/models.pyapi/features/versioning/serializers.pyapi/tests/unit/features/test_unit_features_models.pyapi/tests/unit/features/versioning/test_unit_versioning_serializers.pyapi/tests/unit/features/versioning/test_unit_versioning_versioning_service.pyapi/tests/unit/features/versioning/test_unit_versioning_views.pyapi/tests/unit/features/workflows/core/test_unit_workflows_models.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
| def get_superseded_live_feature_state(self) -> typing.Optional["FeatureState"]: | ||
| """Return the live feature state of this state's lineage (same | ||
| environment, feature and segment) that this state supersedes when it | ||
| goes live. Only meaningful for v1 versioning; identity overrides have | ||
| no versioned lineage. | ||
| """ | ||
| # A segment override's FeatureSegment row can be recreated with it, so | ||
| # match the lineage by its segment, not by the FeatureSegment row. | ||
| if self.feature_segment_id is None: | ||
| lineage_filter = Q(feature_segment__isnull=True) | ||
| else: | ||
| lineage_filter = Q( | ||
| feature_segment__segment_id=self.feature_segment.segment_id # type: ignore[union-attr] | ||
| ) | ||
|
|
||
| superseded: FeatureState | None = ( | ||
| FeatureState.objects.filter( | ||
| lineage_filter, | ||
| environment_id=self.environment_id, | ||
| feature_id=self.feature_id, | ||
| identity__isnull=True, | ||
| version__isnull=False, | ||
| live_from__isnull=False, | ||
| live_from__lte=timezone.now(), | ||
| ) | ||
| .exclude(id=self.id) | ||
| .order_by("-version") | ||
| .first() | ||
| ) | ||
| return superseded |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant model methods and call sites.
git ls-files api/features/models.py api/core/workflows_services.py | cat
echo '--- model outline ---'
ast-grep outline api/features/models.py --view expanded || true
echo '--- workflows_services outline ---'
ast-grep outline api/core/workflows_services.py --view expanded || true
echo '--- search for __gt__ and get_superseded_live_feature_state ---'
rg -n "def __gt__|get_superseded_live_feature_state|_publish_feature_states|live_from|version" api/features/models.py api/core/workflows_services.pyRepository: Flagsmith/flagsmith
Length of output: 20392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant slices only.
echo '--- api/features/models.py lines around __gt__ ---'
sed -n '560,640p' api/features/models.py
echo '--- api/features/models.py lines around get_superseded_live_feature_state ---'
sed -n '760,830p' api/features/models.py
echo '--- api/core/workflows_services.py relevant slice ---'
sed -n '1,260p' api/core/workflows_services.py | sed -n '1,220p'Repository: Flagsmith/flagsmith
Length of output: 12469
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact implementation of the comparison and any tests around it.
python3 - <<'PY'
from pathlib import Path
path = Path('api/features/models.py')
text = path.read_text()
for start_marker in ['def __gt__', 'def get_superseded_live_feature_state']:
idx = text.find(start_marker)
print(f'=== {start_marker} at {idx} ===')
if idx != -1:
start = text.rfind('\n', 0, idx)
end = text.find('\n def ', idx + 1)
if end == -1:
end = len(text)
print(text[start+1:end])
PY
echo '--- tests mentioning live_from or superseded state ---'
rg -n "superseded_live_feature_state|live_from.*version|version.*live_from|__gt__" api testsRepository: Flagsmith/flagsmith
Length of output: 12367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant unit tests and the helper that consumes this method.
sed -n '880,960p' api/tests/unit/features/test_unit_features_models.py
echo '---'
sed -n '1000,1065p' api/tests/unit/features/test_unit_features_models.py
echo '---'
sed -n '630,670p' api/tests/unit/util/mappers/test_unit_mappers_engine.py
echo '---'
sed -n '40,80p' api/core/workflows_services.pyRepository: Flagsmith/flagsmith
Length of output: 7706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any explicit expectation that the "current" live state is the one with latest live_from.
rg -n "latest live_from|most recent live from|superseded live state|get_superseded_live_feature_state|__gt__" api/tests/unit/features api/tests/unit/util/mappersRepository: Flagsmith/flagsmith
Length of output: 580
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '880,960p' api/tests/unit/features/test_unit_features_models.py
echo '---'
sed -n '1000,1065p' api/tests/unit/features/test_unit_features_models.py
echo '---'
sed -n '630,670p' api/tests/unit/util/mappers/test_unit_mappers_engine.py
echo '---'
sed -n '40,80p' api/core/workflows_services.py
echo '---'
rg -n "latest live_from|most recent live from|superseded live state|get_superseded_live_feature_state|__gt__" api/tests/unit/features api/tests/unit/util/mappersRepository: Flagsmith/flagsmith
Length of output: 8133
Order superseded states by live_from first get_superseded_live_feature_state() should sort by -live_from, -version; ordering by version alone can pick the wrong live state when commit order and scheduled live times diverge, which makes the successor inherit the wrong bucketing salt.
There was a problem hiding this comment.
Somehow this should use get_live_feature_states directly
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "def get_live_feature_states" -A 40 api/features/managers.pyLength of output: 1940
@Zaimwa9, agreed — that's a nice simplification and it also resolves the ordering concern I raised, since get_live_feature_states already encodes the canonical v1 "live" definition (deleted_at__isnull=True, live_from__lte=now, version__isnull=False). Something like:
def get_superseded_live_feature_state(self) -> typing.Optional["FeatureState"]:
if self.feature_segment_id is None:
lineage_filter = Q(feature_segment__isnull=True)
else:
lineage_filter = Q(
feature_segment__segment_id=self.feature_segment.segment_id # type: ignore[union-attr]
)
return (
FeatureState.objects.get_live_feature_states(
environment=self.environment,
additional_filters=lineage_filter,
feature_id=self.feature_id,
identity__isnull=True,
)
.exclude(id=self.id)
.order_by("-live_from", "-version")
.first()
)This keeps the lineage/segment matching intact while delegating the "what counts as live" logic to the manager (which is already exercised by other call sites like get_environment_default), and orders by live_from first so the most recently gone-live state wins over version alone.
✏️ Learnings added
Learnt from: Zaimwa9
Repo: Flagsmith/flagsmith PR: 7963
File: api/features/models.py:785-814
Timestamp: 2026-07-13T08:18:39.411Z
Learning: In `api/features/models.py`, prefer reusing `FeatureState.objects.get_live_feature_states(environment=..., additional_filters=..., **kwargs)` from `api/features/managers.py` instead of manually duplicating the v1 "live" filter (`live_from__isnull=False, live_from__lte=now, version__isnull=False`) when querying for live feature states, as seen in `get_environment_default()` and `get_superseded_live_feature_state()`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…ate recreation Multivariate variant bucketing is salted on the feature state id, so any flow that recreates a feature state (publishing a new v2 version, editing multivariate weights) changed the salt and re-bucketed already-enrolled identities. Add an mv_hashing_salt field that get_multivariate_feature_state_value seeds on (falling back to the id), preserved across clone() so recreation keeps the original seed. The engine mapper feeds the salt through django_id, so edge and local evaluation stay stable without an engine document schema change. Also add a BEFORE_CREATE guard that raises when a v2 feature state is recreated outside clone() (the previous version already had it but the new row carries no salt), to catch future regressions in tests. Closes #7913
gagantrivedi
force-pushed
the
fix/7913-mv-hashing-salt
branch
from
71ba4ee to
0822078
Compare
… guard to segment overrides
gagantrivedi
force-pushed
the
fix/7913-mv-hashing-salt
branch
from
0822078 to
a32e197
Compare
Zaimwa9
force-pushed
the
fix/7913-mv-hashing-salt-inheritance
branch
from
9a426e3 to
003520e
Compare
github-actions
Bot
added
docs
|
Redundant following #7914 |
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #7913
Builds on #7914. Two recreation paths bypass
clone()and still re-bucketed multivariate variants:ChangeRequestCommitServicenow carries the superseded live state's salt onto drafts at commit time.Also: succession helpers on
FeatureState(used byclone()), afeatures.feature_state.mv_variants_rebucketedwarning event for unknown v1 recreation paths (added to the events catalogue), andclone()copies multivariate values in id order so variant order is deterministic.Alternatives considered
BEFORE_CREATEhook): covers unknown paths too, but heals them silently — a flow bypassingclone()would never surface and be fixed. Rejected in favour of declared, reviewable call sites plus the guard tripwire (raise under v2, warning event under v1).clone()(copy the old state, apply the request on top): the request already defines the entire new state, so the clone would contribute nothing but the salt — while starting from old values instead of defaults silently changes what partial requests return, and the rewired create path would touch shared serialisers far beyond the two affected flows. Possible later refactor.How did you test this code?
New unit tests: variant assignment stable across a change request commit (fails without the fix), override delete + recreate in a draft inherits the salt via the API, warning event fires only for unknown v1 paths, lineage lookup helper.