[fix] Fixed subnet division rule validation on empty subnet #866 #886
Conversation
nemesifier
changed the title
| if not master_subnet: | ||
| raise ValidationError( | ||
| {'master_subnet': _('Master subnet must have a valid subnet.')} | ||
| ) |
There was a problem hiding this comment.
Can this code be triggered at all? Why there's not test for it?
There was a problem hiding this comment.
Yes @nemesifier this will be triggered, I am adding a test for it.
praptisharma28
force-pushed
the
subnetvalidation
branch
from
ddb4adc to
ae8e6a9
Compare
| master_subnet = self.master_subnet.subnet | ||
| if not master_subnet: | ||
| raise ValidationError( | ||
| {'master_subnet': _('Master subnet must have a valid subnet.')} |
There was a problem hiding this comment.
| {'master_subnet': _('Master subnet must have a valid subnet.')} | |
| {'master_subnet': _('Master subnet must be a valid subnet.')} |
| try: | ||
| invalid_subnet = Subnet( | ||
| name='Invalid Subnet' | ||
| ) # Create a subnet without setting its 'subnet' attribute |
There was a problem hiding this comment.
| ) # Create a subnet without setting its 'subnet' attribute | |
| ) # Instantiate a subnet without setting its 'subnet' attribute |
Please move this comment before the instantiation to increase readability.
| ) | ||
| # Ensure default error message is also present | ||
| self.assertIn( | ||
| 'This field cannot be null.', e.message_dict.get('master_subnet', []) |
There was a problem hiding this comment.
Are two validation error raised for this field?
If that's the case, we do not need to add another custom error message, it's redundant.
Let's just ensure the default error provided by the framework is there.
Change the code accordingly, preventing the exception from being raised may be enough (add pass preceded by a comment).
There was a problem hiding this comment.
Yes, two were raised and caused the test to fail hence I had to consider both. Doing as you say.
| ) | ||
| rule.full_clean() | ||
| except ValidationError as e: | ||
| # Ensure custom error message is present |
There was a problem hiding this comment.
you can remove these comments because the test code is pretty self explanatory
| number_of_subnets=2, | ||
| number_of_ips=2, | ||
| organization=self.org, | ||
| # master_subnet is intentionally omitted |
There was a problem hiding this comment.
this is a good comment, leave it here
| number_of_subnets=2, | ||
| number_of_ips=2, | ||
| organization=self.org, | ||
| master_subnet=invalid_subnet, # Set an invalid master_subnet |
There was a problem hiding this comment.
| master_subnet=invalid_subnet, # Set an invalid master_subnet | |
| master_subnet=invalid_subnet, # Invalid master_subnet |
| ) | ||
| rule.full_clean() | ||
| except ValidationError as e: | ||
| # Ensure custom error message is present |
There was a problem hiding this comment.
@praptisharma28 it seems I had forgot to submit my review, sorry for that. Please let me know if the comments above are clear, if not I will give you further explanations via DM.
| @@ -108,7 +108,7 @@ def _validate_master_subnet_consistency(self): | |||
| master_subnet = self.master_subnet.subnet | |||
| if not master_subnet: | |||
| raise ValidationError( | |||
There was a problem hiding this comment.
I didn't explain myself.
If Django is already raising an error if this field is empty, do not raise another error. Just remove this raise and let Django raise its own error.
| e.message_dict.get('master_subnet', []), | ||
| ) | ||
| else: | ||
| self.fail('ValidationError not raised') |
There was a problem hiding this comment.
This test should not be removed.
praptisharma28
force-pushed
the
subnetvalidation
branch
from
45b8a9c to
21ad1b9
Compare
Fixes #866
- the most important check is to verify that the master subnet is not empty, this check is now performed by a dedicated method _validate_master_subnet_validity - removed test_empty_master_subnet, because it's not adding any value since that case is already covered by Django
nemesifier
force-pushed
the
subnetvalidation
branch
from
3dad010 to
2805a74
Compare
|
Cool, got what you did here, thanks @nemesifier ! |
Fixes #866
-The issue occurs because the code assumes that self.master_subnet and self.master_subnet.subnet are always set, but in this case, they can be None. By adding a check at the beginning of the method, we ensure that a proper validation error is raised when the master subnet is empty or not set.
-The validation error will be caught and displayed in the UI instead of causing a 500 internal server error.