Refactor GraphQL filter arguments generation #285
Conversation
|
|
||
| args = {} | ||
| instance = filterset() | ||
|
|
There was a problem hiding this comment.
I'm surprised that we're not calling graphene_django's get_filtering_args_from_filterset somewhere in this function to construct some baseline args - it seems like it's doing a lot with filterset_class.base_filters that we don't have in this function. Just want to make sure we're not losing functionality by rolling our own function here without calling the base implementation.
There was a problem hiding this comment.
As far as I've seen, the output generated by get_filtering_args_from_filterset wasn't that great because we are using a lot of nautobot specific filters, I've opened only one issue but I've found many more related to other field types like region because it's a MPTT model.
Another thing to consider is the output of get_filtering_args_from_filterset is not easy to work with so if we call it first, we'll need to inspect what we are getting back to understand what is good and what needs to be cleaned up.
| @@ -486,36 +487,72 @@ def test_graphql_query_format(self): | |||
| self.assertEqual(site_names, ["Site 1", "Site 2"]) | |||
|
|
|||
|
|
|||
| class GraphQLQuery(APITestCase): | |||
| class GraphQLQuery(TestCase): | |||
There was a problem hiding this comment.
Are we losing test coverage by dropping APITestCase here?
There was a problem hiding this comment.
I don't think so
Actually we are getting more flexibility by not using the API since we have now access to the ExecutionResult object directly and we can access the errors as python Objects
|
I added more unit tests, I think it should be good to go |
Co-authored-by: Jathan McCollum <jathan@gmail.com>
Fixes: #283
This PR replaces the function
get_filtering_args_from_filtersetfromgraphene_django.filter.utilswith a simpler version that supports all nautobot's specific filters.We can't use
get_filtering_args_from_filtersetfromgraphene_django.filter.utilsbecause we are using a lot of nautobot specific filters defined innautobot.utilities.filtersornautobot.extras.filtersAs part of this PR, I'm also planning to add more unit tests for GraphQL, for now I have a good coverage for DeviceType filter arguments and I'm planning to add similar tests for IPAddressType and SiteType filter arguments