refactor (and test) field type logic

Author: claytondaley

drf_writable_nested/mixins.py

@@ -11,6 +11,7 @@
 from rest_framework.exceptions import ValidationError
 from rest_framework.fields import empty
 from rest_framework.relations import ManyRelatedField
+from rest_framework.serializers import BaseSerializer
 from rest_framework.validators import UniqueValidator, UniqueTogetherValidator
 
 # permit writable nested serializers
@@ -426,7 +427,60 @@ def update(self, instance, validated_data):
         return super(UniqueFieldsMixin, self).update(instance, validated_data)
 
 
-class RelatedSaveMixin(serializers.Serializer):
+class FieldLookupMixin(serializers.Serializer):
+
+    def _get_model_field(self, source):
+        """Returns the field on the model"""
+        # for serializers like ModelSerializer, the Meta.model can be used to classify fields
+        if not hasattr(self, 'Meta') or not hasattr(self.Meta, 'model'):
+            return None
+        try:
+            return self.Meta.model._meta.get_field(source)
+        except FieldDoesNotExist:
+            pass
+        try:
+            # If `related_name` is not set, field name does not include
+            # `_set` -> remove it and check again
+            default_postfix = '_set'
+            if source.endswith(default_postfix):
+                return self.Meta.model._meta.get_field(source[:-len(default_postfix)])
+        except FieldDoesNotExist:
+            pass
+        return None
+
+    TYPE_READ_ONLY = 'read-only'
+    TYPE_LOCAL = 'local'
+    TYPE_DIRECT = 'direct'
+    TYPE_REVERSE = 'reverse'
+
+    _cache_field_types = None
+
+    @property
+    def field_types(self):
+        if self._cache_field_types is None:
+            self._populate_field_types()
+        return self._cache_field_types
+
+    def _populate_field_types(self):
+        self._cache_field_types = {}
+        for field_name, field in self.fields.items():
+            if field.read_only:
+                self._cache_field_types[field_name] = self.TYPE_READ_ONLY
+                continue
+            if not isinstance(field, BaseSerializer):
+                self._cache_field_types[field_name] = self.TYPE_LOCAL
+                continue
+            if field.source == '*':
+                self._cache_field_types[field_name] = self.TYPE_DIRECT
+                continue
+            related_field = self._get_model_field(field.source)
+            if isinstance(related_field, ForeignObjectRel):
+                self._cache_field_types[field_name] = self.TYPE_REVERSE
+                continue
+            self._cache_field_types[field_name] = self.TYPE_DIRECT
+
+
+class RelatedSaveMixin(FieldLookupMixin):
     """
     RelatedSaveMixin handes the saving of nested fields, both direct and reverse relations:
      - Direct relations needs to be saved first
@@ -435,16 +489,23 @@ class RelatedSaveMixin(serializers.Serializer):
     """
     _is_saved = False
 
+    def run_validation(self, data=empty):
+        self._validated_data = super(RelatedSaveMixin, self).run_validation(data)
+        self._errors = {}
+        return self._validated_data
+
     def to_internal_value(self, data):
         """Injects the PK of this field into reverse relations so they validate when created in to_internal_value."""
-        self._make_reverse_relations_valid(data)
+        self._make_reverse_relations_valid()
         return super(RelatedSaveMixin, self).to_internal_value(data)
 
-    def _make_reverse_relations_valid(self, data):
+    def _make_reverse_relations_valid(self):
         """Make the reverse field optional since we may not have a key for the base object."""
-        for field_name, (field, related_field) in self._get_reverse_fields().items():
-            if data.get(field.source) is None:
+        for field_name, field in self.fields.items():
+            if self.field_types[field_name] != self.TYPE_REVERSE:
                 continue
+            # we know this is a reverse so reverse_field.field is valid
+            related_field = self._get_model_field(field.source).field
             if isinstance(field, serializers.ListSerializer):
                 field = field.child
             if isinstance(field, serializers.ModelSerializer):
@@ -455,11 +516,6 @@ def _make_reverse_relations_valid(self, data):
                         # found the matching field, move on
                         break
 
-    def run_validation(self, data=empty):
-        self._validated_data = super(RelatedSaveMixin, self).run_validation(data)
-        self._errors = {}
-        return self._validated_data
-
     def save(self, **kwargs):
         """We already converted the inputs into a model so we need to save that model"""
         if self._is_saved:
@@ -473,59 +529,13 @@ def save(self, **kwargs):
         self._save_reverse_relations(reverse_relations, instance=instance)
         return instance
 
