Skip to content

Conversation

@bctiemann
Copy link
Contributor

@bctiemann bctiemann commented Nov 19, 2025

Closes: #19338

Breaking change. Adds exact: and in_list: and is_null: semantics to GraphQL queries for id fields and enum field types, which also means that bare values are no longer supported.

This means the following queries are valid:

{
    location_list( filters: {
        status: {exact: STATUS_PLANNED},
        OR: {status: {exact: STATUS_STAGING}}
    }) {
        id site {id}
    }
}
{
    location_list( filters: {
        status: {in_list: [STATUS_PLANNED, STATUS_STAGING]}
    }) {
        id site {id}
    }
}

However the following (which worked previously) is not:

{
    location_list( filters: {
        status: STATUS_PLANNED,
        OR: {status: STATUS_STAGING}
    }) {
        id site {id}
    }
}

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.

Looks good overall, but I found a few that I think are missing:

  • CabledObjectModelFilterMixin.cable_end
  • InterfaceBaseFilterMixin.mode
  • DataSourceFilter.status
  • ObjectChangeFilter.action
  • PowerOutletFilter.status
  • RackReservationFilter.status
  • SiteFilter.time_zone
  • VirtualMachineFilter.start_on_boot (new field)
  • L2VPNFilter.status

class ChangeLogFilterMixin(BaseFilterMixin):
id: ID | None = strawberry.UNSET
id: FilterLookup[ID] | None = strawberry_django.filter_field()
# TODO: "changelog" is not a valid field name; needs to be updated for ObjectChange
Copy link
Contributor Author

@bctiemann bctiemann Nov 21, 2025

Choose a reason for hiding this comment

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

@jeremystretch This filter field (changelog) doesn't work before or after my change here; I think this is enough of an unrelated behavior change that it needs a separate bugfix ticket.

@@ -1,10 +1,9 @@
from typing import Annotated, TYPE_CHECKING

from django.db.models import Q
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for jumping in here, but I think this is worth a quick second look. 🙂

I dug into why some IDEs report Q as unused (PyCharm in my case), and it seems to be because it's being shadowed by the from dcim.constants import * line – constants.py imports Q as well, so the later wildcard import becomes the apparent source of Q and the explicit from django.db.models import Q ends up flagged as unused.

I’d actually prefer to keep the explicit from django.db.models import Q here for the following reason:

Stability: Relying on constants.py to re-export Q feels a bit fragile. If we ever clean up imports in constants.py, this file would break unexpectedly. That effectively turns constants.py into a public API for Django utilities, which doesn’t seem like its intent.

As a compromise, we could either suppress the linter/IDE warning here, or move the explicit from django.db.models import Q import after the wildcard import so that tools clearly associate usages of Q with the explicit import instead of the side effect from constants.py.

Curious what you think, and totally fine if this goes against existing style or conventions! I just wanted to surface the potential fragility before the explicit import gets dropped. Thanks again for working on this!

Copy link
Member

Choose a reason for hiding this comment

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

Agree on this suggestion, particularly in the context of this PR/work. But, perhaps we should have a housekeeping issue to use __all__ in dcim.constants (and probably all constants modules) to properly define what is intended to be exported.

Copy link
Contributor Author

@bctiemann bctiemann Nov 21, 2025

Choose a reason for hiding this comment

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

I do agree with the principle of the suggestion — I hate relying on implicit imports from other files (mostly because it causes so many headaches when writing unit tests and you have to mock them in the original module path rather than wherever they're being imported from; lots of hours spent debugging that kind of thing).

I'm happy to add it back in here regardless of the IDE grumbling about it because especially in this GraphQL stuff, PyCharm doesn't handle a lot of other patterns very well at all, like all those nested BaseFilterLookup[Annotated['FooEnum']] things. It's already going to look pretty messy. But I'm married to either answer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants