From 7eba73f58d94a109775631494d6fa00aafaac72d Mon Sep 17 00:00:00 2001 From: Les Orchard Date: Wed, 31 Aug 2011 16:41:31 -0400 Subject: [PATCH] Bug 672875 - Fix DB race conditions in actioncounter and contentflagging apps * MySQL UNIQUE constraint doesn't work the way I thought it did for NULL values in columns. So, moved to calculating a unique hash in Python and store that as a unique column * actioncounters and contentflagging converted to use South migrations * South schema migrations to add unique_hash column and remove unused session_key column. * South data migration to calculate unique_hash for all existing rows. * Removed previously implemented bandaid fix that would delete multiple columns when integrity issue encountered. * Cron job for actioncounters now deletes counters over two months old for anonymous users as a means of garbage collection. --- apps/actioncounters/cron.py | 12 ++ .../actioncounters/migrations/0001_initial.py | 116 ++++++++++++++++++ .../migrations/0002_unique_hash_index.py | 92 ++++++++++++++ .../migrations/0003_update_unique_hashes.py | 110 +++++++++++++++++ apps/actioncounters/migrations/__init__.py | 0 apps/actioncounters/models.py | 67 +++++----- apps/actioncounters/tests.py | 38 +++--- apps/actioncounters/utils.py | 36 +++--- .../migrations/0001_initial.py | 95 ++++++++++++++ .../migrations/0002_unique_hash_index.py | 91 ++++++++++++++ .../migrations/0003_update_unique_hashes.py | 87 +++++++++++++ apps/contentflagging/migrations/__init__.py | 0 apps/contentflagging/models.py | 46 +++---- apps/contentflagging/tests.py | 30 +++-- apps/contentflagging/utils.py | 36 +++--- .../08-actioncounters-convert-to-south.sql | 4 + .../09-contentflagging-convert-to-south.sql | 4 + 17 files changed, 745 insertions(+), 119 deletions(-) create mode 100644 apps/actioncounters/migrations/0001_initial.py create mode 100644 apps/actioncounters/migrations/0002_unique_hash_index.py create mode 100644 apps/actioncounters/migrations/0003_update_unique_hashes.py create mode 100644 apps/actioncounters/migrations/__init__.py create mode 100644 apps/contentflagging/migrations/0001_initial.py create mode 100644 apps/contentflagging/migrations/0002_unique_hash_index.py create mode 100644 apps/contentflagging/migrations/0003_update_unique_hashes.py create mode 100644 apps/contentflagging/migrations/__init__.py create mode 100644 migrations/08-actioncounters-convert-to-south.sql create mode 100644 migrations/09-contentflagging-convert-to-south.sql diff --git a/apps/actioncounters/cron.py b/apps/actioncounters/cron.py index c2a1e3309b9..cab91bc7423 100644 --- a/apps/actioncounters/cron.py +++ b/apps/actioncounters/cron.py @@ -5,6 +5,7 @@ import cronjobs # TODO: Figure out a way to do this per-class? Would need to break up some of the SQL calls. +ACTIONCOUNTERS_ANON_GC_WINDOW = getattr(settings, "ACTIONCOUNTERS_ANON_GC_WINDOW", "2 MONTH") ACTIONCOUNTERS_RECENT_COUNT_WINDOW = getattr(settings, "ACTIONCOUNTERS_RECENT_COUNT_WINDOW", "14 DAY") @cronjobs.register @@ -27,6 +28,17 @@ def get_update(ct_pk, obj_pk): cursor = connection.cursor() + # Garbage collect any counters for anonymous users over a certain age. + cursor.execute(""" + DELETE + FROM actioncounters_actioncounterunique + WHERE + user_id IS NULL AND + modified < date_sub(now(), INTERVAL %(interval)s) + """ % dict( + interval=ACTIONCOUNTERS_ANON_GC_WINDOW + )) + # Any counters too old for the window should be set to 0 cursor.execute(""" SELECT content_type_id, object_pk, name diff --git a/apps/actioncounters/migrations/0001_initial.py b/apps/actioncounters/migrations/0001_initial.py new file mode 100644 index 00000000000..55460a839fa --- /dev/null +++ b/apps/actioncounters/migrations/0001_initial.py @@ -0,0 +1,116 @@ +# encoding: 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 model 'TestModel' + db.create_table('actioncounters_testmodel', ( + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('title', self.gf('django.db.models.fields.CharField')(unique=True, max_length=255)), + ('views_total', self.gf('django.db.models.fields.IntegerField')(default=0, db_index=True, blank=True)), + ('views_recent', self.gf('django.db.models.fields.IntegerField')(default=0, db_index=True, blank=True)), + ('boogs_total', self.gf('django.db.models.fields.IntegerField')(default=0, db_index=True, blank=True)), + ('boogs_recent', self.gf('django.db.models.fields.IntegerField')(default=0, db_index=True, blank=True)), + ('likes_total', self.gf('django.db.models.fields.IntegerField')(default=0, db_index=True, blank=True)), + ('likes_recent', self.gf('django.db.models.fields.IntegerField')(default=0, db_index=True, blank=True)), + ('frobs_total', self.gf('django.db.models.fields.IntegerField')(default=0, db_index=True, blank=True)), + ('frobs_recent', self.gf('django.db.models.fields.IntegerField')(default=0, db_index=True, blank=True)), + )) + db.send_create_signal('actioncounters', ['TestModel']) + + # Adding model 'ActionCounterUnique' + db.create_table('actioncounters_actioncounterunique', ( + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('content_type', self.gf('django.db.models.fields.related.ForeignKey')(related_name='content_type_set_for_actioncounterunique', to=orm['contenttypes.ContentType'])), + ('object_pk', self.gf('django.db.models.fields.CharField')(max_length=32)), + ('name', self.gf('django.db.models.fields.CharField')(max_length=64, db_index=True)), + ('total', self.gf('django.db.models.fields.IntegerField')()), + ('ip', self.gf('django.db.models.fields.CharField')(db_index=True, max_length=40, null=True, blank=True)), + ('session_key', self.gf('django.db.models.fields.CharField')(db_index=True, max_length=40, null=True, blank=True)), + ('user_agent', self.gf('django.db.models.fields.CharField')(db_index=True, max_length=128, null=True, blank=True)), + ('user', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['auth.User'], null=True, blank=True)), + ('modified', self.gf('django.db.models.fields.DateTimeField')(auto_now=True, blank=True)), + )) + db.send_create_signal('actioncounters', ['ActionCounterUnique']) + + + def backwards(self, orm): + + # Deleting model 'TestModel' + db.delete_table('actioncounters_testmodel') + + # Deleting model 'ActionCounterUnique' + db.delete_table('actioncounters_actioncounterunique') + + + models = { + 'actioncounters.actioncounterunique': { + 'Meta': {'object_name': 'ActionCounterUnique'}, + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'content_type_set_for_actioncounterunique'", 'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'ip': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '40', 'null': 'True', 'blank': 'True'}), + 'modified': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '64', 'db_index': 'True'}), + 'object_pk': ('django.db.models.fields.CharField', [], {'max_length': '32'}), + 'session_key': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '40', 'null': 'True', 'blank': 'True'}), + 'total': ('django.db.models.fields.IntegerField', [], {}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}), + 'user_agent': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '128', 'null': 'True', 'blank': 'True'}) + }, + 'actioncounters.testmodel': { + 'Meta': {'object_name': 'TestModel'}, + 'boogs_recent': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'boogs_total': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'frobs_recent': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'frobs_total': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'likes_recent': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'likes_total': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'title': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '255'}), + 'views_recent': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'views_total': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}) + }, + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + } + } + + complete_apps = ['actioncounters'] diff --git a/apps/actioncounters/migrations/0002_unique_hash_index.py b/apps/actioncounters/migrations/0002_unique_hash_index.py new file mode 100644 index 00000000000..d48d07f66f2 --- /dev/null +++ b/apps/actioncounters/migrations/0002_unique_hash_index.py @@ -0,0 +1,92 @@ +# encoding: 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): + + # Deleting field 'ActionCounterUnique.session_key' + db.delete_column('actioncounters_actioncounterunique', 'session_key') + + # Adding field 'ActionCounterUnique.unique_hash' + db.add_column('actioncounters_actioncounterunique', 'unique_hash', self.gf('django.db.models.fields.CharField')(blank=True, max_length=32, null=True, db_index=True), keep_default=False) + + + def backwards(self, orm): + + # Adding field 'ActionCounterUnique.session_key' + db.add_column('actioncounters_actioncounterunique', 'session_key', self.gf('django.db.models.fields.CharField')(blank=True, max_length=40, null=True, db_index=True), keep_default=False) + + # Deleting field 'ActionCounterUnique.unique_hash' + db.delete_column('actioncounters_actioncounterunique', 'unique_hash') + + + models = { + 'actioncounters.actioncounterunique': { + 'Meta': {'object_name': 'ActionCounterUnique'}, + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'content_type_set_for_actioncounterunique'", 'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'ip': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '40', 'null': 'True', 'blank': 'True'}), + 'modified': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '64', 'db_index': 'True'}), + 'object_pk': ('django.db.models.fields.CharField', [], {'max_length': '32'}), + 'total': ('django.db.models.fields.IntegerField', [], {}), + 'unique_hash': ('django.db.models.fields.CharField', [], {'default': "''", 'unique': 'True', 'max_length': '32', 'db_index': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}), + 'user_agent': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '128', 'null': 'True', 'blank': 'True'}) + }, + 'actioncounters.testmodel': { + 'Meta': {'object_name': 'TestModel'}, + 'boogs_recent': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'boogs_total': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'frobs_recent': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'frobs_total': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'likes_recent': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'likes_total': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'title': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '255'}), + 'views_recent': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'views_total': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}) + }, + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + } + } + + complete_apps = ['actioncounters'] diff --git a/apps/actioncounters/migrations/0003_update_unique_hashes.py b/apps/actioncounters/migrations/0003_update_unique_hashes.py new file mode 100644 index 00000000000..6268138339c --- /dev/null +++ b/apps/actioncounters/migrations/0003_update_unique_hashes.py @@ -0,0 +1,110 @@ +# encoding: utf-8 +import datetime +from south.db import db +from south.v2 import DataMigration +from django.db import models + +class Migration(DataMigration): + + def forwards(self, orm): + "Update the unique hashes on all objects" + + # First, delete older anonymous unique counters, since they're not all + # that useful at this point or worth migrating. + # + # This should also be done in the update_actioncounter_counts cronjob, + # but wasn't implemented before this migration. Doing it now, so the + # data migration has a smaller set to work with. + (orm.ActionCounterUnique.objects + .filter(user=None, modified__lt=datetime.datetime(2011,8,1)) + .delete()) + + # Update all remaining counters with a unique hash. + from actioncounters.utils import get_unique + counters = orm.ActionCounterUnique.objects.all() + for counter in counters: + try: + # Need to duplicate the custom saving code from the model, + # since South's frozen version of it doesn't have the logic. + user, ip, user_agent, unique_hash = get_unique( + counter.content_type, counter.object_pk, counter.name, + ip=counter.ip, user_agent=counter.user_agent, + user=counter.user) + counter.unique_hash = unique_hash + counter.save() + except IntegrityError: + # If there's already a counter with the unique hash, delete + # this one as a duplicate. + counter.delete() + + + def backwards(self, orm): + "Nothing to reverse - the field will be deleted" + + + models = { + 'actioncounters.actioncounterunique': { + 'Meta': {'object_name': 'ActionCounterUnique'}, + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'content_type_set_for_actioncounterunique'", 'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'ip': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '40', 'null': 'True', 'blank': 'True'}), + 'modified': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '64', 'db_index': 'True'}), + 'object_pk': ('django.db.models.fields.CharField', [], {'max_length': '32'}), + 'total': ('django.db.models.fields.IntegerField', [], {}), + 'unique_hash': ('django.db.models.fields.CharField', [], {'max_length': '32', 'unique': 'True', 'null': 'True', 'db_index': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}), + 'user_agent': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '128', 'null': 'True', 'blank': 'True'}) + }, + 'actioncounters.testmodel': { + 'Meta': {'object_name': 'TestModel'}, + 'boogs_recent': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'boogs_total': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'frobs_recent': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'frobs_total': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'likes_recent': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'likes_total': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'title': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '255'}), + 'views_recent': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}), + 'views_total': ('django.db.models.fields.IntegerField', [], {'default': '0', 'db_index': 'True', 'blank': 'True'}) + }, + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + } + } + + complete_apps = ['actioncounters'] diff --git a/apps/actioncounters/migrations/__init__.py b/apps/actioncounters/migrations/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/apps/actioncounters/models.py b/apps/actioncounters/models.py index f40a40c434f..41966285ca1 100644 --- a/apps/actioncounters/models.py +++ b/apps/actioncounters/models.py @@ -1,6 +1,7 @@ """Models for activity counters""" import logging +import hashlib from django.db import models from django.conf import settings @@ -40,45 +41,25 @@ def get_unique_for_request(self, object, action_name, request, create=True): refrain from creating a new one if the intent is just to check existence.""" content_type = ContentType.objects.get_for_model(object) - user, ip, user_agent, session_key = get_unique(request) - try: - - if create: - return self.get_or_create( - content_type=content_type, object_pk=object.pk, - name=action_name, - ip=ip, user_agent=user_agent, user=user, - session_key=session_key, - defaults=dict( total=0 )) - else: - try: - return ( - self.get( - content_type=content_type, object_pk=object.pk, - name=action_name, - ip=ip, user_agent=user_agent, user=user, - session_key=session_key,), - False - ) - except ActionCounterUnique.DoesNotExist: - return ( None, False ) - - except MultipleObjectsReturned: - # HACK: There seems to be a race condition in get_or_create. :( - # Happens very rarely, but when it does, seems like the best thing - # to do is try to clean up? - counters = self.filter( - content_type=content_type, object_pk=object.pk, - name=action_name, - ip=ip, user_agent=user_agent, user=user, - session_key=session_key) - counter = counters[0] - for c in counters[1:]: c.delete() - return ( counter, False ) + user, ip, user_agent, unique_hash = get_unique(content_type, object.pk, + action_name, + request=request) + if create: + return self.get_or_create(unique_hash=unique_hash, + defaults=dict(content_type=content_type, object_pk=object.pk, + name=action_name, ip=ip, + user_agent=user_agent, user=user, + total=0)) + else: + try: + return (self.get(unique_hash=unique_hash), False) + except ActionCounterUnique.DoesNotExist: + return (None, False) class ActionCounterUnique(models.Model): """Action counter for a unique request / user""" + objects = ActionCounterUniqueManager() content_type = models.ForeignKey( @@ -96,17 +77,29 @@ class ActionCounterUnique(models.Model): ip = models.CharField(max_length=40, editable=False, db_index=True, blank=True, null=True) - session_key = models.CharField(max_length=40, editable=False, - db_index=True, blank=True, null=True) user_agent = models.CharField(max_length=128, editable=False, db_index=True, blank=True, null=True) user = models.ForeignKey(User, editable=False, db_index=True, blank=True, null=True) + # HACK: As it turns out, MySQL doesn't consider two rows with NULL values + # in a column as duplicates. So, resorting to calculating a unique hash in + # code. + unique_hash = models.CharField(max_length=32, editable=False, + unique=True, db_index=True, null=True) + modified = models.DateTimeField( _('date last modified'), auto_now=True, blank=False) + def save(self, *args, **kwargs): + # Ensure unique_hash is updated whenever the object is saved + user, ip, user_agent, unique_hash = get_unique( + self.content_type, self.object_pk, self.name, + ip=self.ip, user_agent=self.user_agent, user=self.user) + self.unique_hash = unique_hash + super(ActionCounterUnique, self).save(*args, **kwargs) + def increment(self, min=0, max=1): return self._change_total(1, min, max) diff --git a/apps/actioncounters/tests.py b/apps/actioncounters/tests.py index 6e18372bf20..fb2710b524b 100644 --- a/apps/actioncounters/tests.py +++ b/apps/actioncounters/tests.py @@ -47,39 +47,47 @@ def tearDown(self): # logging.debug("SQL %s" % sql) pass - def mk_request(self, user=None, session_key=None, ip='192.168.123.123', + def mk_request(self, user=None, ip='192.168.123.123', user_agent='FakeBrowser 1.0'): request = HttpRequest() request.user = user and user or AnonymousUser() - if session_key: - request.session = Session() - request.session.session_key = session_key request.method = 'GET' request.META['REMOTE_ADDR'] = ip request.META['HTTP_USER_AGENT'] = user_agent return request + @attr('bad_multiple') def test_bad_multiple_counters(self): """Force multiple counters, possibly result of race condition, ensure graceful handling""" - request = self.mk_request() - user, ip, user_agent, session_key = get_unique(request) - + action_name = "likes" obj_1 = self.obj_1 obj_1_ct = ContentType.objects.get_for_model(obj_1) + request = self.mk_request() + user, ip, user_agent, unique_hash = get_unique(obj_1_ct, obj_1.pk, + action_name, request) + + # Create an initial counter record directly. u1 = ActionCounterUnique(content_type=obj_1_ct, object_pk=obj_1.pk, - name="likes", total=1, ip=ip, user_agent=user_agent, user=user, - session_key=session_key) + name=action_name, total=1, ip=ip, user_agent=user_agent, + user=user) u1.save() - u2 = ActionCounterUnique(content_type=obj_1_ct, object_pk=obj_1.pk, - name="likes", total=1, ip=ip, user_agent=user_agent, user=user, - session_key=session_key) - u2.save() - + # Adding a duplicate counter should be prevented at the model level. + try: + u2 = ActionCounterUnique(content_type=obj_1_ct, object_pk=obj_1.pk, + name=action_name, total=1, ip=ip, user_agent=user_agent, + user=user) + u2.save() + ok_(False, "This should have triggered an IntegrityError") + except: + pass + + # Try get_unique_for_request, which should turn up the single unique + # record created earlier. try: (u, created) = ActionCounterUnique.objects.get_unique_for_request(obj_1, - 'likes', request) + action_name, request) eq_(False, created) except MultipleObjectsReturned, e: ok_(False, "MultipleObjectsReturned should not be raised") diff --git a/apps/actioncounters/utils.py b/apps/actioncounters/utils.py index 91df3a3a708..fc36cf25342 100644 --- a/apps/actioncounters/utils.py +++ b/apps/actioncounters/utils.py @@ -1,10 +1,13 @@ -from django.conf import settings import re import logging +import hashlib + +from django.conf import settings # this is not intended to be an all-knowing IP address regex IP_RE = re.compile('\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}') + def get_ip(request): """ Retrieves the remote IP address from the request data. If the user is @@ -37,30 +40,31 @@ def get_ip(request): return ip_address -def get_unique(request, use_session_key=False): +def get_unique(content_type, object_pk, name, request=None, ip=None, user_agent=None, user=None): """Extract a set of unique identifiers from the request. This set will be made up of one of the following combinations, depending on what's available: - * user, None, None, None - * None, None, None, session_key - * None, ip, user_agent, None + * user, None, None, unique_MD5_hash + * None, ip, user_agent, unique_MD5_hash """ - if request.user.is_authenticated(): - user = request.user - ip = user_agent = session_key = None - else: - user = None - session_key = ( - ( use_session_key and hasattr(request, 'session') ) and - request.session.session_key or None ) - if session_key: + if request: + if request.user.is_authenticated(): + user = request.user ip = user_agent = None else: + user = None ip = get_ip(request) user_agent = request.META.get('HTTP_USER_AGENT', '')[:255] - return ( user, ip, user_agent, session_key ) - + # HACK: Build a hash of the fields that should be unique, let MySQL + # chew on that for a unique index. Note that any changes to this algo + # will create all new unique hashes that don't match any existing ones. + hash_text = "\n".join(unicode(x) for x in ( + content_type.pk, object_pk, name, ip, user_agent, + (user and user.pk or 'None') + )) + unique_hash = hashlib.md5(hash_text).hexdigest() + return (user, ip, user_agent, unique_hash) diff --git a/apps/contentflagging/migrations/0001_initial.py b/apps/contentflagging/migrations/0001_initial.py new file mode 100644 index 00000000000..bc9e08408d4 --- /dev/null +++ b/apps/contentflagging/migrations/0001_initial.py @@ -0,0 +1,95 @@ +# encoding: 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 model 'ContentFlag' + db.create_table('contentflagging_contentflag', ( + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('flag_status', self.gf('django.db.models.fields.CharField')(default='flagged', max_length=16)), + ('flag_type', self.gf('django.db.models.fields.CharField')(max_length=64, db_index=True)), + ('explanation', self.gf('django.db.models.fields.TextField')(max_length=255, blank=True)), + ('content_type', self.gf('django.db.models.fields.related.ForeignKey')(related_name='content_type_set_for_contentflag', to=orm['contenttypes.ContentType'])), + ('object_pk', self.gf('django.db.models.fields.CharField')(max_length=32)), + ('ip', self.gf('django.db.models.fields.CharField')(max_length=40, null=True, blank=True)), + ('session_key', self.gf('django.db.models.fields.CharField')(max_length=40, null=True, blank=True)), + ('user_agent', self.gf('django.db.models.fields.CharField')(max_length=128, null=True, blank=True)), + ('user', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['auth.User'], null=True, blank=True)), + ('created', self.gf('django.db.models.fields.DateTimeField')(auto_now_add=True, blank=True)), + ('modified', self.gf('django.db.models.fields.DateTimeField')(auto_now=True, blank=True)), + )) + db.send_create_signal('contentflagging', ['ContentFlag']) + + # Adding unique constraint on 'ContentFlag', fields ['content_type', 'object_pk', 'ip', 'session_key', 'user_agent', 'user'] + db.create_unique('contentflagging_contentflag', ['content_type_id', 'object_pk', 'ip', 'session_key', 'user_agent', 'user_id']) + + + def backwards(self, orm): + + # Removing unique constraint on 'ContentFlag', fields ['content_type', 'object_pk', 'ip', 'session_key', 'user_agent', 'user'] + db.delete_unique('contentflagging_contentflag', ['content_type_id', 'object_pk', 'ip', 'session_key', 'user_agent', 'user_id']) + + # Deleting model 'ContentFlag' + db.delete_table('contentflagging_contentflag') + + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'contentflagging.contentflag': { + 'Meta': {'ordering': "('-created',)", 'unique_together': "(('content_type', 'object_pk', 'ip', 'session_key', 'user_agent', 'user'),)", 'object_name': 'ContentFlag'}, + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'content_type_set_for_contentflag'", 'to': "orm['contenttypes.ContentType']"}), + 'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'explanation': ('django.db.models.fields.TextField', [], {'max_length': '255', 'blank': 'True'}), + 'flag_status': ('django.db.models.fields.CharField', [], {'default': "'flagged'", 'max_length': '16'}), + 'flag_type': ('django.db.models.fields.CharField', [], {'max_length': '64', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'ip': ('django.db.models.fields.CharField', [], {'max_length': '40', 'null': 'True', 'blank': 'True'}), + 'modified': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}), + 'object_pk': ('django.db.models.fields.CharField', [], {'max_length': '32'}), + 'session_key': ('django.db.models.fields.CharField', [], {'max_length': '40', 'null': 'True', 'blank': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}), + 'user_agent': ('django.db.models.fields.CharField', [], {'max_length': '128', 'null': 'True', 'blank': 'True'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + } + } + + complete_apps = ['contentflagging'] diff --git a/apps/contentflagging/migrations/0002_unique_hash_index.py b/apps/contentflagging/migrations/0002_unique_hash_index.py new file mode 100644 index 00000000000..610edeb1b03 --- /dev/null +++ b/apps/contentflagging/migrations/0002_unique_hash_index.py @@ -0,0 +1,91 @@ +# encoding: 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): + + try: + # Removing unique constraint on 'ContentFlag', fields ['ip', 'object_pk', 'user_agent', 'content_type', 'session_key', 'user'] + db.delete_unique('contentflagging_contentflag', ['ip', 'object_pk', 'user_agent', 'content_type_id', 'session_key', 'user_id']) + except: + # This constraint may have already been removed, so ignore exceptions + pass + + # Deleting field 'ContentFlag.session_key' + db.delete_column('contentflagging_contentflag', 'session_key') + + # Adding field 'ContentFlag.unique_hash' + db.add_column('contentflagging_contentflag', 'unique_hash', self.gf('django.db.models.fields.CharField')(max_length=32, unique=True, null=True, db_index=True), keep_default=False) + + + def backwards(self, orm): + + # Adding field 'ContentFlag.session_key' + db.add_column('contentflagging_contentflag', 'session_key', self.gf('django.db.models.fields.CharField')(max_length=40, null=True, blank=True), keep_default=False) + + # Deleting field 'ContentFlag.unique_hash' + db.delete_column('contentflagging_contentflag', 'unique_hash') + + # Adding unique constraint on 'ContentFlag', fields ['ip', 'object_pk', 'user_agent', 'content_type', 'session_key', 'user'] + db.create_unique('contentflagging_contentflag', ['ip', 'object_pk', 'user_agent', 'content_type_id', 'session_key', 'user_id']) + + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'contentflagging.contentflag': { + 'Meta': {'ordering': "('-created',)", 'object_name': 'ContentFlag'}, + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'content_type_set_for_contentflag'", 'to': "orm['contenttypes.ContentType']"}), + 'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'explanation': ('django.db.models.fields.TextField', [], {'max_length': '255', 'blank': 'True'}), + 'flag_status': ('django.db.models.fields.CharField', [], {'default': "'flagged'", 'max_length': '16'}), + 'flag_type': ('django.db.models.fields.CharField', [], {'max_length': '64', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'ip': ('django.db.models.fields.CharField', [], {'max_length': '40', 'null': 'True', 'blank': 'True'}), + 'modified': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}), + 'object_pk': ('django.db.models.fields.CharField', [], {'max_length': '32'}), + 'unique_hash': ('django.db.models.fields.CharField', [], {'max_length': '32', 'unique': 'True', 'null': 'True', 'db_index': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}), + 'user_agent': ('django.db.models.fields.CharField', [], {'max_length': '128', 'null': 'True', 'blank': 'True'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + } + } + + complete_apps = ['contentflagging'] diff --git a/apps/contentflagging/migrations/0003_update_unique_hashes.py b/apps/contentflagging/migrations/0003_update_unique_hashes.py new file mode 100644 index 00000000000..acc64cd894e --- /dev/null +++ b/apps/contentflagging/migrations/0003_update_unique_hashes.py @@ -0,0 +1,87 @@ +# encoding: utf-8 +import datetime +from south.db import db +from south.v2 import DataMigration +from django.db import models + +class Migration(DataMigration): + + def forwards(self, orm): + "Write your forwards methods here." + # Update all remaining flags with a unique hash. + from contentflagging.utils import get_unique + flags = orm.ContentFlag.objects.all() + for flag in flags: + try: + # Need to duplicate the custom saving code from the model, + # since South's frozen version of it doesn't have the logic. + user, ip, user_agent, unique_hash = get_unique( + flag.content_type, flag.object_pk, + ip=flag.ip, user_agent=flag.user_agent, user=flag.user) + flag.unique_hash = unique_hash + flag.save() + except IntegrityError: + # If there's already a flag with the unique hash, delete + # this one as a duplicate. + flag.delete() + + + def backwards(self, orm): + "Write your backwards methods here." + + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'contentflagging.contentflag': { + 'Meta': {'ordering': "('-created',)", 'object_name': 'ContentFlag'}, + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'content_type_set_for_contentflag'", 'to': "orm['contenttypes.ContentType']"}), + 'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'explanation': ('django.db.models.fields.TextField', [], {'max_length': '255', 'blank': 'True'}), + 'flag_status': ('django.db.models.fields.CharField', [], {'default': "'flagged'", 'max_length': '16'}), + 'flag_type': ('django.db.models.fields.CharField', [], {'max_length': '64', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'ip': ('django.db.models.fields.CharField', [], {'max_length': '40', 'null': 'True', 'blank': 'True'}), + 'modified': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}), + 'object_pk': ('django.db.models.fields.CharField', [], {'max_length': '32'}), + 'unique_hash': ('django.db.models.fields.CharField', [], {'max_length': '32', 'unique': 'True', 'null': 'True', 'db_index': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}), + 'user_agent': ('django.db.models.fields.CharField', [], {'max_length': '128', 'null': 'True', 'blank': 'True'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + } + } + + complete_apps = ['contentflagging'] diff --git a/apps/contentflagging/migrations/__init__.py b/apps/contentflagging/migrations/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/apps/contentflagging/models.py b/apps/contentflagging/models.py index 4b72e3052aa..2ade2af8410 100644 --- a/apps/contentflagging/models.py +++ b/apps/contentflagging/models.py @@ -55,25 +55,17 @@ def flag(self, request, object, flag_type, explanation, recipients=None): if flag_type not in dict(FLAG_REASONS): return (None, False) - user, ip, user_agent, session_key = get_unique(request) - content_type = ContentType.objects.get_for_model(object) + user, ip, user_agent, unique_hash = get_unique(content_type, object.pk, + request=request) - try: - cf = ContentFlag.objects.get_or_create( - content_type=content_type, object_pk=object.pk, - ip=ip, user_agent=user_agent, user=user, session_key=session_key, - defaults=dict(flag_type=flag_type, explanation=explanation,)) - - except MultipleObjectsReturned, e: - # HACK: There seems to be a race condition in get_or_create. :( - # Happens very rarely, but when it does, seems like the best thing - # to do is try to clean up? - flags = ContentFlag.objects.filter( - content_type=content_type, object_pk=object.pk, - ip=ip, user_agent=user_agent, user=user, session_key=session_key).all() - cf = ( flags[0], False ) - for c in flags[1:]: c.delete() + cf = ContentFlag.objects.get_or_create( + unique_hash=unique_hash, + defaults=dict(content_type=content_type, + object_pk=object.pk, ip=ip, + user_agent=user_agent, user=user, + flag_type=flag_type, + explanation=explanation)) if recipients: subject = _("{object} Flagged") @@ -94,8 +86,6 @@ class ContentFlag(models.Model): class Meta: ordering = ('-created',) get_latest_by = 'created' - unique_together = (('content_type', 'object_pk', - 'ip', 'session_key', 'user_agent', 'user'),) flag_status = models.CharField( _('current status of flag review'), @@ -106,6 +96,7 @@ class Meta: explanation = models.TextField( _('please explain what content you feel is inappropriate'), max_length=255, blank=True) + content_type = models.ForeignKey( ContentType, editable=False, verbose_name="content type", @@ -119,15 +110,18 @@ class Meta: ip = models.CharField( max_length=40, editable=False, blank=True, null=True) - session_key = models.CharField( - max_length=40, editable=False, - blank=True, null=True) user_agent = models.CharField( max_length=128, editable=False, blank=True, null=True) user = models.ForeignKey( User, editable=False, blank=True, null=True) + # HACK: As it turns out, MySQL doesn't consider two rows with NULL values + # in a column as duplicates. So, resorting to calculating a unique hash in + # code. + unique_hash = models.CharField(max_length=32, editable=False, + unique=True, db_index=True, null=True) + created = models.DateTimeField( _('date submitted'), auto_now_add=True, blank=False, editable=False) @@ -139,6 +133,14 @@ def __unicode__(self): return 'ContentFlag %(flag_type)s -> "%(title)s"' % dict( flag_type=self.flag_type, title=str(self.content_object)) + def save(self, *args, **kwargs): + # Ensure unique_hash is updated whenever the object is saved + user, ip, user_agent, unique_hash = get_unique( + self.content_type, self.object_pk, + ip=self.ip, user_agent=self.user_agent, user=self.user) + self.unique_hash = unique_hash + super(ContentFlag, self).save(*args, **kwargs) + def content_view_link(self): """HTML link to the absolute URL for the linked content object""" object = self.content_object diff --git a/apps/contentflagging/tests.py b/apps/contentflagging/tests.py index a15733dcd8e..263848c23b8 100644 --- a/apps/contentflagging/tests.py +++ b/apps/contentflagging/tests.py @@ -40,38 +40,44 @@ def tearDown(self): # logging.debug("SQL %s" % sql) pass - def mk_request(self, user=None, session_key=None, ip='192.168.123.123', + def mk_request(self, user=None, ip='192.168.123.123', user_agent='FakeBrowser 1.0'): request = HttpRequest() request.user = user and user or AnonymousUser() - if session_key: - request.session = Session() - request.session.session_key = session_key request.method = 'GET' request.META['REMOTE_ADDR'] = ip request.META['HTTP_USER_AGENT'] = user_agent return request + @attr('bad_multiple') def test_bad_multiple_flags(self): """Force multiple flags, possibly result of race condition, ensure graceful handling""" request = self.mk_request() - user, ip, user_agent, session_key = get_unique(request) obj_1 = self.user2 obj_1_ct = ContentType.objects.get_for_model(obj_1) + user, ip, user_agent, unique_hash = get_unique(obj_1_ct, obj_1.pk, + request=request) + # Create an initial record directly. f1 = ContentFlag(content_type=obj_1_ct, object_pk=obj_1.pk, flag_type="Broken thing", - ip=ip, user_agent=user_agent, user=user, session_key=session_key) + ip=ip, user_agent=user_agent, user=user) f1.save() - f2 = ContentFlag(content_type=obj_1_ct, object_pk=obj_1.pk, - flag_type="Broken thing", - ip=ip, user_agent=user_agent, user=user, session_key=session_key) - f2.save() - + # Adding a duplicate should be prevented at the model level. + try: + f2 = ContentFlag(content_type=obj_1_ct, object_pk=obj_1.pk, + flag_type="Broken thing", + ip=ip, user_agent=user_agent, user=user) + f2.save() + except: + pass + + # Try flag, which should turn up the single unique record created + # earlier. try: - flag, created = ContentFlag.objects.flag(request=request, object=self.user2, + flag, created = ContentFlag.objects.flag(request=request, object=obj_1, flag_type='notworking', explanation="It really not go!") ok_(flag is not None) ok_(not created) diff --git a/apps/contentflagging/utils.py b/apps/contentflagging/utils.py index c1c2da9e41b..3f2038c9dab 100644 --- a/apps/contentflagging/utils.py +++ b/apps/contentflagging/utils.py @@ -1,6 +1,8 @@ -from django.conf import settings import re import logging +import hashlib + +from django.conf import settings # this is not intended to be an all-knowing IP address regex IP_RE = re.compile('\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}') @@ -37,31 +39,31 @@ def get_ip(request): return ip_address -def get_unique(request, use_session_key=False): +def get_unique(content_type, object_pk, request=None, ip=None, user_agent=None, user=None): """Extract a set of unique identifiers from the request. This set will be made up of one of the following combinations, depending on what's available: - * user, None, None, None - * None, None, None, session_key - * None, ip, user_agent, None + * user, None, None, unique_MD5_hash + * None, ip, user_agent, unique_MD5_hash """ - if request.user.is_authenticated(): - user = request.user - ip = user_agent = session_key = None - else: - user = None - session_key = ( - ( use_session_key and hasattr(request, 'session') ) and - request.session.session_key or None ) - if session_key: + if request: + if request.user.is_authenticated(): + user = request.user ip = user_agent = None else: + user = None ip = get_ip(request) user_agent = request.META.get('HTTP_USER_AGENT', '')[:255] - return ( user, ip, user_agent, session_key ) - - + # HACK: Build a hash of the fields that should be unique, let MySQL + # chew on that for a unique index. Note that any changes to this algo + # will create all new unique hashes that don't match any existing ones. + hash_text = "\n".join(unicode(x) for x in ( + content_type.pk, object_pk, ip, user_agent, + (user and user.pk or 'None') + )) + unique_hash = hashlib.md5(hash_text).hexdigest() + return (user, ip, user_agent, unique_hash) diff --git a/migrations/08-actioncounters-convert-to-south.sql b/migrations/08-actioncounters-convert-to-south.sql new file mode 100644 index 00000000000..4d3c21808aa --- /dev/null +++ b/migrations/08-actioncounters-convert-to-south.sql @@ -0,0 +1,4 @@ +-- +-- actioncounters app converted to use South, simulate --fake to skip first migration +-- +INSERT INTO `south_migrationhistory` VALUES (null,'actioncounters','0001_initial','2011-08-31 10:00:00'); diff --git a/migrations/09-contentflagging-convert-to-south.sql b/migrations/09-contentflagging-convert-to-south.sql new file mode 100644 index 00000000000..f2f5041c48b --- /dev/null +++ b/migrations/09-contentflagging-convert-to-south.sql @@ -0,0 +1,4 @@ +-- +-- contentflagging app converted to use South, simulate --fake to skip first migration +-- +INSERT INTO `south_migrationhistory` VALUES (null,'contentflagging','0001_initial','2011-08-31 10:00:00');