Merge pull request #19 from edx/foreign-key-key-fields

Allow the use of ForeignKey fields as ConfigurationModel KEY_FIELDS

Author: cpennington

config_models/models.py

@@ -3,7 +3,7 @@
 """
 from __future__ import absolute_import, unicode_literals
 
-from django.db import connection, models
+from django.db import models
 from django.conf import settings
 from django.core.cache import caches, InvalidCacheBackendError
 from django.utils.translation import ugettext_lazy as _
@@ -36,12 +36,7 @@ def _current_ids_subquery(self):
         all the current entries (i.e. the most recent entry for each unique set
         of key values). Only useful if KEY_FIELDS is set.
         """
-        key_fields_escaped = [connection.ops.quote_name(name) for name in self.model.KEY_FIELDS]
-        # The following assumes that the rows with the most recent date also have the highest IDs
-        return "SELECT MAX(id) FROM {table_name} GROUP BY {key_fields}".format(
-            key_fields=', '.join(key_fields_escaped),
-            table_name=self.model._meta.db_table  # pylint: disable=protected-access
-        )
+        return self.values(*self.model.KEY_FIELDS).annotate(max=models.Max('pk')).values('max')
 
     def current_set(self):
         """
@@ -51,12 +46,10 @@ def current_set(self):
         necessaryily mean enbled.
         """
         assert self.model.KEY_FIELDS != (), "Just use model.current() if there are no KEY_FIELDS"
