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

Change filter value for null choices #2632

Closed
jeremystretch opened this issue Nov 30, 2018 · 5 comments
Closed

Change filter value for null choices #2632

jeremystretch opened this issue Nov 30, 2018 · 5 comments
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application

Comments

@jeremystretch
Copy link
Member

Environment

  • Python version: 3.5.2
  • NetBox version: 2.4.8

Proposed Functionality

Currently, filtering for a null value uses the integer zero. For example, to find all devices not assigned to a rack, you would filter for:

/dcim/devices/?rack_id=0

I am proposing that we change this value to the string null:

/dcim/devices/?rack_id=null

This change involves removing the following two lines in settings.py (null is the default choice value):

FILTERS_NULL_CHOICE_LABEL = 'None'
FILTERS_NULL_CHOICE_VALUE = '0'

While this is a simple change, it is fairly disruptive, particularly to API consumers, and should be well communicated upon implementation.

Use Case

Using 0 to represent a null value works fine for ForeignKey fields, however it causes problems when filtering on BooleanFields, as was raised in #2357. I'm not sure why we went with 0 originally. I want to say that it was because earlier versions of the django-filter library didn't have built-in support for null filtering but I could be wrong.

Database Changes

None

External Dependencies

None

@jeremystretch jeremystretch added the status: under review Further discussion is needed to determine this issue's scope and/or implementation label Nov 30, 2018
@jvanderaa
Copy link
Contributor

This would not be disruptive to anything that I have at this moment. None is a good representation.

@mmahacek
Copy link
Contributor

I was about to ask "Why not both" until I saw the part about boolean fields. I am not using any API calls to filter for null values, so this wouldn't be an issue for me.

@lampwins
Copy link
Contributor

I am in favor of this change. I would also say this is merely a "breaking change" to something that is already broken.

@jeremystretch pertaining to communication, do you think it is too late to squeeze this into v2.5?

@jeremystretch
Copy link
Member Author

Yeah, let's knock it out in v2.5. It's not terribly disruptive, and should be a trivial change for situations where it causes problems. I'd like to take a look at revamping query filters in the near future, and this is a blocker for that work.

@DanSheps
Copy link
Member

I would say what if you deprecated it in 2.5.0 and remove it in 2.5.5 (or whatever arbitrary version) where null filters and 0 filters.

If this is not possible to deprecated (because it requires x amount of new code to check for both) then I would be in favour of just doing it in 2.5.0

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application API change and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Nov 30, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jan 16, 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

No branches or pull requests

5 participants