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

Closed
peterisaac-00 wants to merge 11 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 8 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
peterisaac-00 Mar 14, 2026
1d81dd8
[fix] Fix indentation and use organization_id
peterisaac-00 Mar 14, 2026
cfffbcb
[fix] Use organization_id consistently to avoid DB queries
peterisaac-00 Mar 14, 2026
ba15b67
[feature] Allow zero subnets in SubnetDivisionRule
peterisaac-00 Mar 16, 2026
34b545a
[fix] Fix unterminated string in number_of_subnets help_text
peterisaac-00 Mar 16, 2026
bf61fef
[fix] Remove duplicate fields and fix help_text
peterisaac-00 Mar 16, 2026
1f309d1
[tests] Update test to allow zero subnets
peterisaac-00 Mar 16, 2026
efb189f
[tests] Update test to allow zero subnets
peterisaac-00 Mar 16, 2026
105a232
Merge branch 'master' into fix/allow-zero-subnets
nemesifier Mar 16, 2026
a3c6540
[tests] Fix indentation and line length
peterisaac-00 Mar 17, 2026
5231d5c
[fix] Fix indentation in test
peterisaac-00 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
15 changes: 13 additions & 2 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 @@ -35,8 +35,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)],

@coderabbitai coderabbitai Bot Mar 16, 2026

Copy link
Copy Markdown
Contributor

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 +73,13 @@ def rule_class(self):
return import_string(self.type)

def clean(self):
# Auto-fill organization from master subnet

@coderabbitai coderabbitai Bot Mar 16, 2026

Copy link
Copy Markdown
Contributor

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
14 changes: 3 additions & 11 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,20 +129,12 @@ 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 for number_of_subnets
rule.full_clean()

@coderabbitai coderabbitai Bot Mar 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify if create_subnets handles number_of_subnets=0 specially
rg -n -A 20 'def create_subnets' openwisp_controller/subnet_division/rule_types/base.py

# Check if there's any special handling for zero subnets in provisioning
rg -n 'number_of_subnets.*==.*0|number_of_subnets.*0' openwisp_controller/subnet_division/

Repository: openwisp/openwisp-controller

Length of output: 2452

🏁 Script executed:

# Check the create_ips method to see if it handles empty subnets list
rg -n -B 5 -A 25 'def create_ips' openwisp_controller/subnet_division/rule_types/base.py

# Look for any special handling or comments about number_of_subnets=0
rg -n -B 3 -A 3 'number_of_subnets.*0' openwisp_controller/subnet_division/rule_types/

# Check the SubnetDivisionRule model for constraints or validation
rg -n -A 30 'class SubnetDivisionRule' openwisp_controller/subnet_division/models.py

Repository: openwisp/openwisp-controller

Length of output: 2118

🏁 Script executed:

# Find the AbstractSubnetDivisionRule model definition
fd -t f 'models.py' openwisp_controller/subnet_division/ | head -5

# Read the abstract model to understand field validation
rg -n -B 5 -A 50 'class AbstractSubnetDivisionRule' openwisp_controller/subnet_division/models.py

# Look for any logic or comments about assigning IPs directly from master subnet
rg -n 'master_subnet|direct.*IP|assign.*direct' openwisp_controller/subnet_division/

Repository: openwisp/openwisp-controller

Length of output: 14305

🏁 Script executed:

# Get more context on the number_of_subnets field definition
rg -n -B 5 -A 15 'number_of_subnets.*=' openwisp_controller/subnet_division/base/models.py | head -60

# Check if there's any TODO or FIXME comments related to number_of_subnets=0
rg -n 'TODO|FIXME|XXX' openwisp_controller/subnet_division/rule_types/base.py

# Look at the full create_ips method for any number_of_subnets=0 handling
rg -n -A 40 'def create_ips' openwisp_controller/subnet_division/rule_types/base.py | head -70

# Check if there are any other tests about number_of_subnets=0 provisioning
rg -n -B 5 -A 10 'number_of_subnets.*0' openwisp_controller/subnet_division/tests/test_models.py

Repository: openwisp/openwisp-controller

Length of output: 7150

Add integration test to verify IP provisioning behavior when number_of_subnets=0.

The current test validates that number_of_subnets=0 passes model validation, but the documented feature—"assign IP addresses directly from the main subnet"—is not implemented. When number_of_subnets=0, the create_subnets method produces an empty list, causing create_ips to generate zero IP addresses.

Add an integration test that:

  1. Creates a rule with number_of_subnets=0
  2. Triggers provisioning (e.g., adding the template to a config)
  3. Asserts that IP addresses are assigned directly from the master subnet
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@openwisp_controller/subnet_division/tests/test_models.py` around lines 132 -
137, The integration test should ensure that when a SubnetDivisionRule is
created with number_of_subnets=0 the provisioning assigns IPs from the master
subnet; update behavior in SubnetDivisionRule by changing create_subnets and/or
create_ips so that if number_of_subnets == 0 create_subnets returns a single
entry representing the master subnet (or make create_ips detect zero and iterate
over the master subnet) and then generate IP addresses from that master subnet;
add a new integration test that creates a rule with number_of_subnets=0,
triggers provisioning (e.g., add the template/config that calls
SubnetDivisionRule.provision or equivalent code path), and assert IP objects
were created with addresses within the master subnet.

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

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