Merge #1550

1550: V3 api refactor r=rehandalal a=mythmon

I think this makes the API a lot easier to understand. Previously, there was a confusing mix of hierarchy and recursion. When querying `/v3/api/recipe/{id}/`, some fields were placed at the top level, and other fields were placed within a latest or approved revision key, which then had a recipe key that duplicated those top level keys again.

The goal here is not to minimize bytes-on-the-wire, but to make the overall structure clearer and better reflect the underlying data. As a bonus, enabled states are now exposed in the API.

Co-authored-by: Mike Cooper <[email protected]>

Author: bors[bot]

normandy/recipes/api/v3/serializers.py

@@ -5,10 +5,29 @@
 from normandy.base.api.serializers import UserSerializer
 from normandy.recipes import filters
 from normandy.recipes.api.fields import ActionImplementationHyperlinkField
-from normandy.recipes.models import Action, ApprovalRequest, Recipe, RecipeRevision
+from normandy.recipes.models import (
+    Action,
+    ApprovalRequest,
+    EnabledState,
+    Recipe,
+    RecipeRevision,
+    Signature,
+)
 from normandy.recipes.validators import JSONSchemaValidator
 
 
+class CustomizableSerializerMixin:
+    """Serializer Mixin that allows callers to exclude fields on instance of this serializer."""
+
+    def __init__(self, *args, **kwargs):
+        exclude_fields = kwargs.pop("exclude_fields", [])
+        super().__init__(*args, **kwargs)
+
+        if exclude_fields:
+            for field in exclude_fields:
+                self.fields.pop(field)
+
+
 class ActionSerializer(serializers.ModelSerializer):
     arguments_schema = serializers.JSONField()
     implementation_url = ActionImplementationHyperlinkField()
@@ -28,83 +47,106 @@ class Meta:
         fields = ["approved", "approver", "comment", "created", "creator", "id"]
 
 
+class EnabledStateSerializer(CustomizableSerializerMixin, serializers.ModelSerializer):
+    creator = UserSerializer()
+
+    class Meta:
+        model = EnabledState
+        fields = ["revision_id", "created", "creator", "enabled", "carryover_from"]
+
+
 class RecipeRevisionSerializer(serializers.ModelSerializer):
+    action = serializers.SerializerMethodField(read_only=True)
     approval_request = ApprovalRequestSerializer(read_only=True)
     comment = serializers.CharField(required=False)
+    creator = UserSerializer(source="user", read_only=True)
     date_created = serializers.DateTimeField(source="created", read_only=True)
+    enabled_states = EnabledStateSerializer(many=True, exclude_fields=["revision_id"])
     recipe = serializers.SerializerMethodField(read_only=True)
-    user = UserSerializer()
 
     class Meta:
         model = RecipeRevision
         fields = [
+            "action",
             "approval_request",
+            "arguments",
+            "bug_number",
             "comment",
             "date_created",
+            "enabled_states",
+            "enabled",
+            "extra_filter_expression",
+            "filter_expression",
+            "filter_object",
             "id",
-            "recipe",
-            "user",
             "identicon_seed",
+            "name",
+            "recipe",
+            "creator",
+            "updated",
         ]
 
     def get_recipe(self, instance):
