-
-
Notifications
You must be signed in to change notification settings - Fork 507
Validation of the fields
argument to bulk_update
#2808
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,6 +1,6 @@ | ||||||
from collections.abc import Sequence | ||||||
|
||||||
from django.core.exceptions import FieldError | ||||||
from django.core.exceptions import FieldDoesNotExist, FieldError | ||||||
from django.db.models.base import Model | ||||||
from django.db.models.fields.related import RelatedField | ||||||
from django.db.models.fields.related_descriptors import ( | ||||||
|
@@ -12,7 +12,7 @@ | |||||
from django.db.models.fields.reverse_related import ForeignObjectRel | ||||||
from mypy.checker import TypeChecker | ||||||
from mypy.errorcodes import NO_REDEF | ||||||
from mypy.nodes import ARG_NAMED, ARG_NAMED_OPT, ARG_STAR, CallExpr, Expression | ||||||
from mypy.nodes import ARG_NAMED, ARG_NAMED_OPT, ARG_STAR, CallExpr, Expression, ListExpr | ||||||
from mypy.plugin import FunctionContext, MethodContext | ||||||
from mypy.types import AnyType, Instance, LiteralType, ProperType, TupleType, TypedDictType, TypeOfAny, get_proper_type | ||||||
from mypy.types import Type as MypyType | ||||||
|
@@ -695,3 +695,58 @@ def validate_select_related(ctx: MethodContext, django_context: DjangoContext) - | |||||
_validate_select_related_lookup(ctx, django_context, django_model.cls, lookup_value) | ||||||
|
||||||
return ctx.default_return_type | ||||||
|
||||||
|
||||||
def _validate_bulk_update_field( | ||||||
ctx: MethodContext, | ||||||
model_cls: type[Model], | ||||||
field_name: str, | ||||||
) -> bool: | ||||||
opts = model_cls._meta | ||||||
try: | ||||||
field = opts.get_field(field_name) | ||||||
except FieldDoesNotExist as e: | ||||||
ctx.api.fail(str(e), ctx.context) | ||||||
return False | ||||||
|
||||||
if not field.concrete or field.many_to_many: | ||||||
ctx.api.fail(f'bulk_update() can only be used with concrete fields. Got "{field_name}"', ctx.context) | ||||||
return False | ||||||
|
||||||
all_pk_fields = set(opts.pk_fields) | ||||||
for parent in opts.all_parents: | ||||||
all_pk_fields.update(parent._meta.pk_fields) | ||||||
|
||||||
if field in all_pk_fields: | ||||||
ctx.api.fail(f'bulk_update() cannot be used with primary key fields. Got "{field_name}"', ctx.context) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return False | ||||||
|
||||||
return True | ||||||
|
||||||
|
||||||
def validate_bulk_update(ctx: MethodContext, django_context: DjangoContext) -> MypyType: | ||||||
""" | ||||||
Type check the `fields` argument passed to `QuerySet.bulk_update(...)`. | ||||||
|
||||||
Extracted and adapted from `django.db.models.query.QuerySet.bulk_update` | ||||||
Mirrors tests from `django/tests/queries/test_bulk_update.py` | ||||||
""" | ||||||
if not ( | ||||||
isinstance(ctx.type, Instance) | ||||||
and (django_model := helpers.get_model_info_from_qs_ctx(ctx, django_context)) is not None | ||||||
and len(ctx.args) >= 2 | ||||||
and ctx.args[1] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, don't forget to support |
||||||
and isinstance((fields_args := ctx.args[1][0]), ListExpr) | ||||||
): | ||||||
return ctx.default_return_type | ||||||
|
||||||
if len(fields_args.items) == 0: | ||||||
ctx.api.fail("Field names must be given to bulk_update()", ctx.context) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return ctx.default_return_type | ||||||
|
||||||
for field_arg in fields_args.items: | ||||||
field_name = helpers.resolve_string_attribute_value(field_arg, django_context) | ||||||
if field_name is not None: | ||||||
_validate_bulk_update_field(ctx, django_model.cls, field_name) | ||||||
|
||||||
return ctx.default_return_type |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,146 @@ | ||||||
- case: bulk_update_valid_fields | ||||||
installed_apps: | ||||||
- myapp | ||||||
main: | | ||||||
from myapp.models import Article, Author, Category | ||||||
# Valid single field updates | ||||||
articles = Article.objects.all() | ||||||
Article.objects.bulk_update(articles, ["title"]) | ||||||
Article.objects.bulk_update(articles, ["content"]) | ||||||
Article.objects.bulk_update(articles, ["published"]) | ||||||
# Valid multiple field updates | ||||||
Article.objects.bulk_update(articles, ["title", "content"]) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
just for the variaty. |
||||||
Article.objects.bulk_update(articles, ["title", "content", "published"]) | ||||||
# Valid foreign key field updates (by field name and attname) | ||||||
Article.objects.bulk_update(articles, ["author"]) | ||||||
Article.objects.bulk_update(articles, ["author_id"]) | ||||||
Article.objects.bulk_update(articles, ["category"]) | ||||||
Article.objects.bulk_update(articles, ["category_id"]) | ||||||
# Valid updates on different models | ||||||
authors = Author.objects.all() | ||||||
Author.objects.bulk_update(authors, ["name"]) | ||||||
Author.objects.bulk_update(authors, ["email"]) | ||||||
categories = Category.objects.all() | ||||||
Category.objects.bulk_update(categories, ["name"]) | ||||||
Category.objects.bulk_update(categories, ["parent"]) | ||||||
Category.objects.bulk_update(categories, ["parent_id"]) | ||||||
# Variables containing field names | ||||||
field_name = "title" | ||||||
Article.objects.bulk_update(articles, [field_name]) | ||||||
# Dynamic field lists | ||||||
def get_fields() -> list[str]: | ||||||
return ["title"] | ||||||
Article.objects.bulk_update(articles, get_fields()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, test at least one error with |
||||||
files: | ||||||
- path: myapp/__init__.py | ||||||
- path: myapp/models.py | ||||||
content: | | ||||||
from django.db import models | ||||||
class Category(models.Model): | ||||||
name = models.CharField(max_length=100) | ||||||
parent = models.ForeignKey('self', on_delete=models.CASCADE, null=True, blank=True) | ||||||
class Author(models.Model): | ||||||
name = models.CharField(max_length=100) | ||||||
email = models.EmailField() | ||||||
class Tag(models.Model): | ||||||
name = models.CharField(max_length=50) | ||||||
class Article(models.Model): | ||||||
title = models.CharField(max_length=200) | ||||||
content = models.TextField() | ||||||
published = models.BooleanField(default=False) | ||||||
author = models.ForeignKey(Author, on_delete=models.CASCADE) | ||||||
category = models.ForeignKey(Category, on_delete=models.CASCADE) | ||||||
tags = models.ManyToManyField(Tag) | ||||||
- case: bulk_update_invalid_fields | ||||||
installed_apps: | ||||||
- myapp | ||||||
main: | | ||||||
from myapp.models import Article, Author, Category | ||||||
from typing import Literal | ||||||
articles = Article.objects.all() | ||||||
# Empty fields list | ||||||
Article.objects.bulk_update() # E: Missing positional arguments "objs", "fields" in call to "bulk_update" of "Manager" [call-arg] | ||||||
Article.objects.bulk_update(articles, []) # E: Field names must be given to bulk_update() [misc] | ||||||
# Invalid field names (Django's FieldError) | ||||||
Article.objects.bulk_update(articles, ["nonexistent"]) # E: Article has no field named 'nonexistent' [misc] | ||||||
Article.objects.bulk_update(articles, ["invalid_field"]) # E: Article has no field named 'invalid_field' [misc] | ||||||
# Cannot update primary key fields | ||||||
Article.objects.bulk_update(articles, ["id"]) # E: bulk_update() cannot be used with primary key fields. Got "id" [misc] | ||||||
# Mixed valid and invalid fields | ||||||
Article.objects.bulk_update(articles, ["title", "nonexistent"]) # E: Article has no field named 'nonexistent' [misc] | ||||||
Article.objects.bulk_update(articles, ["id", "title"]) # E: bulk_update() cannot be used with primary key fields. Got "id" [misc] | ||||||
# Whitespace-only field names | ||||||
Article.objects.bulk_update(articles, [""]) # E: Article has no field named '' [misc] | ||||||
Article.objects.bulk_update(articles, [" "]) # E: Article has no field named ' ' [misc] | ||||||
# ManyToMany is not a concrete updatable field | ||||||
Article.objects.bulk_update(articles, ["tags"]) # E: bulk_update() can only be used with concrete fields. Got "tags" [misc] | ||||||
# Multiple invalid fields | ||||||
Article.objects.bulk_update(articles, ["nonexistent1", "nonexistent2"]) # E: Article has no field named 'nonexistent1' [misc] # E: Article has no field named 'nonexistent2' [misc] | ||||||
# Primary key with valid fields | ||||||
Article.objects.bulk_update(articles, ["title", "id", "content"]) # E: bulk_update() cannot be used with primary key fields. Got "id" [misc] | ||||||
# Literal type variables are validated | ||||||
invalid_field: Literal["nonexistent"] = "nonexistent" | ||||||
Article.objects.bulk_update(articles, [invalid_field]) # E: Article has no field named 'nonexistent' [misc] | ||||||
pk_field: Literal["id"] = "id" | ||||||
Article.objects.bulk_update(articles, [pk_field]) # E: bulk_update() cannot be used with primary key fields. Got "id" [misc] | ||||||
# Test with different models | ||||||
authors = Author.objects.all() | ||||||
Author.objects.bulk_update(authors, ["id"]) # E: bulk_update() cannot be used with primary key fields. Got "id" [misc] | ||||||
Author.objects.bulk_update(authors, ["invalid"]) # E: Author has no field named 'invalid' [misc] | ||||||
categories = Category.objects.all() | ||||||
Category.objects.bulk_update(categories, ["id"]) # E: bulk_update() cannot be used with primary key fields. Got "id" [misc] | ||||||
Category.objects.bulk_update(categories, ["invalid"]) # E: Category has no field named 'invalid' [misc] | ||||||
files: | ||||||
- path: myapp/__init__.py | ||||||
- path: myapp/models.py | ||||||
content: | | ||||||
from django.db import models | ||||||
class Category(models.Model): | ||||||
name = models.CharField(max_length=100) | ||||||
parent = models.ForeignKey('self', on_delete=models.CASCADE, null=True, blank=True) | ||||||
class Author(models.Model): | ||||||
name = models.CharField(max_length=100) | ||||||
email = models.EmailField() | ||||||
class Article(models.Model): | ||||||
title = models.CharField(max_length=200) | ||||||
content = models.TextField() | ||||||
published = models.BooleanField(default=False) | ||||||
author = models.ForeignKey(Author, on_delete=models.CASCADE) | ||||||
category = models.ForeignKey(Category, on_delete=models.CASCADE) | ||||||
tags = models.ManyToManyField('myapp.Tag') | ||||||
class Tag(models.Model): | ||||||
name = models.CharField(max_length=50) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.