-        return self.get_queryset().extra(           # pylint: disable=no-member
-            where=["{table_name}.id IN ({subquery})".format(
-                table_name=self.model._meta.db_table,  # pylint: disable=protected-access, no-member
-                subquery=self._current_ids_subquery(),
-            )],
-            select={'is_active': 1},  # This annotation is used by the admin changelist. sqlite requires '1', not 'True'
+        return self.get_queryset().filter(
+            pk__in=self._current_ids_subquery()
+        ).annotate(
+            is_active=models.Value(1, output_field=models.IntegerField())
         )
 
     def with_active_flag(self):
@@ -65,18 +58,17 @@ def with_active_flag(self):
         if it's the most recent entry for that combination of keys.
         """
         if self.model.KEY_FIELDS:
-            subquery = self._current_ids_subquery()
-            return self.get_queryset().extra(           # pylint: disable=no-member
-                select={'is_active': "{table_name}.id IN ({subquery})".format(
-                    table_name=self.model._meta.db_table,  # pylint: disable=protected-access, no-member
-                    subquery=subquery,
-                )}
+            return self.get_queryset().annotate(
+                is_active=models.ExpressionWrapper(
+                    models.Q(pk__in=self._current_ids_subquery()),
+                    output_field=models.IntegerField(),
+                )
+            )
+        return self.get_queryset().annotate(
+            is_active=models.ExpressionWrapper(
+                models.Q(pk=self.model.current().pk),
+                output_field=models.IntegerField(),
             )
-        return self.get_queryset().extra(           # pylint: disable=no-member
-            select={'is_active': "{table_name}.id = {pk}".format(
-                table_name=self.model._meta.db_table,  # pylint: disable=protected-access, no-member
-                pk=self.model.current().pk,  # pylint: disable=no-member
-            )}
         )
 
 

config_models/tests/test_config_models.py

@@ -1,4 +1,3 @@
-# -*- coding: utf-8 -*-
 """
 Tests of ConfigurationModel
 """
@@ -119,6 +118,21 @@ def test_active_annotation(self, mock_cache):
         self.assertEqual(rows[1].string_field, 'first')
         self.assertEqual(rows[1].is_active, False)
 
+    def test_keyed_active_annotation(self, mock_cache):
+        mock_cache.get.return_value = None
+
+        with freeze_time('2012-01-01'):
+            ExampleKeyedConfig.objects.create(left='left', right='right', user=self.user, string_field='first')
+
+        ExampleKeyedConfig.objects.create(left='left', right='right', user=self.user, string_field='second')
+
+        rows = ExampleKeyedConfig.objects.with_active_flag().order_by('-change_date')
+        self.assertEqual(len(rows), 2)
+        self.assertEqual(rows[0].string_field, 'second')
+        self.assertEqual(rows[0].is_active, True)
+        self.assertEqual(rows[1].string_field, 'first')
+        self.assertEqual(rows[1].is_active, False)
+
     def test_always_insert(self, __):
         config = ExampleConfig(changed_by=self.user, string_field='first')
         config.save()
@@ -196,17 +210,18 @@ class ExampleKeyedConfig(ConfigurationModel):
     """
     cache_timeout = 300
 
-    KEY_FIELDS = ('left', 'right')
+    KEY_FIELDS = ('left', 'right', 'user')
 
     left = models.CharField(max_length=30)
     right = models.CharField(max_length=30)
+    user = models.ForeignKey('auth.User')
 
     string_field = models.TextField()
     int_field = models.IntegerField(default=10)
 
     def __str__(self):
-        return "ExampleKeyedConfig(enabled={}, left={}, right={}, string_field={}, int_field={})".format(
-            self.enabled, self.left, self.right, self.string_field, self.int_field
+        return "ExampleKeyedConfig(enabled={}, left={}, right={}, user={}, string_field={}, int_field={})".format(
+            self.enabled, self.left, self.right, self.user, self.string_field, self.int_field
         )
 
 
@@ -225,12 +240,12 @@ def setUp(self):
     @ddt.unpack
     def test_cache_key_name(self, left, right, _mock_cache):
         self.assertEqual(
-            ExampleKeyedConfig.cache_key_name(left, right),
-            'configuration/ExampleKeyedConfig/current/{},{}'.format(left, right)
+            ExampleKeyedConfig.cache_key_name(left, right, self.user),
+            'configuration/ExampleKeyedConfig/current/{},{},{}'.format(left, right, self.user)
         )
 
     @ddt.data(
-        ((), 'left,right'),
+        ((), 'left,right,user'),
         (('left', 'right'), 'left,right'),
         (('left', ), 'left')
     )
@@ -245,15 +260,15 @@ def test_key_values_cache_key_name(self, args, expected_key, _mock_cache):
     def test_no_config_empty_cache(self, left, right, mock_cache):
         mock_cache.get.return_value = None
 
-        current = ExampleKeyedConfig.current(left, right)
+        current = ExampleKeyedConfig.current(left, right, self.user)
         self.assertEqual(current.int_field, 10)
         self.assertEqual(current.string_field, '')
-        mock_cache.set.assert_called_with(ExampleKeyedConfig.cache_key_name(left, right), current, 300)
+        mock_cache.set.assert_called_with(ExampleKeyedConfig.cache_key_name(left, right, self.user), current, 300)
 
     @ddt.data(('a', 'b'), ('c', 'd'))
     @ddt.unpack
     def test_no_config_full_cache(self, left, right, mock_cache):
-        current = ExampleKeyedConfig.current(left, right)
+        current = ExampleKeyedConfig.current(left, right, self.user)
         self.assertEqual(current, mock_cache.get.return_value)
 
     def test_config_ordering(self, mock_cache):
@@ -265,30 +280,34 @@ def test_config_ordering(self, mock_cache):
                 left='left_a',
                 right='right_a',
                 string_field='first_a',
+                user=self.user,
             ).save()
 
             ExampleKeyedConfig(
                 changed_by=self.user,
                 left='left_b',
                 right='right_b',
                 string_field='first_b',
+                user=self.user,
             ).save()
 
         ExampleKeyedConfig(
             changed_by=self.user,
             left='left_a',
             right='right_a',
             string_field='second_a',
+            user=self.user,
         ).save()
         ExampleKeyedConfig(
             changed_by=self.user,
             left='left_b',
             right='right_b',
             string_field='second_b',
+            user=self.user,
         ).save()
 
-        self.assertEqual(ExampleKeyedConfig.current('left_a', 'right_a').string_field, 'second_a')
-        self.assertEqual(ExampleKeyedConfig.current('left_b', 'right_b').string_field, 'second_b')
+        self.assertEqual(ExampleKeyedConfig.current('left_a', 'right_a', self.user).string_field, 'second_a')
+        self.assertEqual(ExampleKeyedConfig.current('left_b', 'right_b', self.user).string_field, 'second_b')
 
     def test_cache_set(self, mock_cache):
         mock_cache.get.return_value = None
@@ -297,56 +316,67 @@ def test_cache_set(self, mock_cache):
             changed_by=self.user,
             left='left',
             right='right',
+            user=self.user,
             string_field='first',
         )
         first.save()
 
