Fixed duplicate slugs.

Before a slug could get generated again, which threw an error since it was not unique, but treated as a kind of primary key.

Now if the same slug gets generated again, we generate one other slug with one more character.

CHANGELOG

@@ -1,6 +1,11 @@
 Changelog
 =========
 
+2.6 (dev)
+----------------
+
+* Fix for the rare case of duplicate slug (secret id) generation.
+
 2.5 (2014-01-21)
 ----------------
 

docs/settings.rst

@@ -9,7 +9,9 @@ behavior without touching the code:
 .. glossary::
 
     ``DPASTE_SLUG_LENGTH``
-        Integer. Length of the random slug for each new snippet.
+        Integer. Length of the random slug for each new snippet. In the rare
+        case an existing slug is generated again, the length will increase by
+        one more character.
         Default: ``4``
 
     ``DPASTE_SLUG_CHOICES``

dpaste/migrations/0006_auto__add_unique_snippet_secret_id.py

@@ -0,0 +1,39 @@
+# -*- coding: utf-8 -*-
+import datetime
+from south.db import db
+from south.v2 import SchemaMigration
+from django.db import models
+
+
+class Migration(SchemaMigration):
+
+    def forwards(self, orm):
+        # Adding unique constraint on 'Snippet', fields ['secret_id']
+        db.create_unique('dpaste_snippet', ['secret_id'])
+
+
+    def backwards(self, orm):
+        # Removing unique constraint on 'Snippet', fields ['secret_id']
+        db.delete_unique('dpaste_snippet', ['secret_id'])
+
+
+    models = {
+        u'dpaste.snippet': {
+            'Meta': {'ordering': "('-published',)", 'object_name': 'Snippet'},
+            'content': ('django.db.models.fields.TextField', [], {}),
+            'expire_type': ('django.db.models.fields.PositiveSmallIntegerField', [], {'default': '1'}),
+            'expires': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'blank': 'True'}),
+            u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            u'level': ('django.db.models.fields.PositiveIntegerField', [], {'db_index': 'True'}),
+            'lexer': ('django.db.models.fields.CharField', [], {'default': "'python'", 'max_length': '30'}),
+            u'lft': ('django.db.models.fields.PositiveIntegerField', [], {'db_index': 'True'}),
+            'parent': ('django.db.models.fields.related.ForeignKey', [], {'blank': 'True', 'related_name': "'children'", 'null': 'True', 'to': u"orm['dpaste.Snippet']"}),
+            'published': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
+            u'rght': ('django.db.models.fields.PositiveIntegerField', [], {'db_index': 'True'}),
+            'secret_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'unique': 'True', 'null': 'True', 'blank': 'True'}),
+            u'tree_id': ('django.db.models.fields.PositiveIntegerField', [], {'db_index': 'True'}),
+            'view_count': ('django.db.models.fields.PositiveIntegerField', [], {'default': '0'})
+        }
+    }
+
+    complete_apps = ['dpaste']

dpaste/models.py

@@ -1,5 +1,6 @@
 from random import SystemRandom
 
+from django.db import IntegrityError
 from django.db import models
 from django.core.urlresolvers import reverse
 from django.conf import settings
@@ -9,14 +10,23 @@ import mptt
 from dpaste.highlight import LEXER_DEFAULT
 
 R = SystemRandom()
-L = getattr(settings, 'DPASTE_SLUG_LENGTH', 4)
-T = getattr(settings, 'DPASTE_SLUG_CHOICES',
-    'abcdefghijkmnopqrstuvwxyzABCDEFGHJKLMNOPQRSTUVWXYZ1234567890')
-
 ONETIME_LIMIT = getattr(settings, 'DPASTE_ONETIME_LIMIT', 2)
 
-def generate_secret_id(length=L, alphabet=T):
-    return ''.join([R.choice(alphabet) for i in range(length)])
+def generate_secret_id(length=None, alphabet=None, tries=0):
+    length = length or getattr(settings, 'DPASTE_SLUG_LENGTH', 4)
+    alphabet = alphabet or getattr(settings, 'DPASTE_SLUG_CHOICES',
+        'abcdefghijkmnopqrstuvwxyzABCDEFGHJKLMNOPQRSTUVWXYZ1234567890')
+    secret_id = ''.join([R.choice(alphabet) for i in range(length)])
+
+    # Check if this slug already exists, if not, return this new slug
+    try:
+        Snippet.objects.get(secret_id=secret_id)
+    except Snippet.DoesNotExist:
+        return secret_id
+
+    # Otherwise create a new slug which is +1 character longer than the
+    # regular one.
+    return generate_secret_id(length=length+1, tries=tries)
 
 class Snippet(models.Model):
     EXPIRE_TIME = 1
