using cache with AppEngine StorageByKeyName prevents new access_tokens from being stored

[GoogleCodeExporter]

Perhaps I'm missing something, but it looks like when you set the cache 
parameter for StorageByKeyName (in appengine.py) then updated access_tokens 
(acquired when current one expires) are not saved to the store.

The problem is in StorageByKeyName's locked_get() method.

  def locked_get(self):
    """Retrieve Credential from datastore.

    Returns:
      oauth2client.Credentials
    """
    if self._cache:
      json = self._cache.get(self._key_name)
      if json:
        return Credentials.new_from_json(json)

    credentials = None
    entity = self._get_entity()
    if entity is not None:
      credentials = getattr(entity, self._property_name)
      if credentials and hasattr(credentials, 'set_store'):
        credentials.set_store(self)
        if self._cache:
          self._cache.set(self._key_name, credentials.to_json())

    return credentials

Notice is self._cache, then credentials are created from json and returned, and 
credentials.set_store() is never called!

So later when the credentials are used, if the access_token has expired, then 
OAuth2Credentials._do_refresh_request() is called to get a new access_token.  
Since there is not store on these credentials though, store.locked_put() is 
never called, so the new access_token is NOT put in the store and the cache is 
NOT updated.  

Consequently, the original access token in the store is always used and always 
expired from that point on and a new access_token is acquired with each request.

To fix, it seems like set_store() just needs to be called on the credentials 
even if they are pulled from the cache.

Original issue reported on code.google.com by henne...@gmail.com on 1 Aug 2013 at 8:31

[GoogleCodeExporter]

"Notice is self._cache, then credentials are created from json and returned, 
and credentials.set_store() is never called!"

Correct, because the _cache is just a cache of what's already in the store, 
credential.set_store() should not be called there.

If the credential retrieved is invalid or expired then the following code will 
request a new access token and update the credential in the store:

http://code.google.com/p/google-api-python-client/source/browse/oauth2client/cli
ent.py#638

Original comment by jcgregorio@google.com on 5 Aug 2013 at 1:26

• Changed state: Invalid

[GoogleCodeExporter]

This is a valid bug.

You are correct that if the credential is expired, a new access token is 
retrieved, however it is NOT put in the store, because there is no store set on 
the credentials, so that step is skipped.

Start looking at the code in line 638 that you mention.  Follow it down to line 
652 - there is no store on the credential, so it calls do_refresh_request()

https://code.google.com/p/google-api-python-client/source/browse/oauth2client/cl
ient.py#652

In do_refresh_request it does get a new access token, but if you look at line 
694, there still is no store with the credentials, so the store is not updated

https://code.google.com/p/google-api-python-client/source/browse/oauth2client/cl
ient.py#694

This is very easy to test this out and verify that the store does NOT get 
updated when a cache is set after the initial token expires.

Here is the code to verify this bug:

# get credentials with cache set and log the access token
storage = StorageByKeyName(CredentialsModel, credKey, 'credentials', 
cache=memcache)
credentials = storage.get()
logging.debug("credentials.access_token: %s" % credentials.access_token)
#credentials.access_token = None

# do something with the credentials
http = httplib2.Http(cache=memcache)
http = credentials.authorize(http)
service = build('calendar', 'v3', http=http)
calendar = service.calendars().get(calendarId='primary').execute()

Run the above code once to get an initial access token in the cache.  Then 
uncomment the line credentials.access_token = None, which will force the 
credentials to not work and be refreshed (per line 638 as you mention).

Run the code several more times.  You will notice that the access_token stored 
in cache is the SAME token from the original call.  Every time, it's always the 
same!  The new access token that is acquired is not put in the store and not 
put in the cache because of the bug I outlined above.

The result of this bug means that after the initial access_token expires, then 
a refresh request will be made with EVERY call.  This is an extra, unnecessary 
http request which makes everything take longer.

The fix is quite simple.  I have already patched my local copy.

  def locked_get(self):
    """Retrieve Credential from datastore.

    Returns:
      oauth2client.Credentials
    """
    if self._cache:
      json = self._cache.get(self._key_name)
      if json:
        credentials = Credentials.new_from_json(json)
        if credentials and hasattr(credentials, 'set_store'):
          credentials.set_store(self)
        return credentials

    credentials = None
    entity = self._get_entity()
    if entity is not None:
      credentials = getattr(entity, self._property_name)
      if credentials and hasattr(credentials, 'set_store'):
        credentials.set_store(self)
        if self._cache:
          self._cache.set(self._key_name, credentials.to_json())

    return credentials

Original comment by henne...@gmail.com on 5 Aug 2013 at 2:45

[GoogleCodeExporter]

Original comment by jcgregorio@google.com on 6 Aug 2013 at 12:53

• Changed state: Accepted

[GoogleCodeExporter]

This issue was closed by revision e03f352e165c.

Original comment by jcgregorio@google.com on 6 Aug 2013 at 3:31

• Changed state: Fixed