Improve JSON serialization compatibility of User

Previously, the MembershipCache was a subclass of Hash. When serializing
to and deserializing from JSON, the MembershipCache would become a Hash
and blow up. This is a problem in warden-github-rails where I switched
to a custom warden (de)serialization in order to properly support JSON
serialization and also to support mocked users in test environments
between requests[1].

[1]: fphilipe/warden-github-rails@db82470

Author: fphilipe

lib/warden/github/membership_cache.rb

@@ -3,9 +3,13 @@ module GitHub
     # A hash subclass that acts as a cache for organization and team
     # membership states. Only membership states that are true are cached. These
     # are invalidated after a certain time.
-    class MembershipCache < ::Hash
+    class MembershipCache
       CACHE_TIMEOUT = 60 * 5
 
+      def initialize(data)
+        @data = data
+      end
+
       # Fetches a membership status by type and id (e.g. 'org', 'my_company')
       # from cache. If no cached value is present or if the cached value
       # expired, the block will be invoked and the return value, if true,
@@ -27,10 +31,10 @@ def fetch_membership(type, id)
       private
 
       def cached_membership_valid?(type, id)
-        timestamp = fetch(type).fetch(id)
+        timestamp = @data.fetch(type).fetch(id)
 
         if Time.now.to_i > timestamp + CACHE_TIMEOUT
-          fetch(type).delete(id)
+          @data.fetch(type).delete(id)
           false
         else
           true
@@ -40,7 +44,7 @@ def cached_membership_valid?(type, id)
       end
 
       def cache_membership(type, id)
-        hash = self[type] ||= {}
+        hash = @data[type] ||= {}
         hash[id] = Time.now.to_i
       end
     end

lib/warden/github/user.rb

@@ -1,6 +1,6 @@
 module Warden
   module GitHub
-    class User < Struct.new(:attribs, :token, :browser_session_id)
+    class User < Struct.new(:attribs, :token, :browser_session_id, :memberships)
       ATTRIBUTES = %w[id login name gravatar_id avatar_url email company site_admin].freeze
 
       def self.load(access_token, browser_session_id = nil)
@@ -32,7 +32,7 @@ def marshal_load(hash)
       #
       # Returns: true if the user is publicized as an org member
       def organization_public_member?(org_name)
-        memberships.fetch_membership(:org_pub, org_name) do
+        membership_cache.fetch_membership(:org_pub, org_name) do
           api.organization_public_member?(org_name, login)
         end
       end
@@ -46,7 +46,7 @@ def organization_public_member?(org_name)
       #
       # Returns: true if the user has access, false otherwise
       def organization_member?(org_name)
-        memberships.fetch_membership(:org, org_name) do
+        membership_cache.fetch_membership(:org, org_name) do
           api.organization_member?(org_name, login)
         end
       end
@@ -57,7 +57,7 @@ def organization_member?(org_name)
       #
       # Returns: true if the user has access, false otherwise
       def team_member?(team_id)
-        memberships.fetch_membership(:team, team_id) do
+        membership_cache.fetch_membership(:team, team_id) do
           api.team_member?(team_id, login)
         end
       end
@@ -105,8 +105,9 @@ def api
 
       private
 
-      def memberships
-        attribs['member'] ||= MembershipCache.new
+      def membership_cache
+        self.memberships ||= {}
+        @membership_cache ||= MembershipCache.new(memberships)
       end
     end
   end

spec/unit/membership_cache_spec.rb

@@ -1,19 +1,17 @@
 require 'spec_helper'
 
 describe Warden::GitHub::MembershipCache do
-  subject(:cache) do
-    described_class.new
-  end
-
   describe '#fetch_membership' do
     it 'returns false by default' do
+      cache = described_class.new({})
       cache.fetch_membership('foo', 'bar').should be_falsey
     end
 
     context 'when cache valid' do
-      before do
-        cache['foo'] = {}
-        cache['foo']['bar'] = Time.now.to_i - described_class::CACHE_TIMEOUT + 5
+      let(:cache) do
+        described_class.new('foo' => {
+          'bar' => Time.now.to_i - described_class::CACHE_TIMEOUT + 5
+        })
       end
 
       it 'returns true' do
@@ -27,9 +25,10 @@
     end
 
     context 'when cache expired' do
-      before do
-        cache['foo'] = {}
-        cache['foo']['bar'] = Time.now.to_i - described_class::CACHE_TIMEOUT - 5
+      let(:cache) do
+        described_class.new('foo' => {
+          'bar' => Time.now.to_i - described_class::CACHE_TIMEOUT - 5
+        })
       end
 
       context 'when no block given' do
@@ -38,25 +37,47 @@
         end
       end
 
-      it 'deletes the cached value' do
-        cache.fetch_membership('foo', 'bar')
-        cache['foo'].should_not have_key('bar')
-      end
-
       it 'invokes the block' do
         expect { |b| cache.fetch_membership('foo', 'bar', &b) }.
           to yield_control
       end
     end
 
     it 'caches the value when block returns true' do
+      cache = described_class.new({})
       cache.fetch_membership('foo', 'bar') { true }
       cache.fetch_membership('foo', 'bar').should be_truthy
     end
 
     it 'does not cache the value when block returns false' do
+      cache = described_class.new({})
       cache.fetch_membership('foo', 'bar') { false }
       cache.fetch_membership('foo', 'bar').should be_falsey
     end
   end
+
+  it 'uses the hash that is passed to the initializer as storage' do
+    time = Time.now.to_i
+    hash = {
+      'foo' => {
+        'valid' => time,
+        'timedout' => time - described_class::CACHE_TIMEOUT - 5
+      }
+    }
+    cache = described_class.new(hash)
+
+    # Verify that existing data in the hash is used:
+    expect(cache.fetch_membership('foo', 'valid')).to be_true
+    expect(cache.fetch_membership('foo', 'timedout')).to be_false
+
+    # Add new data to the hash:
+    cache.fetch_membership('foo', 'new') { true }
+    cache.fetch_membership('foo', 'false') { false }
+
+    # Verify the final hash state:
+    expect(hash).to eq('foo' => {
+      'valid' => time,
+      'new' => time
+    })
+  end
 end