oauth: http issue 216: DataStore based on String key and Serializable value
also:
AppEngineCredentialStore should use MemcacheService
https://code.google.com/p/google-oauth-java-client/issues/detail?id=47
AppEngineCredentialStore should "un-index" property fields
https://code.google.com/p/google-oauth-java-client/issues/detail?id=31
Depends on:
https://codereview.appspot.com/9029044/
Eyal: careful review of everything
Nick: design review
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client-appengine/src/main/java/com/google/api/client/extensions/appengine/auth/oauth2/AppEngineCredentialStore.java File google-oauth-client-appengine/src/main/java/com/google/api/client/extensions/appengine/auth/oauth2/AppEngineCredentialStore.java (right): https://codereview.appspot.com/9881043/diff/2001/google-oauth-client-appengine/src/main/java/com/google/api/client/extensions/appengine/auth/oauth2/AppEngineCredentialStore.java#newcode91 google-oauth-client-appengine/src/main/java/com/google/api/client/extensions/appengine/auth/oauth2/AppEngineCredentialStore.java:91: public static AppEngineDataStore migrate(AppEngineCredentialStore credentialStore) We should really test ...
10 years, 11 months ago
(2013-05-31 20:22:20 UTC)
#2
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client-appengine/src/main/java/com/google/api/client/extensions/appengine/auth/oauth2/AppEngineCredentialStore.java File google-oauth-client-appengine/src/main/java/com/google/api/client/extensions/appengine/auth/oauth2/AppEngineCredentialStore.java (right): https://codereview.appspot.com/9881043/diff/2001/google-oauth-client-appengine/src/main/java/com/google/api/client/extensions/appengine/auth/oauth2/AppEngineCredentialStore.java#newcode91 google-oauth-client-appengine/src/main/java/com/google/api/client/extensions/appengine/auth/oauth2/AppEngineCredentialStore.java:91: public static AppEngineDataStore migrate(AppEngineCredentialStore credentialStore) On 2013/05/31 20:22:20, ngmiceli ...
10 years, 10 months ago
(2013-06-12 21:17:01 UTC)
#3
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client-appengin...
File
google-oauth-client-appengine/src/main/java/com/google/api/client/extensions/appengine/auth/oauth2/AppEngineCredentialStore.java
(right):
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client-appengin...
google-oauth-client-appengine/src/main/java/com/google/api/client/extensions/appengine/auth/oauth2/AppEngineCredentialStore.java:91:
public static AppEngineDataStore migrate(AppEngineCredentialStore
credentialStore)
On 2013/05/31 20:22:20, ngmiceli wrote:
> We should really test these migration methods.
Done.
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client-java6/sr...
File
google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java
(right):
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client-java6/sr...
google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:178:
public static FileDataStore migrate(FileCredentialStore credentialStore, File
dataDirectory)
On 2013/05/31 20:22:20, ngmiceli wrote:
> ditto re: testing
> :)
Done.
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client-java6/sr...
File
google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java
(right):
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client-java6/sr...
google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java:83:
StoredCredential toStoredCredential() {
On 2013/05/31 20:22:20, ngmiceli wrote:
> Let's include the @since anyway even though its @Deprecated.
not relevant since it's package private
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client/src/main...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java
(right):
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:156:
typedDataStore = builder.typedDataStore;
On 2013/05/31 20:22:20, ngmiceli wrote:
> I think we should make this an exclusive-or. The user should be able to
continue
> to use the credentialStore or upgrade to the new typedDataStore, but they
> shouldn't be able to use both. That way there's no confusion about which
takes
> priority. This should be documented clearly.
> EDIT: I see you're doing this in the Builder, so let's just make sure it's
> clearly documented both on the class level and in the Builder.
Deprecated the CredentialStore usage, so it is now really not XOR, but more of
deprecated vs. not.
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client/src/main...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/CredentialStore.java
(right):
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/CredentialStore.java:38:
* @deprecated (scheduled to be removed in 1.17) Use {@link DataStore} instead.
On 2013/05/31 20:22:20, ngmiceli wrote:
> Please elaborate to let them know they should use the #of method with the
> correct type
Added mention of StoredCredential. I think more likely developers will look at
this in context, e.g. in the JavaDoc of the concrete implementation they are
using.
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client/src/main...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java
(right):
https://codereview.appspot.com/9881043/diff/2001/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java:144:
public static TypedDataStore<StoredCredential> typed(DataStore dataStore) throws
IOException {
On 2013/05/31 20:22:20, ngmiceli wrote:
> [optional] Eyal finds this helper method superfluous. Nick thinks the name is
> awkward. We like to complain.
Changed the name to "getDefaultDataStore", but the util method is pretty helpful
I think.
10 years, 10 months ago
(2013-06-17 16:22:40 UTC)
#6
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client-appengi...
File
google-oauth-client-appengine/src/main/java/com/google/api/client/extensions/appengine/auth/oauth2/AppEngineCredentialStore.java
(right):
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client-appengi...
google-oauth-client-appengine/src/main/java/com/google/api/client/extensions/appengine/auth/oauth2/AppEngineCredentialStore.java:42:
* {@link StoredCredential} instead,
On 2013/06/17 15:18:06, peleyal wrote:
> [OPTIONAL] Maybe sample code will be better here?
The 2 points of context where CredentialStore is used explains what method to
use instead. So it isn't sample code per se, but I think it's pretty close.
For example instead of Credential.Builder.setCredentialStore(new
AppEngineCredentialStore()) you call Credential.Builder.setDataStoreFactory(new
AppEngineDataStoreFactory()). So here the JavaDoc of setCredentialStore should
be explaining this, so it is shared among all of the CredentialStore's.
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client-java6/s...
File
google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java
(right):
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client-java6/s...
google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:43:
* {@link StoredCredential} instead, optionally using
On 2013/06/17 15:18:06, peleyal wrote:
> [OPTIONAL] ditto - code sample?
ditto
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client-java6/s...
File
google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java
(right):
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client-java6/s...
google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java:30:
* @deprecated (scheduled to be removed in 1.17) Use {@link FileDataStoreFactory}
instead.
On 2013/06/17 15:18:06, peleyal wrote:
> Schedule to be removed in 1.17...
>
> 1.15.1 is going to be GA
> maybe it's the right time we should discuss what should be our next version
> number. in my opinion it should be 2.0 or 2.1 and not 1.16, 1.17...
We already discussed the version naming, and you were part of those discussions.
I thought we agreed to keep it as 1.X.
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client-java6/s...
File
google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStoreTest.java
(right):
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client-java6/s...
google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStoreTest.java:193:
File file = createTempFile();
On 2013/06/17 15:18:06, peleyal wrote:
> file.deleteOnExit()?
already done in createTempFile()
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client-servlet...
File
google-oauth-client-servlet/src/main/java/com/google/api/client/extensions/jdo/auth/oauth2/JdoCredentialStore.java
(right):
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client-servlet...
google-oauth-client-servlet/src/main/java/com/google/api/client/extensions/jdo/auth/oauth2/JdoCredentialStore.java:130:
public final void migrateTo(JdoDataStoreFactory dataStore) throws IOException {
On 2013/06/17 15:18:06, peleyal wrote:
> dataStore --> dataStoreFactory
Done.
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client-servlet...
google-oauth-client-servlet/src/main/java/com/google/api/client/extensions/jdo/auth/oauth2/JdoCredentialStore.java:140:
public final void migrateTo(DataStore<StoredCredential> typedDataStore) throws
IOException {
On 2013/06/17 15:18:06, peleyal wrote:
> please rename to credentialDataStore
> typedDataStore -> credentialDataStore
Done.
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client-servlet...
File
google-oauth-client-servlet/src/test/java/com/google/api/client/extensions/jdo/auth/oauth2/JdoCredentialStoreTest.java
(right):
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client-servlet...
google-oauth-client-servlet/src/test/java/com/google/api/client/extensions/jdo/auth/oauth2/JdoCredentialStoreTest.java:98:
public void testMigrateTo() throws Exception {
On 2013/06/17 15:18:06, peleyal wrote:
> I see that this code is copied between the different tests (Jdo, File,
> AppEngine)
>
> Why not create an abstract class (like you did so nicely with
> AbstractDataStoreFactoryTest)?
> with the following abstract methods
>
> createCredentialDataStore()
> and
> CreateFactory()
>
> I know that this and the other classes are deprecated, but anyway I think it
> should be implemented
This is a one-off that will be removed when we delete CredentialStore so I don't
think it is worth investing engineering effort on building a testing
infrastructure around it. Also, the amount of duplicate code is pretty small
and simple.
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java
(right):
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:87:
/** Credential persistence store or {@code null} for none. */
On 2013/06/17 15:18:06, peleyal wrote:
> maybe it is good to add comment here (and below) that credentialStore and
> credentialDataStore can't live together. you have to choose to work with one
of
> them
Done on the setter methods
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:70:
* If you need to persist the access token in a data store, use {@link
DataStoreFactory} and
On 2013/06/17 15:18:06, peleyal wrote:
> [optional] add sample code
Done in DataStoreCredentialRefreshListener.
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java
(right):
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java:41:
public static final String DEFAULT_DATA_STORE_ID = "credentials";
On 2013/06/17 15:18:06, peleyal wrote:
> why don't you use class name here?
because data store ID must be <= 30 chars with no periods.
https://codereview.appspot.com/9881043/diff/24001/pom.xml
File pom.xml (right):
https://codereview.appspot.com/9881043/diff/24001/pom.xml#newcode208
pom.xml:208: <dependency>
On 2013/06/17 15:18:06, peleyal wrote:
> Does that mean that our library has a dependency in mysql and datanucleus now?
> even for app engine or android projects?
No, this is just dependency management which is a central listing of all
projects we use throughout with their version #s. You'd have to look at
dependencies under "<build>" to see *actual* dependencies.
LGTM https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java File google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java (right): https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java#newcode41 google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java:41: public static final String DEFAULT_DATA_STORE_ID = "credentials"; but ...
10 years, 10 months ago
(2013-06-17 20:20:01 UTC)
#7
LGTM
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java
(right):
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java:41:
public static final String DEFAULT_DATA_STORE_ID = "credentials";
but can't you use the class simple\short name?
On 2013/06/17 16:22:40, yanivi wrote:
> On 2013/06/17 15:18:06, peleyal wrote:
> > why don't you use class name here?
>
> because data store ID must be <= 30 chars with no periods.
https://codereview.appspot.com/9881043/diff/24001/pom.xml
File pom.xml (right):
https://codereview.appspot.com/9881043/diff/24001/pom.xml#newcode208
pom.xml:208: <dependency>
OK.
Great to know that :) thanks
On 2013/06/17 16:22:40, yanivi wrote:
> On 2013/06/17 15:18:06, peleyal wrote:
> > Does that mean that our library has a dependency in mysql and datanucleus
now?
> > even for app engine or android projects?
>
> No, this is just dependency management which is a central listing of all
> projects we use throughout with their version #s. You'd have to look at
> dependencies under "<build>" to see *actual* dependencies.
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java File google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java (right): https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java#newcode41 google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java:41: public static final String DEFAULT_DATA_STORE_ID = "credentials"; On 2013/06/17 ...
10 years, 10 months ago
(2013-06-18 15:07:41 UTC)
#8
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java
(right):
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java:41:
public static final String DEFAULT_DATA_STORE_ID = "credentials";
On 2013/06/17 20:20:01, peleyal wrote:
> but can't you use the class simple\short name?
> On 2013/06/17 16:22:40, yanivi wrote:
> > On 2013/06/17 15:18:06, peleyal wrote:
> > > why don't you use class name here?
> >
> > because data store ID must be <= 30 chars with no periods.
>
Done.
10 years, 10 months ago
(2013-06-18 15:36:09 UTC)
#9
On 2013/06/18 15:07:41, yanivi wrote:
>
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/mai...
> File
>
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java
> (right):
>
>
https://codereview.appspot.com/9881043/diff/24001/google-oauth-client/src/mai...
>
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/StoredCredential.java:41:
> public static final String DEFAULT_DATA_STORE_ID = "credentials";
> On 2013/06/17 20:20:01, peleyal wrote:
> > but can't you use the class simple\short name?
> > On 2013/06/17 16:22:40, yanivi wrote:
> > > On 2013/06/17 15:18:06, peleyal wrote:
> > > > why don't you use class name here?
> > >
> > > because data store ID must be <= 30 chars with no periods.
> >
>
> Done.
LGTM 2
On 2013/06/18 16:11:36, yanivi wrote: > For better context: I made the exact same change ...
10 years, 10 months ago
(2013-06-18 21:09:59 UTC)
#12
On 2013/06/18 16:11:36, yanivi wrote:
> For better context: I made the exact same change in StoredChannel in the api
> changeset, so I wanted to make sure they both followed the same practice.
LGTM 3 (final final final)
Issue 9881043: oauth: http issue 216: DataStore based on String key and Serializable value
(Closed)
Created 10 years, 11 months ago by yanivi
Modified 10 years, 10 months ago
Reviewers: peleyal, ngmiceli
Base URL: https://code.google.com/p/google-oauth-java-client/
Comments: 37
