-
-
Notifications
You must be signed in to change notification settings - Fork 7k
Validation on many to many field when default=None #9790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds validation to prevent runtime errors when using PrimaryKeyRelatedField
with many=True
and default=None
on ManyToMany fields, which causes crashes when trying to save the serializer.
- Adds validation in
get_fields()
method to detect and prevent the problematic configuration - Includes comprehensive tests to verify the validation works correctly
- Updates contributing documentation with helpful testing tips
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
rest_framework/serializers.py | Adds validation logic to detect ManyToMany fields with invalid default=None configuration |
tests/test_serializer.py | Adds test class to verify the new validation raises appropriate errors |
docs/community/contributing.md | Adds documentation for database testing and pytest output options |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Hi @peterthomassen, I've already resolved both comments with your suggestions. Please, let me know if I can help with anything else so you can merge the PR! |
Updated the error message for ManyToManySourceSerializer to remove suggestion for default value to make the test work
lgtm. Please ping me once @browniebroke's comments are addressed, and I'll approve. (I believe the approval would be dismissed by those changes if I did it now.) |
Hi @browniebroke and @peterthomassen , I have addressed both comments. Please, let me know if there is anything else you think is missing! Cheers! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! Thank you for contributing this.
Note: Before submitting a code change, please review our contributing guidelines.
Description
Hi!
First of all, thank you for the library, it's very useful for my work!
While working on a project, I got a 500 error in production due to a configuration error on my part (copy-paste 🤦♂️): when using a
PrimaryKeySerializer
on a ManyToMany field and settingdefault=None
like this:It crashes at runtime with the following exception when no value is provided for
targets
field:Now it seems simple and intuitive, but in production, with thousands of lines, dozens of serializers, and hundreds of fields, it took me a long time to figure out what the problem was.
To prevent errors in production, I added a new validation in the
get_fields()
method with a super descriptive message indicating a solution (I also added some tests to check this new behavior).I couldn't find an earlier place to perform the validation, but this change allows the validation to run for other methods such as
create()
orupdate()
. If you have a better place where the code block could be moved, please let me know, and I'll make the necessary adjustments.Finally, since this is my first time collaborating on the project, I added some help to the contributing.md file to explain a few things, such as the
-s
parameter in pytests, which allows you to obtain the output during testing, ordjango.test.TestCase
inheritance so you can use the Django DB. If you think it shouldn't be there, or you don't like the format, let me know and I can make the changes.Thanks and best regards!