Loading of client_secrets JSON file backed by a cache

Filed as http://code.google.com/p/google-api-python-client/issues/detail?id=164

[jcgregorio_google, 2012-07-11 16:21:42]

http://codereview.appspot.com/6349087/diff/1/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/1/oauth2client/clientsecrets.py#ne...
oauth2client/clientsecrets.py:122: from google.appengine.api import memcache
Anything that depends on App Engine explicitly should go in
oauth2client.appengine, but I don't think moving it there is a good idea.
Instead, pass in a memcache instance via the parameters, that will also make
testing easier as the tests don't need to rely on App Engine, you can just
create a simple mock of the memcache interface.

[alexv, 2012-07-11 18:59:35]

http://codereview.appspot.com/6349087/diff/1/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/1/oauth2client/clientsecrets.py#ne...
oauth2client/clientsecrets.py:122: from google.appengine.api import memcache
On 2012/07/11 16:21:42, jcgregorio_google wrote:
> Anything that depends on App Engine explicitly should go in
> oauth2client.appengine, but I don't think moving it there is a good idea.
> Instead, pass in a memcache instance via the parameters, that will also make
> testing easier as the tests don't need to rely on App Engine, you can just
> create a simple mock of the memcache interface.

Good point! 
Done. At least, this is how I imagine it.

[alexv, 2012-07-12 10:52:25]

Actually, NDB doesn't pick up RPCs that were not originally registered with its
autobatcher. Plus, in this specific case I don't think async is really worth it.

So, here's a simplified version, w/o async.

[jcgregorio_google, 2012-07-12 12:55:01]

http://codereview.appspot.com/6349087/diff/2002/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/2002/oauth2client/clientsecrets.py...
oauth2client/clientsecrets.py:122: cache: a cache service client that implements
get() and set() methods.
missing 'filename'

http://codereview.appspot.com/6349087/diff/2002/oauth2client/clientsecrets.py...
oauth2client/clientsecrets.py:141: contents = { _CLIENT_TYPE: client_type,
_CLIENT_INFO: client_info }
Why not use the same format that's in the client_secrets.json file? I.e.

   contents = { client_type: client_info }

It is also easy to pull apart:

   client_type, client_info = obj.iteritems().next()

[alexv, 2012-07-12 14:00:41]

Thanks!

http://codereview.appspot.com/6349087/diff/2002/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/2002/oauth2client/clientsecrets.py...
oauth2client/clientsecrets.py:122: cache: a cache service client that implements
get() and set() methods.
On 2012/07/12 12:55:01, jcgregorio_google wrote:
> missing 'filename'

Done.

http://codereview.appspot.com/6349087/diff/2002/oauth2client/clientsecrets.py...
oauth2client/clientsecrets.py:141: contents = { _CLIENT_TYPE: client_type,
_CLIENT_INFO: client_info }
On 2012/07/12 12:55:01, jcgregorio_google wrote:
> Why not use the same format that's in the client_secrets.json file? I.e.
> 
>    contents = { client_type: client_info }
> 
> It is also easy to pull apart:
> 
>    client_type, client_info = obj.iteritems().next()

Done. Overthinking from the prev. implementation.

[jcgregorio_google, 2012-07-12 15:37:24]

http://codereview.appspot.com/6349087/diff/7002/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/7002/oauth2client/clientsecrets.py...
oauth2client/clientsecrets.py:111: Tipical cache storage would be App Engine
memcache service,
Typical

http://codereview.appspot.com/6349087/diff/7002/oauth2client/clientsecrets.py...
oauth2client/clientsecrets.py:122: filename: path to a client_secrets.json file
on a filesystem
filename: string, Path to

In general the pattern is:

parameter-name: type, Complete sentence describing parameter.

http://codereview.appspot.com/6349087/diff/7002/tests/test_oauth2client_clien...
File tests/test_oauth2client_clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/7002/tests/test_oauth2client_clien...
tests/test_oauth2client_clientsecrets.py:83: self.cache = {}
self.last_get_ns = None
self.last_set_ns = None

[alexv, 2012-07-12 18:37:10]

http://codereview.appspot.com/6349087/diff/7002/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/7002/oauth2client/clientsecrets.py...
oauth2client/clientsecrets.py:111: Tipical cache storage would be App Engine
memcache service,
On 2012/07/12 15:37:24, jcgregorio_google wrote:
> Typical

Done.

http://codereview.appspot.com/6349087/diff/7002/oauth2client/clientsecrets.py...
oauth2client/clientsecrets.py:122: filename: path to a client_secrets.json file
on a filesystem
On 2012/07/12 15:37:24, jcgregorio_google wrote:
> filename: string, Path to
> 
> In general the pattern is:
> 
> parameter-name: type, Complete sentence describing parameter.

