Fixed #19463 -- Add UUIDField #2923

mjtamlyn · 2014-07-15T10:28:51Z

Added UUIDField. Not particularly complicated this one - no custom lookups or anything. Casts all values to uuid.UUID instances in python.

TODO:

More form field tests
Documentation

Squashed patch should be marked as fixing #19463.

timgraham · 2014-07-15T11:15:56Z

django/contrib/postgres/forms/uuid_field.py

+import uuid
+
+from django.core.exceptions import ValidationError
+from django import forms


I'd alphabetize this before 'django.core'

timgraham · 2014-07-15T11:26:40Z

LGTM.

akaariai · 2014-07-16T10:23:29Z

I don't feel that good about adding UUID field for PostgreSQL only - UUID field isn't at all postgresql specific feature. OTOH I do understand why you want to implement it as a PostgreSQL only feature.

Also, UUID field is almost always used as a primary key, so it should be tested as a primary key. The field should get an automatic value when saving a new object into the database (the value can be generated in Python).

I recall a random discussion some time ago on IRC about problems with custom UUID field and admin - if we add UUID field, it should work automatically in admin, too. Unfortunately I don't recall details about this discussion, it was likely about using UUID as a primary key in admin. Maybe we need some simple tests with UUID as primary key in admin?

akaariai · 2014-07-16T10:48:16Z

The simplest solution I can think of for getting automatically a new primary key UUID value when saving a new object is addition of a new method generate_new_pk_value(self, connection) to Field. Then, if it returns something not None, the return value is used as primary key value for the model when inserting a new model. Addition of the generated UUID to the model can be done in Model._save_table(), somewhere around "if not pk_set: fields = [f for f in fields ...].

schmitch · 2014-07-17T20:43:11Z

I wouldn't put that in contrib, too. When you look at the code you would see, that its not "that" hard to add to other databases. We would only changed the field based on the connection. Like postgresql uses the db_type uuid, while sqlite3 uses a char field, while mysql uses uuid, too. wouldn't be too hard since the uuid getting created in python and not in the database.

schinckel · 2014-07-19T12:02:57Z

django/contrib/postgres/fields/uuid_field.py

+from django.utils.translation import ugettext_lazy as _
+
+
+class UUIDField(six.with_metaclass(SubfieldBase, Field)):


I'd suggest an auto keyword to the __init__ for this field type.

Actually, the more I think about it, perhaps it should be even more aggressive:

def get_default(self): default = super(UUIDField, self).get_default() if not default and not self.null: default = uuid.uuid4() return default

That is, unless we are allowing NULL, always create a value.

Hmm. It occurs to me that if primary_key=True, and we do this, it would look like the object already exists in the database before it actually is saved.

Although, since postgres cannot create UUIDs, that's going to present a problem anyway.

Postges can create uuids, when installing the extension.
Am 19.07.2014 15:20 schrieb "Matthew Schinckel" notifications@github.com:

In django/contrib/postgres/fields/uuid_field.py:

@@ -0,0 +1,43 @@
+import uuid
+
+from django.contrib.postgres import forms
+from django.core.exceptions import ValidationError
+from django.db.models import Field, SubfieldBase
+from django.utils import six
+from django.utils.translation import ugettext_lazy as _
+
+
+class UUIDField(six.with_metaclass(SubfieldBase, Field)):

Hmm. It occurs to me that if primary_key=True, and we do this, it would
look like the object already exists in the database before it actually is
saved.

Although, since postgres cannot create UUIDs, that's going to present a
problem anyway.

—
Reply to this email directly or view it on GitHub
https://github.com/django/django/pull/2923/files#r15143651.

There is no justification to be this aggressive in the field definition. Making an auto-generating uuid is as simple as UUIDField(default=uuid.uuid4). This should probably be a documented pattern.

Yeah, I was wound up on the fact that my similar field was not setting the value correctly on a default, but I was doing something stupid. The default= is a better pattern (and is extensible to dates, etc).

mjtamlyn · 2014-07-24T09:52:26Z

