Skip to content

Conversation

@kkthxbye-code
Copy link
Contributor

Fixes: #11685

  • Changes the CachedValue types of IPAddressField and IPNetworkField to inet and cidr.
  • Creates a child class of TextField called CachedValueField (not sure about the name). Reasoning as discussed on slack is to prevent adding special lookup types to the global TextField field.
  • A NetContainsOrEquals lookup has been added to CachedValueField which casts the string value to inet and does a >>= lookup on it.
  • Changing the type create a new migration, however as far as I can tell, it's pretty much a NOP migration, there should be no change to the database schema. Not sure if this is preventable.
  • If the search query in partial mode parses as a IP address, all CachedValues of type cidr are queried to check if they contain the IP being searched for.

There's some screenshots here: #11665 (comment)

There's still time to evaluate if we want to do something special about searching for inet type fields (IPAddresses), which we can handle in this PR or later.

@kkthxbye-code
Copy link
Contributor Author

Forgot to mention, for this to function for existing objects the user needs to run the reindex mangement command.

@jeremystretch
Copy link
Member

Great work @kkthxbye-code, thank you!

Changing the type create a new migration, however as far as I can tell, it's pretty much a NOP migration, there should be no change to the database schema. Not sure if this is preventable.

In such cases where a generated migration doesn't actually effect a material schema change, I like to just alter the field's definition in the most recent relevant migration. (Happy to handle this myself.)

the user needs to run the reindex mangement command

Any thoughts on how to best handle this? Should we add reindex to the upgrade script? (That would help fix #11658 too.)

Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

Awesome work!

@jeremystretch
Copy link
Member

jeremystretch commented Feb 17, 2023

We might also want to consider reassigning field weights. Currently, containing prefixes/aggregates have the same precedence (100) as an exact IP address match (see screenshot). We could tweak the weights a bit across the three models so that IPs < prefixes < aggregates (e.g. 100, 110, and 120).

Screenshot 2023-02-17 at 16-10-14 Search NetBox

@kkthxbye-code
Copy link
Contributor Author

kkthxbye-code commented Feb 18, 2023

the user needs to run the reindex mangement command

Any thoughts on how to best handle this? Should we add reindex to the upgrade script? (That would help fix #11658 too.)

My only worry about doing it in the upgrade script, is that the reindexing can take a while. Maybe we can conditionally run it if there has been applied any migration earlier in the script?

Edit: Thinking about it for 2 more seconds, this PR technically generates no migrations (if we remove it like you suggested), so maybe that's not a good way to handle it.

Thanks for the rest of the feedback, I'll address them shortly!

@kkthxbye-code
Copy link
Contributor Author

I addressed the feedback @jeremystretch. Only outstanding issue is how to make the user reindex. I see the following options:

  • reindex on all runs of ./upgrade.sh
  • reindex when running ./upgrade.sh, but only if migrations have been applied in the same run. This would require that we leave the "empty" migration in.
  • Just note it in the release notes that reindex should be executed. We might also add a notice to the end of the ./upgrade.sh output telling the user to run reindex if the release notes say so. The search still works without reindexing, just not with the new feature.

@jeremystretch
Copy link
Member

What if we delete the cache in a new migration, and update upgrade.sh to run migrations if none already exist. That way we're only ever rebuilding the whole thing if there's a need to do so. This would solve for #11658 as well.

@kkthxbye-code
Copy link
Contributor Author

kkthxbye-code commented Feb 19, 2023

What if we delete the cache in a new migration, and update upgrade.sh to run migrations if none already exist. That way we're only ever rebuilding the whole thing if there's a need to do so. This would solve for #11658 as well.

That's a great idea, do you want to implement it or should I add it to the PR?

We might need to check up on how netbox docker is doing things, I think it might need to be updated as well to support reindexing when no cached values exist.

@jeremystretch
Copy link
Member

That's a great idea, do you want to implement it or should I add it to the PR?

I'll handle it under #11787; let's leave this as-is.

@jeremystretch jeremystretch merged commit 25278be into netbox-community:develop Feb 19, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show containing Prefixes and Aggregates when doing a site-wide partial search

2 participants