Code

Opened 3 years ago

Last modified 4 weeks ago

#15279 assigned Bug

Inheritance of fields from a single abstract base class through multiple abstract classes causes errors.

Reported by: melinath Owned by: melinath
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: abstract inheritance
Cc: stephen.r.burrows@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Assume the following models in app:

from django.db import models


class Orange(models.Model):
    pass


class BaseClass(models.Model):
    field = models.ForeignKey(Orange)
    class Meta:
        abstract = True


class Parent1(BaseClass):
    class Meta:
        abstract=True


class Parent2(BaseClass):
    class Meta:
        abstract=True


class Child(Parent1, Parent2):
    pass

Currently, this definition will raise the following errors during model validation:

Unhandled exception in thread started by <bound method Command.inner_run of <django.core.management.commands.runserver.Command object at 0x101470490>>
Traceback (most recent call last):
  File "..../django/core/management/commands/runserver.py", line 88, in inner_run
    self.validate(display_num_errors=True)
  File "..../django/core/management/base.py", line 253, in validate
    raise CommandError("One or more models did not validate:\n%s" % error_text)
django.core.management.base.CommandError: One or more models did not validate:
app.child: Accessor for field 'field' clashes with related field 'Orange.child_set'. Add a related_name argument to the definition for 'field'.
app.child: Accessor for field 'field' clashes with related field 'Orange.child_set'. Add a related_name argument to the definition for 'field'.

Using the %(app_label)s_%(class)s_related syntax only makes things worse:

Unhandled exception in thread started by <bound method Command.inner_run of <django.core.management.commands.runserver.Command object at 0x10146e4d0>>
Traceback (most recent call last):
  File "..../django/core/management/commands/runserver.py", line 88, in inner_run
    self.validate(display_num_errors=True)
  File "..../django/core/management/base.py", line 253, in validate
    raise CommandError("One or more models did not validate:\n%s" % error_text)
django.core.management.base.CommandError: One or more models did not validate:
app.child: Accessor for field 'field' clashes with related field 'Orange.app_child_related'. Add a related_name argument to the definition for 'field'.
app.child: Reverse query name for field 'field' clashes with related field 'Orange.app_child_related'. Add a related_name argument to the definition for 'field'.
app.child: Accessor for field 'field' clashes with related field 'Orange.app_child_related'. Add a related_name argument to the definition for 'field'.
app.child: Reverse query name for field 'field' clashes with related field 'Orange.app_child_related'. Add a related_name argument to the definition for 'field'.

Instead of causing errors, it seems like the field should only be inherited once from BaseClass. My patch handles this as follows: On each field instance, track the class it was originally declared for and the first non-abstract class that it shows up in. Then, when a field is being added to a class, check to make sure that it only gets added if it isn't a "duplicate". (The patch will incidentally also need to move the get_FIELD_display method declaration into cls._meta.add_field.)

Attachments (3)

multiple_abstract_inheritance_r15458.diff (1.8 KB) - added by melinath 3 years ago.
ticket15279_r15553.diff (4.8 KB) - added by melinath 3 years ago.
patch_r16324.diff (7.0 KB) - added by melinath 3 years ago.

Download all attachments as: .zip

Change History (13)

Changed 3 years ago by melinath

comment:1 Changed 3 years ago by melinath

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set

comment:2 Changed 3 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

Changed 3 years ago by melinath

comment:3 Changed 3 years ago by melinath

I've added a version of the ticket that should accomplish what I'm trying to do. Unfortunately, it seems to cause some issues... the test suite has a number of exceptions which don't otherwise appear. I will look into it.

======================================================================
ERROR: test_more_more_more (regressiontests.aggregation_regress.tests.AggregationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../django/tests/regressiontests/aggregation_regress/tests.py", line 631, in test_more_more_more
    qs = HardbackBook.objects.annotate(n_authors=Count('authors')).values('name', 'n_authors')
  File ".../django/django/db/models/manager.py", line 147, in annotate
    return self.get_query_set().annotate(*args, **kwargs)
  File ".../django/django/db/models/query.py", line 644, in annotate
    is_summary=False)
  File ".../django/django/db/models/sql/query.py", line 965, in add_aggregate
    field_list, opts, self.get_initial_alias(), False)
  File ".../django/django/db/models/sql/query.py", line 1236, in setup_joins
    "Choices are: %s" % (name, ", ".join(names)))
FieldError: Cannot resolve keyword 'authors' into field. Choices are: book_ptr, contact, id, isbn, name, pages, price, pubdate, publisher, rating, store, weight