-    def _get_reverse_fields(self):
-        reverse_fields = OrderedDict()
-        if not hasattr(self, 'Meta') or not hasattr(self.Meta, 'model'):
-            # No model means no reverse fields (without the need to iterate)
-            return reverse_fields
-        for field_name, field in self.fields.items():
-            if field.read_only:
-                continue
-            try:
-                related_field, direct = self._get_related_field(field)
-            except FieldDoesNotExist:
-                continue
-            if direct:
-                continue
-            reverse_fields[field_name] = (field, related_field)
-        return reverse_fields
-
-    def _get_related_field(self, field):
-        model_class = self.Meta.model
-        try:
-            related_field = model_class._meta.get_field(field.source)
-        except FieldDoesNotExist:
-            # If `related_name` is not set, field name does not include
-            # `_set` -> remove it and check again
-            default_postfix = '_set'
-            if field.source.endswith(default_postfix):
-                related_field = model_class._meta.get_field(
-                    field.source[:-len(default_postfix)])
-            else:
-                raise
-        if isinstance(related_field, ForeignObjectRel):
-            return related_field.field, False
-        return related_field, True
-
     def _save_direct_relations(self, kwargs):
-        """Save direct relations so related objects have FKs when committing the base instance"""
+        """Save direct relations so FKs exist when committing the base instance"""
         for field_name, field in self.fields.items():
-            if field.read_only:
+            if self.field_types[field_name] != self.TYPE_DIRECT:
                 continue
-            if not isinstance(field, serializers.BaseSerializer):
+            if not isinstance(self._validated_data, dict) or field_name not in self._validated_data:
                 continue
-            # source='*' will never be found in _validated_data
-            if isinstance(self._validated_data, dict) and self._validated_data.get(field_name) is None:
-                continue
-            # this serializer looks like a ModelSerializer
-            if hasattr(self, 'Meta') and hasattr(self.Meta, 'model'):
-                try:
-                    _, direct = self._get_related_field(field)
-                    # don't try to process reverse relations
-                    if not direct:
-                        continue
-                except FieldDoesNotExist:
-                    pass
             # reinject validated_data
             field._validated_data = self._validated_data[field_name]
             self._validated_data[field_name] = field.save(**kwargs.pop(field_name, {}))
@@ -534,25 +544,27 @@ def _extract_reverse_relations(self, kwargs):
         """Removes revere relations from _validated_data to avoid FK integrity issues"""
         # Remove related fields from validated data for future manipulations
         related_objects = []
-        for field_name, (field, related_field) in self._get_reverse_fields().items():
-            if self._validated_data.get(field.source) is None:
+        for field_name, field in self.fields.items():
+            if self.field_types[field_name] != self.TYPE_REVERSE:
+                continue
+            if not isinstance(self._validated_data, dict) or field_name not in self._validated_data:
                 continue
             serializer = field
             if isinstance(serializer, serializers.ListSerializer):
                 serializer = serializer.child
             if isinstance(serializer, serializers.ModelSerializer):
                 related_objects.append((
                     field,
-                    related_field,
                     self._validated_data.pop(field.source),
-                    kwargs.pop(field_name, {}),
+                    kwargs.get(field_name, {}),
                 ))
         return related_objects
 
     def _save_reverse_relations(self, related_objects, instance):
         """Inject the current object as the FK in the reverse related objects and save them"""
-        for field, related_field, data, kwargs in related_objects:
+        for field, data, kwargs in related_objects:
             # inject the PK from the instance
+            related_field = self._get_model_field(field.source).field
             if isinstance(field, serializers.ListSerializer):
                 for obj in data:
                     obj[related_field.name] = instance
@@ -600,24 +612,31 @@ def run_validation(self, data=empty):
         return self._validated_data
 
 
-class FocalSaveMixin(serializers.Serializer):
+class FocalSaveMixin(FieldLookupMixin):
 
     @transaction.atomic
     def save(self, **kwargs):
         match_on = {}
         m2m_relations = {}
-        defaults = self.validated_data.copy()
-        for k, v in kwargs.items():
-            defaults[k] = v
-        for field_name, field in self.get_fields().items():
+        defaults = {}
+        for field_name, field in self.fields.items():
             if self.match_on == '__all__' or field_name in self.match_on:
-                match_on[field.source or field_name] = defaults.pop(field_name, None)
-            elif isinstance(field, ManyRelatedField):
-                m2m_relations[field_name] = defaults.pop(field_name, None)
+                # add to match_on dict
+                match_on[field.source or field_name] = kwargs.get(field_name, self._validated_data.get(field_name))
+            if isinstance(field, ManyRelatedField):
+                # we can't provide m2m values as kwargs; must use set() instead
+                m2m_relations[field_name] = kwargs.get(field_name, self._validated_data.get(field_name))
+            elif self.field_types[field_name] == self.TYPE_LOCAL:
+                # need to check kwargs since there's no pre-processing
+                defaults[field_name] = kwargs.get(field_name, self._validated_data.get(field_name))
+            elif self.field_types[field_name] == self.TYPE_DIRECT:
+                # kwargs should have been injected when direct relations were saved
+                defaults[field_name] = self._validated_data.get(field_name)
+            # make reverse relations aren't sent to a create
         # a parent serializer may inject a value that isn't among the fields, but is in `match_on`
         for key in self.match_on:
