Skip to content

Add network_cidr field to community serializer - #635

Open
pederhan wants to merge 5 commits into
unioslo:masterfrom
pederhan:serialize-community-network-address
Open

pederhan wants to merge 5 commits into
unioslo:masterfrom
pederhan:serialize-community-network-address

Conversation

@pederhan

@pederhan pederhan commented Jul 2, 2026

Copy link
Copy Markdown
Member

This PR adds a new network_cidr field to serialized communities, which is the network CIDR of the network it is bound to. Improves API ergonomics.

Rationale

In order to contact a community's endpoint, we are reliant on the network CIDR the community is bound to, i.e.

/networks/10.0.0.0/24/communities/1
          ^^^^^^^^^^^

But what we receive from the API for a Community object currently is something like this:

{
  "name": "mycommunity",
  "id": 1,
  "network": 123,
  // ...
}

Thus, consumers cannot construct the URL for the community based on just that object.

New field: network_cidr

By adding the network CIDR to the serialized community, we can now construct the URL based exclusively on the community data without having to perform extra lookups or stitching together data from multiple objects:

{
  "name": "mycommunity",
  "id": 1,
  "network": 123,
  "network_cidr": "10.0.0.1/24",
  // ...
}

Improves mreg-api ergonomics

Interacting with communities is awkward in mreg-api currently, because we need to manually fetch the network first, and pass that to every method that interacts with communities:

# Fetch network, use network to fetch community
net = client.network.get("10.0.0.0/24")
com = client.network.community.get("mycom", net) 

client.network.community.update(com, net, description="My new description")
client.network.community.add_host(com, net, some_host_obj)
client.network.community.delete(com, net)

With this PR, the network CIDR is bound to the community object, and we can thus fetch the network as part of the community fetch operation (and , and we don't need pass around the network object:

# No need to pass around network, can be fetched implicitly and discarded after fetching community
com = client.network.community.get("mycom", "10.0.0.0/24") 

client.network.community.update(com, description="My new description")
client.network.community.add_host(com, some_host_obj)
client.network.community.delete(com)

@pederhan
pederhan requested a review from terjekv July 2, 2026 09:41
@coveralls

coveralls commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Coverage Status

coverage: 97.769% (+0.002%) from 97.767% — pederhan:serialize-community-network-address into unioslo:master

@pederhan

pederhan commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Need to change terminology to "CIDR", even though it's not used a lot throughout the codebase. We usually use the field name network for this kind of value, but that's already taken by the network ID in this case.

@pederhan pederhan changed the title Include network address in community serializer Add network_cidr field to community serializer Jul 2, 2026

@terjekv terjekv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For lenient clients this should be a fairly safe and sound extension. One thing is that get_network_cidr() always dereferences obj.network so unless we use select_related("network"), list/nested responses will probably add one network lookup per serialized community? How often do we serlalize out the community?

@terjekv
terjekv force-pushed the serialize-community-network-address branch from 6c0a7b4 to f350f69 Compare July 4, 2026 14:16
@pederhan

pederhan commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

How often do we serlalize out the community?

Only when fetching hosts, right? So this does affect endpoints that list hosts, because each host deserializes its community:

mreg/mreg/api/v1/views.py

Lines 328 to 335 in bbe8fa7

def _host_prefetcher(qs):
return qs.prefetch_related(
"bacnetid", "cnames", "hinfo", "loc", "mxs", "ptr_overrides", "txts",
"srvs", "naptrs", "sshfp_set", "hostgroups", "hostpolicyroles", "contacts",
).prefetch_related(
Prefetch("ipaddresses", queryset=Ipaddress.objects.order_by("ipaddress")),
Prefetch("hostcommunitymapping_set", queryset=HostCommunityMapping.objects.select_related("community")),
)

Potential regression for /hosts endpoint? Particularly when listing all hosts. Some regression tests would be really nice.

I'll look at this in late July/early August.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants