Opened 5 years ago

Closed 5 years ago

#24749 closed Bug (invalid)

contribute_to_class(virtual_only=True) is ignored

Reported by: TiM Owned by: nobody
Component: Database layer (models, ORM) Version: 1.8
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

/django/db/models/fields/init.py

def contribute_to_class(self, cls, name, virtual_only=False):
  self.set_attributes_from_name(name) 
  self.model = cls
  if virtual_only:
      cls._meta.add_field(self, virtual=True)

  self.attname, self.column = self.get_attname_column()
  self.concrete = self.column is not None


def get_attname_column(self):
    attname = self.get_attname()
    column = self.db_column or attname
    return attname, column

/django/db/models/options.py

def concrete_fields(self):

  return make_immutable_fields_list("concrete_fields", (f for f in self.fields if f.concrete)

concrete_fields() is checking for concrete property but

column = self.db_column or attname

will ensure it's never false :(

Change History (5)

comment:1 Changed 5 years ago by Tim Graham

Easy pickings: unset
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

comment:2 Changed 5 years ago by Marten Kenbeek

Severity: Release blockerNormal

I don't see a release-blocking bug here. In every built-in case (e.g. [1], [2], [3]), non-concrete fields explicitly override this behaviour, so any built-in non-concrete fields have concrete = False. So while it doesn't make any sense, it is working as advertised for all built-in fields (and the documentation says nothing about custom fields in that regard).

Now this definitely should be cleaned up and/or properly documented (especially in the "Writing custom model fields"[4] section). This ties in to #16508 as well, I think.

[1] https://github.com/django/django/blob/master/django/db/models/fields/related.py#L1276
[2] https://github.com/django/django/blob/master/django/db/models/fields/related.py#L1730
[3] https://github.com/django/django/blob/master/django/contrib/contenttypes/fields.py#L32
[4] https://docs.djangoproject.com/en/1.8/howto/custom-model-fields/#writing-a-field-subclass

comment:3 Changed 5 years ago by Tim Graham

Summary: contribute_to_class(virtual_field=True) is ignoredcontribute_to_class(virtual_only=True) is ignored

Anssi said this parameter may be misnamed:

If I recall correctly the real significance of virtual_only is that fields added with this flag are copied to child models. Generic relations need this as they need to know the model they are used on. Normal fields are not copied, instead they are fetched directly from the parent model when _meta is queried.

So, it might be possible to rename virtual_only to something like copy_to_childs, and then virtuality of a field would be determined only on it having a column.

https://groups.google.com/d/msg/django-developers/nkEuOnIf4Ao/IPp5E11WXDkJ

comment:4 Changed 5 years ago by Tim Graham

After another look, the virtual_only flag controls whether or not the field is added to Options.virtual_fields so maybe it's named appropriately after all?

comment:5 Changed 5 years ago by Tim Graham

Resolution: invalid
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top