Add support for source with attributes in extra_kwargs of ModelSerializer#9077
Add support for source with attributes in extra_kwargs of ModelSerializer#9077BergLucas wants to merge 15 commits intoencode:mainfrom
Conversation
|
___________________________________ summary ____________________________________ |
auvipy
left a comment
There was a problem hiding this comment.
And this should also require documentation update as far as I can understand!
b10dfc2 to
c1ccc37
Compare
auvipy
left a comment
There was a problem hiding this comment.
does this change conform with the principle of adding compatibility with django? I'm not fully convinced about the merit of the new change
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
I think this is still relevant and shouldn't be marked as stale. |
| if attr not in attr_info.relations: | ||
| break |
There was a problem hiding this comment.
This seems to cause trailing attributes to be ignored if they are "after" other relation attributes, but are not a relation themselves. Is this intended?
If so, I think this scenario should also have a test case.
There was a problem hiding this comment.
It is not really intended. Is it possible to have attributes other than relations with sufficient information in this function to be able to follow them? As far as I know, this isn't the case, so I just did it this way. It's also possible that I misunderstood your comment
|
@auvipy Do you know why the bot doesn't remove stale label when there is a new comment? |
|
@BergLucas Hey 👋🏻 Is it possible for you to rebase this branch from the latest main, so it won't be closed as stale and we can push for another tour of review & testing? Thanks in advance. |
…://github.com/BergLucas/django-rest-framework into improvement/source-attributes-in-extra_kwargs
|
Hi @ulgens , Thanks for your reviews! I've rebased the branch and responded to the comments. (Sorry for the delay, I've been quite busy these last few weeks.) |
There was a problem hiding this comment.
Pull request overview
Adds support for using dotted source paths (e.g. user.username) in ModelSerializer.Meta.extra_kwargs so that DRF can infer the correct serializer field type/validators from related model fields, without requiring explicit field declarations.
Changes:
- Update
ModelSerializer.get_fields()to resolve dottedsourcepaths across model relations when building fields. - Add a regression test asserting the generated field representation includes validators from related model fields (e.g.
User.username). - Document the new
extra_kwargscapability with an example mapping serializer fields to related model attributes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
rest_framework/serializers.py |
Adds dotted-source traversal to choose the correct related model metadata when building auto-generated fields. |
tests/test_model_serializer.py |
Adds coverage for dotted source paths in extra_kwargs and expected field mapping/validators. |
docs/api-guide/serializers.md |
Documents using extra_kwargs with source to map serializer fields to related model fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if attr not in attr_info.relations: | ||
| break | ||
|
|
||
| attr_model = attr_info.relations[attr].related_model |
There was a problem hiding this comment.
The dotted source traversal rewrites the field based on related model metadata, but it currently follows to-many relations too (e.g. M2M or reverse FK). That can generate a scalar field for sources like groups.name, which will then fail/behave incorrectly at runtime because attribute traversal hits a RelatedManager.
Consider bailing out when an intermediate relation has to_many=True (and keep source unchanged so build_field raises ImproperlyConfigured), or raise a clearer configuration error for dotted sources that traverse collections.
| if attr not in attr_info.relations: | |
| break | |
| attr_model = attr_info.relations[attr].related_model | |
| relation_info = attr_info.relations.get(attr) | |
| if relation_info is None: | |
| break | |
| if getattr(relation_info, 'to_many', False): | |
| # Do not rewrite sources that traverse to-many relations. | |
| break | |
| attr_model = relation_info.related_model |
| source_attrs = source.split('.') | ||
| source_info = info | ||
| source_model = model | ||
|
|
||
| attr_info = info | ||
| attr_model = model | ||
|
|
||
| for attr in source_attrs[:-1]: | ||
| if attr not in attr_info.relations: | ||
| break | ||
|
|
||
| attr_model = attr_info.relations[attr].related_model | ||
| attr_info = model_meta.get_field_info(attr_model) | ||
| else: | ||
| attr = source_attrs[-1] | ||
| if ( | ||
| attr in attr_info.fields_and_pk | ||
| or attr in attr_info.relations | ||
| or hasattr(attr_model, attr) | ||
| or attr == self.url_field_name | ||
| ): | ||
| source = attr | ||
| source_info = attr_info | ||
| source_model = attr_model |
There was a problem hiding this comment.
This logic runs for every auto-generated field, even when source has no dotted path. You can avoid the extra split()/loop and repeated get_field_info() calls by guarding the new traversal with something like if '.' in source: (or if len(source_attrs) > 1: after splitting).
| source_attrs = source.split('.') | |
| source_info = info | |
| source_model = model | |
| attr_info = info | |
| attr_model = model | |
| for attr in source_attrs[:-1]: | |
| if attr not in attr_info.relations: | |
| break | |
| attr_model = attr_info.relations[attr].related_model | |
| attr_info = model_meta.get_field_info(attr_model) | |
| else: | |
| attr = source_attrs[-1] | |
| if ( | |
| attr in attr_info.fields_and_pk | |
| or attr in attr_info.relations | |
| or hasattr(attr_model, attr) | |
| or attr == self.url_field_name | |
| ): | |
| source = attr | |
| source_info = attr_info | |
| source_model = attr_model | |
| source_info = info | |
| source_model = model | |
| if '.' in source: | |
| source_attrs = source.split('.') | |
| attr_info = info | |
| attr_model = model | |
| for attr in source_attrs[:-1]: | |
| if attr not in attr_info.relations: | |
| break | |
| attr_model = attr_info.relations[attr].related_model | |
| attr_info = model_meta.get_field_info(attr_model) | |
| else: | |
| attr = source_attrs[-1] | |
| if ( | |
| attr in attr_info.fields_and_pk | |
| or attr in attr_info.relations | |
| or hasattr(attr_model, attr) | |
| or attr == self.url_field_name | |
| ): | |
| source = attr | |
| source_info = attr_info | |
| source_model = attr_model |
| 'first_name': {'source': 'user.first_name'}, | ||
| 'last_name': {'source': 'user.last_name'} |
There was a problem hiding this comment.
The new example uses dotted sources to pull fields from the related user. By default, ModelSerializer.create()/update() does not support writable dotted-source fields (it asserts unless you set read_only=True or implement explicit create/update handling). It’d help to either mark these example fields as read_only=True (via extra_kwargs) or add a short note clarifying the write behavior.
| 'first_name': {'source': 'user.first_name'}, | |
| 'last_name': {'source': 'user.last_name'} | |
| 'first_name': {'source': 'user.first_name', 'read_only': True}, | |
| 'last_name': {'source': 'user.last_name', 'read_only': True}, |
|
|
||
|
|
There was a problem hiding this comment.
The test covers the happy path for dotted sources, but it doesn’t cover an important failure mode introduced by the traversal: dotted sources that cross a to-many relation (M2M or reverse FK) should raise ImproperlyConfigured instead of generating a field that can’t be resolved at runtime. Adding an assertion test for a source like user.groups.name (or any to-many hop) would prevent regressions here.
| def test_source_with_to_many_raises_improperly_configured(self): | |
| class UserProfile(models.Model): | |
| user = models.ForeignKey(User, on_delete=models.CASCADE) | |
| class InvalidUserProfileSerializer(serializers.ModelSerializer): | |
| groups = serializers.CharField(source='user.groups.name') | |
| class Meta: | |
| model = UserProfile | |
| fields = ('groups',) | |
| with self.assertRaises(ImproperlyConfigured): | |
| InvalidUserProfileSerializer() |
refs #4688
Description
Hello dear maintainers, it's my first contribution to django rest framework so I hope I didn't do anything wrong.
In the pull request referenced above, there is someone that mentioned that we could not use source with attributes in extra_kwargs of a ModelSerializer.
For example, the following code would create an error:
I think it could be a very interesting feature because at the moment, when we have a foreign key to another model, we're obliged to specify the fields explicitly in the serializer. However, this means that if we have special validators on the fields of our model, we're obliged to put them back on the serializer fields.
For example, the
usernamefield of Django's defaultUserhas a special validator so if we just define a basicCharField, it would not validate the data the same way as the model would validate it:This could create a difference between the way the model validates data and the way the serializer validates data if we forgot a validator or if we change the model without changing the serializer.
In this pull request, I added this feature so that we could just specify the model field we want in the source of extra_kwargs and it will generate the right field on the serializer.
The code may seem a little odd, but I've tried to keep the changes in one place only. I've also tried to maintain a good error message so that, if at any point the path to the field is wrong, it returns the full path in the error message and not just part of it.
Thanks for reading and please let me know if there are any changes that could be made.