Skip to content

Commit

Permalink
feat(nimbus): Warn about prefFlips that could use setPref variables (#…
Browse files Browse the repository at this point in the history
…11143)

Because

- we now have multiple ways to set prefs on Firefox Desktop (setPref
variables and the prefFlips feature); and
- setting the same pref via both mechanisms can cause unenrollments

this commit:

- adds warnings for the prefFlips feature when existing setPref
variables could be used.

Fixes #10842
  • Loading branch information
brennie committed Aug 2, 2024
1 parent 8b3341b commit b3d57ba
Show file tree
Hide file tree
Showing 6 changed files with 278 additions and 31 deletions.
95 changes: 81 additions & 14 deletions experimenter/experimenter/experiments/api/v5/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from typing import Any, NotRequired, Optional, Self, TypedDict

import jsonschema
import packaging
from django.conf import settings
from django.contrib.auth.models import User
from django.db import models, transaction
Expand Down Expand Up @@ -1231,6 +1232,10 @@ def __init__(self, *args, **kwargs):
str, NimbusFeatureConfig.VersionedSchemaRange
] = {}

self.desktop_setpref_schemas: Optional[
dict[NimbusFeatureConfig, NimbusVersionedSchema]
] = None

def to_representation(self, instance):
data = super().to_representation(instance)
data["feature_config"] = None
Expand Down Expand Up @@ -1399,7 +1404,7 @@ def _validate_branch(
feature_config=feature_config,
feature_value=feature_value_data["value"],
localizations=localizations,
schemas_in_range=self.schemas_by_feature_id[feature_config.id],
schemas_in_range=self.schemas_by_feature_id[feature_config.slug],
suppress_errors=suppress_errors,
)

Expand Down Expand Up @@ -1428,9 +1433,8 @@ def append(self, msg: str, warning: bool):
else:
self.errors.append(msg)