-        ExampleKeyedConfig.current('left', 'right')
+        ExampleKeyedConfig.current('left', 'right', self.user)
 
-        mock_cache.set.assert_called_with(ExampleKeyedConfig.cache_key_name('left', 'right'), first, 300)
+        mock_cache.set.assert_called_with(ExampleKeyedConfig.cache_key_name('left', 'right', self.user), first, 300)
 
     def test_key_values(self, mock_cache):
         mock_cache.get.return_value = None
 
         with freeze_time('2012-01-01'):
-            ExampleKeyedConfig(left='left_a', right='right_a', changed_by=self.user).save()
-            ExampleKeyedConfig(left='left_b', right='right_b', changed_by=self.user).save()
+            ExampleKeyedConfig(left='left_a', right='right_a', user=self.user, changed_by=self.user).save()
+            ExampleKeyedConfig(left='left_b', right='right_b', user=self.user, changed_by=self.user).save()
 
-        ExampleKeyedConfig(left='left_a', right='right_a', changed_by=self.user).save()
-        ExampleKeyedConfig(left='left_b', right='right_b', changed_by=self.user).save()
+        ExampleKeyedConfig(left='left_a', right='right_a', user=self.user, changed_by=self.user).save()
+        ExampleKeyedConfig(left='left_b', right='right_b', user=self.user, changed_by=self.user).save()
 
         unique_key_pairs = ExampleKeyedConfig.key_values()
         self.assertEqual(len(unique_key_pairs), 2)
-        self.assertEqual(set(unique_key_pairs), set([('left_a', 'right_a'), ('left_b', 'right_b')]))
+        self.assertEqual(
+            set(unique_key_pairs),
+            set([('left_a', 'right_a', self.user.id), ('left_b', 'right_b', self.user.id)])
+        )
         unique_left_keys = ExampleKeyedConfig.key_values('left', flat=True)
         self.assertEqual(len(unique_left_keys), 2)
         self.assertEqual(set(unique_left_keys), set(['left_a', 'left_b']))
 
     def test_key_string_values(self, mock_cache):
         """ Ensure str() vs unicode() doesn't cause duplicate cache entries """
-        ExampleKeyedConfig(left='left', right=u'〉☃', enabled=True, int_field=10, changed_by=self.user).save()
+        ExampleKeyedConfig(
+            left='left',
+            right=u'\N{RIGHT ANGLE BRACKET}\N{SNOWMAN}',
+            enabled=True,
+            int_field=10,
+            user=self.user,
+            changed_by=self.user
+        ).save()
         mock_cache.get.return_value = None
 
-        entry = ExampleKeyedConfig.current('left', u'〉☃')
+        entry = ExampleKeyedConfig.current('left', u'\N{RIGHT ANGLE BRACKET}\N{SNOWMAN}', self.user)
         key = mock_cache.get.call_args[0][0]
         self.assertEqual(entry.int_field, 10)
         mock_cache.get.assert_called_with(key)
         self.assertEqual(mock_cache.set.call_args[0][0], key)
 
         mock_cache.get.reset_mock()
-        entry = ExampleKeyedConfig.current(u'left', u'〉☃')
+        entry = ExampleKeyedConfig.current(u'left', u'\N{RIGHT ANGLE BRACKET}\N{SNOWMAN}', self.user)
         self.assertEqual(entry.int_field, 10)
         mock_cache.get.assert_called_with(key)
 
     def test_current_set(self, mock_cache):
         mock_cache.get.return_value = None
 
         with freeze_time('2012-01-01'):
-            ExampleKeyedConfig(left='left_a', right='right_a', int_field=0, changed_by=self.user).save()
-            ExampleKeyedConfig(left='left_b', right='right_b', int_field=0, changed_by=self.user).save()
+            ExampleKeyedConfig(left='left_a', right='right_a', int_field=0, user=self.user, changed_by=self.user).save()
+            ExampleKeyedConfig(left='left_b', right='right_b', int_field=0, user=self.user, changed_by=self.user).save()
 
-        ExampleKeyedConfig(left='left_a', right='right_a', int_field=1, changed_by=self.user).save()
-        ExampleKeyedConfig(left='left_b', right='right_b', int_field=2, changed_by=self.user).save()
+        ExampleKeyedConfig(left='left_a', right='right_a', int_field=1, user=self.user, changed_by=self.user).save()
+        ExampleKeyedConfig(left='left_b', right='right_b', int_field=2, user=self.user, changed_by=self.user).save()
 
         queryset = ExampleKeyedConfig.objects.current_set()
         self.assertEqual(len(queryset.all()), 2)
