Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement standard lookup expressions for all filter fields #4121

Closed
lampwins opened this issue Feb 7, 2020 · 10 comments · Fixed by #4189
Closed

Implement standard lookup expressions for all filter fields #4121

lampwins opened this issue Feb 7, 2020 · 10 comments · Fixed by #4189
Assignees
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application

Comments

@lampwins
Copy link
Contributor

lampwins commented Feb 7, 2020

Environment

  • Python version: 3.7
  • NetBox version: 2.7.4

Proposed Functionality

#3482 proposes implementing negation filters in the API. To realistically implement this, we need to do so in a dynamic way in a base filter class. In this issue, I propose a way to easily do this, but in doing so, to also support a number of other standard lookup expressions, beyond just negation.

I propose the support of these lookup expressions on field types for which is makes logical sense:

  • n - negated exact
  • isw - case insensitive starts with
  • nisw - negated case insensitive starts with
  • iew - case insensitive ends with
  • niew - negated insensitive ends with
  • ic - insensitive contains
  • nic - negated insensitive contains
  • lt - less than
  • nlt - negated less than
  • lte - less than or equal
  • nlte - negated less than or equal
  • gt - greater than
  • ngt - negated greater than
  • gte - greater than or equal
  • ngte - negated greater than or equal

It may seem like a lot, but they are cheap to add and maintain. Only certain fields will support certain lookups. For example, only numeric based fields will get less than/greater than/etc.

These lookup expressions are exposed as <field_name>__<lookup_expr>.

This is implemented by using a custom base filterset class which overrides the get_filters() method to extend fields with more lookup expressions.

Use Case

Today most all filters offer only exact or iexact lookup expressions and only on querset filter() clauses, which also does not allow for negation. Many users often want to lookup based on partial matches of individual fields (instead of the aggregated nature of the q filter). Also pointed out in #3482 is a strong desire to support negation filters (queryset exclude() clauses).

As an example, in networking often a lot of logic is put into a device's hostname. It would be useful to do lookups based on the hostname string starting or ending with a certain string, without having to deal with the potential for unwanted results from the use of the q filter.

Database Changes

None

External Dependencies

This will require the use of a common base filterset class used across the board. This should not present any actual issues, but should still be noted.

@DanSheps
Copy link
Member

DanSheps commented Feb 8, 2020

This most likely would resolve the closed #4092

@lampwins lampwins self-assigned this Feb 9, 2020
@lampwins lampwins added status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application labels Feb 9, 2020
@hSaria
Copy link
Contributor

hSaria commented Feb 9, 2020

Can we use the same names for those already in Django's QuerySet API? We already expose access to querysets to the user, like custom scripts and reports, so people already used to working with those would feel right at home with this change. Similarly, people that get familiar with the lookups via the API will benefit from knowing them when writing a custom script or report.

@spolack
Copy link

spolack commented Feb 10, 2020

Is it possible to test against ´null´ value? At least for exact and negated exact it would be pretty helpful.

@spolack
Copy link

spolack commented Feb 10, 2020

Correct me if i'm wrong, it looks like in your proposed filterset the negated lower/greater then filters are redundant.
gt equals nlte
lt equals ngte
lte equals ngt
gte equals nlt

@lampwins
Copy link
Contributor Author

@hSaria these do map to the underlying django ORM filter expressions by the same names, but my reasoning for doing it this way is two-fold, one it keeps the filter names as short as possible which helps in cutting down on query string length because some clients still impose eventual URL length limits and two, doing it this way means we always use acronyms which instills some consistency because the numeric operators are already implemented as acronyms under the hood.

@lampwins
Copy link
Contributor Author

@spolack I will look into supporting the isnull lookup but I have some concerns about being able to generically support it in all cases.

And yes the redundancy of the negated numeric operators was previously pointed out. It's what I get for hastily listing things by hand :)

@DanSheps
Copy link
Member

DanSheps commented Feb 10, 2020

I am not sure how I feel about not including filters that are negates that have an equal filter. My view that it makes your intentions very clear when using a negate filter, whereas your intention might not be as clear with a different filter.

If these negate ones map to underlying filters in the ORM, I would honestly leave them in-tact.

@DanSheps
Copy link
Member

Another query, do you think we could dump the __in query and replace it with this as well?

@lampwins
Copy link
Contributor Author

@DanSheps the negation of binary numeric comparisons are unnecessary because the inverse operations are already available. For example, the truth table for:

a < b

Is exactly the same as

not (a >= b)

As for the in filter lookup, it should not be needed because we already support the logical OR operation on almost all fields in the API. Supporting the in lookup would require values to be passed as list.

@DanSheps
Copy link
Member

Yeah, I was just thinking, we do have a id__in filter for at least a few fields, this would allow consolidation I think.

jeremystretch added a commit that referenced this issue Mar 4, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants