diff --git a/experimenter/experimenter/experiments/api/v5/serializers.py b/experimenter/experimenter/experiments/api/v5/serializers.py index 0ed8dda482..3d0cb17538 100644 --- a/experimenter/experimenter/experiments/api/v5/serializers.py +++ b/experimenter/experimenter/experiments/api/v5/serializers.py @@ -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, ) diff --git a/experimenter/experimenter/experiments/models.py b/experimenter/experimenter/experiments/models.py index 8bdcbc433b..72034f81a5 100644 --- a/experimenter/experimenter/experiments/models.py +++ b/experimenter/experimenter/experiments/models.py @@ -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. @@ -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]): @@ -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 diff --git a/experimenter/experimenter/experiments/tests/api/v5/test_serializers/test_nimbus_ready_for_review_serializer.py b/experimenter/experimenter/experiments/tests/api/v5/test_serializers/test_nimbus_ready_for_review_serializer.py index 83b070500a..7cb16853db 100644 --- a/experimenter/experimenter/experiments/tests/api/v5/test_serializers/test_nimbus_ready_for_review_serializer.py +++ b/experimenter/experimenter/experiments/tests/api/v5/test_serializers/test_nimbus_ready_for_review_serializer.py @@ -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( *( @@ -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", ), @@ -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", @@ -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( @@ -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", @@ -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", @@ -3467,6 +3576,9 @@ 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 + ), ], ) @@ -3474,8 +3586,8 @@ def test_validate_feature_versioned(self): 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], ) @@ -3502,8 +3614,8 @@ 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 ], } ] @@ -3511,7 +3623,7 @@ def test_validate_feature_versioned(self): }, ) - def test_validate_feature_versioned_localized(self): + def test_validate_feature_versioned_localized_schema_errors(self): json_schema = json.dumps( { "type": "object", @@ -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, @@ -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( @@ -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", @@ -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, @@ -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, @@ -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], ) @@ -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( diff --git a/experimenter/experimenter/experiments/tests/test_models.py b/experimenter/experimenter/experiments/tests/test_models.py index a8d0971e74..f1e69bfb0c 100644 --- a/experimenter/experimenter/experiments/tests/test_models.py +++ b/experimenter/experimenter/experiments/tests/test_models.py @@ -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) ) @@ -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) ) @@ -4001,6 +4001,7 @@ def test_schemas_between_versions(self): (2, 0, 1), (2, 0, 2), (2, 1, 0), + (2, 1, 1), ) }, ) @@ -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),