Duplicate error msg fix for IP address field #9647
Conversation
Hi @browniebroke , |
|
Hi @browniebroke , |
|
|
||
|
|
||
| # Define the serializer in setUp | ||
| class TestSerializerTestCase(TestCase): |
There was a problem hiding this comment.
I would have expected this test to be next to the existin ones:
There was a problem hiding this comment.
Thanks for pointing out the existing test cases @browniebroke , I did some changes and incorporated my test cases within the same class, and did some minor changes along with it.
|
Thank you @auvipy @browniebroke for your reviews. How comfortable do we feel with allowing highly anon contributions. We've allowed it in the past, but we could take a different approach. |
Noted @tomchristie! , But could we at least fix this bug for now ? |
I don't have a strong opinion about that. I don't mind as long as the quality of the contribution passes our standard, and that they're respectful and considerate. People have to start contributing somewhere and having just created a GitHub account shouldn't be a blocker. Not using their real name is a personal decision which, while I'm not being a big fan of, I respect. Might be better to discuss this separately, I wonder if I'm missing anything... Is there a risk with doing so? How do we enforce it? What should be wary of? |
We'll get to it when we'll get to it. It looks good but there is no rush for merging it, as far as I'm concerned |
Sure @browniebroke , no worries :) |
tests/test_model_serializer.py
Outdated
| def test_invalid_ipv6_for_ipv6_field(self): | ||
| """Test that an invalid IPv6 raises only an IPv6-related error.""" | ||
| self.model._meta.get_field("address").protocol = "IPv6" # Set field to IPv6 only | ||
| invalid_data = {"address": "invalid-ip"} | ||
| serializer = self.serializer_class(data=invalid_data) | ||
|
|
||
| with self.assertRaises(ValidationError) as context: | ||
| serializer.is_valid(raise_exception=True) | ||
|
|
||
| self.assertEqual( | ||
| str(context.exception.detail["address"][0]), | ||
| "Enter a valid IPv6 address." | ||
| ) |
There was a problem hiding this comment.
I tried to run this on our main branch and it doesn't raise the duplicated error messages as reported in the issue. The test fails, but it raises a single validation message (the generic one Enter a valid IPv4 or IPv6 address.).
While it's a nice idea to try to share the test setup, I don't think it works very well. Basically, patching the field protocol on the model is not the same as defining a fresh model/serializer.
We should probably be a bit more exhaustive and define 3 models -or at least 3 separate fields- as I'd rather have these tests cover more realisticallyt how DRF is used in the wild.
There was a problem hiding this comment.
Hi @browniebroke
You're right! The issue was due to modifying protocol dynamically on an existing field, which doesn't work as expected. I've now defined 3 separate models (IPv4Model, IPv6Model, and BothProtocolsModel) and their corresponding serializers. This ensures the correct validation messages are raised for each protocol type. Thanks for catching this!
Summarizing the PR
Bug - When user creates a modelSerializers (for field models.GenericIPAddressField) , serializer.is_valid(raise_exception=True) gives 2 validation error messages for protocol - "IPv4" and "IPv6" (not for "both")
Fix - I tweaked few methods to account for 2 more validators - validate_ipv6_address and validate_ipv4_address along with their protocol.
This will give only one validation error for any protocol avoiding redundant error messages
Now as you pointed out that on your main branch when you tested it for ipv6 protocol, it gave you a generic message - "Enter a valid IPv4 or IPv6 address."
So I dug deeper and found out that when the data contains ":" (colon) in it, and has protocol either "both" or "IPv6" - [This maybe the case you could have encountered]
, the validations resorts to the generic error message only .
PFB the screenshot.
So for this case as well, I added 2 more test cases for it in order to have more branch coverage , I hope thats all right.
Please share your views.
There was a problem hiding this comment.
Hi @browniebroke , please let me know if any further changes are required
|
Naming isn't the issue, please do present however you wish. Just pointing out that the space can at times be overwhelmed with bot accounts and uninvested users demanding attention, and that it's okay for us forcefully weed that out. Let's make this a more inviting space. |
This PR fixes the issue of duplicate error message for GenericIPAddress Field in serializer
Issue - https://github.com/encode/django-rest-framework/issues/9645
Issue
-> While getting serializer field for corresponding model field and its attributes from get_field_kwargs() method,
the IP address field didn't take protocol defined from User , it assumed the default value by default, which was 'both'
-> Also, the validator for ip address are 3 as defined in [ip_address_validator_map] , but while getting corresponding
serializer field (for GenericIPAddress) validate_ipv46_address was only being considered.
Fix
-> Added protocol attribute for consideration and as well as took into account other vaidators as well, so that by default
validate_ipv46_address is not attached to validators.
Also set the default value of protocol to "both" as described in IPAddress Field