Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#9039 closed (fixed)

Form validation problem for model with ForeignKey having unique=True,blank=True,null=True

Reported by: nategriswold Owned by: Karen Tracey
Component: Forms Version: 1.0
Severity: Keywords: model-validation
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I'm having problems validating a form for the below models. Multiple null values in the below "Thing" model's "other" column seem to prevent the basic ModelForm from validating. This also happens for OneToOneFields of the same nature. Normal django db api functions and the database do not seem to have any problem. Basically, django forms are treating null as conflicting with null but the backend and db api are not. I don't think this is intended.

I also tried this in an older revision of django (r7477, using form_for_model) and did not have the problem there.

Thanks

class OtherThing(models.Model):
   pass

class Thing(models.Model):
   other = models.ForeignKey('OtherThing', null=True, blank=True, unique=True)

>>> import django
>>> django.get_version()
>>> from django.forms import ModelForm
>>> from test.models import *
>>>
>>> class ThingForm(ModelForm):
...   class Meta:
...     model = Thing
...
>>> Thing.objects.all()
[]
>>> ThingForm({}).save()
<Thing: Thing object>
>>> ThingForm({}).save()
Traceback (most recent call last):
 File "<console>", line 1, in ?
 File "/home/griswold/lib/python/django/forms/models.py", line 302, in save
   return save_instance(self, self.instance, self._meta.fields,
fail_message, commit)
 File "/home/griswold/lib/python/django/forms/models.py", line 36, in
save_instance
   raise ValueError("The %s could not be %s because the data didn't"
ValueError: The Thing could not be created because the data didn't validate.
>>>
>>> Thing.objects.create()
<Thing: Thing object>
>>> Thing.objects.create()
<Thing: Thing object>
>>> Thing.objects.all()
[<Thing: Thing object>, <Thing: Thing object>, <Thing: Thing object>]
mysql> show create table test_thing\G
*************************** 1. row ***************************
       Table: test_thing
Create Table: CREATE TABLE `test_thing` (
  `id` int(11) NOT NULL auto_increment,
  `other_id` int(11) default NULL,
  PRIMARY KEY  (`id`),
  KEY `test_thing_other_id` (`other_id`),
  CONSTRAINT `other_id_refs_id_52a1a3a0adf89f6` FOREIGN KEY (`other_id`) REFERENCES `test_otherthing` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1
1 row in set (0.00 sec)
}}}[[BR]]

Attachments (2)

unqieu-validate-nulls.diff (1.7 KB) - added by Alex Gaynor 8 years ago.
unqieu-validate-nulls.2.diff (1.8 KB) - added by Alex Gaynor 8 years ago.
Ok this should work

Download all attachments as: .zip

Change History (16)

Changed 8 years ago by Alex Gaynor

Attachment: unqieu-validate-nulls.diff added

comment:1 Changed 8 years ago by Alex Gaynor

Ignore that patch, it doesn't work.

Changed 8 years ago by Alex Gaynor

Ok this should work

comment:2 Changed 8 years ago by anonymous

Keywords: model-validation added

comment:3 Changed 8 years ago by paltman

for what it is worth I have reviewed and tested this patch and can verify/validate that it is working as intended. I currently have my system patched, but would be nice to get it into trunk so that i don't have to remember to apply this patch in my different environments.

comment:4 Changed 8 years ago by SebastienWacquiez

I just used the .2 patch to allow null value inside a "unique_together", it just work.

comment:5 Changed 8 years ago by smcoll

Has patch: set

i also confirm that this works, and would like to see it rolled into the code.

comment:6 Changed 8 years ago by Karen Tracey

I do not understand the approach taken in the patch. Adding lookup_kwargs["%s__isnull" % field_name] = False to the database lookup that checks for uniqueness results in obviously not going to match anything queries such as:

SELECT (1) AS `a` FROM `model_forms_patient` WHERE (`model_forms_patient`.`special_id` IS NOT NULL AND `model_forms_patient`.`special_id` IS NULL)

or an unnecessary condition like:

SELECT (1) AS `a` FROM `model_forms_patient` WHERE (`model_forms_patient`.`special_id` IS NOT NULL AND `model_forms_patient`.`special_id` = 1 )

Why not simply avoid adding the field to those checked if it can be null and the provided value is empty? What am i missing here?

comment:7 Changed 8 years ago by Karen Tracey

Owner: changed from nobody to Karen Tracey
Status: newassigned
Triage Stage: UnreviewedAccepted

comment:8 Changed 8 years ago by Karen Tracey

Resolution: fixed
Status: assignedclosed

(In [9239]) Fixed #9039 -- Don't perform unique checks on NULL values, since NULL != NULL in SQL.

comment:9 Changed 8 years ago by Karen Tracey

(In [9240]) [1.0.X] Fixed #9039 -- Don't perform unique checks on NULL values, since NULL != NULL in SQL.

Backport of [9239] from trunk.

comment:10 Changed 8 years ago by Ian Kelly

Resolution: fixed
Status: closedreopened

I'm reopening this for further discussion because the test case fails in Oracle. The assumption that null doesn't count for uniqueness only holds when all the columns in the constraint are null. As a result, the form passes validation but then results in an IntegrityError when we try to save it.

Probably the ideal solution would be to allow the form to validate depending on the backend, so that this would work when possible and fail validation when not possible. But I don't think we currently do any backend-specific form validation, and I'm wary of what the implications of adding that might be -- especially in a multi-db scenario.

comment:11 Changed 8 years ago by Karen Tracey

Ugh. Agreed that best would be if we could validate the form in a way that matched what the backend was going to enforce, but we don't seem to have anything like that in place and like you I'd be wary of moving in that direction. Could we just remove the 2nd save in the testcase to avoid triggering the error? We're left with a form that will validate when the acutaul DB save() is going to fail on at least one backend. Not ideal but more attractive to me than having the form refuse to validate values that some DBs find acceptable. And in the general case form validation doesn't guarantee that the DB save is actually going to work, so code should be prepared to deal with DB errors even after a successful validate()...right?

comment:12 Changed 8 years ago by Ian Kelly

Yeah, that makes the most sense to me as well.

comment:13 Changed 8 years ago by Karen Tracey

Resolution: fixed
Status: reopenedclosed

(In [9288]) Fixed #9039 take 2 -- Modified the new tests added in [9239] so they pass on Oracle.

comment:14 Changed 8 years ago by Karen Tracey

(In [9290]) [1.0.X] Fixed #9039 take 2 -- Modified the new tests added in [9240] so they pass on Oracle.

Backport of [9288] from trunk, also updated svnmerge.py metadata.

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