kaoslabsinc / dj-kaos-utils

Set of utilities and helpers for rapid development of Django applications
https://dj-kaos-utils.readthedocs.io
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Implementation of `to_internal_value()` in `RelatedModelSerializer` might be wrong. #7

Closed gerdemb closed 1 year ago

gerdemb commented 1 year ago

https://www.loom.com/share/534db2e3d88045c99252a0190e18a250

SUMMARY I think object create/update isn’t supposed to happen in the to_internal_value() method of RelatedModelSerializer and should be done in update() and create() methods. However, the code does seem to work correctly.

Related DRF documentation: https://www.django-rest-framework.org/api-guide/serializers/#saving-instances https://www.django-rest-framework.org/api-guide/serializers/#overriding-serialization-and-deserialization-behavior

https://github.com/kaoslabsinc/dj-kaos-utils/blob/617aca24ccf5cfed35a1e9864a42c401336d7534/dj_kaos_utils/rest/serializers.py#L31-L55

keyvanm commented 1 year ago

interesting point

keyvanm commented 1 year ago

I wonder if we can somehow automatically delegate the creation of objects to the create and update methods in the serializer.

gerdemb commented 1 year ago

Performing creates and updates in the to_internal_value() function also hampers testing. For example, the following test case throws a ValidationError because the default of can_update is False, but simply deserializing data shouldn't trigger and database modifications.

def test_deserializer(db, product, product_data):
    serializer = ProductSerializer(data=product_data)
    serializer.is_valid(raise_exception=True)
    data = serializer.validated_data

Error

./tests/products/test_product_serializer.py::test_deserializer Failed: [undefined]rest_framework.exceptions.ValidationError: {'brand': {'non_field_errors': ErrorDetail(string='This api is not configured to update existing objects', code='invalid')}, 'category': {'non_field_errors': ErrorDetail(string='This api is not configured to update existing objects', code='invalid')}, 'image': [ErrorDetail(string='This field may not be null.', code='null')]}
db = None, product = <Product: Stanley Johns PhD>
product_data = {'_amount_per_case': 319179359.136753, '_amount_unit': 'kg', 'brand': {'name': 'Dr. Jay Moses', 'slug': 'pay-choose-degree'}, 'category': {'name': 'Debbie Mcdaniel', 'slug': 'everybody-best'}, ...}

    def test_deserializer(db, product, product_data):
        serializer = ProductSerializer(data=product_data)
>       serializer.is_valid(raise_exception=True)

tests/products/test_product_serializer.py:70: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = ProductSerializer(data={'uuid': '18a3fd16-0c22-482f-8a40-a058a1936486', 'name': 'Stanley Johns PhD', 'brand': {'name':...), ('lbs', 'lbs')])
    price_per_case = DecimalField(allow_null=True, decimal_places=2, max_digits=12, required=False)

    def is_valid(self, *, raise_exception=False):
        assert hasattr(self, 'initial_data'), (
            'Cannot call `.is_valid()` as no `data=` keyword argument was '
            'passed when instantiating the serializer instance.'
        )

        if not hasattr(self, '_validated_data'):
            try:
                self._validated_data = self.run_validation(self.initial_data)
            except ValidationError as exc:
                self._validated_data = {}
                self._errors = exc.detail
            else:
                self._errors = {}

        if self._errors and raise_exception:
>           raise ValidationError(self.errors)
E           rest_framework.exceptions.ValidationError: {'brand': {'non_field_errors': ErrorDetail(string='This api is not configured to update existing objects', code='invalid')}, 'category': {'non_field_errors': ErrorDetail(string='This api is not configured to update existing objects', code='invalid')}, 'image': [ErrorDetail(string='This field may not be null.', code='null')]}

venv/lib/python3.10/site-packages/rest_framework/serializers.py:235: ValidationError
keyvanm commented 1 year ago

is to_internal_value() called before or after create() and update()?

gerdemb commented 1 year ago

Goals

  1. A configurable depth read serializer using depth parameter. Specifying nested serialization. Override build_relational_fieldset() and build_nested_field. Example code: (https://github.com/keyvanm/pricesource/blob/ft/simplify-update-create/pricesource/restaurants/rest/serializers/serializers.py)
  2. Definition of create/update actions
// NOTE: create/update actions use nested serializers. get action uses a RelatedField (uuid)
// Create product, get category
{
    "name": "Created Product",
    "category": "uuid-xxxx"
}
// Create product, create category
{
    "name": "Created Product",
    "category": {
        "name": "Created Category"
    }
}
// Create product, update category
{
    "name": "Created Product",
    "category": {
        "uuid": "uuid-yyyy",
        "name": "Updated Category"
    }
}

// NOTE: When updating, url is 'product/uuid-aaa/' so product's uuid does not need to be in data
// Update product, get category
{
    "name": "Updated Product",
    "category": "uuid-xxxx"
}
// Update product, create category
{
    "name": "Updated Product",
    "category": {
        "name": "Created Category"
    }
}
// Update product, update category
{
    "name": "Updated Product",
    "category": {
        "uuid": "uuid-yyyy",
        "name": "Updated Category"
    }
}

// Edge cases: create category with no fields
{
    "category": {}
}
// Edge cases: update category with no fields
{
    "category": {
        "uuid": "uuid-yyyy",
    }
}
gerdemb commented 1 year ago

Partial implementation of a nested create/update serializer

    def create(self, validated_data):
        if self.can_create:
            return super().create(validated_data)
        else:
            raise serializers.ValidationError({
                api_settings.NON_FIELD_ERRORS_KEY:
                "This api is not configured to create new objects"
            })

    def update(self, instance, validated_data):
        if self.can_update:
            return super().update(instance, validated_data)
        else:
            raise serializers.ValidationError({
                api_settings.NON_FIELD_ERRORS_KEY:
                "This api is not configured to update existing objects"
            })

    def to_internal_value(self, data):
        lookup_field = self.lookup_field
        assert lookup_field is not None, "You should specify lookup_field"

        if lookup_field in data:
            lookup_value = data.pop(lookup_field)

            model = self.Meta.model
            try:
                self.instance = model.objects.get(**{lookup_field: lookup_value})
            except model.DoesNotExist:
                raise serializers.ValidationError({
                    lookup_field:
                    f"{model._meta.object_name} matching query {lookup_field}={lookup_value} does not exist."
                })
        return super().to_internal_value(data)
gerdemb commented 1 year ago

ISSUES

  1. How to switch between a nested serializer and slug related field depending on the which type of data is POSTed? Pseudo code:

if(field_value is slug):
  field = SlugRelatedField
if(field_value is nested_dictionary)
  field = NestedSerializer
gerdemb commented 1 year ago

I think this issue can be closed with the new version of WritableNestedSerializer