Skip to content

Duplicate error msg fix for IP address field #9647

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion rest_framework/utils/field_mapping.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,10 @@ def get_field_kwargs(field_name, model_field):
if isinstance(model_field, models.GenericIPAddressField):
validator_kwarg = [
validator for validator in validator_kwarg
if validator is not validators.validate_ipv46_address
if validator not in [validators.validate_ipv46_address, validators.validate_ipv6_address, validators.validate_ipv4_address]
]
kwargs['protocol'] = getattr(model_field, 'protocol', 'both')

# Our decimal validation is handled in the field code, not validator code.
if isinstance(model_field, models.DecimalField):
validator_kwarg = [
Expand Down
80 changes: 78 additions & 2 deletions tests/test_model_serializer.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

from rest_framework import serializers
from rest_framework.compat import postgres_fields
from rest_framework.exceptions import ValidationError

from .models import NestedForeignKeySource

Expand Down Expand Up @@ -409,7 +410,8 @@ class Meta:


class TestGenericIPAddressFieldValidation(TestCase):
def test_ip_address_validation(self):

def setUp(self):
class IPAddressFieldModel(models.Model):
address = models.GenericIPAddressField()

Expand All @@ -418,12 +420,86 @@ class Meta:
model = IPAddressFieldModel
fields = '__all__'

s = TestSerializer(data={'address': 'not an ip address'})
self.serializer_class = TestSerializer
self.model = IPAddressFieldModel

def test_ip_address_validation(self):
s = self.serializer_class(data={'address': 'not an ip address'})
self.assertFalse(s.is_valid())
self.assertEqual(1, len(s.errors['address']),
'Unexpected number of validation errors: '
'{}'.format(s.errors))

def test_invalid_ipv4_for_ipv4_field(self):
"""Test that an invalid IPv4 raises only an IPv4-related error."""
self.model._meta.get_field("address").protocol = "IPv4" # Set field to IPv4 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 IPv4 address."
)

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."
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Daksh2000 Daksh2000 Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Screenshot 2025-02-14 at 11 26 35 PM

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @browniebroke , please let me know if any further changes are required


def test_invalid_both_protocol(self):
"""Test that an invalid IP raises a combined error message when protocol is both."""
self.model._meta.get_field("address").protocol = "both" # Allow both IPv4 & IPv6
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 IPv4 or IPv6 address."
)

def test_valid_ipv4(self):
"""Test that a valid IPv4 passes validation."""
self.model._meta.get_field("address").protocol = "IPv4"
valid_data = {"address": "192.168.1.1"}
serializer = self.serializer_class(data=valid_data)
self.assertTrue(serializer.is_valid())

def test_valid_ipv6(self):
"""Test that a valid IPv6 passes validation."""
self.model._meta.get_field("address").protocol = "IPv6"
valid_data = {"address": "2001:db8::ff00:42:8329"}
serializer = self.serializer_class(data=valid_data)
self.assertTrue(serializer.is_valid())

def test_valid_ipv4_for_both_protocol(self):
"""Test that a valid IPv4 is accepted when protocol is 'both'."""
self.model._meta.get_field("address").protocol = "both"
valid_data = {"address": "192.168.1.1"}
serializer = self.serializer_class(data=valid_data)
self.assertTrue(serializer.is_valid())

def test_valid_ipv6_for_both_protocol(self):
"""Test that a valid IPv6 is accepted when protocol is 'both'."""
self.model._meta.get_field("address").protocol = "both"
valid_data = {"address": "2001:db8::ff00:42:8329"}
serializer = self.serializer_class(data=valid_data)
self.assertTrue(serializer.is_valid())


@pytest.mark.skipif('not postgres_fields')
class TestPosgresFieldsMapping(TestCase):
Expand Down
Loading