Skip to content

adding warnings for __isnull (#3033)#3174

Open
DeXtAr47-oss wants to merge 8 commits intotypeddjango:masterfrom
DeXtAr47-oss:added-mypy-plugin-for-incorrectly-using-__isnull-filters-in-queryset.py
Open

adding warnings for __isnull (#3033)#3174
DeXtAr47-oss wants to merge 8 commits intotypeddjango:masterfrom
DeXtAr47-oss:added-mypy-plugin-for-incorrectly-using-__isnull-filters-in-queryset.py

Conversation

@DeXtAr47-oss
Copy link
Contributor

This PR adds the feature to generate warnings while incorrectly using __isnull feature,

  • Detect lookups ending with __isnull
  • Check whether the provided value is Literal[True]
  • Remove the __isnull suffix to obtain the field lookup path.
  • Resolve the lookup using django_context.resolve_lookup_into_field.
  • Inspect the resolved Django Field.null attribute.
  • If the field has null=False, emit a mypy error.

Related issues

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Please, add tests

Copy link
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

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

A few quick drive-by comments...


try:
field, _ = django_context.resolve_lookup_into_field(model_cls, lookup)
except (LookupsAreUnsupported, Exception):
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need Exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to skip for invalid fields or unsupported lookup chains instead of raising an error

Copy link
Contributor

Choose a reason for hiding this comment

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

We should use more narrow exception types then, this catches too many things

provided_type = get_proper_type(provided_type)

if lookup_kwarg.endswith("__isnull"):
is_true_literal = isinstance(provided_type, LiteralType)
Copy link
Member

Choose a reason for hiding this comment

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

This checks that it's a literal type, but not that it's True.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for pointing that out I would make the changes by checking the provided_type.value is True

Comment on lines +26 to +42
if lookup_kwarg.endswith("__isnull"):
is_true_literal = isinstance(provided_type, LiteralType)

if is_true_literal:
lookup = lookup_kwarg[:-8]

try:
field, _ = django_context.resolve_lookup_into_field(model_cls, lookup)
except (LookupsAreUnsupported, Exception):
field = None

if field is not None and not getattr(field, "null", False):
ctx.api.fail(
f'Filed "{field.name}" does not allow NULL;'
f'using "__isnull=True" will always return an empty queryset.',
ctx.context,
)
Copy link
Member

@ngnpope ngnpope Mar 10, 2026

Choose a reason for hiding this comment

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

Instead of the fragility of slicing 8 characters from the end of the string, this could be made more legible, e.g.

        lookup_path, sep, lookup_name = lookup_kwarg.rpartition(LOOKUP_SEP)
        is_true_literal = isinstance(provided_type, LiteralType) and provided_type.value is True

        if lookup_name == "isnull" and is_true_literal:
            try:
                field, _ = django_context.resolve_lookup_into_field(model_cls, lookup_path)
            except LookupsAreUnsupported:
                pass
            else:
                if field is not None and field.null is False:
                    ctx.api.fail(
                        f'Field "{field.name}" does not allow NULL;'
                        f'using "__isnull=True" will always return an empty queryset.',
                        ctx.context,
                    )

LOOKUP_SEP can be imported via from django.db.models.constants import LOOKUP_SEP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sure I will do that

@UnknownPlatypus
Copy link
Contributor

@DeXtAr47-oss Thanks for the work on this!
There are a lot of ci failure, could you assess them to see if they are true or false positives ?

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Feature request : warning when incorrectly using __isnull filters

4 participants