Code

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1705 closed defect (fixed)

If you make an instance of a class without supplying kwargs the result is an instance with defects.

Reported by: emil.gruno@… Owned by: adrian
Component: Core (Other) Version:
Severity: normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Intro

I am puzzled by the way one create an instance of a class, you have to
parse valid (memberdata) fieldname='foobar' to create the instance with the
memberdata attributes from self._meta.fields

Example;

The first example shows what i mean;

mysite/myapp/models.py

# -*- coding: cp1252 -*-
from django.db import models
class TestModel(models.Model):
    memberdata = models.CharField(maxlength=100)

>>> from mysite.myapp.models import TestModel
>>> t = TestModel()
>>> dir(t)

['AddManipulator', 'ChangeManipulator', 'DoesNotExist', '__class__',
'__delattr_
_', '__dict__', '__doc__', '__eq__', '__getattribute__', '__hash__',
'__init__',
 '__metaclass__', '__module__', '__ne__', '__new__', '__reduce__',
'__reduce_ex_
_', '__repr__', '__setattr__', '__str__', '__weakref__',
'_collect_sub_objects',
 '_default_manager', '_get_FIELD_display', '_get_FIELD_filename',
'_get_FIELD_he
ight', '_get_FIELD_size', '_get_FIELD_url', '_get_FIELD_width',
'_get_image_dime
nsions', '_get_next_or_previous_by_FIELD',
'_get_next_or_previous_in_order', '_g
et_pk_val', '_meta', '_prepare', '_save_FIELD_file',
'_set_related_many_to_many'
, 'add_to_class', 'delete', 'objects', 'save', 'validate']

id, memberdata etc.. is missing

>>> t = TestModel(memberdata='Hello World')
>>> dir(t)

['AddManipulator', 'ChangeManipulator', 'DoesNotExist', '__class__',
'__delattr_
_', '__dict__', '__doc__', '__eq__', '__getattribute__', '__hash__',
'__init__',
 '__metaclass__', '__module__', '__ne__', '__new__', '__reduce__',
'__reduce_ex_
_', '__repr__', '__setattr__', '__str__', '__weakref__',
'_collect_sub_objects',
 '_default_manager', '_get_FIELD_display', '_get_FIELD_filename',
'_get_FIELD_he
ight', '_get_FIELD_size', '_get_FIELD_url', '_get_FIELD_width',
'_get_image_dime
nsions', '_get_next_or_previous_by_FIELD',
'_get_next_or_previous_in_order', '_g
et_pk_val', '_meta', '_prepare', '_save_FIELD_file',
'_set_related_many_to_many'
, 'add_to_class', 'delete', 'id', 'memberdata', 'objects', 'save',
'validate']

if i had a member function like this:

def __repr__(self):
        return self.memberdata

And call it like this:

t = TestModel()
t + enter

This will fail to represent it self. =/ i think it should
not.

the following patch will fix that:

Index: magic-removal/django/db/models/base.py
===================================================================
--- magic-removal/django/db/models/base.py      (revision 2747)
+++ magic-removal/django/db/models/base.py      (working copy)
 -107,6 +107,9 @@
                     setattr(self, f.attname, val)
             if kwargs:
                 raise TypeError, "'%s' is an invalid keyword argument
for this function" % kwargs.keys()[0]
+        else:
+            for f in self._meta.fields:
+                setattr(self, f.attname, None)
         for i, arg in enumerate(args):
             setattr(self, self._meta.fields[i].attname, arg)
         dispatcher.send(signal=signals.post_init,
sender=self.__class__, instance=self)

Cheers,

Emil Gruno

Attachments (0)

Change History (3)

comment:1 Changed 8 years ago by ubernostrum

So far as I know, that's deliberate; until you assign at least one actual field/value pair, there's little to no point in having an ID or many of the other attributes. For example, in your case where the repr returns the 'memberdata' field, it shouldn't work unless something's actually been assigned to 'memberdata'.

comment:2 Changed 8 years ago by lukeplant

ubernostrum,

The problem is that setting a field/value after creating the instance doesn't fix things up. So for instance, if you have a model with field1, field2, and field3, this works

m = MyModel(field1='foo')
m.field2 = 'bar'
m.save() # field3 will have been set to a default.

But this doesn't:

m = MyModel()
m.field1 = 'foo'
m.field2 = 'bar'
m.save() # Raises an AttributeError

I've come across this lots of times (especially in my migration scripts and at the interactive prompt, and always just put up with it till now, but I would like to see it fixed. Another important use case would be:

m = MyModel()
fill_in_core_data(m)
m.save()

...
# MyExtendedModel has similar interface to MyModel
m2 = MyExtendedModel()
fill_in_core_data(m2)
m2.extra_field = 'foo'
m2.save()

There is an easier and more robust fix than the patch though -- just remove the if kwargs check.

comment:3 Changed 8 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new to closed

(In [2767]) magic-removal: Fixed #1705 - creating instances of models without supplying any keyword arguments.
(NB - diff is misleading - we need diff tools that support significant whitespace!)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.