Prefixes filter improvements

[tgoff]

Enables all standard fields in the prefixes API within the data. netbox_prefixes datasource. this also includes allowing multiple tags to be specified.

Notably, custom fields are still not supported, which will be more complex to implement.

[fbreckle]

Phew, this sure is some black magic!

Let me think about this a little bit. Usually the code here is very KISS. Explicitly tested, easily extendible. This approach is not. I'm not sure if this kind of logic has a place in a terraform provider. On the other hand, the amount of possible parameters in these APIs is really quite ridiculous and doing it like this spares us lots of work.

Things to consider:

1. How does the user find out the list of possible filters?
   We cannot expect people to read source code to get a list of the supported options.
2. How do we handle testing?
3. Is it easier to go "one step back" and generate/template KISS code instead of using this approach?
4. Can we make a general filtering page covering all pluralized data sources?
   We would have to explain how the filters are built in go-netbox or even netbox itself. There are also the upstream docs on filtering for reference.
5. Can this approach be generalized in a way that allows it to be used for all pluralized data source types?
   I guess it could be, didn't put too much thought in it
6. Is this code readable enough for contributors to understand?
   Since reflect stuff is always somewhat intricate and hard to follow, I think this should have more comments.
7. An old question that actually has a six month old stub page, yay: Do we event want to use the technical filter definitions directly, or do we make user-friendly mappings like vlan_vid_less_than_equalfor VlanVidLte?
   I guess, a simple framework could be built that allows the user to supply vlan_vid_less_than_equal and would then use VlanVidLte under the hood. This would actually allow to use both variants. Hm!

Now, while writing this, I kinda warmed up on the approach. I think it needs some more refining and user experience / documentation is definitely something that has to be considered. I would also be very interested in your thoughts about these questions!

The bad news is that I'm quite busy lately and I really hate Netbox 3.5 not being supported atm (also this, so I wanted to do that next.

Edit: Now I wrote all of this without thanking you for your work and nice MR, so: Thanks for this MR :D

[tgoff]

Thanks for the response. With some distance from this and playing with some other stuff in NetBox, I had the realization that maybe this isnt the right way to handle complex queries. Netbox has a GraphQL API (https://demo.netbox.dev/static/docs/graphql-api/overview/) and there is a GraphQL terraform provider (https://registry.terraform.io/providers/sullivtr/graphql/latest/docs) - which would allow these kind of complex queries with existing providers. The netbox provider could then be used for more simple queries and for actual resource CRUD. I havent had a chance to write up the terraform to confirm this works for my use case, and the GraphQL API doesnt help with Custom Fields (netbox-community/netbox#7598), but that was going to be harder to get working in the netbox provider anyway and can generally be handled via tags instead.

I pretty much agree with all of your points, and I do agree this is less of a priority than supporting the latest Netbox Version (as there is a workaround and its probably not a super common use case)

[fbreckle]

I think that a terraform provider for netbox should actually be able to handle the filtering. As a user, I would absolutely hate having to use two providers to work with netbox.

Another thing I wanted to note: What makes this implementation alluring to me is that it's "just" about filtering in data-sources. In no way would I consider this kind of stuff in a regular resource.

I'll mark this as a draft to revisit it later. I would be open to merge this - since it does not seem to break existing functionality - if the change is something you need urgently.

[zeddD1abl0]

After having written a new Plural Data Source for "netbox_tags", I actually really like this as a standardised way to add filtering. It would allow for you to:

A) Break this out into another utility function that is generic across ALL Plural Data Sources, allowing users to have standard filtering across every Plural Data Source, rather than just "some" filters on "some" Plural Data Sources.

B) Provide support for "all" filters, even if they add new filters in the future. If they decided you should be able to filter IP Addresses by ASN or something, there's no need to do a code change. You simply add a test in for this filter, and away you go.

For documentation purposes, you can point users to the REST API Documentation, so they can test their "Data Source request" before they even use it. Notably, this also fixes a minor gripe of mine that none of the filtering currently in place actually gives a list of the filtering possibilities.

https://registry.terraform.io/providers/e-breuninger/netbox/latest/docs/data-sources/asns tells me nothing about the filters I can use, only that filters have a Name and a Value. That's not useful to the end user, especially if they just want a quick reference. Most of the time, if I want to learn what filtering there is, I do actually go back to the source code so I can see what was implemented in the switch filter.

