ref: Have RedisSessionStore properties declared explicitly (#27109)

Replace the implicit assignment of attributes (via __getattr__ and
__setattr__) to be stored to Redis under arbitrary names. This clarifies
the source of the attributes and prevents some infinite recursion bugs.

src/sentry/pipeline/__init__.py

@@ -5,7 +5,7 @@ from sentry import analytics
 from sentry.models import Organization
 from sentry.utils import json
 from sentry.utils.hashlib import md5_text
-from sentry.utils.session_store import RedisSessionStore
+from sentry.utils.session_store import RedisSessionStore, redis_property
 from sentry.web.frontend.base import BaseView
 from sentry.web.helpers import render_to_response
 
@@ -106,6 +106,17 @@ class NestedPipelineView(PipelineView):
         return nested_pipeline.current_step()
 
 
+class PipelineSessionStore(RedisSessionStore):
+    uid = redis_property("uid")
+    provider_model_id = redis_property("provider_model_id")
+    provider_key = redis_property("provider_key")
+    org_id = redis_property("org_id")
+    signature = redis_property("signature")
+    step_index = redis_property("step_index")
+    config = redis_property("config")
+    data = redis_property("data")
+
+
 class Pipeline:
     """
     Pipeline provides a mechanism to guide the user through a request
@@ -138,7 +149,7 @@ class Pipeline:
 
     @classmethod
     def get_for_request(cls, request):
-        state = RedisSessionStore(request, cls.pipeline_name, ttl=INTEGRATION_EXPIRATION_TTL)
+        state = PipelineSessionStore(request, cls.pipeline_name, ttl=INTEGRATION_EXPIRATION_TTL)
         if not state.is_valid():
             return None
 
@@ -167,7 +178,9 @@ class Pipeline:
 
         self.request = request
         self.organization = organization
-        self.state = RedisSessionStore(request, self.pipeline_name, ttl=INTEGRATION_EXPIRATION_TTL)
+        self.state = PipelineSessionStore(
+            request, self.pipeline_name, ttl=INTEGRATION_EXPIRATION_TTL
+        )
         self.provider = self.provider_manager.get(provider_key)
         self.provider_model = provider_model
 

src/sentry/utils/session_store.py

@@ -13,6 +13,14 @@ class RedisSessionStore:
     the request session. Useful for storing data too large to be stored into
     the session cookie.
 
+    The attributes to be backed by Redis must be declared in a subclass using
+    the `redis_property` function. Do not instantiate RedisSessionStore without
+    extending it to add properties. For example:
+
+    >>> class HotDogSessionStore(RedisSessionStore):
+    >>>     bun = redis_property("bun")
+    >>>     condiment = redis_property("condiment")
+
     NOTE: Assigning attributes immediately saves their value back into the
           redis key assigned for this store. Be aware of the multiple
           round-trips implication of this.
@@ -39,9 +47,9 @@ class RedisSessionStore:
     """
 
     def __init__(self, request, prefix, ttl=EXPIRATION_TTL):
-        self.__dict__["request"] = request
-        self.__dict__["prefix"] = prefix
-        self.__dict__["ttl"] = ttl
+        self.request = request
+        self.prefix = prefix
+        self.ttl = ttl
 
     @property
     def _client(self):
@@ -86,19 +94,25 @@ class RedisSessionStore:
 
         return loads(state_json)
 
-    def __getattr__(self, key):
-        state = self.get_state()
+
+def redis_property(key: str):
+    """Declare a property backed by Redis on a RedisSessionStore class."""
+
+    def getter(store: "RedisSessionStore"):
+        state = store.get_state()
 
         try:
             return state[key] if state else None
         except KeyError as e:
             raise AttributeError(e)
 
-    def __setattr__(self, key, value):
-        state = self.get_state()
+    def setter(store: "RedisSessionStore", value):
+        state = store.get_state()
 
         if state is None:
             return
 
         state[key] = value
-        self._client.setex(self.redis_key, self.ttl, dumps(state))
+        store._client.setex(store.redis_key, store.ttl, dumps(state))
+
+    return property(getter, setter)

tests/sentry/utils/test_session_store.py

@@ -1,55 +1,49 @@
 from unittest import TestCase
 
-from django.http import HttpRequest
+from django.test import Client, RequestFactory
 
-from sentry.utils.session_store import RedisSessionStore
+from sentry.utils.session_store import RedisSessionStore, redis_property
 
 
 class RedisSessionStoreTestCase(TestCase):
-    def test_store_values(self):
-        request = HttpRequest()
-        request.session = {}
+    class TestRedisSessionStore(RedisSessionStore):
+        some_value = redis_property("some_value")
+
+    def setUp(self) -> None:
+        self.request = RequestFactory().get("")
+        self.request.session = Client().session
 
-        store = RedisSessionStore(request, "test-store")
-        store.regenerate()
+        self.store = self.TestRedisSessionStore(self.request, "test-store")
+
+    def test_store_values(self):
+        self.store.regenerate()
 
-        assert "store:test-store" in request.session
+        assert "store:test-store" in self.request.session
 
-        store.some_value = "test_value"
-        store2 = RedisSessionStore(request, "test-store")
+        self.store.some_value = "test_value"
+        store2 = self.TestRedisSessionStore(self.request, "test-store")
 
         assert store2.is_valid()
         assert store2.some_value == "test_value"
 
         with self.assertRaises(AttributeError):
-            store.missing_key
+            self.store.missing_key
 
-        store.clear()
+        self.store.clear()
+        assert self.request.session.modified
 
     def test_store_complex_object(self):
-        request = HttpRequest()
-        request.session = {}
+        self.store.regenerate({"some_value": {"deep_object": "value"}})
 
-        store = RedisSessionStore(request, "test-store")
-        store.regenerate({"some_value": {"deep_object": "value"}})
-
-        store2 = RedisSessionStore(request, "test-store")
+        store2 = self.TestRedisSessionStore(self.request, "test-store")
 
         assert store2.some_value["deep_object"] == "value"
 
-        store.clear()
+        self.store.clear()
 
     def test_uninitialized_store(self):
-        request = HttpRequest()
-        request.session = {}
-
-        store = RedisSessionStore(request, "test-store")
-
-        assert not store.is_valid()
-        assert store.get_state() is None
-        assert store.some_key is None
-
-        store.setting_but_no_state = "anything"
-        assert store.setting_but_no_state is None
+        assert not self.store.is_valid()
+        assert self.store.get_state() is None
+        assert self.store.some_value is None
 
-        store.clear()
+        self.store.clear()