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 committed Dec 17, 2024
1 parent 8638eeb commit 106397d
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 28 deletions.
12 changes: 9 additions & 3 deletions experimenter/experimenter/experiments/api/v5/serializers.py
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -3205,13 +3205,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 Down Expand Up @@ -3305,7 +3305,7 @@ def test_validate_feature_versioned_unsupported_versions(
),
(
NimbusExperiment.Version.FIREFOX_110,
NimbusExperiment.Version.FIREFOX_121,
NimbusExperiment.Version.FIREFOX_120,
[(120, 0, 0)],
),
]
Expand Down Expand Up @@ -3626,12 +3626,12 @@ 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",
),
Expand Down Expand Up @@ -3970,8 +3970,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 Down
6 changes: 4 additions & 2 deletions experimenter/experimenter/experiments/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -3965,7 +3965,7 @@ def test_schemas_between_versions(self):
minor=minor,
patch=patch,
)
for major in range(1, 3)
for major in range(1, 4)
for minor in range(3)
for patch in range(3)
)
Expand All @@ -3978,7 +3978,7 @@ def test_schemas_between_versions(self):
feature_config=feature,
version=versions[(major, minor, patch)],
)
for major in range(1, 3)
for major in range(1, 4)
for minor in range(3)
for patch in range(3)
)
Expand All @@ -4001,6 +4001,7 @@ def test_schemas_between_versions(self):
(2, 0, 1),
(2, 0, 2),
(2, 1, 0),
(2, 1, 1),
)
},
)
Expand Down Expand Up @@ -4190,6 +4191,7 @@ def test_get_versioned_schema_range(self):
schemas=[
schemas[versions[v]]
for v in (
(123, 1, 0),
(123, 0, 0),
(122, 1, 0),
(122, 0, 0),
Expand Down

0 comments on commit 106397d

Please sign in to comment.