======================================================================
ERROR: test_inherited_fields (regressiontests.model_inheritance_regress.tests.ModelInheritanceTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../django/tests/regressiontests/model_inheritance_regress/tests.py", line 253, in test_inherited_fields
    m2mchildren = list(M2MChild.objects.filter(articles__isnull=False))
  File ".../django/django/db/models/manager.py", line 141, in filter
    return self.get_query_set().filter(*args, **kwargs)
  File ".../django/django/db/models/query.py", line 550, in filter
    return self._filter_or_exclude(False, *args, **kwargs)
  File ".../django/django/db/models/query.py", line 568, in _filter_or_exclude
    clone.query.add_q(Q(*args, **kwargs))
  File ".../django/django/db/models/sql/query.py", line 1170, in add_q
    can_reuse=used_aliases, force_having=force_having)
  File ".../django/django/db/models/sql/query.py", line 1058, in add_filter
    negate=negate, process_extras=process_extras)
  File ".../django/django/db/models/sql/query.py", line 1236, in setup_joins
    "Choices are: %s" % (name, ", ".join(names)))
FieldError: Cannot resolve keyword 'articles' into field. Choices are: id, m2mbase_ptr, name

======================================================================
ERROR: test_table_exists (modeltests.proxy_model_inheritance.tests.ProxyModelInheritanceTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../django/tests/modeltests/proxy_model_inheritance/tests.py", line 25, in setUp
    call_command('syncdb', verbosity=0)
  File ".../django/django/core/management/__init__.py", line 166, in call_command
    return klass.execute(*args, **defaults)
  File ".../django/django/core/management/base.py", line 220, in execute
    output = self.handle(*args, **options)
  File ".../django/django/core/management/base.py", line 351, in handle
    return self.handle_noargs(**options)
  File ".../django/django/core/management/commands/syncdb.py", line 121, in handle_noargs
    custom_sql = custom_sql_for_model(model, self.style, connection)
  File ".../django/django/core/management/sql.py", line 150, in custom_sql_for_model
    app_dir = os.path.normpath(os.path.join(os.path.dirname(models.get_app(model._meta.app_label).__file__), 'sql'))
  File ".../django/django/db/models/loading.py", line 140, in get_app
    raise ImproperlyConfigured("App with label %s could not be found" % app_label)
ImproperlyConfigured: App with label model_inheritance could not be found

======================================================================
ERROR: test_distinct_for_inherited_m2m_in_list_filter (regressiontests.admin_changelist.tests.ChangeListTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../django/tests/regressiontests/admin_changelist/tests.py", line 197, in test_distinct_for_inherited_m2m_in_list_filter
    m.list_editable, m)
  File ".../django/django/contrib/admin/views/main.py", line 66, in __init__
    self.query_set = self.get_query_set()
  File ".../django/django/contrib/admin/views/main.py", line 193, in get_query_set
    raise IncorrectLookupParameters
IncorrectLookupParameters

======================================================================
FAIL: test_bug_8245 (regressiontests.bug8245.tests.Bug8245Test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../django/tests/regressiontests/bug8245/tests.py", line 18, in test_bug_8245
    'autodiscover should have raised a "Bad admin module" error.')
AssertionError: autodiscover should have raised a "Bad admin module" error.

======================================================================
FAIL: test_load_error (regressiontests.templates.tests.TemplateTagLoading)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../django/tests/regressiontests/templates/tests.py", line 1578, in test_load_error
    self.assertTrue('ImportError' in e.args[0])
AssertionError: False is not True

----------------------------------------------------------------------

comment:4 Changed 3 years ago by melinath

It should also be noted that my "test" currently consists of checking for duplicate fields. Declaring the model in a models.py file prevented runtests from working, and custom validation modeled after management.validation failed to find any errors.[1] If anyone has a suggestion on how to test more directly, I'd love to hear it.

[1] https://github.com/melinath/django/commit/a872227b1724c098d3a9d3bf8eaa87e0c6644a52

comment:5 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to Bug

Changed 3 years ago by melinath

comment:6 Changed 3 years ago by melinath

  • Easy pickings unset
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

This version of the patch works a lot better. The tests actually test things, and the behavior is more consistent. However, I'm starting to wonder if this is really the way to go about this. I feel like I'm constantly struggling against the way that Options keeps track of fields. For example, I had to add code to the _fill_x_cache methods on Options to eliminate duplication there, because options.add_field isn't called for inherited fields. This makes sense, generally, since in the past, fields have not known what models they were attached to. However, since this patch introduces field-based tracking of both the model the field was first declared on and the first concrete model the field was present on, it would be possible to take that farther. Options could add every field on the model to a cache, then dish out local fields or inherited fields based on whether the options' model is the same as the first concrete model for the field.

However, a change like that would be fairly invasive and far beyond the scope of this patch, nor am I convinced that it is the best course of action in terms of efficiency.

comment:7 Changed 3 years ago by melinath

  • Keywords abstract inheritance added
  • Status changed from new to assigned
  • UI/UX unset

comment:8 Changed 19 months ago by ptone

#19465 demonstrates that this issue is in no way specific to related fields - but fields in general

comment:9 Changed 19 months ago by ptone

  • Summary changed from Inheritance of rel fields from a single abstract base class through multiple abstract classes causes errors. to Inheritance of fields from a single abstract base class through multiple abstract classes causes errors.

comment:10 Changed 4 weeks ago by timo

  • Patch needs improvement set

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from melinath to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.