@classmethod
def _validate_feature_value(
cls,
self,
*,
application: str,
channel: str,
Expand All @@ -1440,7 +1444,7 @@ def _validate_feature_value(
localizations: Optional[dict[str, Any]],
suppress_errors: bool,
) -> ValidateFeatureResult:
result = cls.ValidateFeatureResult()
result = self.ValidateFeatureResult()

if schemas_in_range.unsupported_in_range:
result.append(
Expand All @@ -1467,7 +1471,7 @@ def _validate_feature_value(

if application == NimbusExperiment.Application.DESKTOP:
result.extend_with_result(
cls._validate_feature_value_with_schema(
self._validate_feature_value_with_schema(
feature_config,
schemas_in_range,
feature_value,
Expand All @@ -1477,7 +1481,7 @@ def _validate_feature_value(
)
else:
result.extend(
cls._validate_feature_value_with_fml(
self._validate_feature_value_with_fml(
schemas_in_range,
NimbusFmlLoader.create_loader(application, channel),
feature_config,
Expand All @@ -1488,37 +1492,36 @@ def _validate_feature_value(

return result

@classmethod
def _validate_feature_value_with_schema(
cls,
self,
feature_config: NimbusFeatureConfig,
schemas_in_range: NimbusFeatureConfig.VersionedSchemaRange,
value: str,
localizations: Optional[dict[str, Any]],
suppress_errors: bool,
) -> ValidateFeatureResult:
result = cls.ValidateFeatureResult()
result = self.ValidateFeatureResult()

json_value = json.loads(value)
for schema in schemas_in_range.schemas:
if schema.schema: # Only None in tests.
json_schema = json.loads(schema.schema)
if not localizations:
result.extend(
cls._validate_schema(json_value, json_schema, schema.version),
self._validate_schema(json_value, json_schema, schema.version),
suppress_errors,
)
else:
for locale_code, substitutions in localizations.items():
try:
substituted_value = cls._substitute_localizations(
substituted_value = self._substitute_localizations(
json_value, substitutions, locale_code
)
except LocalizationError as e:
result.append(str(e), suppress_errors)
continue

if schema_errors := cls._validate_schema(
if schema_errors := self._validate_schema(
substituted_value, json_schema, schema.version
):
err_msg = (
Expand Down Expand Up @@ -1546,14 +1549,78 @@ def _validate_feature_value_with_schema(
continue

result.extend_with_result(
cls._validate_desktop_feature_value_pref_lengths(
self._validate_desktop_feature_value_pref_lengths(
feature_config,
schema,
json_value,
suppress_errors,
),
)

if feature_config.slug == NimbusConstants.DESKTOP_PREFFLIPS_SLUG:
result.extend_with_result(
self._validate_desktop_pref_flips_value(
json_value,
schemas_in_range.min_version,
schemas_in_range.max_version,
)
)

return result

def _validate_desktop_pref_flips_value(
self,
value: dict[str, Any],
min_version: packaging.version.Version,
max_version: Optional[packaging.version.Version],
) -> ValidateFeatureResult:
result = self.ValidateFeatureResult()

if not self.desktop_setpref_schemas:
schemas = (
NimbusVersionedSchema.objects.filter(
NimbusFeatureVersion.objects.between_versions_q(
min_version,
max_version,
prefix="version",
),
feature_config__application=NimbusExperiment.Application.DESKTOP,
)
.exclude(set_pref_vars={})
.prefetch_related("feature_config", "version")
)

self.desktop_setpref_schemas = {}
for schema in schemas:
self.desktop_setpref_schemas.setdefault(schema.feature_config, []).append(
schema
)

for pref in value.get("prefs", {}):
for feature_config, schemas in self.desktop_setpref_schemas.items():
conflicting_versions = []

for schema in schemas:
if pref in schema.set_pref_vars.values():
conflicting_versions.append(schema.version.as_packaging_version())

if conflicting_versions:
if len(conflicting_versions) == 1:
versions = str(conflicting_versions[0])
else:
versions = (
f"{min(conflicting_versions)}-{max(conflicting_versions)}"
)

msg = NimbusConstants.WARNING_FEATURE_VALUE_IN_VERSIONS.format(
versions=versions,
warning=NimbusConstants.WARNING_PREF_FLIPS_PREF_CONTROLLED_BY_FEATURE.format(
pref=pref, feature_config_slug=feature_config.slug
),
)

result.append(msg, warning=True)

return result

@classmethod
Expand Down Expand Up @@ -1692,7 +1759,7 @@ def _validate_feature_configs(self, data):
# Cache the versioned schema range for each feature so we can re-use
# them in the validation for each branch.
self.schemas_by_feature_id = {
feature_config.id: feature_config.get_versioned_schema_range(
feature_config.slug: feature_config.get_versioned_schema_range(
min_version,
max_version,
)
Expand Down
5 changes: 5 additions & 0 deletions experimenter/experimenter/experiments/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,7 @@ class EmailType(models.TextChoices):
)

ERROR_FEATURE_VALUE_IN_VERSIONS = "In versions {versions}: {error}"
WARNING_FEATURE_VALUE_IN_VERSIONS = "Warning: In versions {versions}: {warning}"

WARNING_ROLLOUT_PREF_REENROLL = (
"WARNING: One or more features of this rollouts sets prefs and this rollout is "
Expand Down Expand Up @@ -799,6 +800,10 @@ class EmailType(models.TextChoices):
"The prefFlips feature is only available on Firefox 128 on the ESR branch."
)

WARNING_PREF_FLIPS_PREF_CONTROLLED_BY_FEATURE = (
"Pref '{pref}' is controlled by a variable in feature {feature_config_slug}'"
)


RISK_QUESTIONS = {
"BRAND": (
Expand Down
21 changes: 17 additions & 4 deletions experimenter/experimenter/experiments/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1480,8 +1480,8 @@ def __str__(self): # pragma: no cover

def schemas_between_versions(
self,
min_version: packaging.version,
max_version: Optional[packaging.version],
min_version: packaging.version.Version,
max_version: Optional[packaging.version.Version],
) -> QuerySet["NimbusVersionedSchema"]:
return (
self.schemas.filter(
Expand All @@ -1495,6 +1495,12 @@ def schemas_between_versions(

@dataclass
class VersionedSchemaRange:
# The minimum version of the range.
min_version: packaging.version.Version

# The maximum version of the range.
max_version: Optional[packaging.version.Version]

# The versioned schemas in the requested range, or a single element list
# with an unversioned schema.
schemas: list["NimbusVersionedSchema"]
Expand All @@ -1507,8 +1513,8 @@ class VersionedSchemaRange:

def get_versioned_schema_range(
self,
min_version: packaging.version,
max_version: Optional[packaging.version],
min_version: packaging.version.Version,
max_version: Optional[packaging.version.Version],
) -> VersionedSchemaRange:
unsupported_versions: list[NimbusFeatureVersion] = []

Expand Down Expand Up @@ -1576,6 +1582,8 @@ def get_versioned_schema_range(
# There are versioned schemas outside this range. This feature
# is unsupported in this range.
return NimbusFeatureConfig.VersionedSchemaRange(
min_version=min_version,
max_version=max_version,
schemas=[],
unsupported_in_range=True,
unsupported_versions=[],
Expand All @@ -1586,6 +1594,8 @@ def get_versioned_schema_range(
schemas = [self.schemas.get(version=None)]

return NimbusFeatureConfig.VersionedSchemaRange(
min_version=min_version,
max_version=max_version,
schemas=schemas,
unsupported_in_range=False,
unsupported_versions=unsupported_versions,
Expand Down Expand Up @@ -1655,6 +1665,9 @@ def __repr__(self): # pragma: no cover
def __str__(self): # pragma: no cover
return f"{self.major}.{self.minor}.{self.patch}"

def as_packaging_version(self) -> packaging.version.Version:
return packaging.version.parse(str(self))


class NimbusVersionedSchema(models.Model):
feature_config = models.ForeignKey(
Expand Down
Loading

0 comments on commit b3d57ba

Please sign in to comment.