-            if key not in self.get_fields().keys():
-                match_on[key] = defaults.pop(key, None)
+            if key not in self.fields.keys():
+                match_on[key] = kwargs.get(key, None)
         try:
             match, updated = self.do_save(match_on, defaults)
             if not updated:
@@ -655,7 +674,6 @@ def many_init(cls, *args, **kwargs):
         if meta is None:
             class Meta:
                 pass
-
             meta = Meta
             setattr(cls, 'Meta', meta)
         list_serializer_class = getattr(meta, 'list_serializer_class', None)
@@ -687,7 +705,7 @@ def run_validation(self, data=empty):
         self._validated_data = super(NestedSaveSerializer, self).run_validation(data)
         # restore Unique or UniqueTogether
         self.restore_validation_unique(validators)
-        return self.validated_data
+        return self._validated_data
 
     def remove_validation_unique(self):
         """
@@ -763,7 +781,7 @@ def do_save(self, match_on, defaults):
         try:
             return super(GetOrCreateNestedSerializerMixin, self).do_save(match_on, defaults)
         except self.queryset.model.DoesNotExist:
-            return self.queryset.model(**match_on, **defaults), True
+            return self.queryset.model(**defaults), True
 
 
 class UpdateOrCreateNestedSerializerMixin(UpdateDoSaveMixin, GetOrCreateNestedSerializerMixin):
@@ -774,4 +792,4 @@ class CreateOnlyNestedSerializerMixin(NestedSaveSerializer):
     """Creates requested object or fails."""
 
     def do_save(self, match_on, defaults):
-        return self.queryset.model(**match_on, **defaults), True
+        return self.queryset.model(**defaults), True

tests/test_field_lookup.py

@@ -0,0 +1,110 @@
+from django.db import models
+from django.test import TestCase
+from rest_framework import serializers
+
+from drf_writable_nested import mixins
+
+
+class LookupChild(models.Model):
+    name = models.TextField()
+
+
+class LookupParent(models.Model):
+    child = models.ForeignKey(LookupChild, on_delete=models.CASCADE)
+
+
+class LookupGrandParent(models.Model):
+    child = models.ForeignKey(LookupParent, on_delete=models.CASCADE)
+
+
+class LookupReverseChild(models.Model):
+    name = models.TextField()
+    parent = models.ForeignKey(LookupParent, on_delete=models.CASCADE, related_name='children')
+
+
+class ChildSerializer(mixins.FieldLookupMixin, serializers.ModelSerializer):
+    class Meta:
+        model = LookupChild
+        fields = '__all__'
+
+
+class ReverseChildSerializer(mixins.FieldLookupMixin, serializers.ModelSerializer):
+    class Meta:
+        model = LookupReverseChild
+        fields = '__all__'
+
+
+class ParentSerializer(mixins.FieldLookupMixin, serializers.ModelSerializer):
+    class Meta:
+        model = LookupParent
+        fields = '__all__'
+    # source of a 1:many relationship
+    child = ChildSerializer()
+    children = ReverseChildSerializer(many=True)
+
+
+class GrandParentSerializer(mixins.FieldLookupMixin, serializers.ModelSerializer):
+    class Meta:
+        model = LookupGrandParent
+        fields = '__all__'
+    # source of a 1:many relationship
+    child = ParentSerializer()
+
+
+class FieldTypesTest(TestCase):
+
+    def test_field_types_grandparent(self):
+        serializer = GrandParentSerializer()
+        self.assertEqual(
+            {
+                'id': serializer.TYPE_READ_ONLY,
+                'child': serializer.TYPE_DIRECT,
+            },
+            serializer.field_types
+        )
+
+    def test_field_types_parent(self):
+        serializer = GrandParentSerializer()
+        self.assertEqual(
+            {
+                'id': serializer.TYPE_READ_ONLY,
+                'child': serializer.TYPE_DIRECT,
+                'children': serializer.TYPE_REVERSE,
+            },
+            serializer.fields['child'].field_types
+        )
+
+    def test_field_types_child(self):
+        serializer = GrandParentSerializer()
+        self.assertEqual(
+            {
+                'id': serializer.TYPE_READ_ONLY,
+                'name': serializer.TYPE_LOCAL,
+            },
+            serializer.fields['child'].fields['child'].field_types
+        )
+
+    def test_field_types_reversechild(self):
+        serializer = GrandParentSerializer()
+        self.assertEqual(
+            {
+                'id': serializer.TYPE_READ_ONLY,
+                'name': serializer.TYPE_LOCAL,
+                # must have a nested serializer to be "direct" otherwise it's just a local value
+                'parent': serializer.TYPE_LOCAL,
+            },
+            serializer.fields['child'].fields['children'].child.field_types
+        )
+
+
+class GetModelFieldTest(TestCase):
+
+    def test_reverse(self):
+        serializer = ParentSerializer()
+        model_field = serializer._get_model_field('children')
+        print(type(model_field))
+        # opposite side of a ForeignKey is a ManyToOne
+        self.assertIsInstance(
+            model_field,
+            models.ManyToOneRel,
+        )