Skip to content

Conversation

pheus
Copy link
Contributor

@pheus pheus commented Oct 15, 2025

Fixes: #20541

Refactors filter methods to enable dynamic query prefixing for greater flexibility in nested queries. Improves error handling for invalid network inputs and ensures consistency across IPAM filter implementations.


Context

This follows the discussion in Bug #20466 and builds on PR #20538 (which addressed IPAddressFilter.assigned). The goal here is to complete the prefixing/validation sweep within ipam.graphql.filters only and ensure nested filters consistently target the correct related model.

Summary of changes

  • Prefix-aware Q objects (nested filtering):
    Standardize construction of lookups as Q(**{f"{prefix}…": …}) in custom filter methods so nested queries are resolved against the intended model path.
  • Input safeguards:
    contains/parent-style helpers now guard network parsing with try/except, skipping invalid entries rather than failing the entire filter.
  • Targeted refactors for clarity/consistency:
    Minor internal tidy-ups (e.g., shared helper for device-based lookups) while preserving existing behavior.

🚨 Breaking change (GraphQL): AggregateFilter

Aggregate.prefix is a concrete field, not a relation. To avoid double-qualifying lookups in nested contexts, this PR proposes:

  • Replace the nested filter with a field lookup:
    prefix: FilterLookup[str] | None
  • Add a field-level contains(value: [CIDR]) that applies prefix__net_contains to the Aggregate field itself.

Client impact (input shape changed for Aggregates only):

  • Before
    aggregate_list(filters: { prefix: { contains: "10.0.0.0/8" } }) { prefix }
  • After
    • containment:
      aggregate_list(filters: { contains: ["10.0.0.0/8"] }) { prefix }
    • equality:
      aggregate_list(filters: { prefix: { exact: "10.0.0.0/8" } }) { prefix }

No other GraphQL inputs are changed.

Files/areas touched (IPAM only)

Upgrade notes

  • Breaking (Aggregates only): Update GraphQL clients to use:
    • aggregate_list(filters: { contains: ["<cidr>"] }) for containment, and
    • aggregate_list(filters: { prefix: { exact: "<cidr>" } }) for equality.
  • No changes required for other IPAM GraphQL filters.

Related

Refactors filter methods to enable dynamic query prefixing for greater
flexibility in nested queries. Improves error handling for invalid
network inputs and ensures consistency across IPAM filter
implementations.

Fixes netbox-community#20541
@jnovinger jnovinger requested review from a team and jnovinger and removed request for a team October 15, 2025 17:53
Copy link
Member

@jnovinger jnovinger left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough audit, @pheus. The fixes look good and I agree the breaking change for AggregateFilter is the correct choice.

Please add tests covering the nested query/filter scenarios that these fixes enable as well as the fixes from #20538 (which I merged and you probably want to rebase on). Please also test the error handling improvement where invalid networks are skipped rather than breaking the entire filter.

You can add custom test methods to the appropriate model test classes in netbox/ipam/tests/test_api.py (e.g., IPAddressTest, IPRangeTest, PrefixTest, AggregateTest). These inherit from APIViewTestCase which provides self.client, self.user, and other helpers. Please mark these tests with @tag('regression'), which you see elsewhere in the same file. For examples, see the templated-tests that are defined in netbox/netbox/tests/test_graphql.py.

@pheus
Copy link
Contributor Author

pheus commented Oct 19, 2025

Thanks for the thoughtful review — the guidance is super helpful!

I’m working on this next, but it’ll take me a bit to put together all the fixtures and assertions. I’ll push an update as soon as it’s ready. Thanks again for the review!

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.

Ensure all custom GraphQL filter methods apply prefix in Q(...) for nested filters

2 participants