Done.

http://codereview.appspot.com/6349087/diff/7002/tests/test_oauth2client_clien...
File tests/test_oauth2client_clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/7002/tests/test_oauth2client_clien...
tests/test_oauth2client_clientsecrets.py:83: self.cache = {}
On 2012/07/12 15:37:24, jcgregorio_google wrote:
> self.last_get_ns = None
> self.last_set_ns = None

Done.

http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.p...
oauth2client/clientsecrets.py:108: def load_cached(filename, cache):
Hey, I was thinking to change the signature to

  def load_cached(filename, cache=None)

Where cache=None just falls back to loadfile(filename). This seems to follow
other libs (e.g. httplib2) closer. Plus, that way other pieces of oauth2client
could be alternated (where needed for e.g. App Engine) and be totally backwards
compatible, for instance:

  flow_from_clientsecrets(filename, scope, message=None, cache=None)

or, OAuth2DecoratorFromClientSecrets:
  def __init__(self, filename, scope, message=None, cache=None):

or, would it be better for the above __init__() code to just load_cached()
instead of (currenty) loadfile().

What do you think?

[jcgregorio_google, 2012-07-13 13:39:40]

http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.p...
oauth2client/clientsecrets.py:108: def load_cached(filename, cache):
Sounds good to me, rename this function to loadfile, rename the original
loadfile to _loadfile. You can also add the optional 'cache' arguments to all
the *FromClientSecrets classes and methods if you feel motivated :)

On 2012/07/12 18:37:10, crhyme wrote:
> Hey, I was thinking to change the signature to
> 
>   def load_cached(filename, cache=None)
> 
> Where cache=None just falls back to loadfile(filename). This seems to follow
> other libs (e.g. httplib2) closer. Plus, that way other pieces of oauth2client
> could be alternated (where needed for e.g. App Engine) and be totally
backwards
> compatible, for instance:
> 
>   flow_from_clientsecrets(filename, scope, message=None, cache=None)
> 
> or, OAuth2DecoratorFromClientSecrets:
>   def __init__(self, filename, scope, message=None, cache=None):
> 
> or, would it be better for the above __init__() code to just load_cached()
> instead of (currenty) loadfile().
> 
> What do you think?

[alexv, 2012-07-13 14:50:51]

The optional 'cache' param change for *FromClientSecrets is trivial but I can
add some test cases for client.py and appengine.py if needed.

http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.p...
oauth2client/clientsecrets.py:108: def load_cached(filename, cache):
On 2012/07/13 13:39:40, jcgregorio_google wrote:
> Sounds good to me, rename this function to loadfile, rename the original
> loadfile to _loadfile. You can also add the optional 'cache' arguments to all
> the *FromClientSecrets classes and methods if you feel motivated :)
> 
> On 2012/07/12 18:37:10, crhyme wrote:
> > Hey, I was thinking to change the signature to
> > 
> >   def load_cached(filename, cache=None)
> > 
> > Where cache=None just falls back to loadfile(filename). This seems to follow
> > other libs (e.g. httplib2) closer. Plus, that way other pieces of
oauth2client
> > could be alternated (where needed for e.g. App Engine) and be totally
> backwards
> > compatible, for instance:
> > 
> >   flow_from_clientsecrets(filename, scope, message=None, cache=None)
> > 
> > or, OAuth2DecoratorFromClientSecrets:
> >   def __init__(self, filename, scope, message=None, cache=None):
> > 
> > or, would it be better for the above __init__() code to just load_cached()
> > instead of (currenty) loadfile().
> > 
> > What do you think?
> 

Done.

[jcgregorio_google, 2012-07-13 15:24:35]

On 2012/07/13 14:50:51, crhyme wrote:
> The optional 'cache' param change for *FromClientSecrets is trivial but I can
> add some test cases for client.py and appengine.py if needed.

Yes, please do.

> 
> http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.py
> File oauth2client/clientsecrets.py (right):
> 
>
http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.p...
> oauth2client/clientsecrets.py:108: def load_cached(filename, cache):
> On 2012/07/13 13:39:40, jcgregorio_google wrote:
> > Sounds good to me, rename this function to loadfile, rename the original
> > loadfile to _loadfile. You can also add the optional 'cache' arguments to
all
> > the *FromClientSecrets classes and methods if you feel motivated :)
> > 
> > On 2012/07/12 18:37:10, crhyme wrote:
> > > Hey, I was thinking to change the signature to
> > > 
> > >   def load_cached(filename, cache=None)
> > > 
> > > Where cache=None just falls back to loadfile(filename). This seems to
follow
> > > other libs (e.g. httplib2) closer. Plus, that way other pieces of
> oauth2client
> > > could be alternated (where needed for e.g. App Engine) and be totally
> > backwards
> > > compatible, for instance:
> > > 
> > >   flow_from_clientsecrets(filename, scope, message=None, cache=None)
> > > 
> > > or, OAuth2DecoratorFromClientSecrets:
> > >   def __init__(self, filename, scope, message=None, cache=None):
> > > 
> > > or, would it be better for the above __init__() code to just load_cached()
> > > instead of (currenty) loadfile().
> > > 
> > > What do you think?
> > 
> 
> Done.

