Merge pull request #3110 from hypothesis/cache-feature-flags

Refactor feature flag to access through Client + caching

h/features.py

@@ -100,34 +100,53 @@ def __repr__(self):
         return '<Feature {f.name} everyone={f.everyone}>'.format(f=self)
 
 
-def flag_enabled(request, name):
-    """
-    Determine if the named feature is enabled for the current request.
-
-    If the feature has no override in the database, it will default to False.
-    Features must be documented, and an UnknownFeatureError will be thrown if
-    an undocumented feature is interrogated.
-    """
-    if name not in FEATURES:
-        raise UnknownFeatureError(
-            '{0} is not a valid feature name'.format(name))
-
-    feat = Feature.get_by_name(name)
-
-    # Features that don't exist in the database are off.
-    if feat is None:
+class Client(object):
+    def __init__(self, request):
+        self.request = request
+
+        all_ = request.db.query(Feature).filter(
+            Feature.name.in_(FEATURES.keys())).all()
+        self._cache = {f.name: f for f in all_}
+
+    def __call__(self, name):
+        return self.enabled(name)
+
+    def enabled(self, name):
+        """
+        Determine if the named feature is enabled for the current request.
+
+        If the feature has no override in the database, it will default to
+        False. Features must be documented, and an UnknownFeatureError will be
+        thrown if an undocumented feature is interrogated.
+        """
+        if name not in FEATURES:
+            raise UnknownFeatureError(
+                '{0} is not a valid feature name'.format(name))
+
+        feature = self._cache.get(name)
+
+        # Features that don't exist in the database are off.
+        if feature is None:
+            return False
+        # Features that are on for everyone are on.
+        if feature.everyone:
+            return True
+        # Features that are on for admin are on if the current user is an
+        # admin.
+        if feature.admins and role.Admin in self.request.effective_principals:
+            return True
+        # Features that are on for staff are on if the current user is a staff
+        # member.
+        if feature.staff and role.Staff in self.request.effective_principals:
+            return True
         return False
-    # Features that are on for everyone are on.
-    if feat.everyone:
-        return True
-    # Features that are on for admin are on if the current user is an admin.
-    if feat.admins and role.Admin in request.effective_principals:
-        return True
-    # Features that are on for staff are on if the current user is a staff
-    # member.
-    if feat.staff and role.Staff in request.effective_principals:
-        return True
-    return False
+
+    def all(self):
+        """
+        Returns a dict mapping feature flag names to enabled states
+        for the user associated with a given request.
+        """
+        return {name: self.enabled(name) for name in FEATURES.keys()}
 
 
 def remove_old_flags():
@@ -163,14 +182,6 @@ def remove_old_flags_on_boot(event):
     transaction.commit()
 
 
-def all(request):
-    """
-    Returns a dict mapping feature flag names to enabled states
-    for the user associated with a given request.
-    """
-    return {k: flag_enabled(request, k) for k in FEATURES.keys()}
-
-
 # Deprecated dedicated endpoint for feature flag data,
 # kept for compatibility with older clients (<= 0.8.6).
 # Newer clients get feature flag data as part of the session data
@@ -181,10 +192,10 @@ def all(request):
              renderer='json',
              http_cache=(0, {'no_store': False}))
 def features_status(request):
-    return all(request)
+    return request.feature.all()
 
 
 def includeme(config):
-    config.add_request_method(flag_enabled, name='feature')
+    config.add_request_method(Client, name='feature', reify=True)
     config.add_route('features_status', '/app/features')
     config.scan(__name__)

h/session.py

@@ -1,7 +1,6 @@
 # -*- coding: utf-8 -*-
 from pyramid.session import SignedCookieSessionFactory
 
-from h import features
 from h import models
 from h.security import derive_key
 
@@ -11,7 +10,7 @@ def model(request):
     session['csrf'] = request.session.get_csrf_token()
     session['userid'] = request.authenticated_userid
     session['groups'] = _current_groups(request)
-    session['features'] = features.all(request)
+    session['features'] = request.feature.all()
     session['preferences'] = {}
     user = request.authenticated_user
     if user and not user.sidebar_tutorial_dismissed:

h/test/features_test.py

@@ -28,107 +28,90 @@ def features_pending_removal_override(request):
     request.addfinalizer(patcher.stop)
 
 
