Skip to content

Conversation

pheus
Copy link
Contributor

@pheus pheus commented Sep 7, 2025

Fixes: #19523

Summary

Add filters to quickly find used/unused types and query by count:

  • has_instances (bool) — UI, REST, GraphQL
  • instance_count (int with =, gt, gte, lt, lte, range) — REST, GraphQL

Implementation

  • DeviceTypeFilterSet & ModuleTypeFilterSet: add has_instances and instance_count using count_related(); a single method handles integer lookups.
  • GraphQL: Strawberry filters via @filter_field; annotate instance_count and delegate lookups with process_filters(prefix="…instance_count__").
  • REST examples:
    • /api/dcim/device-types/?has_instances=true
    • /api/dcim/device-types/?instance_count__gte=10
    • /api/dcim/module-types/?has_instances=false
  • Tests: filterset tests for boolean & numeric cases.

@jnovinger jnovinger requested review from a team and jeremystretch and removed request for a team September 8, 2025 03:40
@pheus pheus force-pushed the 19523-add-instance-filter-for-devicetype-and-moduletype branch 3 times, most recently from 993e985 to c8d4d85 Compare September 10, 2025 09:46
@pheus
Copy link
Contributor Author

pheus commented Sep 10, 2025

Thanks for the thoughtful review and patience. I’ve updated the PR to support querying by instance count. While working on the GraphQL side I used ComparisonFilterLookup so we can support exact/gt/gte/lt/lte/range, which seems like a good fit for counts.

Would it be useful to migrate other numeric counters (e.g., front_port_template_count, interface_template_count, etc.) to ComparisonFilterLookup for consistency? If so, would you prefer a follow‑up issue/PR, or should I include it here?

One question (happy to move to a discussion if you’d prefer): booleans in GraphQL are exposed as FilterLookup[bool] rather than a plain bool, which introduces additional lookups (contains, ends_with, exact, in_list, is_null, …). For has_instances I used BaseFilterLookup[bool] to limit it to exact/in_list/is_null. Is that acceptable, or would you rather I switch to bool or FilterLookup[bool]?

Happy to update the PR either way.

@pheus pheus requested a review from jeremystretch September 10, 2025 10:01
@pheus pheus changed the title Closes #19523: Add instances filter to Module and Device Types Closes #19523: Add instance count filter to Module and Device Types Sep 10, 2025
@pheus pheus force-pushed the 19523-add-instance-filter-for-devicetype-and-moduletype branch from c8d4d85 to c860bff Compare September 10, 2025 11:58
@jeremystretch
Copy link
Member

Thanks @pheus, it's clear you've put a lot of work into this.

One thing to always keep in mind when working on a new feature is to ask "Does this approach scale?" In this case, for instance, what if we had to add the same filters to ten other models? Or 100 other models? That would introduce a ton of boilerplate, which would be impractical to maintain long-term.

There are a few things we can do to simplify the implementation here. First, I want to point out that we typically don't explicitly define filters for individual lookups (gt, lt, etc.) on the FilterSet classes: These are generated automatically by BaseFilterSet to avoid thousands of lines of boilerplate.

Next, if we look at the UI view for the DeviceType list, we can see that instance_count is already annotated on the queryset:

netbox/netbox/dcim/views.py

Lines 1105 to 1108 in 192440a

class DeviceTypeListView(generic.ObjectListView):
queryset = DeviceType.objects.annotate(
instance_count=count_related(Device, 'device_type')
)

Our instance_count filter can reference this value directly; there's no need for a custom method, at least as far as the UI is concerned.

Now DeviceTypeViewSet, which serves the REST API does not explicitly annotate this value, but DeviceTypeSerializer does include a device_count RelatedObjectCountField field which adds the annotation (unless the field has been omitted by request):

device_count = RelatedObjectCountField('instances')

We could convert this to a normal read-only field and move the annotation to the base queryset to ensure the filter always works with the REST API.

I'm not sure offhand what the preferred approach would be for the GraphQL API, but presumably we have a pattern elsewhere for handling this sort of field. (And if not, that in itself might be something worth exploring.)

We should also consider whether there's a need for an explicit boolean filter. In this case, has_instances=True is effectively identical to instance_count__gt=0. The only place where I can see the explicit boolean being helpful is in the UI, where its presence would simplify the inclusion of a form field.

@pheus
Copy link
Contributor Author

pheus commented Sep 15, 2025

First of all, thank you so much for taking the time to review the code and explain all these details to me. This is really helpful. I really appreciate it! 💟

Second, the more I dig into the inner workings of NetBox, the more impressed I am by how much thoughtful helper code lives in the background to make per‑model implementations straightforward.


I completely understand your point about reducing duplication and boilerplate. I had assumed this might be an edge case without a more generic solution.

I’ll take the time to read through everything and work out a cleaner, simplified approach.

I know it takes a lot of time to review code and write such thorough explanations, so thank you again for all of this!

@pheus pheus marked this pull request as draft September 15, 2025 17:05
@pheus pheus force-pushed the 19523-add-instance-filter-for-devicetype-and-moduletype branch from c860bff to b8c0bca Compare September 19, 2025 14:29
Introduces `instance_count` filters to enable queries based on the
existence and count of the associated device or module instances.
Updates forms, filtersets, and GraphQL schema to support these filters.
@pheus pheus force-pushed the 19523-add-instance-filter-for-devicetype-and-moduletype branch from b8c0bca to c66b9f8 Compare September 21, 2025 14:09
@pheus
Copy link
Contributor Author

pheus commented Sep 21, 2025

Thanks for your patience!

I tried to find a common NetBox pattern that would let a FilterSet reuse the view‑level annotation. Since FilterSets operate on model fields or custom methods, I couldn’t find a clean way to say “this field is already annotated.” As a stop‑gap, I added an AnnotatedCountFilter that sets a private _is_annotated flag so BaseFilterSet skips model‑field checks. It works, but I’m not a fan of introducing a special‑case filter for what seems like a niche need.

As an alternative, I explored a CounterCacheField. Because it’s a real model field, all FilterSet lookups just work, and reads should be faster (with a small write hit). I pushed a WIP here:
19523-add-instance-filter-for-devicetype-and-moduletype-countcachefieldhttps://github.com/pheus/netbox/tree/19523-add-instance-filter-for-devicetype-and-moduletype-countcachefield
Open items: add ModuleType, a migration to backfill counts, and fix an issue where creating a Device fires two post_save signals (counter goes +2). I haven’t pinned down the cause yet.

Naming: do you prefer instance_count or device_count? The REST API currently exposes device_count, so switching to instance_count would be breaking. I left a TODO to align in v4.5, but I’m happy to standardize on device_count now and update the PR.

I may be missing an obvious approach here. Any guidance on the preferred direction (reuse annotation vs. small helper vs. counter cache) would be really appreciated. And if this FR is creating more work than it saves, I’m happy to step back.

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.

Device Types - Filter - With Instances (Yes / No)
2 participants