-        serializer = RecipeSerializer(
-            instance.serializable_recipe,
-            exclude_fields=["latest_revision", "approved_revision", "approval_request"],
+        serializer = RecipeLinkSerializer(instance.recipe)
+        return serializer.data
+
+    def get_action(self, instance):
+        serializer = ActionSerializer(
+            instance.action, read_only=True, context={"request": self.context.get("request")}
         )
         return serializer.data
 
 
-class RecipeSerializer(serializers.ModelSerializer):
-    action = serializers.SerializerMethodField(read_only=True)
+class SignatureSerializer(serializers.ModelSerializer):
+    timestamp = serializers.DateTimeField(read_only=True)
+    signature = serializers.ReadOnlyField()
+    x5u = serializers.ReadOnlyField()
+    public_key = serializers.ReadOnlyField()
+
+    class Meta:
+        model = Signature
+        fields = ["timestamp", "signature", "x5u", "public_key"]
+
+
+class RecipeSerializer(CustomizableSerializerMixin, serializers.ModelSerializer):
+    # read-only fields
+    approved_revision = RecipeRevisionSerializer(read_only=True)
+    latest_revision = RecipeRevisionSerializer(read_only=True)
+    signature = SignatureSerializer(read_only=True)
+
+    # write-only fields
     action_id = serializers.PrimaryKeyRelatedField(
         source="action", queryset=Action.objects.all(), write_only=True
     )
-    approval_request = ApprovalRequestSerializer(read_only=True)
-    approved_revision = RecipeRevisionSerializer(read_only=True)
-    arguments = serializers.JSONField()
-    enabled = serializers.BooleanField(read_only=True)
-    extra_filter_expression = serializers.CharField(required=False, allow_blank=True)
-    filter_expression = serializers.CharField(read_only=True)
-    filter_object = serializers.JSONField(required=False)
-    last_updated = serializers.DateTimeField(read_only=True)
-    latest_revision = RecipeRevisionSerializer(read_only=True)
-    name = serializers.CharField()
-    identicon_seed = serializers.CharField(required=False)
-    comment = serializers.CharField(required=False)
-    bug_number = serializers.IntegerField(required=False)
+    arguments = serializers.JSONField(write_only=True)
+    extra_filter_expression = serializers.CharField(
+        required=False, allow_blank=True, write_only=True
+    )
+    filter_object = serializers.JSONField(required=False, write_only=True)
+    name = serializers.CharField(write_only=True)
+    identicon_seed = serializers.CharField(required=False, write_only=True)
+    comment = serializers.CharField(required=False, write_only=True)
+    bug_number = serializers.IntegerField(required=False, write_only=True)
 
     class Meta:
         model = Recipe
         fields = [
-            "action",
-            "action_id",
-            "approval_request",
+            # read-only
             "approved_revision",
+            "id",
+            "latest_revision",
+            "signature",
+            # write-only
+            "action_id",
             "arguments",
-            "enabled",
             "extra_filter_expression",
-            "filter_expression",
             "filter_object",
-            "id",
-            "is_approved",
-            "last_updated",
-            "latest_revision",
             "name",
             "identicon_seed",
             "comment",
             "bug_number",
         ]
 
-    def __init__(self, *args, **kwargs):
-        exclude_fields = kwargs.pop("exclude_fields", [])
-        super().__init__(*args, **kwargs)
-
-        if exclude_fields:
-            for field in exclude_fields:
-                if field in self.fields:
-                    self.fields.pop(field)
-
     def get_action(self, instance):
         serializer = ActionSerializer(
             instance.action, read_only=True, context={"request": self.context.get("request")}
@@ -244,3 +286,8 @@ def validate_filter_object(self, value):
             raise serializers.ValidationError(errors)
 
         return value
+
+
+class RecipeLinkSerializer(RecipeSerializer):
+    class Meta(RecipeSerializer.Meta):
+        fields = ["approved_revision_id", "id", "latest_revision_id"]

normandy/recipes/api/v3/views.py

@@ -38,7 +38,6 @@ class ActionViewSet(CachingViewsetMixin, viewsets.ReadOnlyModelViewSet):
 
     queryset = Action.objects.all()
     serializer_class = ActionSerializer
-    pagination_class = None
 
 
 class RecipeFilters(django_filters.FilterSet):
@@ -67,14 +66,22 @@ class RecipeViewSet(CachingViewsetMixin, UpdateOrCreateModelViewSet):
 
     queryset = (
         Recipe.objects.all()
-        # Foreign keys
-        .select_related("latest_revision")
-        .select_related("latest_revision__action")
-        .select_related("latest_revision__approval_request")
-        # Many-to-many
-        .prefetch_related("latest_revision__channels")
-        .prefetch_related("latest_revision__countries")
-        .prefetch_related("latest_revision__locales")
+        .select_related(
+            "approved_revision__action",
+            "approved_revision__approval_request",
+            "latest_revision__action",
+            "latest_revision__approval_request",
+        )
+        .prefetch_related(
+            "approved_revision__channels",
+            "approved_revision__countries",
+            "approved_revision__enabled_states",
+            "approved_revision__locales",
+            "latest_revision__channels",
+            "latest_revision__countries",
+            "latest_revision__enabled_states",
+            "latest_revision__locales",
+        )
     )
     serializer_class = RecipeSerializer
     filterset_class = RecipeFilters
@@ -138,6 +145,7 @@ def enable(self, request, pk=None):
                 status=status.HTTP_409_CONFLICT,
             )
 
+        recipe.latest_revision.refresh_from_db()
         return Response(RecipeSerializer(recipe).data)
 
     @action(detail=True, methods=["POST"])
@@ -155,17 +163,11 @@ def disable(self, request, pk=None):
 class RecipeRevisionViewSet(viewsets.ReadOnlyModelViewSet):
     queryset = (
         RecipeRevision.objects.all()
-        .select_related("action")
-        .select_related("approval_request")
-        .select_related("recipe")
-        # Many-to-many
-        .prefetch_related("channels")
-        .prefetch_related("countries")
-        .prefetch_related("locales")
+        .select_related("action", "approval_request", "recipe")
+        .prefetch_related("enabled_states", "channels", "countries", "locales")
     )
     serializer_class = RecipeRevisionSerializer
     permission_classes = [AdminEnabledOrReadOnly, permissions.DjangoModelPermissionsOrAnonReadOnly]
-    pagination_class = None
 
     @action(detail=True, methods=["POST"])
     def request_approval(self, request, pk=None):
@@ -188,7 +190,6 @@ class ApprovalRequestViewSet(viewsets.ReadOnlyModelViewSet):
     queryset = ApprovalRequest.objects.all()
     serializer_class = ApprovalRequestSerializer
     permission_classes = [AdminEnabledOrReadOnly, permissions.DjangoModelPermissionsOrAnonReadOnly]
-    pagination_class = None
 
     @action(detail=True, methods=["POST"])
     def approve(self, request, pk=None):

normandy/recipes/models.py

@@ -276,8 +276,13 @@ def revise(self, force=False, **data):
 
     @transaction.atomic
     def save(self, *args, **kwargs):
-        if self.is_dirty(check_relationship=True):
-            dirty_fields = self.get_dirty_fields(check_relationship=True)
+        dirty_fields = {
+            k: v
+            for k, v in self.get_dirty_fields(check_relationship=True, verbose=True).items()
+            if v["saved"] != v["current"]
+        }
+
+        if dirty_fields:
             dirty_field_names = list(dirty_fields.keys())
 
             if (
@@ -492,6 +497,9 @@ class ApprovalRequest(models.Model):
     )
     comment = models.TextField(null=True)
 
+    class Meta:
+        ordering = ("id",)
+
     class NotActionable(Exception):
         pass
 
@@ -571,6 +579,9 @@ class Action(DirtyFieldsMixin, models.Model):
         Signature, related_name="action", null=True, blank=True, on_delete=models.CASCADE
     )
 
+    class Meta:
+        ordering = ("id",)
+
     errors = {
         "duplicate_branch_slug": "Feature branch slugs must be unique within an experiment",
         "duplicate_branch_value": "Feature branch values must be unique within an experiment",
@@ -641,8 +652,12 @@ def update_signature(self):
 
     @transaction.atomic
     def save(self, *args, **kwargs):
-        if self.is_dirty(check_relationship=True):
-            dirty_fields = self.get_dirty_fields(check_relationship=True)
+        dirty_fields = {
+            k: v
+            for k, v in self.get_dirty_fields(check_relationship=True, verbose=True).items()
+            if v["saved"] != v["current"]
+        }
+        if dirty_fields:
             dirty_field_names = list(dirty_fields.keys())
 
             if (