New-Style Serializers

[claytondaley]

This is a PR of my increasingly-mature patch (discussed in #57) that provides a new style of Serializer. Instead of depending on the parent serializer to make Create/Update decisions, each nested serializer manages its own behavior. This makes it possible to use different semantics (e.g. Get/Create) for different nested serializers and to control the fields that are used in matching (i.e. match_on).

New-style Serializers use the following semantics:

• Get: Find a match (or fail) without updating
• Update: Find a match (or fail) and update
• Create: Create a new object (or fail)
• Combined serializers (e.g. UpdateOrCreate) are also available

The current implementation correctly handles forward- and reverse- foreign key relationships as well as serialization of ManyToMany fields. A cursory skim of issues suggests that it may address #19, #26, #28, #41, #85, #89, #90, and #99. Due to design decisions I think it also fixes #48, #49, and #88.

The only thing I haven't worked out is backwards compatibility with the existing classes (e.g. preventing default behavior when the nested serializer inherits from new-style classes).

[codecov-io]

Codecov Report

Merging #101 (7a82fb7) into master (87a1476) will decrease coverage by 9.92%. The diff coverage is 82.31%.

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
- Coverage   98.59%   88.66%   -9.93%     
==========================================
  Files           3        3              
  Lines         213      538     +325     
==========================================
+ Hits          210      477     +267     
- Misses          3       61      +58     

Impacted Files	Coverage Δ
drf_writable_nested/mixins.py	88.40% <82.31%> (-10.11%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 87a1476...608b299. Read the comment docs.

[claytondaley]

I removed the last comment. It looks like I've done the same thing by default (i.e. passing the entire kwargs to each instance).

[ruscoder]

Hello @claytondaley! Thanks for the contributions, I see a lot of work have been done. I'll try to find time to review your PR.

[claytondaley]

Thanks. No urgency on my behalf. I had to get back into the package to add a more complex matching behavior for my company which prompted me to revisit/refactor the base functionality. Starting to cover your test cases helped me verify functionality during the rewrite and fix a couple bugs (e.g. field_name vs. field.source).

It has also drawn my attention to several feature gaps that will need to be resolved or documented:

• partial
• GenericRelation
• ~~some OneToOne behaviors~~

[claytondaley]

I went ahead and committed all of the tests from test_writable_nested_model_serializer, re-implemented using new-style serializers (found in tests/serializers.py). I currently raise an Exception for partial and GenericRelation to make it clear why those tests (6x) are failing. Technically, one of the partial tests already passes as-is since the only thing I don't do is propagate partial to nested serializers. I'll take a stab at that after I implement the work-requirement that got me back into this.

I also made a small change to test_update_raise_protected_error to get it passing. The test expects a ValidationError when trying to remove a protected relation, My current implementation raises the original ProtectedError. By contrast, for test_nested_partial_update_failed_with_empty_direct_fk_object I retain the existing logic that converts TypeError and ValueError to ValidationError.

I could catch and convert ProtectedError to ValidationError as well, but I'm surprised that serializer.save() is raising ValidationError; I expect that out of is_valid(). I wonder if you'd be open to permitting Type/Value/Protected to pass unaltered from save() as part of the documentation around using the new-style serializers.

[claytondaley]

I just had a work requirement that partial might have solved so I was thinking through it. I'm not sure it makes sense to propagate partial in new style serializers. Specifically, the fields on a serializer are configured when the class is defined. To make a nested serializer partial, you'd have to do one of the following:

1. Do it when it's defined. This is valid, but isn't dynamically configurable (e.g. the current test).
2. Reconfigure when constructing. Unfortunately, this would affect any other (non-partial) users of the serializer.
3. Reconfigure when calling. Unfortunately super().save() (which I use) doesn't have a kwarg for this right now.

The feature certainly could be added/supported, but I wonder if you'd agree that it's not a test worth supporting as-is.

[tybritten]

Just came across this and it looks like it could address my issue- i'm using unique_together which is breaking the curent drf-writeable-nested. I'm not sure if it would also fix my other issue- i have M:M relations between two of the sub-objects which then fails on initial creation (does not exist)

[claytondaley]

If you'd like to test it out for your case, I'm happy to provide guidance and bug fixes on the brach in my repo (which will also update this PR).

[TJHeeringa]

I have many serializer in which I would like one related field to be writable, but not the other related fields. From the description this seems to fix that. Is there any indication for when this is ready/going to be merged?

[claytondaley]

Yes it should work for your use case. Unless the sentiment of the maintainers have changed, the only functional gap is the ability to mix these with the legacy classes. It doesn't sounds like that matters to you. If you wanted to test it and find it satisfactory, I can try to figure out what it would take to fully integrate the two if that was the blocker. My guess is "not much".

[TJHeeringa]

Thank you for your quick response. I have installed the latest version of your branch in my venv using

pip install git+https://github.com/claytondaley/drf-writable-nested.git@da8dc88f09faa074e99d4113cbb9c100aa65fc86#egg=drf-writable-nested

(posting the command as reference for others if they want to test your branch too.)

In the next few days I should have some time to update my serializers using the new-style-serializers and test it for my use case (stuff mentioned above and #89).

[TJHeeringa]

Hi @claytondaley,

I have tried to convert my setting to your new style serializer but have not succeeded.

Not everything I have is completely relevant, so I will try to reduce it to a minimal version.

Models

class Profile(models.Model):
    slug = models.SlugField(default=uuid.uuid4)
    name = models.CharField()
    associations = models.manyToMany(Association, through=Membership)

class Membership(models.Model):
    slug = models.SlugField(default=uuid.uuid4)
    profile = models.Foreignkey(Profile)
    association = models.Foreignkey(Association)

class Association(models.Model):
    slug = models.SlugField(default=uuid.uuid4)

class Data(models.Models):
    slug = models.SlugField(default=uuid.uuid4)
    value = models.CharField()
    membership = models.Foreignkey(Membership)
    association = models.Foreignkey(Association)

Serializers:

class ProfileImportSerializer(mixins.CreateOnlyNestedSerializer, HyperlinkedModelSerializer):
    class Meta:
        model=Profile
        extra_kwargs = {
            'url': {'lookup_field': 'slug'},
        }

class DataImportSerializer(mixins.CreateOnlyNestedSerializer, HyperlinkedModelSerializer):
    class Meta:
        model=Data
        extra_kwargs = {
            'url': {'lookup_field': 'slug'},
            'membership': {'lookup_field': 'slug'},
            'association': {'lookup_field': 'slug'},
        }

class MembershipImportSerializer(mixins.RelatedSaveMixin, HyperlinkedModelSerializer):
    profile = ProfileImportSerializer()
    data = DataImportSerializer(many=True)

    class Meta:
        model=Membership
        extra_kwargs = {
            'url': {'lookup_field': 'slug'},
            'profile': {'lookup_field': 'slug'},
            'association': {'lookup_field': 'slug'},
        }

I pass something like

data = [
{
    "association": "/associations/<slug>"
    "data": [
        {"value": <str1>, "association": "/associations/<slug>"},
        {"value": <str2>, "association": "/associations/<slug>"}
    ],
    "profile": {"name": <name1>}
},
{
    "association": "/associations/<slug>"
    "data": [
        {"value": <str3>, "association": "/associations/<slug>"},
        {"value": <str4>, "association": "/associations/<slug>"}
    ]
    "profile": {"name": <name2>}
}
]

to the serializer using

serializer = MembershipImportSerializer(many=True, data=data)

So far so good. When calling serializer.is_valid(raise_exception=True) no errors are triggered, but when calling serializer.save() the following error is raised:

ValueError: Cannot assign "OrderedDict([("name": <name1>)])": "Membership.profile" must be a "Profile" instance.

Not sure what goes wrong here.

--- EDIT ---

Changed HyperlinkedIdentitySerializer to HyperlinkedModelSerializer

[claytondaley]

@TJHeeringa can you give me the full stack? I don't see any lines from drf-writeable-nested.

And what is HyperlinkedIdentitySerializer?

[TJHeeringa]

~HyperlinkedIdentitySerializer~HyperlinkedModelSerializer is a default Django Rest Framework serializer that provides an url attribute based on the lookup_field.

Seems the RelatedSaveMixin doesn't get called properly when using many=True. When I change the serializer call from

serializer = MembershipImportSerializer(many=True, data=data)

to

serializer = MembershipImportSerializer(data=data[0])

then drf-writable-nested is called. I got another error, but that turned out to be that I didn't have the membership included in the fields in the Data serializer. Then it works.

Upon closer inspection of the RelatedSaveMixin I saw that there was no many_init. The NestedSaveSerializer does, so I change

class MembershipImportSerializer(mixins.RelatedSaveMixin, HyperlinkedModelSerializer):
    profile = ProfileImportSerializer()
    data = DataImportSerializer(many=True)

    class Meta:
        model=Membership
        extra_kwargs = {
            'url': {'lookup_field': 'slug'},
            'profile': {'lookup_field': 'slug'},
            'association': {'lookup_field': 'slug'},
        }

to

class MembershipImportSerializer(mixins.CreateOnlyNestedSerializerMixin, HyperlinkedModelSerializer):
    profile = ProfileImportSerializer()
    data = DataImportSerializer(many=True)

    class Meta:
        model=Membership
        extra_kwargs = {
            'url': {'lookup_field': 'slug'},
            'profile': {'lookup_field': 'slug'},
            'association': {'lookup_field': 'slug'},
        }

and then calling the serializer like

serializer = MembershipImportSerializer(many=True, data=data)

fully works.

[claytondaley]

Thanks. Yes it looks like the RelatedSaveMixin needs to have a custom ListSerializer as well. I'll add a test case and figure out how much of NestedSaveListSerializer (perhaps all) needs to be included there.

[TJHeeringa]

Also seems that the ReadOnlyFields from Django Rest Framework are breaking. I added a ReadOnlyField on the Data serializer, and got .

[claytondaley]

Based on the location in the code, it looks like you're matching on a read only field. In that case, the system needs a way to convert the JSON value into an internal value for the purposes of matching.

[claytondaley]

OK so it looks like you might hit this code even if you don't match_on that specific field so I'll ignore the exception knowing that it will result in an error later if you attempt to match on it.

[claytondaley]

@TJHeeringa that's should be a quick fix for your read only fields. Still need to add a test. The Related ListSerializer will take me longer.

[rollue]

Really looking forward to this, as we use id and uuid in models. We keep the django default id, but use uuid to expose the data in our external API's. This will fix #19.

@claytondaley Do you have any idea how long this might take? We can wait a few weeks, but anything longer than that we have to resort to using the method described in the DRF doc.

[claytondaley]

@mhoonjeon more of a question for maintainers. I'm currently unable to run/write test cases as I noted in #132. I'm not clear what the maintainers would need to actually merge this in.

[TJHeeringa]

@mhoonjeon more of a question for maintainers. I'm currently unable to run/write test cases as I noted in #132. I'm not clear what the maintainers would need to actually merge this in.

@ruscoder @ir4y Could you help us out by making clear what is needed in order to merge this very useful PR?

[ruscoder]

Hello! Sorry for the long response from our side. @claytondaley What about backward compatibility for now?

[claytondaley]

@ruscoder I'm happy to work on backwards compat, I was trying to verify that some behaviors were acceptable (e.g. starting here). If it wasn't going to be accepted without improvement, compat wasn't a priority.

What's the ideal compat result?

• A nested new-style serializer preempts default create behavior?
• Do we need to support the reverse case i.e. old nested in new (which honestly might just work)?

FYI I have a broken bone in my elbow. I seem to type without too much pain, but it makes everything else in my day slower so I will have less time to make progress on this.

[auvipy]

I think an easier up-gradation path is the only concern. great work.

[ruscoder]

@claytondaley For existing users of the library it will be difficult to upgrade to the new style serializers but we will need to support the old-style serializers too. The code has a lot of differences and it can take much time to support both versions in one package.

So, I suggest you publish and maintain new style serializers as a separate package. We'll make links in README to the new style serializers package with a description of why it can be useful.

What do you think?

[auvipy]

you can add the changes https://github.com/chibisov/drf-extensions here too, I am a maintainer of that package.

[claytondaley]

I can certainly make a new library and contribute it to something like JazzBand, but was trying to avoid splitting the base of users/contributors. I actually started with your library. I kept adding features based on our needs (often mirrored by issues here). Hoping to contribute it back, I even tried to preserve methods like field type definitions, but ended up needing enough local workarounds that it was simpler to refactor the method.

[claytondaley]

I've offered to develop a migration path to help keep the user/contributor bases together, but need a clear idea of what you would expect backwards compatibility to support.

EDIT: It may even be possible to simulate the old-style syntax while using the new-style serializers. All you really need to do is automatically create nested serializers that follow the default rules of the existing serializers. A BackwardsCompat mixin or metaclass might work for everything but the unsupported edge cases like partial and GenericRelation.

[auvipy]

documenting the migration path will be great

[TJHeeringa]

I don't know how I can add to this, but I think it a good idea to make a decision.

As far as I can see there are 3 choices:

• include in this package; requires migration path, but not multiple packages floating around doing the same thing
• move it to jazzband;
• move it to drf-extentions

Not sure what the pro's and cons are for jazzband and drf-extentions, but both time the code base is split.