@@ -28,7 +38,8 @@ class Snippet(models.Model):
         (EXPIRE_ONETIME, _(u'One time snippet')),
     )
 
-    secret_id = models.CharField(_(u'Secret ID'), max_length=255, blank=True, null=True)
+    secret_id = models.CharField(_(u'Secret ID'), max_length=255, blank=True, null=True,
+        unique=True)
     content = models.TextField(_(u'Content'))
     lexer = models.CharField(_(u'Lexer'), max_length=30, default=LEXER_DEFAULT)
     published = models.DateTimeField(_(u'Published'), auto_now_add=True)

dpaste/tests/test_snippet.py

@@ -358,15 +358,16 @@ class SnippetTestCase(TestCase):
         pygmentize('code', lexer_name='python')
         pygmentize('code', lexer_name='doesnotexist')
 
-    # This is actually a bad test. It is possible to have duplicates
-    # because even if its random, it can generate two random, equal strings.
-    #
-    # def test_random_slug_generation(self):
-    #     """
-    #     Generate 1000 random slugs, make sure we have no duplicates.
-    #     """
-    #     from dpaste.models import generate_secret_id
-    #     result_list = []
-    #     for i in range(0, 1000):
-    #         result_list.append(generate_secret_id())
-    #     self.assertEqual(len(set(result_list)), 1000)
+    @override_settings(DPASTE_SLUG_LENGTH=1)
+    def test_random_slug_generation(self):
+        """
+        Set the max length of a slug to 1, so we wont have more than 60
+        different slugs (with the default slug choice string). With 100
+        random slug generation we will run into duplicates, but those
+        slugs are extended now.
+        """
+        for i in range(0, 100):
+            Snippet.objects.create(content='foobar')
+        slug_list = Snippet.objects.values_list(
+            'secret_id', flat=True).order_by('published')
+        self.assertEqual(len(set(slug_list)), 100)

dpaste/urls/dpaste.py

@@ -10,8 +10,8 @@ urlpatterns = patterns('dpaste.views',
     url(r'^diff/$', 'snippet_diff', name='snippet_diff'),
     url(r'^history/$', 'snippet_history', name='snippet_history'),
     url(r'^delete/$', 'snippet_delete', name='snippet_delete'),
-    url(r'^(?P<snippet_id>[a-zA-Z0-9]{%d})/?$' % L, 'snippet_details', name='snippet_details'),
-    url(r'^(?P<snippet_id>[a-zA-Z0-9]{%d})/delete/$' % L, 'snippet_delete', name='snippet_delete'),
-    url(r'^(?P<snippet_id>[a-zA-Z0-9]{%d})/gist/$' % L, 'snippet_gist', name='snippet_gist'),
-    url(r'^(?P<snippet_id>[a-zA-Z0-9]{%d})/raw/?$' % L, 'snippet_details', {'template_name': 'dpaste/snippet_details_raw.html', 'is_raw': True}, name='snippet_details_raw'),
+    url(r'^(?P<snippet_id>[a-zA-Z0-9]{%d,})/?$' % L, 'snippet_details', name='snippet_details'),
+    url(r'^(?P<snippet_id>[a-zA-Z0-9]{%d,})/delete/$' % L, 'snippet_delete', name='snippet_delete'),
+    url(r'^(?P<snippet_id>[a-zA-Z0-9]{%d,})/gist/$' % L, 'snippet_gist', name='snippet_gist'),
+    url(r'^(?P<snippet_id>[a-zA-Z0-9]{%d,})/raw/?$' % L, 'snippet_details', {'template_name': 'dpaste/snippet_details_raw.html', 'is_raw': True}, name='snippet_details_raw'),
 )

setup.py

@@ -22,7 +22,7 @@ long_description = u'\n\n'.join((
 
 setup(
     name='dpaste',
-    version='2.5',
+    version='2.6',
     description='dpaste is a Django based pastebin. It\'s intended to run '
                 'separately but its also possible to be installed into an '
                 'existing Django project like a regular app.',