recipe-server: Add per-action validation of recipe arguments

Fixes mozilla#599

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."""

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={

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"}'