I've added some tests and notes in the docs about UUIDField(primary_key=True, default=uuid4). I think this is a good enough pattern (and also allows the user to choose a different uuid function if they wish, or none if their application generates uuids client side always. I don't think any extra machinery other than default should be necessary.

As for the admin, I found a number of closed (fixed) tickets relating to non-integer primary keys. If we find any issues with this in the admin I'll happily resolve them.

mjtamlyn · 2014-07-24T09:57:22Z

@akaariai @timgraham further review would be appreciated.

claudep · 2014-07-24T19:33:11Z

django/db/models/fields/__init__.py

+        super(UUIDField, self).__init__(**kwargs)
+
+    def get_internal_type(self):
+        return "CharField"


Marc, did you try to store by default in a binary column? AFAIK PostgreSQL is using a binary field internally.

There's a few issues with using BinaryField:

MySQL-Python for python 3 has no support

According to the docs they can't be filtered on, which would be a fairly significant flaw

They data format stored would be different to that likely used in string representations (i.e. in URLs, json etc) which would be the hex format. This means a user (or Django) would have to convert that string into a uuid.UUID object and then get the binary representation. I guess we might be doing this anyway.

Aside from space in the database, I don't see any particular benefit if doing this.

TBH, I haven't tried it out, it may prove to be a good idea, but I'd want some significant motivation for it.

schmitch · 2014-07-24T21:45:40Z

This looks really good. The only things I scared about are Forms with UUIDs, thats somewhat unnormal to enter a UUID as a user. Also the explicit need for default= is somewhat "strange".

And it's too bad that this won't go into 1.7

mjtamlyn · 2014-07-29T10:09:18Z

django/db/models/fields/__init__.py

+    def get_internal_type(self):
+        return "CharField"
+
+    def db_type(self, connection):


If this is in core, should it be using internal types instead?

mjtamlyn · 2014-08-05T09:15:17Z

Updated fixing docs issues.

schmitch · 2014-08-06T08:34:13Z

the build is failing hard due to git errors, not sure why only some are failing..
I've run the failing builts on my box and it failed (I didn't rebased)
so for me this looks merge ready..
Maybe you should rebase manually..

kezabelle · 2014-08-06T12:58:13Z

django/db/models/fields/__init__.py

+    description = 'Universally unique identifier'
+
+    def __init__(self, **kwargs):
+        kwargs['max_length'] = 36


