From a9c9fbcb7c0875ec57eca6b12981384bf996a8cd Mon Sep 17 00:00:00 2001 From: Mike Cooper Date: Wed, 29 Mar 2017 10:48:01 -0700 Subject: [PATCH] recipe-server: Add per-action validation of recipe arguments Fixes #599 --- recipe-server/normandy/recipes/models.py | 58 ++++++++++++++++++- .../normandy/recipes/tests/test_api.py | 2 +- .../normandy/recipes/tests/test_models.py | 52 ++++++++++++++++- 3 files changed, 109 insertions(+), 3 deletions(-) diff --git a/recipe-server/normandy/recipes/models.py b/recipe-server/normandy/recipes/models.py index c125f3164..52396623e 100644 --- a/recipe-server/normandy/recipes/models.py +++ b/recipe-server/normandy/recipes/models.py @@ -250,7 +250,7 @@ def save(self, *args, **kwargs): super().save(*args, **kwargs) -class RecipeRevision(models.Model): +class RecipeRevision(DirtyFieldsMixin, models.Model): id = models.CharField(max_length=64, primary_key=True) parent = models.OneToOneField('self', null=True, on_delete=models.CASCADE, related_name='child') @@ -324,6 +324,11 @@ def hash(self): return hashlib.sha256(data.encode()).hexdigest() def save(self, *args, **kwargs): + # validation + previous_arguments = self.parent.arguments if self.parent else None + self.action.validate_arguments(self.arguments, old_arguments=previous_arguments) + + # automatic fields if not self.created: self.created = timezone.now() self.id = self.hash() @@ -341,6 +346,18 @@ class Action(models.Model): arguments_schema_json = models.TextField(default='{}', validators=[validate_json]) + errors = { + 'duplicate_branch_slug': ('Feature branch slugs must be unique within an ' + 'experiment: "{slug}" is duplicated.'), + 'duplicate_branch_value': ('Feature branch values must be unique within an ' + 'experiment: "{value}" is duplicated.'), + 'duplicate_experiment_slug': ('Experiment slugs must be globally unique: "{slug}" ' + 'is duplicated'), + 'immutable_branches': ('Experiment branches cannot change after experiment ' + 'creation. To change the feature branches, launch a ' + 'new experiment'), + } + @property def arguments_schema(self): return json.loads(self.arguments_schema_json) @@ -373,6 +390,45 @@ def save(self, *args, **kwargs): self.implementation_hash = self.compute_implementation_hash() super().save(update_fields=['implementation_hash']) + def validate_arguments(self, arguments, old_arguments=None): + """ + Test if `arguments` follows all action-specific rules. + + If there is a previous version of the arguments, they should + be passed as `old_arguments`. Some validation rules depend on + the history of the arguments. + + Raises `ValidationError` if any rules are violated. + """ + if self.name == 'preference-experiment': + # Feature branch slugs should be unique within an experiment. + branch_slugs = set() + branch_values = set() + for branch in arguments['branches']: + if branch['slug'] in branch_slugs: + raise ValidationError(self.errors['duplicate_branch_slug'] + .format(slug=branch['slug'])) + if branch['value'] in branch_values: + raise ValidationError(self.errors['duplicate_branch_value'] + .format(value=branch['value'])) + branch_slugs.add(branch['slug']) + branch_values.add(branch['value']) + + # Experiment slugs should be unique. + experiment_recipes = Recipe.objects.filter(latest_revision__action=self) + experiment_slugs = set() + if old_arguments is None: + # This is a newly created revision, so its slug should not be in the DB + experiment_slugs.add(arguments['slug']) + for recipe in experiment_recipes: + if recipe.arguments['slug'] in experiment_slugs: + raise ValidationError(self.errors['duplicate_experiment_slug'] + .format(slug=arguments['slug'])) + + # Once published, branches of a feature experiment cannot be modified. + if old_arguments is not None and arguments['branches'] != old_arguments['branches']: + raise ValidationError(self.errors['immutable_branches']) + class Client(object): """A client attempting to fetch a set of recipes.""" diff --git a/recipe-server/normandy/recipes/tests/test_api.py b/recipe-server/normandy/recipes/tests/test_api.py index 316899235..8598f9c28 100644 --- a/recipe-server/normandy/recipes/tests/test_api.py +++ b/recipe-server/normandy/recipes/tests/test_api.py @@ -232,7 +232,7 @@ def test_it_can_change_action_for_recipes(self, api_client): assert recipe.action == action def test_it_can_change_arguments_for_recipes(self, api_client): - recipe = RecipeFactory(arguments_json='') + recipe = RecipeFactory(arguments_json='{}') action = ActionFactory( name='foobarbaz', arguments_schema={ diff --git a/recipe-server/normandy/recipes/tests/test_models.py b/recipe-server/normandy/recipes/tests/test_models.py index 8ec3ffd2a..af716ab07 100644 --- a/recipe-server/normandy/recipes/tests/test_models.py +++ b/recipe-server/normandy/recipes/tests/test_models.py @@ -44,6 +44,56 @@ def test_recipes_used_by_empty(self): RecipeFactory.create_batch(2, action=action, enabled=False) assert list(action.recipes_used_by) == [] + def test_validate_arguments_it_works(self): + action = ActionFactory(name='nothing special') + # does not raise an exception + action.validate_arguments({}) + + def test_validate_arguments_preference_exeriments_unique_branch_slugs(self): + action = ActionFactory(name='preference-experiment') + arguments = { + 'branches': [ + {'slug': 'unique', 'value': 'a'}, + {'slug': 'duplicate', 'value': 'b'}, + {'slug': 'duplicate', 'value': 'c'} + ] + } + with pytest.raises(ValidationError) as exc_info: + action.validate_arguments(arguments) + assert exc_info.value.message == (action.errors['duplicate_branch_slug'] + .format(slug='duplicate')) + + def test_validate_arguments_preference_exeriments_unique_branch_values(self): + action = ActionFactory(name='preference-experiment') + arguments = { + 'branches': [ + {'slug': 'a', 'value': 'unique'}, + {'slug': 'b', 'value': 'duplicate'}, + {'slug': 'c', 'value': 'duplicate'} + ] + } + with pytest.raises(ValidationError) as exc_info: + action.validate_arguments(arguments) + assert exc_info.value.message == (action.errors['duplicate_branch_value'] + .format(value='duplicate')) + + def test_validate_arguments_preference_experiments_unique_experiment_slug(self): + action = ActionFactory(name='preference-experiment') + arguments = {'slug': 'duplicate', 'branches': []} + RecipeFactory(action=action, arguments=arguments) + with pytest.raises(ValidationError) as exc_info: + action.validate_arguments(arguments) + assert exc_info.value.message == (action.errors['duplicate_experiment_slug'] + .format(slug='duplicate')) + + def test_validate_arguments_preference_experiments_immutable_branches(self): + action = ActionFactory(name='preference-experiment') + old_arguments = {'branches': [{'slug': 'old', 'value': 'old'}]} + new_arguments = {'branches': [{'slug': 'new', 'value': 'new'}]} + with pytest.raises(ValidationError) as exc_info: + action.validate_arguments(old_arguments, new_arguments) + assert exc_info.value.message == action.errors['immutable_branches'] + @pytest.mark.django_db class TestRecipe(object): @@ -299,7 +349,7 @@ def test_recipe_update_locales(self): assert recipe.locales.count() == 0 def test_recipe_update_arguments(self): - recipe = RecipeFactory(arguments_json='') + recipe = RecipeFactory(arguments_json='{}') recipe.update(arguments={'something': 'value'}) assert recipe.arguments_json == '{"something": "value"}'