Skip to content

[feature] Allow zero subnets in SubnetDivisionRuleFix/allow zero subnets #1287

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.

Sign up for GitHub

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
peterishakvr12 wants to merge 11 commits into
base: master
Choose a base branch
from
Open

[feature] Allow zero subnets in SubnetDivisionRuleFix/allow zero subnets #1287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e27d40b
[fix] Auto-fill organization from master subnet in SubnetDivisionRule
peterishakvr12 Mar 14, 2026
1d81dd8
[fix] Fix indentation and use organization_id
peterishakvr12 Mar 14, 2026
cfffbcb
[fix] Use organization_id consistently to avoid DB queries
peterishakvr12 Mar 14, 2026
ba15b67
[feature] Allow zero subnets in SubnetDivisionRule
peterishakvr12 Mar 16, 2026
34b545a
[fix] Fix unterminated string in number_of_subnets help_text
peterishakvr12 Mar 16, 2026
bf61fef
[fix] Remove duplicate fields and fix help_text
peterishakvr12 Mar 16, 2026
1f309d1
[tests] Update test to allow zero subnets
peterishakvr12 Mar 16, 2026
efb189f
[tests] Update test to allow zero subnets
peterishakvr12 Mar 16, 2026
105a232
Merge branch 'master' into fix/allow-zero-subnets
nemesifier Mar 16, 2026
a3c6540
[tests] Fix indentation and line length
peterishakvr12 Mar 17, 2026
5231d5c
[fix] Fix indentation in test
peterishakvr12 Mar 17, 2026
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
25 changes: 13 additions & 12 deletions openwisp_controller/subnet_division/base/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,6 @@

class AbstractSubnetDivisionRule(TimeStampedEditableModel, OrgMixin):
_subnet_division_rule_update_queue = dict()
# It is used to monitor changes in fields of a SubnetDivisionRule object
# An entry is added to the queue from pre_save signal in the following format
#
# '<rule-uid>: {
# '<field-name>': '<old-value>',
# }
#
# In post_save signal, it is checked whether entry for SubnetDivisionRule object
# exists in this queue. If it exists changes are made to related objects.

type = models.CharField(max_length=200, choices=app_settings.SUBNET_DIVISION_TYPES)
master_subnet = models.ForeignKey(
swapper.get_model_name("openwisp_ipam", "Subnet"), on_delete=models.CASCADE
Expand All @@ -35,8 +25,12 @@ class AbstractSubnetDivisionRule(TimeStampedEditableModel, OrgMixin):
)
number_of_subnets = models.PositiveSmallIntegerField(
verbose_name=_("Number of Subnets"),
help_text=_("Indicates how many subnets will be created"),
validators=[MinValueValidator(1)],
help_text=_(
"Indicates how many subnets will be created. "
"Set to 0 to assign IP addresses directly "
"from the main subnet."
),
validators=[MinValueValidator(0)],
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Allowing 0 here still conflicts with non-zero subnet assumptions in validation.

Line [43] enables number_of_subnets=0, but current validators still enforce child-subnet assumptions (_validate_master_subnet_consistency and _validate_ip_address_consistency). This can reject valid zero-subnet scenarios or validate against the wrong target subnet size.

💡 Suggested alignment for zero-subnet mode 
diff --git a/openwisp_controller/subnet_division/base/models.py b/openwisp_controller/subnet_division/base/models.py
@@
-        available = 2 ** (self.size - master_subnet.prefixlen)
-        # Account for the reserved subnet
-        available -= 1
-        if self.number_of_subnets >= available:
+        available = 2 ** (self.size - master_subnet.prefixlen)
+        # Account for the reserved subnet only when subnets are requested
+        available -= 1
+        if self.number_of_subnets > 0 and self.number_of_subnets >= available:
             raise ValidationError(
@@
     def _validate_ip_address_consistency(self):
         try:
-            next(
-                ip_network(str(self.master_subnet.subnet)).subnets(new_prefix=self.size)
-            )[self.number_of_ips - 1]
+            target_subnet = ip_network(str(self.master_subnet.subnet))
+            if self.number_of_subnets > 0:
+                target_subnet = next(target_subnet.subnets(new_prefix=self.size))
+            target_subnet[self.number_of_ips - 1]
         except IndexError:
             raise ValidationError(
                 {
                     "number_of_ips": _(
                         f"Generated subnets of size /{self.size} cannot accommodate "
                         f"{self.number_of_ips} IP Addresses."
                     )
                 }
             )
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@openwisp_controller/subnet_division/base/models.py` at line 43, The model
currently allows number_of_subnets=0 via validators=[MinValueValidator(0)] but
the validation methods _validate_master_subnet_consistency and
_validate_ip_address_consistency assume at least one subnet and therefore reject
or miscompute zero-subnet cases; fix by either tightening the field validator to
MinValueValidator(1) if zero should be disallowed, or (preferable for
zero-subnet support) keep MinValueValidator(0) and update
_validate_master_subnet_consistency and _validate_ip_address_consistency to
early-return (no-op) when self.number_of_subnets == 0 so they skip child-subnet
checks and avoid referencing a non-existent target subnet size.

)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
size = models.PositiveSmallIntegerField(
verbose_name=_("Size of subnets"),
Expand Down Expand Up @@ -69,6 +63,13 @@ def rule_class(self):
return import_string(self.type)

def clean(self):
# Auto-fill organization from master subnet
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Remove redundant inline comment in clean().

The comment at Line [76] repeats what the code already makes clear and can be dropped for cleaner readability.

As per coding guidelines, "Avoid unnecessary comments or docstrings for code that is already clear."

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@openwisp_controller/subnet_division/base/models.py` at line 76, Remove the
redundant inline comment inside the clean() method (the comment that states
"Auto-fill organization from master subnet") since it simply repeats what the
code does; delete that comment line from the clean() function in models.py and
leave the implementation unchanged to improve readability.

if (
self.master_subnet_id
and self.master_subnet.organization_id is not None
and not self.organization_id
):
self.organization_id = self.master_subnet.organization_id
super().clean()
self._validate_label()
self._validate_master_subnet_validity()
Expand Down
16 changes: 4 additions & 12 deletions openwisp_controller/subnet_division/tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,21 +129,13 @@ def test_field_validations(self):
context_manager.exception.message_dict, expected_message_dict
)

with self.subTest("Test rule does not provision any subnet"):
with self.subTest("Test rule allows zero subnets"):
options = default_options.copy()
options["number_of_subnets"] = 0
rule = SubnetDivisionRule(**options)
with self.assertRaises(ValidationError) as context_manager:
rule.full_clean()
expected_message_dict = {
"number_of_subnets": [
"Ensure this value is greater than or equal to 1."
]
}
self.assertDictEqual(
context_manager.exception.message_dict, expected_message_dict
)

# Should not raise ValidationError
rule.full_clean()

with self.subTest("Test rule does not provision any IP"):
options = default_options.copy()
options["number_of_ips"] = 0
Expand Down
Loading