Skip to content

Commit

Permalink
bug(nimbus): Filter versioned schemas inclusively (#11965)
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 authored Dec 18, 2024
1 parent 2738e9d commit 5dbf2db
Show file tree
Hide file tree
Showing 4 changed files with 266 additions and 53 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 @@ -1468,12 +1468,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
Loading

0 comments on commit 5dbf2db

Please sign in to comment.