[jcgregorio_google, 2012-07-13 15:24:40]

http://codereview.appspot.com/6349087/diff/11002/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/6349087/diff/11002/oauth2client/appengine.py#ne...
oauth2client/appengine.py:457: cache: an optional cache service client that
implements get() and set()
An

http://codereview.appspot.com/6349087/diff/11002/tests/test_oauth2client_clie...
File tests/test_oauth2client_clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/11002/tests/test_oauth2client_clie...
tests/test_oauth2client_clientsecrets.py:73: 'afilethatisntthere.json'))
Looks like there are multiple tests that need a filename guaranteed not to
exist. Break out as a module level constant.

[alexv, 2012-07-13 17:47:12]

Hopefully I got this right.

http://codereview.appspot.com/6349087/diff/11002/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/6349087/diff/11002/oauth2client/appengine.py#ne...
oauth2client/appengine.py:457: cache: an optional cache service client that
implements get() and set()
On 2012/07/13 15:24:40, jcgregorio_google wrote:
> An

Done.

http://codereview.appspot.com/6349087/diff/11002/tests/test_oauth2client_clie...
File tests/test_oauth2client_clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/11002/tests/test_oauth2client_clie...
tests/test_oauth2client_clientsecrets.py:73: 'afilethatisntthere.json'))
On 2012/07/13 15:24:40, jcgregorio_google wrote:
> Looks like there are multiple tests that need a filename guaranteed not to
> exist. Break out as a module level constant.

Done.

[jcgregorio_google, 2012-07-16 16:00:14]

http://codereview.appspot.com/6349087/diff/5011/tests/test_oauth2client_appen...
File tests/test_oauth2client_appengine.py (right):

http://codereview.appspot.com/6349087/diff/5011/tests/test_oauth2client_appen...
tests/test_oauth2client_appengine.py:78: 
two blank lines

[alexv, 2012-07-16 16:10:55]

http://codereview.appspot.com/6349087/diff/5011/tests/test_oauth2client_appen...
File tests/test_oauth2client_appengine.py (right):

http://codereview.appspot.com/6349087/diff/5011/tests/test_oauth2client_appen...
tests/test_oauth2client_appengine.py:78: 
On 2012/07/16 16:00:14, jcgregorio_google wrote:
> two blank lines

Done.

[jcgregorio_google, 2012-07-16 16:17:15]

http://codereview.appspot.com/6349087/diff/17001/tests/test_oauth2client_clie...
File tests/test_oauth2client_clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/17001/tests/test_oauth2client_clie...
tests/test_oauth2client_clientsecrets.py:127:
self.assertIsNone(self.cache_mock.last_set_ns)
assertIsNone only works on Python 2.7, please use assertNotEqual.

[alexv, 2012-07-16 16:52:50]

Sorry, forgot about < 2.7

http://codereview.appspot.com/6349087/diff/17001/tests/test_oauth2client_clie...
File tests/test_oauth2client_clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/17001/tests/test_oauth2client_clie...
tests/test_oauth2client_clientsecrets.py:127:
self.assertIsNone(self.cache_mock.last_set_ns)
On 2012/07/16 16:17:15, jcgregorio_google wrote:
> assertIsNone only works on Python 2.7, please use assertNotEqual.

Done.

[jcgregorio_google, 2012-07-16 20:17:42]

LGTM.

Committed at
http://code.google.com/p/google-api-python-client/source/detail?r=9e3a9336962...

On 2012/07/16 16:52:50, crhyme wrote:
> Sorry, forgot about < 2.7
> 
>
http://codereview.appspot.com/6349087/diff/17001/tests/test_oauth2client_clie...
> File tests/test_oauth2client_clientsecrets.py (right):
> 
>
http://codereview.appspot.com/6349087/diff/17001/tests/test_oauth2client_clie...
> tests/test_oauth2client_clientsecrets.py:127:
> self.assertIsNone(self.cache_mock.last_set_ns)
> On 2012/07/16 16:17:15, jcgregorio_google wrote:
> > assertIsNone only works on Python 2.7, please use assertNotEqual.
> 
> Done.