Is there a reason that I'm missing (probably) that the field's length is 36 (and each DB's backend definition is 36, with the exception of postgres, which has the native datatype)?
Removing the dashes (if a string) or calling .hex turns the value into 32 chars, which both uuid.UUID() and postgres' datatype support as a input format, and both use the dash-inclusive output format.

oh that's what i've missed. thats true there is something wrong on that line

Good point, the 36 is with the hyphens. I'll correct that

mjtamlyn · 2014-08-07T09:22:33Z

Same project using UUIDField as a pk/"secondary" key/general field at https://github.com/mjtamlyn/uuid-test

It seems the correct signature for a primary key is:
id = UUIDField(primary_key=True, default=uuid.uuid4, editable=False).

collinanderson · 2014-08-07T15:50:45Z

django/db/models/fields/__init__.py

@@ -6,12 +6,14 @@
 import datetime
 import decimal
 import math
+import uuid


would it make sense to lazy-load uuid as most projects won't need it? It could be done with just one more line of code in this file and in django/forms/fields.py

You could make the same argument for all kinds of modules throughout django - starting with the import of decimal two lines above.

Python's uuid is a single python file. The overhead to import it at server-start time is so negligible it's not worth doing anything.

It's true, but decimal and datetime are much more popular.

mjtamlyn · 2014-08-16T09:48:27Z

Rebased against master.

Should be considered blocked by #3047 so I can remove SubFieldBase

timgraham · 2014-09-04T17:48:00Z

django/forms/fields.py

+        value = super(UUIDField, self).to_python(value)
+        if value in self.empty_values:
+            return None
+        if isinstance(value, six.string_types):


(same pattern as above, if you change it)

timgraham · 2014-09-04T17:50:38Z

Please add 1.8 release notes, otherwise LGTM.

timgraham · 2014-09-04T17:52:10Z

model_fields.test_uuid.TestSaveLoad.test_null_handling fails on Oracle.

mjtamlyn · 2014-09-04T17:54:12Z

I thought my last commit should fix that (ed970d4)... I'm not sure if the CI did something weird though because the list of commits on the oracle build is not the same as the list on the main build page...

timgraham · 2014-09-04T17:58:05Z

Hm, I don't have insight. Push some changes and try it again.

mjtamlyn · 2014-09-08T17:55:09Z

Squashed into a single commit with release notes. IMO, this is now RFC.

timgraham · 2014-09-15T12:30:26Z

docs/ref/databases.txt

@@ -92,7 +92,8 @@ below for information on how to set up your database correctly.
 PostgreSQL notes
 ================

-Django supports PostgreSQL 9.0 and higher.
+Django supports PostgreSQL 9.0 and higher. It requires the use of Psycopg2
+2.0.9 or higher.


Please add to release notes as well.

timgraham · 2014-09-15T12:32:54Z

Ship it!

Uses native support in postgres, and char(32) on other backends.

mjtamlyn · 2014-09-16T09:08:38Z

Merged in ed78212

timgraham reviewed Jul 15, 2014
View reviewed changes

schinckel reviewed Jul 19, 2014
View reviewed changes

mjtamlyn changed the title ~~[contrib.postgres] UUIDField~~ Fixed #19463 -- Add UUIDField Jul 24, 2014

claudep reviewed Jul 24, 2014
View reviewed changes

mjtamlyn reviewed Jul 29, 2014
View reviewed changes

kezabelle reviewed Aug 6, 2014
View reviewed changes

collinanderson reviewed Aug 7, 2014
View reviewed changes

mjtamlyn force-pushed the dcp-uuid branch from 2f20a46 to 59e3447 Compare September 4, 2014 14:46

timgraham reviewed Sep 4, 2014
View reviewed changes

mjtamlyn force-pushed the dcp-uuid branch from dfd316d to ca1b3ac Compare September 8, 2014 17:45

timgraham reviewed Sep 15, 2014
View reviewed changes

Fixed django#19463 -- Added UUIDField

1f2745e

Uses native support in postgres, and char(32) on other backends.

mjtamlyn force-pushed the dcp-uuid branch from ca1b3ac to 1f2745e Compare September 16, 2014 09:07

mjtamlyn closed this Sep 16, 2014

mjtamlyn deleted the dcp-uuid branch September 16, 2014 09:08

crccheck mentioned this pull request Nov 21, 2014

404 on Django 1.7.1 crccheck/django-object-actions#29

Closed

		from django.utils.translation import ugettext_lazy as _


		class UUIDField(six.with_metaclass(SubfieldBase, Field)):

Uh oh!

Fixed #19463 -- Add UUIDField #2923

Fixed #19463 -- Add UUIDField #2923

Uh oh!

Conversation

mjtamlyn commented Jul 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timgraham commented Jul 15, 2014

Uh oh!

akaariai commented Jul 16, 2014

Uh oh!

akaariai commented Jul 16, 2014

Uh oh!

schmitch commented Jul 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjtamlyn commented Jul 24, 2014

Uh oh!

mjtamlyn commented Jul 24, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmitch commented Jul 24, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjtamlyn commented Aug 5, 2014

Uh oh!

schmitch commented Aug 6, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjtamlyn commented Aug 7, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjtamlyn commented Aug 16, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timgraham commented Sep 4, 2014

Uh oh!

timgraham commented Sep 4, 2014

Uh oh!

mjtamlyn commented Sep 4, 2014

Uh oh!

timgraham commented Sep 4, 2014

Uh oh!

mjtamlyn commented Sep 8, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timgraham commented Sep 15, 2014

Uh oh!

mjtamlyn commented Sep 16, 2014

Uh oh!

Uh oh!