|
|
|
Add appengineutil package with OAuth helpers for App Engine
Patch Set 1 #Patch Set 2 : . #
Total comments: 12
Total messages: 2
|
adg
https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineutil.go File oauth/appengineutil/appengineutil.go (right): https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineutil.go#newcode147 oauth/appengineutil/appengineutil.go:147: type datastoreCache struct { You should export this so ...
|
13 years ago (2013年01月08日 00:50:56 UTC) #1 | ||||||||||||||||||||||||||||||
https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... File oauth/appengineutil/appengineutil.go (right): https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... oauth/appengineutil/appengineutil.go:147: type datastoreCache struct { You should export this so that it can be used independently of the other stuff you have here. https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... oauth/appengineutil/appengineutil.go:148: c appengine.Context Export these fields as Context and User https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... oauth/appengineutil/appengineutil.go:150: } add a Kind field, so the user can specify a custom datastore kind you could make a blank kind default to "oauth.Token" https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... oauth/appengineutil/appengineutil.go:157: item, e := memcache.Get(d.c, d.u.ID) memcache keys should have some kind of prefix (perhaps the Kind?), to avoid stepping on other cache keys https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... oauth/appengineutil/appengineutil.go:173: func (d datastoreCache) PutToken(t *oauth.Token) (err error) { don't bother with the named return var here, it just makes the function more tricky https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... oauth/appengineutil/appengineutil.go:177: return return err https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... oauth/appengineutil/appengineutil.go:181: encoded, e := json.Marshal(t) s/e/err/ https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... oauth/appengineutil/appengineutil.go:182: if e == nil { if err != nil { return err } regularity trumps concise code https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... oauth/appengineutil/appengineutil.go:183: memcache.Set(d.c, &memcache.Item{ check the error return and log the error, if any https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... oauth/appengineutil/appengineutil.go:188: return return nil
https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... File oauth/appengineutil/appengineutil.go (right): https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... oauth/appengineutil/appengineutil.go:83: config.RedirectURL = getRedirectURL(c) I think you should export getRedirectURL too, and don't mutate config inside MustOAuth and NewOAuthCallbackHandlerFunc https://codereview.appspot.com/6998048/diff/5/oauth/appengineutil/appengineut... oauth/appengineutil/appengineutil.go:118: // Make a local copy of the config. I don't think this is necessary. We can probably rely on the OAuth config being static.