Skip to content

Commit

Permalink
bug(nimbus): Filter versioned schemas inclusively
Browse files Browse the repository at this point in the history 
Because:

- the firefox_max_version logic uses inclusive ranges; and
- filtering schema ranges was using exclusive ranges, which is not
  correct behaviour

This commit

- updates schema ranges to be inclusive

Fixes #11002
  • Loading branch information
@brennie
brennie committed Dec 18, 2024
1 parent 8638eeb commit 736c5e9
Show file tree
Hide file tree
Showing 4 changed files with 181 additions and 42 deletions.
12 changes: 9 additions & 3 deletions experimenter/experimenter/experiments/api/v5/serializers.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1464,12 +1464,18 @@ def _validate_feature_value(
unsupported_version_strs = [
str(v) for v in schemas_in_range.unsupported_versions
]
min_unsupported_version = min(unsupported_version_strs)
max_unsupported_version = max(unsupported_version_strs)
if len(unsupported_version_strs) == 1:
unsupported_versions = unsupported_version_strs[0]
else:
min_unsupported_version = min(unsupported_version_strs)
max_unsupported_version = max(unsupported_version_strs)
unsupported_versions = (
f"{min_unsupported_version}-{max_unsupported_version}"
)
result.append(
NimbusConstants.ERROR_FEATURE_CONFIG_UNSUPPORTED_IN_VERSIONS.format(
feature_config=feature_config.name,
versions=f"{min_unsupported_version}-{max_unsupported_version}",
versions=unsupported_versions,
),
suppress_errors,
)
Expand Down
51 changes: 35 additions & 16 deletions experimenter/experimenter/experiments/models.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1825,7 +1825,7 @@ def get_versioned_schema_range(
min_supported_version = NimbusExperiment.Version.parse(min_supported_version)

if min_supported_version > min_version:
if max_version is not None and min_supported_version >= max_version:
if max_version is not None and min_supported_version > max_version:
# We will not have any NimbusVerionedSchemas in this
# version range. The best we can do is use the
# unversioned schema.
Expand Down Expand Up @@ -1910,6 +1910,23 @@ def between_versions_q(
*,
prefix: Optional[str] = None,
) -> Q:
"""Return a query object that can be used to select all versions between lower and
upper bounds (inclusive).
Args:
min_version:
The lower bound (inclusive).
max_version:
The upper bound (inclusive).
prefix:
An optional prefix to prepend to the field names. This allows the Q object
to be used by related models.
Returns:
The query object.
"""
if prefix is not None:

def prefixed(**kwargs: dict[str, Any]):
Expand All @@ -1921,27 +1938,29 @@ def prefixed(**kwargs: dict[str, Any]):
return kwargs

# (a, b, c) >= (d, e, f)
# := (a > b) | (a = b & d > e) | (a = b & d = e & c >= f)
# == (a > b) | (a = b & (d > e | (d = e & c >= f)))
# := (a > d) | (a == d & b > e) | (a == d & b == e & c >= f)
# == (a > d) | (a == d & (b > e | (b == e & c >= f)

# packaging.version.Version uses major.minor.micro, but
# NimbusFeatureVersion uses major.minor.patch (semver).
q = Q(**prefixed(major__gt=min_version.major)) | Q(
**prefixed(major=min_version.major)
) & (
Q(**prefixed(minor__gt=min_version.minor))
| Q(**prefixed(minor=min_version.minor, patch__gte=min_version.micro))
q = Q(**prefixed(major__gt=min_version.major)) | (
Q(**prefixed(major=min_version.major))
& (
Q(**prefixed(minor__gt=min_version.minor))
| Q(**prefixed(minor=min_version.minor, patch__gte=min_version.micro))
)
)

if max_version is not None:
# (a, b, c) < (d, e, f)
# := (a < d) | (a == d & b < e) | (a == d & b == e & c < f)
# == (a < d) | (a == d & (b < e | (b == e & c < f)))
q &= Q(**prefixed(major__lt=max_version.major)) | Q(
**prefixed(major=max_version.major)
) & (
Q(**prefixed(minor__lt=max_version.minor))
| Q(**prefixed(minor=max_version.minor, patch__lt=max_version.micro))
# (a, b, c) <= (d, e, f)
# := (a < d) | (a == d & b < e) | (a = d & b == e & c <= f)
# == (a < d) | (a == d & (b < e | (b == e & c <= f)))
q &= Q(**prefixed(major__lt=max_version.major)) | (
Q(**prefixed(major=max_version.major))
& (
Q(**prefixed(minor__lt=max_version.minor))
| Q(**prefixed(minor=max_version.minor, patch__lte=max_version.micro))
)
)

return q
Expand Down
154 changes: 133 additions & 21 deletions ...nter/experiments/tests/api/v5/test_serializers/test_nimbus_ready_for_review_serializer.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3196,6 +3196,71 @@ def setUp(self):
)
}

@parameterized.expand(
[
# min == max
(NimbusExperiment.Version.FIREFOX_120, NimbusExperiment.Version.FIREFOX_120),
(NimbusExperiment.Version.FIREFOX_121, NimbusExperiment.Version.FIREFOX_121),
(NimbusExperiment.Version.FIREFOX_122, NimbusExperiment.Version.FIREFOX_122),
# min <= max (bounded)
(NimbusExperiment.Version.FIREFOX_120, NimbusExperiment.Version.FIREFOX_121),
(NimbusExperiment.Version.FIREFOX_120, NimbusExperiment.Version.FIREFOX_122),
(NimbusExperiment.Version.FIREFOX_121, NimbusExperiment.Version.FIREFOX_122),
# min <= max (unbounded)
(NimbusExperiment.Version.FIREFOX_120, NimbusExperiment.Version.NO_VERSION),
(NimbusExperiment.Version.FIREFOX_121, NimbusExperiment.Version.NO_VERSION),
(NimbusExperiment.Version.FIREFOX_122, NimbusExperiment.Version.NO_VERSION),
]
)
def test_validate_feature_range_valid(self, min_version, max_version):
feature = NimbusFeatureConfigFactory.create(
application=NimbusExperiment.Application.DESKTOP,
slug="FEATURE",
name="FEATURE",
schemas=[
NimbusVersionedSchemaFactory.build(version=None, schema=None),
NimbusVersionedSchemaFactory.build(
version=self.versions[(120, 0, 0)],
schema=BASIC_JSON_SCHEMA,
),
NimbusVersionedSchemaFactory.build(
version=self.versions[(121, 0, 0)],
schema=BASIC_JSON_SCHEMA,
),
NimbusVersionedSchemaFactory.build(
version=self.versions[(122, 0, 0)],
schema=BASIC_JSON_SCHEMA,
),
],
)

experiment = NimbusExperimentFactory.create_with_lifecycle(
NimbusExperimentFactory.Lifecycles.CREATED,
targeting_config_slug=NimbusExperiment.TargetingConfig.NO_TARGETING,
application=NimbusExperiment.Application.DESKTOP,
firefox_min_version=min_version,
firefox_max_version=max_version,
feature_configs=[feature],
warn_feature_schema=False,
)

for branch in experiment.treatment_branches:
branch.delete()

feature_value = experiment.reference_branch.feature_values.get(
feature_config=feature,
)
feature_value.value = json.dumps({"directMigrateSingleProfile": True})
feature_value.save()

serializer = NimbusReviewSerializer(
experiment,
data=NimbusReviewSerializer(experiment, context={"user": self.user}).data,
context={"user", self.user},
)

self.assertTrue(serializer.is_valid(), serializer.errors)

@parameterized.expand(
chain(
*(
Expand All @@ -3205,13 +3270,13 @@ def setUp(self):
NimbusExperiment.Version.NO_VERSION,
NimbusConstants.ERROR_FEATURE_CONFIG_UNSUPPORTED_IN_VERSIONS.format(
feature_config="FEATURE",
versions="121.0.0-121.0.0",
versions="121.0.0",
),
as_warning,
),
(
NimbusExperiment.Version.FIREFOX_121,
NimbusExperiment.Version.FIREFOX_122,
NimbusExperiment.Version.FIREFOX_121,
NimbusConstants.ERROR_FEATURE_CONFIG_UNSUPPORTED_IN_RANGE.format(
feature_config="FEATURE",
),
Expand All @@ -3225,6 +3290,9 @@ def setUp(self):
def test_validate_feature_versioned_unsupported_versions(
self, min_version, max_version, expected_error, as_warning
):
"""Testing feature validation with unsupported versions when using the
warn_feature_schema compared to not using it
"""
feature = NimbusFeatureConfigFactory.create(
application=NimbusExperiment.Application.DESKTOP,
slug="FEATURE",
Expand Down Expand Up @@ -3305,12 +3373,12 @@ def test_validate_feature_versioned_unsupported_versions(
),
(
NimbusExperiment.Version.FIREFOX_110,
NimbusExperiment.Version.FIREFOX_121,
NimbusExperiment.Version.FIREFOX_120,
[(120, 0, 0)],
),
]
)
def test_validate_feature_versioned_truncated_range(
def test_validate_feature_versioned_truncated_range_schema_errors(
self, min_version, max_version, expected_versions
):
schema = json.dumps(
Expand Down Expand Up @@ -3387,7 +3455,7 @@ def test_validate_feature_versioned_truncated_range(
},
)

def test_validate_feature_versioned_before_versioned_range(self):
def test_validate_feature_versioned_before_versioned_range_valid(self):
feature = NimbusFeatureConfigFactory.create(
application=NimbusExperiment.Application.DESKTOP,
slug="FEATURE",
Expand Down Expand Up @@ -3442,7 +3510,48 @@ def test_validate_feature_versioned_before_versioned_range(self):

self.assertTrue(serializer.is_valid(), serializer.errors)

def test_validate_feature_versioned(self):
@parameterized.expand(
[
(
NimbusExperiment.Version.FIREFOX_120,
NimbusExperiment.Version.FIREFOX_120,
["120.0.0"],
),
(
NimbusExperiment.Version.FIREFOX_121,
NimbusExperiment.Version.FIREFOX_121,
["121.0.0"],
),
(
NimbusExperiment.Version.FIREFOX_122,
NimbusExperiment.Version.FIREFOX_122,
["122.0.0"],
),
(
NimbusExperiment.Version.FIREFOX_120,
NimbusExperiment.Version.FIREFOX_121,
["121.0.0", "120.0.0"],
),
(
NimbusExperiment.Version.FIREFOX_120,
NimbusExperiment.Version.NO_VERSION,
["122.0.0", "121.0.0", "120.0.0"],
),
(
NimbusExperiment.Version.FIREFOX_121,
NimbusExperiment.Version.NO_VERSION,
["122.0.0", "121.0.0"],
),
(
NimbusExperiment.Version.FIREFOX_122,
NimbusExperiment.Version.NO_VERSION,
["122.0.0"],
),
]
)
def test_validate_feature_versioned_schema_errors(
self, min_version, max_version, error_versions
):
json_schema = json.dumps(
{
"type": "object",
Expand All @@ -3467,15 +3576,18 @@ def test_validate_feature_versioned(self):
NimbusVersionedSchemaFactory.build(
version=self.versions[(121, 0, 0)], schema=json_schema
),
NimbusVersionedSchemaFactory.build(
version=self.versions[(122, 0, 0)], schema=json_schema
),
],
)

experiment = NimbusExperimentFactory.create_with_lifecycle(
NimbusExperimentFactory.Lifecycles.CREATED,
targeting_config_slug=NimbusExperiment.TargetingConfig.NO_TARGETING,
application=NimbusExperiment.Application.DESKTOP,
firefox_min_version=NimbusExperiment.Version.FIREFOX_120,
firefox_max_version=NimbusExperiment.Version.FIREFOX_122,
firefox_min_version=min_version,
firefox_max_version=max_version,
feature_configs=[feature],
)

Expand All @@ -3502,16 +3614,16 @@ def test_validate_feature_versioned(self):
"feature_values": [
{
"value": [
"1 is not of type 'boolean' at version 121.0.0",
"1 is not of type 'boolean' at version 120.0.0",
f"1 is not of type 'boolean' at version {error_version}"
for error_version in error_versions
],
}
]
}
},
)

def test_validate_feature_versioned_localized(self):
def test_validate_feature_versioned_localized_schema_errors(self):
json_schema = json.dumps(
{
"type": "object",
Expand Down Expand Up @@ -3626,19 +3738,19 @@ def test_validate_feature_versioned_localized(self):
NimbusExperiment.Version.NO_VERSION,
NimbusConstants.ERROR_FEATURE_CONFIG_UNSUPPORTED_IN_VERSIONS.format(
feature_config="FEATURE",
versions="121.0.0-121.0.0",
versions="121.0.0",
),
),
(
NimbusExperiment.Version.FIREFOX_121,
NimbusExperiment.Version.FIREFOX_122,
NimbusExperiment.Version.FIREFOX_121,
NimbusConstants.ERROR_FEATURE_CONFIG_UNSUPPORTED_IN_RANGE.format(
feature_config="FEATURE",
),
),
]
)
def test_fml_validate_feature_versioned_unsupported_versions(
def test_fml_validate_feature_versioned_unsupported_versions_error(
self,
min_version,
max_version,
Expand Down Expand Up @@ -3703,7 +3815,7 @@ def test_fml_validate_feature_versioned_unsupported_versions(
serializer.errors,
)

def test_fml_validate_feature_versioned_truncated_range(self):
def test_fml_validate_feature_versioned_truncated_range_fml_error(self):
self.setup_get_fml_errors(
[
NimbusFmlErrorDataClass(
Expand Down Expand Up @@ -3788,7 +3900,7 @@ def test_fml_validate_feature_versioned_truncated_range(self):
serializer.errors,
)

def test_fml_validate_feature_versioned_before_versioned_range(self):
def test_fml_validate_feature_versioned_before_versioned_range_valid(self):
feature = NimbusFeatureConfigFactory.create(
application=NimbusExperiment.Application.FENIX,
slug="FEATURE",
Expand Down Expand Up @@ -3843,7 +3955,7 @@ def test_fml_validate_feature_versioned_before_versioned_range(self):

self.assertTrue(serializer.is_valid(), serializer.errors)

def test_fml_validate_feature_versioned(self):
def test_fml_validate_feature_versioned_fml_error(self):
fml_errors = [
NimbusFmlErrorDataClass(
line=2,
Expand Down Expand Up @@ -3923,7 +4035,7 @@ def test_fml_validate_feature_versioned(self):
serializer.errors,
)

def test_fml_validate_feature_versioned_range_treatment_branch(self):
def test_fml_validate_feature_versioned_range_treatment_branch_fml_errors(self):
fml_errors = [
NimbusFmlErrorDataClass(
line=2,
Expand Down Expand Up @@ -3970,8 +4082,8 @@ def test_fml_validate_feature_versioned_range_treatment_branch(self):
NimbusExperimentFactory.Lifecycles.CREATED,
targeting_config_slug=NimbusExperiment.TargetingConfig.NO_TARGETING,
application=NimbusExperiment.Application.FENIX,
firefox_min_version=NimbusExperiment.Version.FIREFOX_120,
firefox_max_version=NimbusExperiment.Version.FIREFOX_120,
firefox_min_version=NimbusExperiment.Version.FIREFOX_121,
firefox_max_version=NimbusExperiment.Version.FIREFOX_121,
feature_configs=[feature],
)

Expand All @@ -3997,7 +4109,7 @@ def test_fml_validate_feature_versioned_range_treatment_branch(self):
serializer.errors["treatment_branches"][0]["feature_values"][0]["value"][0],
)

def test_fml_validate_feature_versions_no_errors(self):
def test_fml_validate_feature_versioned_unbounded_range_valid(self):
self.setup_fml_no_errors()

feature = NimbusFeatureConfigFactory.create(
Expand Down
Loading

0 comments on commit 736c5e9

Please sign in to comment.