New-Style Serializers #101
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
claytondaley
force-pushed
the
new-style-serializers
branch
from
4c1d8f2
to
1f7b373
Compare
|
I removed the last comment. It looks like I've done the same thing by default (i.e. passing the entire |
|
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. |
|
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. It has also drawn my attention to several feature gaps that will need to be resolved or documented:
|
claytondaley
force-pushed
the
new-style-serializers
branch
from
164f1a9
to
7a82fb7
Compare
|
I went ahead and committed all of the tests from I also made a small change to I could catch and convert |
|
I just had a work requirement that
The feature certainly could be added/supported, but I wonder if you'd agree that it's not a test worth supporting as-is. |
|
Just came across this and it looks like it could address my issue- i'm using |
|
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). |
|
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? |
|
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". |
|
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). |
|
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 ValueError: Cannot assign "OrderedDict([("name": <name1>)])": "Membership.profile" must be a "Profile" instance.Not sure what goes wrong here. --- EDIT ---Changed |
|
@TJHeeringa can you give me the full stack? I don't see any lines from And what is |
|
Seems the RelatedSaveMixin doesn't get called properly when using serializer = MembershipImportSerializer(many=True, data=data)to serializer = MembershipImportSerializer(data=data[0])then Upon closer inspection of the 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. |
|
Thanks. Yes it looks like the |
|
Also seems that the |
|
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. |
|
OK so it looks like you might hit this code even if you don't |
|
@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. |
|
Really looking forward to this, as we use @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. |
|
@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. |
|
Hello! Sorry for the long response from our side. @claytondaley What about backward compatibility for now? |
|
@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?
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. |
claytondaley
force-pushed
the
new-style-serializers
branch
from
608b299
to
d05c9d8
Compare
|
I think an easier up-gradation path is the only concern. great work. |
|
@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? |
|
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. |
|
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 |
|
documenting the migration path will be great |
|
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:
Not sure what the pro's and cons are for jazzband and drf-extentions, but both time the code base is split. |
|
Is there any update on the approval/denial status?? I'd love to see this merged in as it fixes several issues that I've been running into (M2M fields, As for integrating this here or publishing it somewhere else I'd be inclined to agree that a unified code base/development effort has several benefits, but I'd be happy with either solution. |
Codecov Report
@@ Coverage Diff @@
## master #101 +/- ##
==========================================
- Coverage 98.18% 88.66% -9.53%
==========================================
Files 3 3
Lines 220 538 +318
==========================================
+ Hits 216 477 +261
- Misses 4 61 +57
Continue to review full report at Codecov.
|
|
At least part of the hold-up was migration instructions. I've pushed a commit adding some -- and drawing attention to |
claytondaley
force-pushed
the
new-style-serializers
branch
from
05c8c7b
to
9d10781
Compare
|
You reviewed too fast lol... was trying to fix my typos without creating commit noise. |
@claytondaley have you managed to implement old style serialisers using new ones? I believe if we have backward compatibility for existing users using new code base even without some edge cases it will be great. |
|
Yes. All of the test cases have been recreated using new serializers. There are two specific cases where they don't work as noted in this thread: partial behaviors and generic relations. Current partial behavior is hard to replicate for architectural reasons mentioned above. Generic relations are unsupported because I'm not familiar with them and wasn't using them. |
|
Thank you for your contribution. You did a lot and we can merge these changes once it meets all requirements.
Can you please do these things? |
|
Is there a good way to allow others who benefit from the new features to help? I'm not actually a developer so, for example, I will never benefit from any learnings related to |
you should push further, I think you are almost there |
| ############ | ||
|
|
||
|
|
||
| class NewAvatarSerializer(UpdateOrCreateNestedSerializerMixin, serializers.ModelSerializer): |
|
We're using this code with |
|
100% test coverage (which I can work on) and support for |
@auvipy I have noticed that they do not work well together nicely. Because the saving is different, the A little more detail on the The HyperlinkedRelatedField has a method called class HyperlinkedRelatedField(RelatedField):
def get_url(self, obj, view_name, request, format):
"""
Given an object, return the URL that hyperlinks to the object.
May raise a `NoReverseMatch` if the `view_name` and `lookup_field`
attributes are not configured to correctly match the URL conf.
"""
# Unsaved objects will not yet have a valid URL.
if hasattr(obj, 'pk') and obj.pk in (None, ''):
return None
lookup_value = getattr(obj, self.lookup_field)
kwargs = {self.lookup_url_kwarg: lookup_value}
return self.reverse(view_name, kwargs=kwargs, request=request, format=format)To get around this specific error you could for example create the following serializer. You would then use this class DrfWritableSupportedHyperlinkMixin(object):
def get_url(self, obj, view_name, request, format):
"""
Given an object, return the URL that hyperlinks to the object.
May raise a `NoReverseMatch` if the `view_name` and `lookup_field`
attributes are not configured to correctly match the URL conf.
"""
# Unsaved objects will not yet have a valid URL.
if hasattr(obj, 'pk') and obj.pk in (None, ''):
return None
# Drf-writable doesn't save things properly, so we get a dict instead of obj. This causes the previous check to
# fail even though it should pass
if type(obj) is dict and obj.get("pk") in (None, ""):
return None
lookup_value = getattr(obj, self.lookup_field)
kwargs = {self.lookup_url_kwarg: lookup_value}
return self.reverse(view_name, kwargs=kwargs, request=request, format=format)
class DrfWritableSupportedHyperlinkedIdentityField(DrfWritableSupportedHyperlinkMixin, serializers.HyperlinkedIdentityField):
pass
class DrfWritableSupportedHyperlinkedRelatedField(DrfWritableSupportedHyperlinkMixin, serializers.HyperlinkedRelatedField):
pass
class DrfWritableSupportedHyperlinkedModelSerializer(serializers.HyperlinkedModelSerializer):
serializer_related_field = DrfWritableSupportedHyperlinkedRelatedField
serializer_url_field = DrfWritableSupportedHyperlinkedIdentityField |
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 updatingUpdate: Find a match (or fail) and updateCreate: Create a new object (or fail)UpdateOrCreate) are also availableThe current implementation correctly handles forward- and reverse- foreign key relationships as well as serialization of
ManyToManyfields. 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).