зеркало из https://github.com/mozilla/normandy.git
Merge #2238
2238: Enforce argument schemas on model creation r=rehandalal a=mythmon I realized that since we weren't checking schemas at the model level, none of the tests for argument verification checked their schemas. I needed this assurance for the tests I'm writing for #2236, but it's interesting enough to include as a separate PR. Co-authored-by: Mike Cooper <mythmon@gmail.com>
This commit is contained in:
bors[bot]
GitHub
Коммит
04d602ab65
|
|
@ -58,6 +58,13 @@ class FuzzyUnicode(fuzzy.FuzzyText):
|
|||
super(FuzzyUnicode, self).__init__(prefix=prefix, **kwargs)
|
||||
|
||||
|
||||
class FuzzySlug(fuzzy.FuzzyText):
|
||||
"""A FuzzyText factory that is suitable for slugs."""
|
||||
|
||||
def __init__(self, **kwargs):
|
||||
super().__init__(chars="abcdefghijklmnopqrstuvwxyz-", **kwargs)
|
||||
|
||||
|
||||
class UserFactory(DjangoModelFactory):
|
||||
username = FuzzyUnicode()
|
||||
email = Sequence(lambda n: "test%s@example.com" % n)
|
||||
|
|
|
|||
|
|
@ -17,11 +17,11 @@ from rest_framework.reverse import reverse
|
|||
from normandy.base.api.renderers import CanonicalJSONRenderer
|
||||
from normandy.base.utils import filter_m2m, get_client_ip, sri_hash
|
||||
from normandy.recipes import filters
|
||||
from normandy.recipes.geolocation import get_country_code
|
||||
from normandy.recipes.signing import Autographer
|
||||
from normandy.recipes.exports import RemoteSettings
|
||||
from normandy.recipes.validators import validate_json
|
||||
from normandy.recipes.geolocation import get_country_code
|
||||
from normandy.recipes.fields import IdenticonSeedField
|
||||
from normandy.recipes.signing import Autographer
|
||||
from normandy.recipes.validators import JSONSchemaValidator, validate_json
|
||||
|
||||
|
||||
INFO_REQUESTING_RECIPE_SIGNATURES = "normandy.recipes.I001"
|
||||
|
|
@ -163,7 +163,10 @@ class Recipe(DirtyFieldsMixin, models.Model):
|
|||
revision = self.latest_revision
|
||||
|
||||
if "arguments" in data:
|
||||
data["arguments_json"] = json.dumps(data.pop("arguments"))
|
||||
arguments = data.pop("arguments")
|
||||
data["arguments_json"] = json.dumps(arguments)
|
||||
else:
|
||||
arguments = None
|
||||
|
||||
if "filter_object" in data:
|
||||
data["filter_object_json"] = json.dumps(data.pop("filter_object"))
|
||||
|
|
@ -193,6 +196,17 @@ class Recipe(DirtyFieldsMixin, models.Model):
|
|||
locales = data.pop("locales", [])
|
||||
is_clean = False
|
||||
|
||||
if arguments is not None:
|
||||
schema = None
|
||||
if "action_id" in data:
|
||||
schema = Action.objects.get(action_id=data["action_id"]).arguments_schema
|
||||
elif revision:
|
||||
schema = revision.action.arguments_schema
|
||||
|
||||
if schema is not None:
|
||||
schema_validator = JSONSchemaValidator(schema)
|
||||
schema_validator.validate(arguments)
|
||||
|
||||
if not is_clean or force:
|
||||
logger.info(
|
||||
f"Creating new revision for recipe ID [{self.id}]",
|
||||
|
|
@ -698,6 +712,18 @@ class Action(DirtyFieldsMixin, models.Model):
|
|||
|
||||
errors = default()
|
||||
|
||||
# Check for any JSON Schema violations
|
||||
schemaValidator = JSONSchemaValidator(self.arguments_schema)
|
||||
for error in schemaValidator.iter_errors(arguments):
|
||||
current_level = errors
|
||||
path = list(error.path)
|
||||
for part in path[:-1]:
|
||||
current_level = current_level[part]
|
||||
current_level[path[-1]] = error.message
|
||||
|
||||
if errors:
|
||||
raise serializers.ValidationError({"arguments": errors})
|
||||
|
||||
if self.name == "preference-experiment":
|
||||
# Feature branch slugs should be unique within an experiment.
|
||||
branch_slugs = set()
|
||||
|
|
|
|||
|
|
@ -1,11 +1,13 @@
|
|||
import hashlib
|
||||
import os
|
||||
import json
|
||||
|
||||
from django.conf import settings
|
||||
from django.utils import timezone
|
||||
|
||||
import factory
|
||||
|
||||
from normandy.base.tests import FuzzyUnicode, UserFactory
|
||||
from normandy.base.tests import FuzzyUnicode, UserFactory, FuzzySlug
|
||||
from normandy.base.utils import sri_hash
|
||||
from normandy.recipes.models import (
|
||||
Action,
|
||||
|
|
@ -58,6 +60,28 @@ class LocaleFactory(factory.DjangoModelFactory):
|
|||
name = "Swedish"
|
||||
|
||||
|
||||
_action_schemas = None
|
||||
|
||||
|
||||
def get_action_schemas():
|
||||
global _action_schemas
|
||||
if _action_schemas is None:
|
||||
action_schemas_fp = os.path.join(settings.ACTIONS_SCHEMA_DIRECTORY, "schemas.json")
|
||||
with open(action_schemas_fp) as f:
|
||||
action_schemas = json.load(f)
|
||||
|
||||
aliases = settings.ACTIONS_ALIAS_NAMES
|
||||
updates = {}
|
||||
for name in action_schemas:
|
||||
if name in aliases:
|
||||
alias = aliases[name]
|
||||
updates[alias] = action_schemas[name]
|
||||
action_schemas.update(**updates)
|
||||
|
||||
_action_schemas = action_schemas
|
||||
return _action_schemas
|
||||
|
||||
|
||||
class ActionFactory(factory.DjangoModelFactory):
|
||||
class Meta:
|
||||
model = Action
|
||||
|
|
@ -84,6 +108,19 @@ class ActionFactory(factory.DjangoModelFactory):
|
|||
self.signature = None
|
||||
self.save()
|
||||
|
||||
@factory.post_generation
|
||||
def arguments_schema(self, create, extracted=None, **kwargs):
|
||||
if extracted is not None:
|
||||
self.arguments_schema = extracted
|
||||
return
|
||||
|
||||
action_schemas = get_action_schemas()
|
||||
|
||||
if self.name in action_schemas:
|
||||
self.arguments_schema = action_schemas[self.name]
|
||||
else:
|
||||
self.arguments_schema = {}
|
||||
|
||||
|
||||
class RecipeFactory(factory.DjangoModelFactory):
|
||||
class Meta:
|
||||
|
|
@ -176,6 +213,67 @@ class FuzzyIdenticonSeed(factory.fuzzy.FuzzyText):
|
|||
super().__init__(prefix="v1:", **kwargs)
|
||||
|
||||
|
||||
class DictFactory(factory.Factory):
|
||||
class Meta:
|
||||
abstract = True
|
||||
model = dict
|
||||
|
||||
|
||||
class PreferenceExperimentBranchFactory(DictFactory):
|
||||
slug = FuzzySlug()
|
||||
ratio = factory.fuzzy.FuzzyInteger(1, 100)
|
||||
value = factory.fuzzy.FuzzyText()
|
||||
|
||||
|
||||
class PreferenceExperimentArgumentsFactory(DictFactory):
|
||||
slug = FuzzySlug()
|
||||
preferenceName = factory.Sequence(lambda n: f"experiment.pref-{n}")
|
||||
preferenceType = "string"
|
||||
|
||||
@factory.post_generation
|
||||
def branches(self, create, extracted=None, **kwargs):
|
||||
if extracted is not None:
|
||||
self["branches"] = [
|
||||
PreferenceExperimentBranchFactory(**kwargs, **branch) for branch in extracted
|
||||
]
|
||||
else:
|
||||
self["branches"] = PreferenceExperimentBranchFactory.create_batch(2, **kwargs)
|
||||
|
||||
|
||||
class PreferenceRolloutPreferenceFactory(DictFactory):
|
||||
preferenceName = factory.Sequence(lambda n: f"rollout.pref-{n}")
|
||||
value = factory.fuzzy.FuzzyText()
|
||||
|
||||
|
||||
class PreferenceRolloutArgumentsFactory(DictFactory):
|
||||
slug = FuzzySlug()
|
||||
|
||||
@factory.post_generation
|
||||
def preferences(arguments, create, extracted=None, **kwargs):
|
||||
if extracted is not None:
|
||||
arguments["preferences"] = [
|
||||
PreferenceRolloutPreferenceFactory(**kwargs, **spec) for spec in extracted
|
||||
]
|
||||
else:
|
||||
arguments["preferences"] = PreferenceRolloutPreferenceFactory.create_batch(2, **kwargs)
|
||||
|
||||
|
||||
class OptOutStudyArgumentsFactory(DictFactory):
|
||||
name = factory.fuzzy.FuzzyText()
|
||||
description = factory.faker.Faker("paragraph")
|
||||
addonUrl = factory.lazy_attribute(
|
||||
lambda x: f"https://example.com/{x}" + factory.faker.Faker("file_path").generate()
|
||||
)
|
||||
extensionApiId = factory.fuzzy.FuzzyInteger(1, 1000)
|
||||
|
||||
|
||||
argument_factories = {
|
||||
"preference-experiment": PreferenceExperimentArgumentsFactory,
|
||||
"preference-rollout": PreferenceRolloutArgumentsFactory,
|
||||
"opt-out-study": OptOutStudyArgumentsFactory,
|
||||
}
|
||||
|
||||
|
||||
@factory.use_strategy(factory.BUILD_STRATEGY)
|
||||
class RecipeRevisionFactory(factory.DjangoModelFactory):
|
||||
class Meta:
|
||||
|
|
@ -200,6 +298,16 @@ class RecipeRevisionFactory(factory.DjangoModelFactory):
|
|||
]
|
||||
return json.dumps(filters)
|
||||
|
||||
@factory.post_generation
|
||||
def arguments(revision, create, extracted=None, **kwargs):
|
||||
arguments_factory = argument_factories.get(revision.action.name)
|
||||
if arguments_factory:
|
||||
if extracted is None:
|
||||
extracted = {}
|
||||
revision.arguments = arguments_factory(**kwargs, **extracted)
|
||||
elif extracted is not None:
|
||||
revision.arguments = {**extracted, **kwargs}
|
||||
|
||||
|
||||
class DictFactory(factory.Factory):
|
||||
class Meta:
|
||||
|
|
|
|||
|
|
@ -22,10 +22,12 @@ from normandy.recipes.models import (
|
|||
from normandy.recipes.tests import (
|
||||
ActionFactory,
|
||||
ApprovalRequestFactory,
|
||||
fake_sign,
|
||||
OptOutStudyArgumentsFactory,
|
||||
PreferenceExperimentArgumentsFactory,
|
||||
RecipeFactory,
|
||||
RecipeRevisionFactory,
|
||||
SignatureFactory,
|
||||
fake_sign,
|
||||
)
|
||||
from normandy.recipes.filters import StableSampleFilter
|
||||
|
||||
|
|
@ -98,8 +100,10 @@ class TestAction(object):
|
|||
|
||||
|
||||
@pytest.mark.django_db
|
||||
class TestValidateArgumentPreferenceExperiments(object):
|
||||
class TestArgumentValidation(object):
|
||||
"""
|
||||
Test that individual action types correctly validate their arguments.
|
||||
|
||||
This tests methods on Action, usually by creating Recipe instances.
|
||||
"""
|
||||
|
||||
|
|
@ -112,10 +116,9 @@ class TestValidateArgumentPreferenceExperiments(object):
|
|||
class TestPreferenceExperiments(object):
|
||||
def test_no_errors(self):
|
||||
action = ActionFactory(name="preference-experiment")
|
||||
arguments = {
|
||||
"slug": "a",
|
||||
"branches": [{"slug": "a", "value": "a"}, {"slug": "b", "value": "b"}],
|
||||
}
|
||||
arguments = PreferenceExperimentArgumentsFactory(
|
||||
slug="a", branches=[{"slug": "a", "value": "a"}, {"slug": "b", "value": "b"}],
|
||||
)
|
||||
# does not throw when saving the revision
|
||||
recipe = RecipeFactory(action=action, arguments=arguments)
|
||||
|
||||
|
|
@ -127,14 +130,14 @@ class TestValidateArgumentPreferenceExperiments(object):
|
|||
|
||||
def test_preference_experiments_unique_branch_slugs(self):
|
||||
action = ActionFactory(name="preference-experiment")
|
||||
arguments = {
|
||||
"slug": "test",
|
||||
"branches": [
|
||||
arguments = PreferenceExperimentArgumentsFactory(
|
||||
slug="test",
|
||||
branches=[
|
||||
{"slug": "unique", "value": "a"},
|
||||
{"slug": "duplicate", "value": "b"},
|
||||
{"slug": "duplicate", "value": "c"},
|
||||
],
|
||||
}
|
||||
)
|
||||
with pytest.raises(serializers.ValidationError) as exc_info:
|
||||
action.validate_arguments(arguments, RecipeRevisionFactory())
|
||||
error = action.errors["duplicate_branch_slug"]
|
||||
|
|
@ -142,14 +145,14 @@ class TestValidateArgumentPreferenceExperiments(object):
|
|||
|
||||
def test_preference_experiments_unique_branch_values(self):
|
||||
action = ActionFactory(name="preference-experiment")
|
||||
arguments = {
|
||||
"slug": "test",
|
||||
"branches": [
|
||||
arguments = PreferenceExperimentArgumentsFactory(
|
||||
slug="test",
|
||||
branches=[
|
||||
{"slug": "a", "value": "unique"},
|
||||
{"slug": "b", "value": "duplicate"},
|
||||
{"slug": "c", "value": "duplicate"},
|
||||
],
|
||||
}
|
||||
)
|
||||
with pytest.raises(serializers.ValidationError) as exc_info:
|
||||
action.validate_arguments(arguments, RecipeRevisionFactory())
|
||||
error = action.errors["duplicate_branch_value"]
|
||||
|
|
@ -157,15 +160,15 @@ class TestValidateArgumentPreferenceExperiments(object):
|
|||
|
||||
def test_unique_experiment_slug_no_collision(self):
|
||||
action = ActionFactory(name="preference-experiment")
|
||||
arguments_a = {"slug": "a", "branches": []}
|
||||
arguments_b = {"slug": "b", "branches": []}
|
||||
arguments_a = PreferenceExperimentArgumentsFactory()
|
||||
arguments_b = PreferenceExperimentArgumentsFactory()
|
||||
# Does not throw when saving revisions
|
||||
RecipeFactory(action=action, arguments=arguments_a)
|
||||
RecipeFactory(action=action, arguments=arguments_b)
|
||||
|
||||
def test_unique_experiment_slug_new_collision(self):
|
||||
action = ActionFactory(name="preference-experiment")
|
||||
arguments = {"slug": "a", "branches": []}
|
||||
arguments = PreferenceExperimentArgumentsFactory(slug="a")
|
||||
RecipeFactory(action=action, arguments=arguments)
|
||||
|
||||
with pytest.raises(serializers.ValidationError) as exc_info1:
|
||||
|
|
@ -175,8 +178,12 @@ class TestValidateArgumentPreferenceExperiments(object):
|
|||
|
||||
def test_unique_experiment_slug_update_collision(self):
|
||||
action = ActionFactory(name="preference-experiment")
|
||||
arguments_a = {"slug": "a", "branches": []}
|
||||
arguments_b = {"slug": "b", "branches": []}
|
||||
arguments_a = PreferenceExperimentArgumentsFactory(
|
||||
slug="a", branches=[{"slug": "one"}]
|
||||
)
|
||||
arguments_b = PreferenceExperimentArgumentsFactory(
|
||||
slug="b", branches=[{"slug": "two"}]
|
||||
)
|
||||
# Does not throw when saving revisions
|
||||
RecipeFactory(action=action, arguments=arguments_a)
|
||||
recipe = RecipeFactory(action=action, arguments=arguments_b)
|
||||
|
|
@ -225,10 +232,11 @@ class TestValidateArgumentPreferenceExperiments(object):
|
|||
class TestPreferenceRollback(object):
|
||||
def test_no_errors(self):
|
||||
rollback_action = ActionFactory(name="preference-rollback")
|
||||
assert rollback_action.arguments_schema != {}
|
||||
rollout_action = ActionFactory(name="preference-rollout")
|
||||
rollout_recipe = RecipeFactory(
|
||||
action=rollout_action, arguments={"slug": "test-rollout"}
|
||||
)
|
||||
assert rollout_action.arguments_schema != {}
|
||||
|
||||
rollout_recipe = RecipeFactory(action=rollout_action)
|
||||
|
||||
# does not throw when saving the revision
|
||||
arguments = {"rolloutSlug": rollout_recipe.latest_revision.arguments["slug"]}
|
||||
|
|
@ -246,8 +254,7 @@ class TestValidateArgumentPreferenceExperiments(object):
|
|||
class TestOptOutStudy(object):
|
||||
def test_no_errors(self):
|
||||
action = ActionFactory(name="opt-out-study")
|
||||
arguments = {"name": "foo"}
|
||||
recipe = RecipeFactory(action=action, arguments=arguments)
|
||||
recipe = RecipeFactory(action=action)
|
||||
|
||||
# Approve and enable the revision
|
||||
rev = recipe.latest_revision
|
||||
|
|
@ -267,8 +274,8 @@ class TestValidateArgumentPreferenceExperiments(object):
|
|||
|
||||
def test_unique_name_update_collision(self):
|
||||
action = ActionFactory(name="opt-out-study")
|
||||
arguments_a = {"name": "foo"}
|
||||
arguments_b = {"name": "bar"}
|
||||
arguments_a = OptOutStudyArgumentsFactory()
|
||||
arguments_b = OptOutStudyArgumentsFactory()
|
||||
RecipeFactory(action=action, arguments=arguments_a)
|
||||
recipe = RecipeFactory(action=action, arguments=arguments_b)
|
||||
|
||||
|
|
|
|||
|
|
@ -8,7 +8,7 @@ import pytest
|
|||
from django.db import transaction
|
||||
|
||||
from normandy.base.tests import Whatever
|
||||
from normandy.recipes.tests import ActionFactory, RecipeFactory
|
||||
from normandy.recipes.tests import ActionFactory, RecipeFactory, OptOutStudyArgumentsFactory
|
||||
from normandy.studies.models import Extension
|
||||
from normandy.studies.tests import (
|
||||
ExtensionFactory,
|
||||
|
|
@ -222,8 +222,8 @@ class TestExtensionAPI(object):
|
|||
xpi = WebExtensionFileFactory()
|
||||
e = ExtensionFactory(xpi__from_func=xpi.open)
|
||||
a = ActionFactory(name="opt-out-study")
|
||||
r = RecipeFactory(action=a, arguments={"extensionId": e.id})
|
||||
r.revise(arguments={"extensionId": 0})
|
||||
r = RecipeFactory(action=a, arguments={"extensionId": e.id + 1})
|
||||
r.revise(OptOutStudyArgumentsFactory(extensionId=e.id))
|
||||
res = api_client.patch(f"/api/v3/extension/{e.id}/", {"name": "new name"})
|
||||
assert res.status_code == 200
|
||||
assert res.data["name"] == "new name"
|
||||
|
|
@ -243,7 +243,7 @@ class TestExtensionAPI(object):
|
|||
e = ExtensionFactory(xpi__from_func=xpi.open)
|
||||
a = ActionFactory(name="opt-out-study")
|
||||
r = RecipeFactory(action=a, arguments={"extensionId": e.id})
|
||||
r.revise(arguments={"extensionId": 0})
|
||||
r.revise(arguments=OptOutStudyArgumentsFactory(extensionId=e.id + 1))
|
||||
res = api_client.delete(f"/api/v3/extension/{e.id}/")
|
||||
assert res.status_code == 204
|
||||
assert Extension.objects.count() == 0
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче