using id = 0 on get_or_create

[sylvain.lebon@…]

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.

[Simon Charette]

Which backend are you using? I'm asking because I know MySQL has issues with those kind of ids.

[anonymous]

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?

[Christopher Medrela]

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.

[Anssi Kääriäinen]

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?

[Anssi Kääriäinen]

I was just writing a patch for this :)

I will post what I have in case that is helpful and leave this to krzysiumed.

[Anssi Kääriäinen]

Some initial work

[Simon Charette]

FYI django's db backend features already keeps track of this misbehaviour for ​mysql using the allows_primary_key_0 feature. I haven't look where it's used but that's the reason why I did comment:1. It might be worth investigating where this flag is used.

[Anssi Kääriäinen]

It is used in one place currently:

tests/regressiontests/serializers_regress/tests.py:if connection.features.allows_primary_key_0:

[Simon Charette]

Then maybe the BaseDatabaseOperations.validate_autopk_value could use it somehow returning allows_primary_key_0 or value !== 0.

[Anssi Kääriäinen]

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.

[Christopher Medrela]

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

[Anssi Kääriäinen]

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.

[Claude Paroz]

The patch looks good. I've created the ticket #17713 to fix the misnaming of allows_primary_key_0.

[Anssi Kääriäinen]

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.

[Anssi Kääriäinen]

In [17933]:

Fixed #17653 -- Changed MySQL backend to raise a ValueError if zero is used as an AutoField value.

Thanks to Sylvain Lebon for the report, krzysiumed for the patch and charettes and claudep for reviews.

[carneiro.be@…]

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)

[Aymeric Augustin]

Yes, you're seeing the expected behavior introduced by the patch, ie. raising an explicit value error.

[carneiro.be@…]

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?

[Anssi Kääriäinen]

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.

[anonymous]

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!

[Anssi Kääriäinen]

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.

[Nick]

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!

[Ole Laursen]

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";

​http://cristian-radulescu.ro/article/insert-value-0-for-primary-keys-with-auto-increment-in-mysql.html

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.

[Anssi Kääriäinen]

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.

[Drew Winstel]

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

@@ -261 +261 @@
         return kwargs
 
     def get_new_connection(self, conn_params):
+        try:
+            conn_params.pop('ALLOWS_AUTO_PK_0')
+        except KeyError:
+            pass
         conn = Database.connect(**conn_params)
         conn.encoders[SafeText] = conn.encoders[six.text_type]
         conn.encoders[SafeBytes] = conn.encoders[bytes]

django/db/backends/mysql/features.py

@@ -33 +33 @@
     supports_column_check_constraints = False
     can_clone_databases = True
 
+    def __init__(self, connection):
+        super(DatabaseFeatures, self).__init__(connection)
+        self.allows_auto_pk_0 = self.connection.settings_dict['OPTIONS'].get('ALLOWS_AUTO_PK_0', False)
+
     @cached_property
     def _mysql_storage_engine(self):
         "Internal method used in Django tests. Don't rely on this from your code"

django/db/backends/mysql/operations.py

@@ -132 +132 @@
 
     def validate_autopk_value(self, value):
         # MySQLism: zero in AUTO_INCREMENT field does not work. Refs #17653.
-        if value == 0:
+        if value == 0 and not self.connection.features.allows_auto_pk_0:
             raise ValueError('The database backend does not accept 0 as a '
                              'value for AutoField.')
         return value

Also on GitHub: ​https://github.com/drewbrew/django/tree/ticket-17653

[Mariusz Felisiak]

​PR

[GitHub <noreply@…>]

In 83f55aaf:

Fixed #17653 -- Allowed using zero as AutoFields value on MySQL if NO_AUTO_VALUE_ON_ZERO SQL mode is enabled.