Code

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#9649 closed (fixed)

[bug] invalid attribute value passed form model constructor

Reported by: ales_zoulek Owned by: nobody
Component: Database layer (models, ORM) Version: 1.0
Severity: Keywords: bug model orm foreignkey
Cc: ales.zoulek@…, ondrej.kohout@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

How to simulate

models.py

from django.db import models


class CModel(models.Model):
    x = models.CharField(max_length=20)

    def __unicode__(self):
        return self.x


class AModel(models.Model):
    a = models.FloatField()
    b = models.FloatField()
    c = models.ForeignKey(CModel)

    def __unicode__(self):
        return u"%s" % self.a

shell:

In [1]: from t.a import models

In [2]: models.AModel(a=23.0, c=None).c_id

In [3]: models.AModel(b=23.0, c=None).c_id
Out[3]: 23.0

Reason

Invalid if clause in django/db/models/base.py:242

When rel_obj is set to None, but field.null is set to False, the 'val' variable is NOT set in the whole FOR-cycle and thus the previous value gets used.

Solution

insead of:

242                         if rel_obj is None and field.null:
243                             val = None

there should be:

242                         if rel_obj is None:
243                             val = None
244                             if not field.null:
245                                 raise TypeError, "Attribute %s could not be null" % field.name

Attachments (0)

Change History (5)

comment:1 Changed 5 years ago by iky <ondrej.kohout@…>

  • Cc ondrej.kohout@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Validating of assigned values should be done by Filed, not by a Model

Alternative solution:

+++ django/db/models/base.py    (working copy)
@@ -224,7 +224,7 @@
         # keywords, or default.

         for field in fields_iter:
-            rel_obj = None
+            rel_obj = False
             if kwargs:
                 if isinstance(field.rel, ManyToOneRel):
                     try:
@@ -249,7 +249,7 @@
             # instead of field.attname (e.g. "user" instead of "user_id") so
             # that the object gets properly cached (and type checked) by the
             # RelatedObjectDescriptor.
-            if rel_obj:
+            if rel_obj is not False:
                 setattr(self, field.name, rel_obj)
             else:
                 setattr(self, field.attname, val)

Then:

In [2]: models.AModel(b=23.0, c=None).c_id
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)

...<ipython console> in <module>()

...django/django/db/models/base.pyc in __init__(self, *args, **kwargs)
    251             # RelatedObjectDescriptor.
    252             if rel_obj is not False:
--> 253                 setattr(self, field.name, rel_obj)
    254             else:
    255                 setattr(self, field.attname, val)

...django/django/db/models/fields/related.pyc in __set__(self, instance, value)
    259         if value is None and self.field.null == False:
    260             raise ValueError('Cannot assign None: "%s.%s" does not allow null values.' %
--> 261                                 (instance._meta.object_name, self.field.name))
    262         elif value is not None and not isinstance(value, self.field.rel.to):
    263             raise ValueError('Cannot assign "%r": "%s.%s" must be a "%s" instance.' %

ValueError: Cannot assign None: "AModel.c" does not allow null values.

comment:2 Changed 5 years ago by jacob

  • milestone set to 1.1
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 5 years ago by mtredinnick

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

(In [9983]) Fixed #9649 -- Better error handling in model creation.

Previously, you could explicitly assign None to a non-null ForeignKey
(or other) field when creating the model (Child(parent=None), etc). We
now throw an exception when you do that, which matches the behaviour
when you assign None to the attribute after creation.

Thanks to ales.zoulek@… and ondrej.kohout@… for some
analysis of this problem.

comment:4 Changed 5 years ago by mtredinnick

(In [9984]) [1.0.X] Fixed #9649 -- Better error handling in model creation.

Previously, you could explicitly assign None to a non-null ForeignKey
(or other) field when creating the model (Child(parent=None), etc). We
now throw an exception when you do that, which matches the behaviour
when you assign None to the attribute after creation.

Thanks to ales.zoulek@… and ondrej.kohout@… for some
analysis of this problem.

Backport of r9983 from trunk.

comment:5 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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.