Skip to content

Conversation

@rinna11
Copy link
Contributor

@rinna11 rinna11 commented Oct 20, 2025

Fixes: #20422

This allows AggregateFilter and PrefixFilter to filter by their family similar with IPAddressFilter.
I've tested the following queries manually:

aggregate_v6: aggregate_list(filters: {family: FAMILY_6}) {
    prefix
}
prefixes_v4: prefix_list(filters: {family: FAMILY_4}) {
    prefix
    role {
      name
    }
    vrf {
      name
    }
    vlan {
      vid
    }
}

@jnovinger jnovinger requested review from a team and jnovinger and removed request for a team October 20, 2025 02:27
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 work, @rinna11. The approach looks good and correctly follows the existing pattern.

Please add tests covering the family filter for both AggregateFilter and PrefixFilter, with both FAMILY_4 and FAMILY_6 values to verify the filter returns the correct address family results.

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

@rinna11
Copy link
Contributor Author

rinna11 commented Oct 23, 2025

Hi @jnovinger , thank you for the suggestion!
I had a quick look at ipam/tests/test_api.py, and it seems there aren’t any existing GraphQL filter tests for AggregateTest and PrefixTest there. I did find relevant filtering tests in ipam/tests/test_filtersets.py, and those already include conditions for filtering by family. These tests are passing both in the current codebase and with this PR.
Also, when the family field was enabled for IPAddressFilter in #19621 , adding specific filter tests wasn’t required at that time either.

Given that, I would like to kindly ask if we could proceed with merging this PR without adding new tests. Please let me know if you're still up for it after all that—happy to adjust!
Thanks again for your time and review.

@jnovinger
Copy link
Member

@rinna11 , yeah, let's accept this without new tests. I have some work in mind to improve GraphQL filter testing that will eventually cover all of this. If you are interested in seeing some GraphQL tests that were just added, check out #20579. Apologies for pointing you in the wrong direction with the earlier comment.

It looks like you do now have conflicts, though. Please address those and I'll be happy to merge. Thanks for hanging with me!

@rinna11 rinna11 force-pushed the 20422-fix-family-filter branch from 6f2c7e5 to 71581e8 Compare October 27, 2025 02:19
@rinna11
Copy link
Contributor Author

rinna11 commented Oct 27, 2025

@jnovinger , I have rebased the main branch and believe the conflicts have been resolved. Thank you for your patience!

q |= Q(**{f"{prefix}prefix__net_contains": query})
return q

def family(
Copy link
Member

Choose a reason for hiding this comment

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

I think you lost the @strawberry_django.filter_field() decorator application during rebase.

@jnovinger jnovinger self-requested a review October 27, 2025 13:38
@jnovinger jnovinger merged commit 9381564 into netbox-community:main Oct 27, 2025
7 checks passed
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.

GraphQL StrFilterLookup filter on prefix field causes unexpected error with IPNetworkField

2 participants