-def test_flag_enabled_raises_for_undocumented_feature():
-    request = DummyRequest()
-
-    with pytest.raises(features.UnknownFeatureError):
-        features.flag_enabled(request, 'wibble')
-
-
-def test_flag_enabled_raises_for_feature_pending_removal():
-    request = DummyRequest()
-
-    with pytest.raises(features.UnknownFeatureError):
-        features.flag_enabled(request, 'abouttoberemoved')
-
-
-def test_flag_enabled_looks_up_feature_by_name(feature_model):
-    request = DummyRequest()
-
-    features.flag_enabled(request, 'notification')
-
-    feature_model.get_by_name.assert_called_with('notification')
-
-
-def test_flag_enabled_false_if_not_in_database(feature_model):
-    feature_model.get_by_name.return_value = None
-    request = DummyRequest()
-
-    result = features.flag_enabled(request, 'notification')
-
-    assert not result
-
-
-def test_flag_enabled_false_if_everyone_false(feature_model):
-    request = DummyRequest()
-
-    result = features.flag_enabled(request, 'notification')
-
-    assert not result
-
-
-def test_flag_enabled_true_if_everyone_true(feature_model):
-    feature_model.get_by_name.return_value.everyone = True
-    request = DummyRequest()
-
-    result = features.flag_enabled(request, 'notification')
-
-    assert result
-
-
-def test_flag_enabled_false_when_admins_true_normal_request(feature_model):
-    feature_model.get_by_name.return_value.admins = True
-    request = DummyRequest()
-
-    result = features.flag_enabled(request, 'notification')
-
-    assert not result
-
-
-def test_flag_enabled_true_when_admins_true_admin_request(authn_policy,
-                                                          feature_model):
-    authn_policy.effective_principals.return_value = [role.Admin]
-    feature_model.get_by_name.return_value.admins = True
-    request = DummyRequest()
-
-    result = features.flag_enabled(request, 'notification')
-
-    assert result
-
-
-def test_flag_enabled_false_when_staff_true_normal_request(feature_model):
-    """It should return False for staff features if user is not staff.
-
-    If a feature is enabled for staff, and the user is not a staff member,
-    flag_enabled() should return False.
-
-    """
-    # The feature is enabled for staff members.
-    feature_model.get_by_name.return_value.staff = True
-
-    request = DummyRequest()
-
-    assert features.flag_enabled(request, 'notification') is False
-
-
-def test_flag_enabled_true_when_staff_true_staff_request(authn_policy,
-                                                         feature_model):
-    # The authorized user is a staff member.
-    authn_policy.effective_principals.return_value = [role.Staff]
-
-    # The feature is enabled for staff.
-    feature_model.get_by_name.return_value.staff = True
-
-    request = DummyRequest()
-
-    assert features.flag_enabled(request, 'notification') is True
-
-
-@pytest.mark.usefixtures('feature_model')
-def test_all_omits_features_pending_removal():
-    request = DummyRequest()
-
-    assert features.all(request) == {'notification': False}
+class TestClient(object):
+    def test_init_loads_features(self):
+        db.Session.add(features.Feature(name='notification'))
+        db.Session.flush()
+
+        client = self.client()
+        assert client._cache.keys() == ['notification']
+
+    def test_init_skips_database_features_missing_from_dict(self):
+        """
+        Test that init does not load features that are still in the database
+        but not in the FEATURES dict anymore
+        """
+        db.Session.add(features.Feature(name='new_homepage'))
+        db.Session.flush()
+
+        client = self.client()
+        assert len(client._cache) == 0
+
+    def test_init_skips_pending_removal_features(self):
+        db.Session.add(features.Feature(name='abouttoberemoved'))
+        db.Session.flush()
+
+        client = self.client()
+        assert len(client._cache) == 0
+
+    def test_enabled_raises_for_undocumented_feature(self, client):
+        with pytest.raises(features.UnknownFeatureError):
+            client.enabled('wibble')
+
+    def test_enabled_raises_for_feature_pending_removal(self, client):
+        with pytest.raises(features.UnknownFeatureError):
+            client.enabled('abouttoberemoved')
+
+    def test_enabled_false_if_not_in_database(self, client):
+        assert client.enabled('notification') == False
+
+    def test_enabled_false_if_everyone_false(self, client):
+        client._cache['notification'] = features.Feature(everyone=False)
+        assert client.enabled('notification') == False
+
+    def test_enabled_true_if_everyone_true(self, client):
+        client._cache['notification'] = features.Feature(everyone=True)
+        assert client.enabled('notification') == True
+
+    def test_enabled_false_when_admins_true_normal_request(self, client):
+        client._cache['notification'] = features.Feature(admins=True)
+        assert client.enabled('notification') == False
+
+    def test_enabled_true_when_admins_true_admin_request(self,
+                                                         client,
+                                                         authn_policy):
+        client._cache['notification'] = features.Feature(admins=True)
+        authn_policy.effective_principals.return_value = [role.Admin]
+        assert client.enabled('notification') == True
+
+    def test_enabled_false_when_staff_true_normal_request(self, client):
+        client._cache['notification'] = features.Feature(staff=True)
+        assert client.enabled('notification') == False
+
+    def test_enabled_true_when_staff_true_staff_request(self,
+                                                        client,
+                                                        authn_policy):
+        client._cache['notification'] = features.Feature(staff=True)
+        authn_policy.effective_principals.return_value = [role.Staff]
+        assert client.enabled('notification') == True
+
+    def test_all_checks_enabled(self, client, enabled):
+        client.all()
+        enabled.assert_called_with('notification')
+
+    def test_all_omits_features_pending_removal(self, client):
+        assert client.all() == {'notification': False}
+
+    @pytest.fixture
+    def client(self):
+        return features.Client(DummyRequest(db=db.Session))
+
+    @pytest.fixture
+    def enabled(self, request, client):
+        patcher = mock.patch('h.features.Client.enabled')
+        method = patcher.start()
+        request.addfinalizer(patcher.stop)
+        return method
 
 
 def test_remove_old_flag_removes_old_flags():

h/test/session_test.py

@@ -27,14 +27,13 @@ def test_model_sorts_groups(User):
     assert ids == ['__world__', 'c', 'a', 'b']
 
 
-@mock.patch('h.session.features', autospec=True)
-def test_model_includes_features(features, fake_user):
+def test_model_includes_features(fake_user):
     feature_dict = {
         'feature_one': True,
         'feature_two': False,
     }
-    features.all = mock.Mock(return_value=feature_dict)
     request = mock.Mock(authenticated_user=fake_user)
+    request.feature.all.return_value = feature_dict
 
     assert session.model(request)['features'] == feature_dict