@@ -359,10 +389,10 @@ def test_active_annotation(self, mock_cache):
         mock_cache.get.return_value = None
 
         with freeze_time('2012-01-01'):
-            ExampleKeyedConfig.objects.create(left='left_a', right='right_a', string_field='first')
-            ExampleKeyedConfig.objects.create(left='left_b', right='right_b', string_field='first')
+            ExampleKeyedConfig.objects.create(left='left_a', right='right_a', user=self.user, string_field='first')
+            ExampleKeyedConfig.objects.create(left='left_b', right='right_b', user=self.user, string_field='first')
 
-        ExampleKeyedConfig.objects.create(left='left_a', right='right_a', string_field='second')
+        ExampleKeyedConfig.objects.create(left='left_a', right='right_a', user=self.user, string_field='second')
 
         rows = ExampleKeyedConfig.objects.with_active_flag()
         self.assertEqual(len(rows), 3)
@@ -386,41 +416,64 @@ def test_key_values_cache(self, mock_cache):
     def test_equality(self, mock_cache):
         mock_cache.get.return_value = None
 
-        config1 = ExampleKeyedConfig(left='left_a', right='right_a', int_field=1, changed_by=self.user)
+        config1 = ExampleKeyedConfig(left='left_a', right='right_a', int_field=1, user=self.user, changed_by=self.user)
         config1.save()
 
-        config2 = ExampleKeyedConfig(left='left_b', right='right_b', int_field=2, changed_by=self.user, enabled=True)
+        config2 = ExampleKeyedConfig(
+            left='left_b',
+            right='right_b',
+            int_field=2,
+            user=self.user,
+            changed_by=self.user,
+            enabled=True,
+        )
         config2.save()
 
-        config3 = ExampleKeyedConfig(left='left_c', changed_by=self.user)
+        config3 = ExampleKeyedConfig(left='left_c', user=self.user, changed_by=self.user)
         config3.save()
 
         self.assertTrue(
-            ExampleKeyedConfig.equal_to_current({"left": "left_a", "right": "right_a", "int_field": 1})
+            ExampleKeyedConfig.equal_to_current({
+                "left": "left_a",
+                "right": "right_a",
+                "int_field": 1,
+                "user": self.user
+            })
         )
         self.assertTrue(
-            ExampleKeyedConfig.equal_to_current({"left": "left_b", "right": "right_b", "int_field": 2, "enabled": True})
+            ExampleKeyedConfig.equal_to_current({
+                "left": "left_b",
+                "right": "right_b",
+                "int_field": 2,
+                "user": self.user,
+                "enabled": True
+            })
         )
         self.assertTrue(
-            ExampleKeyedConfig.equal_to_current({"left": "left_c"})
+            ExampleKeyedConfig.equal_to_current({"left": "left_c", "user": self.user})
         )
 
         self.assertFalse(
             ExampleKeyedConfig.equal_to_current(
-                {"left": "left_a", "right": "right_a", "int_field": 1, "string_field": "foo"}
+                {"left": "left_a", "right": "right_a", "user": self.user, "int_field": 1, "string_field": "foo"}
             )
         )
         self.assertFalse(
-            ExampleKeyedConfig.equal_to_current({"left": "left_a", "int_field": 1})
+            ExampleKeyedConfig.equal_to_current({"left": "left_a", "user": self.user, "int_field": 1})
         )
         self.assertFalse(
-            ExampleKeyedConfig.equal_to_current({"left": "left_b", "right": "right_b", "int_field": 2})
+            ExampleKeyedConfig.equal_to_current({
+                "left": "left_b",
+                "right": "right_b",
+                "user": self.user,
+                "int_field": 2,
+            })
         )
         self.assertFalse(
-            ExampleKeyedConfig.equal_to_current({"left": "left_c", "int_field": 11})
+            ExampleKeyedConfig.equal_to_current({"left": "left_c", "user": self.user, "int_field": 11})
         )
 
-        self.assertFalse(ExampleKeyedConfig.equal_to_current({}))
+        self.assertFalse(ExampleKeyedConfig.equal_to_current({"user": self.user}))
 
 
 @ddt.ddt