Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#23615 closed Bug (fixed)

Naming a model Check causes Django's check framework to fail with an exception

Reported by: Bram Duvigneau Owned by: Rigel Di Scala
Component: Core (System checks) Version: 1.7
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This causes an SingleRelatedObjectDescriptor is not callable error.

Change History (11)

comment:1 Changed 6 years ago by Collin Anderson

I can't reproduce this. Is the Check model related to another model? Could you post a traceback?

comment:2 Changed 6 years ago by Baptiste Mispelon

Triage Stage: UnreviewedAccepted

I've managed to reproduce with these models:

class Foo(models.Model):
    pass


class Bar(models.Model):
    check = models.ForeignKey('Foo')

It turns out that the name of the model is not the issue but the name of the field is.
Note however that using check = models.IntegerField() doesn't trigger the error.

comment:3 Changed 6 years ago by Rigel Di Scala

Owner: changed from nobody to Rigel Di Scala
Status: newassigned

comment:4 Changed 6 years ago by Rigel Di Scala

I can replicate the issue.

The exception is raised, when running my test using the Django test management command, here: https://github.com/django/django/blob/master/django/core/management/base.py#L418

 if (self.requires_system_checks and
                    not options.get('skip_validation') and  # Remove at the end of deprecation for `skip_validation`.
                    not options.get('skip_checks')):
                self.check()

bmispelon and I found that the problem actually originates here:
https://github.com/django/django/blob/master/django/core/checks/model_checks.py#L14

@register(Tags.models)
def check_all_models(app_configs=None, **kwargs):
    errors = [model.check(**kwargs)
        for model in apps.get_models()
        if app_configs is None or model._meta.app_config in app_configs]
    return list(chain(*errors))

We would therefore need to check (no pun intended) that the check attribute on the model instance does not resolve to a field, before actually calling it as a method, something to the tune of:

if mdl._meta.get_field_by_name("check"):
    pass
Last edited 6 years ago by Rigel Di Scala (previous) (diff)

comment:5 Changed 6 years ago by Rigel Di Scala

Has patch: set
Resolution: fixed
Status: assignedclosed

I have submitted a pull request that amends the behavior of check_all_models(), so that is checks for this problem and returns a meaningful error message.

See: https://github.com/django/django/pull/3362

comment:6 Changed 6 years ago by Loic Bistuer

Resolution: fixed
Status: closednew

@zedr thanks for the patch. In term of procedure though we only mark a ticket as fixed once a patch has been merged. As a matter of fact, tickets close themselves automatically provided the commit messages have the right format.

comment:7 Changed 6 years ago by Rigel Di Scala

@loic I see. Sorry about that.

comment:8 Changed 6 years ago by Collin Anderson

Summary: Naming a model Check causes Django's check framework to fail with an acceptionNaming a model Check causes Django's check framework to fail with an exception

comment:9 Changed 6 years ago by Collin Anderson

Severity: NormalRelease blocker

It seems like we should figure this out before the next release.

Last edited 6 years ago by Collin Anderson (previous) (diff)

comment:10 Changed 6 years ago by Loic Bistuer <loic.bistuer@…>

Resolution: fixed
Status: newclosed

In a5c77417a651c93036cf963e6da518653115be7e:

Fixed #23615 -- Validate that a Model instance's "check" attribute is a method.

The "check" name is a reserved word used by Django's check framework,
and cannot be redefined as something else other than a method, or the check
framework will raise an error.

This change amends the django.core.checks.model_check.check_all_models()
function, so that it verifies that a model instance's attribute "check"
is actually a method. This new check is assigned the id "models.E020".

comment:11 Changed 6 years ago by Loic Bistuer <loic.bistuer@…>

In e8262b59418d68d55a97a25567e4bc4f078ed3cb:

[1.7.x] Fixed #23615 -- Validate that a Model instance's "check" attribute is a method.

The "check" name is a reserved word used by Django's check framework,
and cannot be redefined as something else other than a method, or the check
framework will raise an error.

This change amends the django.core.checks.model_check.check_all_models()
function, so that it verifies that a model instance's attribute "check"
is actually a method. This new check is assigned the id "models.E020".

Conflicts:

docs/ref/checks.txt

Backport of a5c77417a6 from master

Note: See TracTickets for help on using tickets.
Back to Top