Opened 13 years ago
Closed 4 years ago
#17653 closed Bug (fixed)
using id = 0 on get_or_create
Reported by: | Owned by: | Mariusz Felisiak | |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.3 |
Severity: | Normal | Keywords: | |
Cc: | krzysiumed@…, anssi.kaariainen@…, olau@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
When using get_or_create with id=0, django does create an object, but doesn't write the id back in the returned object. The object is therefore not usable. We should get the new object's id, or at least get an error message to prevent using 0 as an id.
Attachments (3)
Change History (34)
follow-up: 2 comment:1 by , 13 years ago
comment:2 by , 13 years ago
Replying to charettes:
Which backend are you using? I'm asking because I know MySQL has issues with those kind of ids.
Indeed, I'm using MySQL. good guess :)
MySQL does create an object with a random id when given 0. Is that to say that this issue is to be considered as a mysql bug?
Shouldn't Django work that around?
comment:3 by , 13 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
The same problem appears when you pass argument id=0
to __init__
of your model (in my example it's Text
), see code below.
>>> t = Text(id=0, text="lorem ipsum") >>> t.id 0 >>> t.save() >>> i.id 0 >>> Text.objects.all() [<Text: Text object>] >>> Text.objects.all()[0].id 1
The problem appears with MySQL
. sqlite3
works properly. I've not checked other backends.
I think that the problem is in django.db.models.fields.AutoField
which is integer (see https://docs.djangoproject.com/en/1.3/ref/models/fields/#autofield) especially it can be zero. It should works like PositiveIntegerField
or something like that. Supporting AutoField
validation can be a solution, but it's backward incompatible. What's more, the validation is not necessary for backends other than MySQL because zero is valid value for id
in most databases. We can check if id
is not zero if the backend is MySQL otherwise don't validate id
, but it makes the issue more comlex and it's still backward incompatible.
comment:4 by , 13 years ago
Cc: | added |
---|
You could do a check in model.base_save(): connection.validate_pk(val), which should just raise an exception if the backend doesn't work with the given value. So, if you try to save a model with id=0, you would get an exception on MySQL on 0-valued PK value, and otherwise things would work as expected. This change should be simple to implement and safe to use. I don't think backwards compatibility for MySQL 0-valued PKs is worth worrying about: the current behavior can be considered a bug.
Another option is to just skip this issue by saying this is a bug in MySQL, not in Django. But as in this case it is pretty easy to prevent the confusing behavior, then why not?
comment:5 by , 13 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 13 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
I was just writing a patch for this :)
I will post what I have in case that is helpful and leave this to krzysiumed.
comment:7 by , 13 years ago
comment:8 by , 13 years ago
It is used in one place currently:
tests/regressiontests/serializers_regress/tests.py:if connection.features.allows_primary_key_0:
comment:9 by , 13 years ago
Then maybe the BaseDatabaseOperations.validate_autopk_value
could use it somehow returning allows_primary_key_0 or value !== 0
.
comment:10 by , 13 years ago
Or the if connection.features.allows_primary_key_0 could be changed to if connection.ops.validate_autopk_value(0).
Now that I look at it, it seems the name for the validate_autopk_value is wrong, maybe check_autopk_value()?
I quickly tested primary key value of 0 using MySQL, and it seems to work. It is the automatic PK which doesn't work for zero value. So, it really isn't about allowing 0 as primary key value, it is about autopk values. I am no MySQL expert, so I might be wrong here.
In general I am opposed to these boolean allows_primary_key_0 type features. An external backend might disallow all non-positive values and they have no way to express that with the boolean. Granted, this one isn't particularly bad. And this really isn't this ticket's problem.
comment:11 by , 13 years ago
Owner: | changed from | to
---|---|
Patch needs improvement: | unset |
Status: | assigned → new |
Sorry for delay, but the ticket is not as easy as I thought.
I attached a patch similar to akaariai's patch. It validates value for primary key in new method BaseDatabaseOperations.validate_autopk_value
in module db.backends
. The method is overriden in DatabaseOperations
in module db.backends.mysql
and it does not use allows_primary_key_0
flag. The method is called by AutoField.get_db_prep_value
from module db.models.fields
. The patch includes simple test.
I'm not sure if get_db_prep_value
is right place for validation. The doc of the method says:
Returns field's value prepared for interacting with the database backend. Used by the default implementations of ``get_db_prep_save``and `get_db_prep_lookup```
so maybe we should add note that get_db_prep_value
do validation and may raise error?
Note that both patches (mine and akaariai's) works for AutoField
as well as for ForeignKey
and ManyToManyField
, because related fields calls AutoField.get_db_prep_value
during executing their get_db_prep_value
method (see https://code.djangoproject.com/browser/django/trunk/django/db/models/fields/related.py#L962).
by , 13 years ago
Attachment: | ticket_17653_v2.diff added |
---|
comment:12 by , 13 years ago
Patch needs improvement: | set |
---|
The get_db_prep_value seems like the right place, because that is called in all the right places. The only other option I can see is to create a new method for this.
The get_db_prep_value is already doing some validation, it calls get_prep_value which calls int(value), and that can raise a ValueError
.
There is a small mistake in the error message. It says zero is not accepted for primary key. This isn't true for MySQL
.
mysql> create table testtbl(id integer primary key); Query OK, 0 rows affected (0.05 sec) mysql> insert into testtbl values(0); Query OK, 1 row affected (0.01 sec) mysql> select * from testtbl; +----+ | id | +----+ | 0 | +----+ 1 row in set (0.00 sec)
I know this is a minor nitpick. But it is also easy to correct. It should say that zero is not allowed for AutoFields
.
There should be a test for bulk_create, too. Maybe, as this ticket is about get_or_create add one for that too, although that might be a bit redundant.
by , 13 years ago
Attachment: | ticket_17653_v3.diff added |
---|
comment:13 by , 13 years ago
Patch needs improvement: | unset |
---|
comment:14 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
The patch looks good. I've created the ticket #17713 to fix the misnaming of allows_primary_key_0.
comment:15 by , 13 years ago
I did some minor cleanups to the patch. Results here: https://github.com/akaariai/django/commit/bc206a3e9ca90e40d626c7d20a07d644418aadfc
I will commit this in a day or two unless somebody objects.
comment:17 by , 12 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
I am now getting the error "The database backend does not accept 0 as a value for AutoField." using the latest SVN version. What am I doing wrong? I'm not even using any AutoField (besides the django automatic id on all models)
comment:18 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Yes, you're seeing the expected behavior introduced by the patch, ie. raising an explicit value error.
comment:19 by , 12 years ago
Alright, I've managed to track what's my problem. I have a ForeignKey to the user model. If this value is 0 it will trigger this exception. I believe the patch isn't checking anywhere if it is in fact an autofield. Or this the desired behavior?
comment:20 by , 12 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:21 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
It seems you are trying to save 0 to a ForeignKey which points to an AutoField. But, this is illegal, as the AutoField will not accept that value and so the ForeignKey can not hold that value either.
I am closing this still as fixed - you can again open this, but please provide more details if you do. The above explanation leaves us guessing if there is some bug or not in the newly introduced behavior. A testcase/testproject would be really valuable.
EDIT: Didn't spot the error message earlier. That might be a little confusing... It says AutoField while the field infact is a ForeignKey. Is this the problem?
comment:22 by , 12 years ago
Sorry that I opened it twice. Yes, I guess I found it a bit confusing that it said AutoField. My first reaction was to check the id of the object, which was not 0 (in fact I was editing something).
But why is it illegal? I mean, it had worked before with no problem. I know it's not a good practice to have a foreign key as 0, but I don't understand how it can be illegal. Pointing a Foreign Key to 0 for me would be just like pointing to an id that doesn't exist, I don't see the difference. I did understand the bug you were trying to fix, but should this also apply to ForeignKeys?
Thanks in advance!
comment:23 by , 12 years ago
So, you are creating ForeignKeys with value 0 knowing they point nowhere? You should set the ForeignKey as null=True and use None instead. You could get incorrect results from some queries if you use 0 instead of null=True.
I don't think ForeignKeys should be fixed to allow 0 as a value. Django's foreign keys are really meant to point at things if they have a value set.
comment:24 by , 11 years ago
So this fix creates another problem when you need to be able to accept a value of 0 (or if you are working on a DB that already has a value of 0!) in an autofield. In my case, I need to accept an ID of zero so that my foreign key can point to zero so that a unique constraint can work properly. (see http://dev.mysql.com/doc/refman/5.6/en/innodb-table-and-index.html , http://mechanics.flite.com/blog/2013/06/18/closing-the-unique-index-null-loophole/ , and other pages that discuss)
Here is an example of my situation. I am collecting school test score data with a marker for demographics (e.g., Race/Ethnicity). Each school should only be allowed to have a single test score entry for each race. There should also be an allowance for a single entry for overall (no race). Logic suggests using a unique constraint on race and school and allowing race to be nullable. However, the SQL rules specify that null values within unique indexes are not counted as duplicates. (That was an unexpected discovery, BTW). So, I either have to create my own unique index field or take off the null constraint on the race field (shown below).
class Race(models.Model): id = models.AutoField(primary_key=True) Name = models.CharField(max_length=32) class School(models.Model) id = models.AutoField(primary_key=True) Name = models.CharField(max_length=32) class TestScore(models.Model) id = models.AutoField(primary_key=True) race = models.ForeignKey(Race) school = models.ForeignKey(School) score = models.DecimalField(max_digits=4, decimal_places=1) class Meta(): unique_together = [ ["school", 'race'], ]
A reasonable value for this special fake-null field would be 0. If you create SQL directly without going through the django cursor, you can create a new field and then change the auto-generated field back to 0.
INSERT INTO race ('name') VALUE ('None') UPDATE race SET id=0 WHERE id = <autogenerated ID>
Performing that sequence through a combination of the ORM and cursor connection works
r = Race(name='None') r.save() connection.cursor().execute('UPDATE race SET id=0 WHERE id=%s', [r.id,]) r = Race.objects.get(id=0)
HOWEVER, creating a record based on that new object raises an error!
>>> test = TestScore(school=school, race=r, score=175.2) File "python2.7/site-packages/django/db/models/base.py", line 546, in save force_update=force_update, update_fields=update_fields) File "python2.7/site-packages/django/db/models/base.py", line 650, in save_base result = manager._insert([self], fields=fields, return_id=update_pk, using=using, raw=raw) File "python2.7/site-packages/django/db/models/manager.py", line 215, in _insert return insert_query(self.model, objs, fields, **kwargs) File "python2.7/site-packages/django/db/models/query.py", line 1661, in insert_query return query.get_compiler(using=using).execute_sql(return_id) File "python2.7/site-packages/django/db/models/sql/compiler.py", line 936, in execute_sql for sql, params in self.as_sql(): File "python2.7/site-packages/django/db/models/sql/compiler.py", line 894, in as_sql for obj in self.query.objs File "python2.7/site-packages/django/db/models/fields/related.py", line 1047, in get_db_prep_save connection=connection) File "python2.7/site-packages/django/db/models/fields/__init__.py", line 304, in get_db_prep_save prepared=False) File "python2.7/site-packages/django/db/models/fields/__init__.py", line 549, in get_db_prep_value value = connection.ops.validate_autopk_value(value) File "python2.7/site-packages/django/db/backends/mysql/base.py", line 288, in validate_autopk_value raise ValueError('The database backend does not accept 0 as a ' ValueError: The database backend does not accept 0 as a value for AutoField.
So, I guess there are two issues here.
1) It is possible to insert a new row with ID 0, but it requires a two-step process of insert followed by update.
2) Once you have this auto value of 0 (whether it was generated through Django or otherwise), you can't actually point at it!
comment:25 by , 11 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
You used to be able to set the id by creating the object, then do a .filter(pk=obj.pk).update(pk=0).
I just want to add that while this may seem a bit far-fetched, this change did indeed break backwards compatibility in a pretty annoying way because of the ForeignKey problem.
It seems to me there would be alternative ways of solving the problem that would keep MySQL in line, e.g.
SET SQL_MODE = "NO_AUTO_VALUE_ON_ZERO";
This would actually fix the error, i.e. make everything work, rather than exporting the problem as a validation error. So I will try reopening the bug. If you disagree, close it again and I will go away.
comment:26 by , 11 years ago
Cc: | added |
---|
comment:27 by , 11 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
Removing ready for checkin as there isn't anything to commit in this ticket.
Maybe we could allow users to set SQL_MODE to NO_AUTO_VALUE_ON_ZERO and then allow usage of zero as autopk value, or maybe we could just allow usage of zero in .update(). However the .update() option seems a bit hard to implement, as we would need information that we are updating to get_db_prep_value(), but there doesn't seem to be any way to do that.
To me it seems a database settings option ALLOW_AUTO_VALUE_ZERO for MySQL to switch the SQL mode, and to also switch off the autopk checking is the best approach.
comment:28 by , 11 years ago
Has patch: | unset |
---|
comment:29 by , 9 years ago
I'm not sure if my means of handling the settings is the right option here, but this seems to work for me (patch is against 1.9.5, not master):
-
django/db/backends/mysql/base.py
diff --git a/django/db/backends/mysql/base.py b/django/db/backends/mysql/base.py index 03f3857..1595356 100644
a b class DatabaseWrapper(BaseDatabaseWrapper): 261 261 return kwargs 262 262 263 263 def get_new_connection(self, conn_params): 264 try: 265 conn_params.pop('ALLOWS_AUTO_PK_0') 266 except KeyError: 267 pass 264 268 conn = Database.connect(**conn_params) 265 269 conn.encoders[SafeText] = conn.encoders[six.text_type] 266 270 conn.encoders[SafeBytes] = conn.encoders[bytes] -
django/db/backends/mysql/features.py
diff --git a/django/db/backends/mysql/features.py b/django/db/backends/mysql/features.py index c8ea8f7..28cb0ad 100644
a b class DatabaseFeatures(BaseDatabaseFeatures): 33 33 supports_column_check_constraints = False 34 34 can_clone_databases = True 35 35 36 def __init__(self, connection): 37 super(DatabaseFeatures, self).__init__(connection) 38 self.allows_auto_pk_0 = self.connection.settings_dict['OPTIONS'].get('ALLOWS_AUTO_PK_0', False) 39 36 40 @cached_property 37 41 def _mysql_storage_engine(self): 38 42 "Internal method used in Django tests. Don't rely on this from your code" -
django/db/backends/mysql/operations.py
diff --git a/django/db/backends/mysql/operations.py b/django/db/backends/mysql/operations.py index 55a5e92..f667614 100644
a b class DatabaseOperations(BaseDatabaseOperations): 132 132 133 133 def validate_autopk_value(self, value): 134 134 # MySQLism: zero in AUTO_INCREMENT field does not work. Refs #17653. 135 if value == 0 :135 if value == 0 and not self.connection.features.allows_auto_pk_0: 136 136 raise ValueError('The database backend does not accept 0 as a ' 137 137 'value for AutoField.') 138 138 return value
Also on GitHub: https://github.com/drewbrew/django/tree/ticket-17653
Which backend are you using? I'm asking because I know MySQL has issues with those kind of ids.