Code

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#9039 closed (fixed)

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

Reported by: nategriswold Owned by: kmtracey
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 6 years ago.
unqieu-validate-nulls.2.diff (1.8 KB) - added by Alex 6 years ago.
Ok this should work

Download all attachments as: .zip

Change History (16)

Changed 6 years ago by Alex

comment:1 Changed 6 years ago by Alex

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

Ignore that patch, it doesn't work.

Changed 6 years ago by Alex

Ok this should work

comment:2 Changed 6 years ago by anonymous

  • Keywords model-validation added

comment:3 Changed 6 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 6 years ago by SebastienWacquiez

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

comment:5 Changed 6 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 6 years ago by kmtracey

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 5 years ago by kmtracey

  • Owner changed from nobody to kmtracey
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

comment:8 Changed 5 years ago by kmtracey

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

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

comment:9 Changed 5 years ago by kmtracey

(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 5 years ago by ikelly

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 5 years ago by kmtracey

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 5 years ago by ikelly

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

comment:13 Changed 5 years ago by kmtracey

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

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

comment:14 Changed 5 years ago by kmtracey

(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.

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.