Contributing a Different/Complementary Approach

[claytondaley]

Background: Before open sourcing our DIY nested writable logic, I wanted to look for existing package(s) -- to avoid fragmenting the users/contributors. I've been digging through this codebase to evaluate it as an alternative to our existing package. My conclusion is that it's missing a key feature (that we use extensively) due to a difference in design.

The Feature: If I follow the logic in this package, it achieves a get-or-create behavior by having the parent serializer lookup related objects by pk (or another package-wide name). Because the parent serializer does the lookup, it's impossible to look up related objects by non-PK fields (e.g. #12) unless you reconfigure the entire project to treat that field as the "pk field".

The Opportunity: Our approach delegates this behavior to the nested serializer. The nested serializer takes the data and returns a model instance (or list of instances) based on how its configured. This make it possible to configure each nested writable serializer with different behaviors. For example:

• different nested serializers can use different update vs. create vs. both rules
• match on pk, name, a composite key like ['field1', 'field2'], or even exact matches using __all__

To better illustrate the difference, consider a usage example:

# a generic serializer for the child Model; can be reused for top-level serialization
class ChildSerializer(ModelSerializer):
    ...  

# this "trivial" class mixes in the desired nested behavior
class GetOrCreateNestedChildSerializer(GetOrCreateSerializerMixin, ChildSerializer):
    pass

# NestedSaveMixin *only* decides the order of saves to ensure FK integrity
class ParentSerializer(NestedSaveMixin, ModelSerializer):

    # the nested serializer returns a model instance based on its configuration
    child = GetOrCreateNestedChildSerializer(
        # default behavior, but makes the point that you *can* change it per-serializer
        queryset=Patient.objects.all(),
        # define the matching criteria
        match_on=['name'],  
    )

An obvious advantage of your approach is that there's less configuration. It's theoretically possible to have dozens of nested serializers and mix the GetOrCreate mixin into the one parent class. At the same time, we need the UniqueFieldsMixin on most (all?) our child serializers; at that point the difference is pretty trivial.

If this package is ever going to support per-nested-serializer configurations, it will need to adopt an approach like this. To avoid creating a competing package, I'd be willing to take the time to get it integrated as a backwards-compatible extension to this package. Either way, I need to add a bunch of the bells-and-whistles found in your package so the "incremental" effort is mostly around backwards compatibility.

Obviously, this isn't a trivial change so I wanted to get buy-in before investing any time into it.

[MightySCollins]

We are looking into making our own logic to handle this as well. Would be great to have a solution built in.

[claytondaley]

Our implementation is available at my fork. This repo is built on master so it should be a drop-in replacement for recent versions of this package. I haven't had the time (or received the green light) to fully integrate it so my classes are still missing some backwards-compatible bells-and-whistles:

• no pk inference on OneToOne relations
• I don't think it will remove related objects if you send in a subset. I suspect it will add/replace them.
• There may be other missing features as I haven't used this package to be super familiar.

If that sounds OK for your situation, replace any existing mixins with the new ones:

• Mix RelatedSaveMixin into the top-level serializer. This will ensure the nested serializers are saved in the right order.
• Mix GetOrCreateNestedSerializerMixin into all nested serializers.
  • Right now GetOrCreate is the only implementation and the Get part is ONLY a Get. It will not update the "got" object. I suspect this is true of both fields and related objects (e.g. if a nested serializers returns a different related object, it won't be updated). An UpdateOrCreate should be an easy add, if needed.
  • match_on defaults to ['pk'] to improve backwards compatibility with this project. It can be changed by setting DEFAULT_MATCH_ON on the serializer or sending the kwarg match_on when creating the serializer. Accepts a list of fields (strs) or the special keyword '__all__'.
  • GetOrCreate serializers need a queryset Meta property or kwarg so they know what to match against.
  • Because GetOrCreate is mixed into the nested (not parent) serializer, Unique is handled internally; you don't need to do anything special (no extra mixins). This mixin is also a subclass of RelatedSaveMixin so you don't need both if you want to do a 3rd layer of nesting.

If you need a specific feature integrated, create a ticket and I'll let you know if it's something I can quickly take care of.

[ruscoder]

Hello @claytondaley! Thank you for the interesting thoughts. Your approach is in style of others drf fields and it looks flexible. I think it will be good if we can implement both approaches, the existing by default and your when we need more flexible control on the child serializer. I’ll try to research this area in free time.

[ir4y]

Hi @claytondaley thank you for your input.

We discussed your proposal and decided that it make sense to support both approaches.
The current one that lookup related objects by pk (it will be default behavior to keep backward capabilities) and another that delegates this behavior to the nested serializer.

Please send the merge request, we will review it and then merge.

Best regards,
Beda Ilya
CTO at beda.software

[claytondaley]

Great. I'm actually taking the time to open source it (with permission) so I can use it in some other projects of mine. I'm still finding/cleaning up little issues (like handling source) and will wrap it up into a PR once I get the two codebases integrated.

[claytondaley]

Just a quick update. I made some architectural changes to better align my approach with the existing code base. I've been using my code in a side project and have been resolving issues as I go. From a code standpoint, I'm close to trying to combine the two approaches, but it won't happen quickly in calendar-time unless someone else is trying to use it and runs into an issue that can only be resolved by prioritizing the effort.

[claytondaley]

In case someone wants/needs to try it out, here's the (public) side project where my new code is in use:

https://github.com/PeteAndersen/swarfarm/blob/1b3229b55b09e043e89c916a68acab16ebeaa2bc/planner/serializers.py#L89

The example not only shows the top-level get-or-create but an example of nesting GetOrCreate serializers to an arbitrary depth across both forward- and reverse-relations.