diff --git a/api/features/migrations/0067_add_feature_state_mv_hashing_salt.py b/api/features/migrations/0067_add_feature_state_mv_hashing_salt.py new file mode 100644 index 000000000000..8e9a19144aeb --- /dev/null +++ b/api/features/migrations/0067_add_feature_state_mv_hashing_salt.py @@ -0,0 +1,23 @@ +# Generated by Django 5.2.15 on 2026-06-30 07:49 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("features", "0066_constrain_feature_type"), + ] + + operations = [ + migrations.AddField( + model_name="featurestate", + name="mv_hashing_salt", + field=models.IntegerField(blank=True, default=None, null=True), + ), + migrations.AddField( + model_name="historicalfeaturestate", + name="mv_hashing_salt", + field=models.IntegerField(blank=True, default=None, null=True), + ), + ] diff --git a/api/features/models.py b/api/features/models.py index e30a6bbb9fda..2733d358e9ab 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -522,6 +522,13 @@ class FeatureState( # to be deprecated! version = models.IntegerField(default=1, null=True) + # Seed for multivariate variant bucketing. Defaults to the feature state's own + # id (see get_multivariate_feature_state_value), but is preserved across clones + # so that recreating a feature state (e.g. publishing a new version or editing + # multivariate weights under v2 versioning) does not re-bucket already-enrolled + # identities. See https://github.com/Flagsmith/flagsmith/issues/7913. + mv_hashing_salt = models.IntegerField(null=True, blank=True, default=None) + class Meta: ordering = ["id"] @@ -669,6 +676,10 @@ def clone( clone = deepcopy(self) clone.id = None clone.uuid = uuid.uuid4() + # Preserve the multivariate bucketing seed so that recreating this feature + # state does not re-bucket already-enrolled identities. If the source never + # had an explicit salt, capture its id (the seed used until now). + clone.mv_hashing_salt = self.mv_hashing_salt or self.id if self.feature_segment: # We can only create a new feature segment if we are cloning to another environment, @@ -769,7 +780,7 @@ def get_multivariate_feature_state_value( mv_options = list(self.multivariate_feature_state_values.all()) percentage_value = get_hashed_percentage_for_object_ids( - [self.id, identity_hash_key] + [self.mv_hashing_salt or self.id, identity_hash_key] ) # Iterate over the mv options in order of id (so we get the same value each @@ -818,6 +829,50 @@ def check_for_duplicate_feature_state(self): # type: ignore[no-untyped-def] "version, segment & identity combination" ) + @hook(BEFORE_CREATE) + def check_mv_hashing_salt_preserved(self): # type: ignore[no-untyped-def] + """Fail if a feature state is recreated without keeping its bucketing salt. + + Under v2 versioning a feature state must be recreated via clone(), which + copies mv_hashing_salt so multivariate variant assignment stays stable for + enrolled identities. clone() always sets a non-null salt, so a freshly + created state with no salt that the previous version already had was made + some other way and would re-bucket those identities. Raise so the + offending path is caught in tests. Identity overrides are skipped: they + are not versioned or cloned. See + https://github.com/Flagsmith/flagsmith/issues/7913. + """ + if ( + self.mv_hashing_salt is not None + or self.identity_id is not None + or not self.environment.use_v2_feature_versioning # type: ignore[union-attr] + ): + return + + previous_version = ( + self.environment_feature_version + and self.environment_feature_version.get_previous_version() + ) + if previous_version is None: + return + + # A segment override's FeatureSegment row is itself cloned per version, so + # match the override by its segment, not by the FeatureSegment row. + if self.feature_segment_id is None: + already_existed = Q(feature_segment__isnull=True) + else: + already_existed = Q( + feature_segment__segment_id=self.feature_segment.segment_id # type: ignore[union-attr] + ) + + if previous_version.feature_states.filter(already_existed).exists(): + raise ValidationError( + "A feature state for this environment, feature and segment existed " + "in the previous version; recreate it via FeatureState.clone() so " + "the mv_hashing_salt is kept and multivariate variant assignment " + "stays stable. See https://github.com/Flagsmith/flagsmith/issues/7913." + ) + @hook(BEFORE_CREATE) def set_live_from(self): # type: ignore[no-untyped-def] """ diff --git a/api/tests/unit/features/test_unit_features_models.py b/api/tests/unit/features/test_unit_features_models.py index f755d05e7041..5002b3498aa4 100644 --- a/api/tests/unit/features/test_unit_features_models.py +++ b/api/tests/unit/features/test_unit_features_models.py @@ -643,6 +643,191 @@ def test_get_multivariate_feature_state_value__with_identity__returns_correct_va assert multivariate_value.value != multivariate_value.initial_value +@mock.patch("features.models.get_hashed_percentage_for_object_ids") +def test_get_multivariate_feature_state_value__no_mv_hashing_salt__seeds_hash_with_id( # type: ignore[no-untyped-def] + mock_get_hashed_percentage, + multivariate_feature, + environment, + identity, +): + # Given + mock_get_hashed_percentage.return_value = 0.0 + feature_state = FeatureState.objects.get( + environment=environment, + feature=multivariate_feature, + identity=None, + feature_segment=None, + ) + assert feature_state.mv_hashing_salt is None + identity_hash_key = identity.get_hash_key() + + # When + feature_state.get_multivariate_feature_state_value( + identity_hash_key=identity_hash_key + ) + + # Then the feature state id seeds the hash + mock_get_hashed_percentage.assert_called_once_with( + [feature_state.id, identity_hash_key] + ) + + +@mock.patch("features.models.get_hashed_percentage_for_object_ids") +def test_get_multivariate_feature_state_value__mv_hashing_salt_set__seeds_hash_with_salt( # type: ignore[no-untyped-def] + mock_get_hashed_percentage, + multivariate_feature, + environment, + identity, +): + # Given + mock_get_hashed_percentage.return_value = 0.0 + feature_state = FeatureState.objects.get( + environment=environment, + feature=multivariate_feature, + identity=None, + feature_segment=None, + ) + feature_state.mv_hashing_salt = 999 + identity_hash_key = identity.get_hash_key() + + # When + feature_state.get_multivariate_feature_state_value( + identity_hash_key=identity_hash_key + ) + + # Then the salt seeds the hash instead of the feature state id + mock_get_hashed_percentage.assert_called_once_with([999, identity_hash_key]) + + +def test_feature_state_clone__multivariate_feature__keeps_variant_bucketing_stable( + multivariate_feature: Feature, + environment: Environment, + environment_two: Environment, +) -> None: + # Given the environment-default feature state for a multivariate feature, and + # the variant each of a range of identities is currently bucketed into + feature_state = FeatureState.objects.get( + environment=environment, + feature=multivariate_feature, + identity=None, + feature_segment=None, + ) + identity_hash_keys = [f"identity-{i}" for i in range(50)] + original_assignment = { + key: feature_state.get_multivariate_feature_state_value(key).id + for key in identity_hash_keys + } + + # When the feature state is recreated by cloning it (e.g. publishing a new + # version or editing multivariate weights under v2 versioning) + cloned_feature_state = feature_state.clone(env=environment_two, as_draft=True) + + # Then the clone keeps the original feature state's id as its bucketing salt + assert cloned_feature_state.id != feature_state.id + assert cloned_feature_state.mv_hashing_salt == feature_state.id + + # and every identity stays in the same variant as before + cloned_assignment = { + key: cloned_feature_state.get_multivariate_feature_state_value(key).id + for key in identity_hash_keys + } + assert cloned_assignment == original_assignment + + +def test_feature_state_clone__existing_mv_hashing_salt__is_preserved( + feature: Feature, + environment: Environment, + environment_two: Environment, +) -> None: + # Given a feature state that already carries a bucketing salt + feature_state = FeatureState.objects.get( + environment=environment, + feature=feature, + identity=None, + feature_segment=None, + ) + feature_state.mv_hashing_salt = 12345 + feature_state.save() + + # When it is cloned + cloned_feature_state = feature_state.clone(env=environment_two, as_draft=True) + + # Then the existing salt is carried over rather than the source id + assert cloned_feature_state.mv_hashing_salt == 12345 + + +def test_feature_state_create__recreates_previous_version_override_directly__raises( + environment_v2_versioning: Environment, + feature: Feature, + segment: Segment, +) -> None: + # Given an initial published version and a later (unpublished) version. The + # later version is created before the segment override exists, so the clone + # receiver does not copy the override into it. + initial_version = EnvironmentFeatureVersion.objects.get( + feature=feature, environment=environment_v2_versioning + ) + later_version = EnvironmentFeatureVersion.objects.create( + feature=feature, environment=environment_v2_versioning + ) + + # and the previous (initial) version gains a segment override + FeatureState.objects.create( + feature=feature, + environment=environment_v2_versioning, + environment_feature_version=initial_version, + feature_segment=FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment_v2_versioning, + environment_feature_version=initial_version, + ), + ) + + # When the same segment's override is recreated directly (not via clone) in + # the later version + # Then the guard rejects it + with pytest.raises(ValidationError): + FeatureState.objects.create( + feature=feature, + environment=environment_v2_versioning, + environment_feature_version=later_version, + feature_segment=FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment_v2_versioning, + environment_feature_version=later_version, + ), + ) + + +def test_feature_state_create__new_segment_override_under_v2__is_allowed( + environment_v2_versioning: Environment, + feature: Feature, + segment: Segment, +) -> None: + # Given a version whose previous version has no override for this segment + later_version = EnvironmentFeatureVersion.objects.create( + feature=feature, environment=environment_v2_versioning + ) + + # When a brand-new override for the segment is created directly + feature_state = FeatureState.objects.create( + feature=feature, + environment=environment_v2_versioning, + environment_feature_version=later_version, + feature_segment=FeatureSegment.objects.create( + feature=feature, + segment=segment, + environment=environment_v2_versioning, + environment_feature_version=later_version, + ), + ) + + # Then it is allowed: a genuinely new override starts a fresh seed + assert feature_state.mv_hashing_salt is None + + @mock.patch.object(FeatureState, "get_multivariate_feature_state_value") def test_get_feature_state_value__multivariate_feature__returns_mv_value( # type: ignore[no-untyped-def] mock_get_mv_feature_state_value, environment, multivariate_feature, identity diff --git a/api/tests/unit/util/mappers/test_unit_mappers_engine.py b/api/tests/unit/util/mappers/test_unit_mappers_engine.py index 667c6e57a7c8..75f49a25d998 100644 --- a/api/tests/unit/util/mappers/test_unit_mappers_engine.py +++ b/api/tests/unit/util/mappers/test_unit_mappers_engine.py @@ -240,6 +240,20 @@ def test_map_feature_state_to_engine__standard_feature__returns_expected_model( assert result == expected_result +def test_map_feature_state_to_engine__mv_hashing_salt_set__uses_salt_as_django_id( + feature_state: FeatureState, +) -> None: + # Given a feature state carrying a bucketing salt that differs from its id + feature_state.mv_hashing_salt = feature_state.id + 1000 + + # When + result = engine.map_feature_state_to_engine(feature_state, mv_fs_values=[]) + + # Then the salt is used as the engine document's django_id so that variant + # bucketing stays stable across feature state recreation + assert result.django_id == feature_state.mv_hashing_salt + + def test_map_feature_state_to_engine__feature_segment__return_expected( segment_multivariate_feature_state: FeatureState, multivariate_feature: "Feature", @@ -710,7 +724,10 @@ def test_map_environment_to_engine__after_v2_versioning_migration__returns_lates assert len(result.feature_states) == 1 mapped_environment_feature_state = result.feature_states[0] - assert mapped_environment_feature_state.django_id == v2_environment_feature_state.id + assert ( + mapped_environment_feature_state.featurestate_uuid + == v2_environment_feature_state.uuid + ) assert mapped_environment_feature_state.enabled is True assert ( mapped_environment_feature_state.feature_state_value @@ -721,7 +738,7 @@ def test_map_environment_to_engine__after_v2_versioning_migration__returns_lates assert len(result.project.segments[0].feature_states) == 1 mapped_segment_override = result.project.segments[0].feature_states[0] - assert mapped_segment_override.django_id == v2_segment_override.id + assert mapped_segment_override.featurestate_uuid == v2_segment_override.uuid assert mapped_segment_override.enabled is True assert mapped_segment_override.feature_state_value == v2_segment_override_value @@ -777,8 +794,8 @@ def test_map_environment_to_engine__v2_versioning_segment_override_removed__retu # Then assert len(environment_model.project.segments[0].feature_states) == 1 assert ( - environment_model.project.segments[0].feature_states[0].django_id - == v3_segment_override.id + environment_model.project.segments[0].feature_states[0].featurestate_uuid + == v3_segment_override.uuid ) diff --git a/api/util/mappers/engine.py b/api/util/mappers/engine.py index ef72abdb9511..9438539f1b84 100644 --- a/api/util/mappers/engine.py +++ b/api/util/mappers/engine.py @@ -152,7 +152,12 @@ def map_feature_state_to_engine( return FeatureStateModel( enabled=feature_state.enabled, - django_id=feature_state.pk, + # Use the multivariate bucketing salt (falling back to the pk) as the + # engine document's django_id. The engine and SDKs seed multivariate + # variant allocation on django_id, so feeding it the salt keeps variant + # assignment stable when a feature state is recreated, without changing + # the engine model or environment document schema. See issue #7913. + django_id=feature_state.mv_hashing_salt or feature_state.pk, feature_state_value=feature_state.get_feature_state_value(), featurestate_uuid=feature_state.uuid, feature_segment=feature_segment_model,