For your 7 questions:

1. The REST API Documentation available in Netbox is a great start if we go this way. So too would be to just include them under the filter header above ( I did this in Req - Add tags datasource #484 )
2. Testing for this shouldn't be hard to do. Pick a single resource with the most filters (Probably IP Address), and write a huge test that covers every filter. If we then move to using this as a generic filtering handler, that tests just about every set of filtering across all Plural Data Sources, and so we wouldn't have to test a specific Plural Data Source.
3. Maybe. But this level of "complex" comes with future "KISS" of "Here is a single, complex, solution that means we no longer need to do filtering on other Plural Data Sources"
4. I wouldn't want a Generalised Filtering page, specifically because not all filters apply to all Data types, and you may be confusing users by suggesting "All of these are possible filters maybe". On second thought, I would have a Generalised Filtering page, and then add the available filters list to every Plural Data Source page as well, making it easy for the end user to understand.
5. Because it uses Type Reflection, it should work across EVERY Plural Data Source. Which is an AMAZING solution. I love it.
6. It may not be very necessary for the general contributor to understand. If you tell people "Hey, for a Plural Data Source, add this magic sauce for filtering", people will use it without diving into the code too much. And, it's not likely to change much going forward, nor will it need to be adjusted constantly once it's in place.
7. Implementing this is pretty easy. Have a "Switch" statement before the function that says "If filter is vlan_vid_less_than_equal set it to VlanVidLte"

As a matter of fact, I did a simple test. Copied the function from this change, put it in the util file as a new function, set up "VLANs" Data Source to use it with no other changes, and all tests worked as expected.

data_source_netbox_vlans.go

	if filter, ok := d.GetOk("filter"); ok {
		filterParams := filter.(*schema.Set)
		for _, f := range filterParams.List() {
			k := f.(map[string]interface{})["name"]
			v := f.(map[string]interface{})["value"]
			key := k.(string)

			switch key {
			case "Hello":
				key = "vid_lte"
			}
			processDataSourceFilterField(params, key, v.(string))
		}
	}

util.go

func processDataSourceFilterField(params interface{}, k string, vString string) error {
	paramName := strcase.ToCamel(strings.Replace(k, "__n", "n", -1))
	paramName = strings.Replace(paramName, "Id", "ID", -1)

	params_reflect := reflect.ValueOf(params).Elem()
	field := params_reflect.FieldByName(paramName)

	if !(field.IsValid()) {
		return fmt.Errorf("'%s' is not a supported filter parameter.  Netbox go SDK does not have the associated parameter [(%s)]", k, paramName)
	}

	if field.Kind() == reflect.Slice {
		// Param is an array/slice
		field.Set(reflect.Append(field, reflect.ValueOf(vString)))
	} else if (reflect.PtrTo(field.Type().Elem()).Elem().Kind()) == reflect.Float64 {
		// ^ This CANT be the best way to do this, but it works
		vFloat, err := strconv.ParseFloat(vString, 64)
		if field.Set(reflect.ValueOf(&vFloat)); err != nil {
			return fmt.Errorf("failed to set parameter [(%s)] with error [(%s)]", paramName, err)
		}
	} else {
		// Param is a scalar
		field.Set(reflect.ValueOf(&vString))
	}
	return nil
}

The chunk of code in data_source_netbox_vlans.go could actually include basically everything in the IF statement, barring the statement itself. And then, when people want to filter a Plural Data Source, they literally just copy-paste that 3 lines:

	if filter, ok := d.GetOk("filter"); ok {
		processDataSourceFilterField(params, filter)
	}

And the function takes care of the rest of it. NOTE: This breaks the custom filtering labels option.

[fbreckle]

As mentioned earlier, I like the general approach of this as well.

to 6:

And, it's not likely to change much going forward, nor will it need to be adjusted constantly once it's in place.

Actually, I would gladly get rid of my (openapi2-based, manually back-ported) go-netbox fork once a viable openapi3-based community-managed this-terraform-provider-compatible go-netbox arrives.
In that case, that library's design would need to support this reflection pattern (obviously not 1:1, but the general principle).