|
|
Created:
11 years, 10 months ago by rafael.naufal Modified:
11 years, 8 months ago Base URL:
https://google-oauth-java-client.googlecode.com/hg/ Visibility:
Public. |
Description[oauth] FileCredentialStore implementation of CredentialStore interface
Patch Set 1 #
Total comments: 21
Patch Set 2 : Applying comments from reviewers [yanivi, rmistry] #Patch Set 3 : Applying comments from reviewers [yanivi, rmistry] #
Total comments: 2
Patch Set 4 : Creating new project google-oauth-client-java6 #
Total comments: 2
Patch Set 5 : Changing the changeset to 1.11 #Patch Set 6 : Changing the changeset to 1.11 #
Total comments: 15
Patch Set 7 : Applying latest comments from [rmistry] #
Total comments: 43
Patch Set 8 : Applying latest comments from [yanivi] and adding dependencies.html #
Total comments: 19
Patch Set 9 : Latest comments from [yanivi] #Patch Set 10 : Latest comments from [yanivi] #
Total comments: 2
MessagesTotal messages: 39
Sign in to reply to this message.
Thanks so much for sending this for review. We really appreciate your contribution. Here are my comments. http://codereview.appspot.com/6242068/diff/1/google-oauth-client-appengine/.c... File google-oauth-client-appengine/.classpath (right): http://codereview.appspot.com/6242068/diff/1/google-oauth-client-appengine/.c... google-oauth-client-appengine/.classpath:1: <classpath> If you don't mind, please don't include the changes to the non-java files in this changeset. We'd prefer to keep using the Maven classpath container for now. http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... File google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:48: public FileCredentialStore(File file) throws IOException { please add a JsonFactory parameter to the constructor so the user may choose an alternative JsonFactory http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:48: public FileCredentialStore(File file) throws IOException { please add JavaDoc that explains the parameters http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:53: Preconditions.checkState(this.file.createNewFile(), I will need to get this reviewed by an official security expert at Google, but for now can you please chmod 600 on the file to reduce the chance of another user accessing this file? For example: Runtime.getRuntime().exec(new String[] {"chmod", "600", file.getAbsolutePath()}); http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:55: } else load the persisted credentials from the file http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:63: generator = JACKSON_FACTORY.createJsonGenerator(Files.newWriter( please call createJsonGenerator(new FileOutputStream(file), JsonEncoding.UTF8) http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:67: throw new RuntimeException("Cannot store credentials on file: ", e); Ouch. This is probably a bug that we don't allow throws IOException on store. I opened a request for that: http://code.google.com/p/google-oauth-java-client/issues/detail?id=40 http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... File google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FilePersistedCredential.java (right): http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FilePersistedCredential.java:26: public class FilePersistedCredential extends GenericJson { can you please make FilePersistedCredential and FilePersistedCredentials package private? http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... File google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FilePersistedCredentials.java (right): http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FilePersistedCredentials.java:59: * user ID whose credential needs to be stored loaded
Sign in to reply to this message.
Thanks, yanivi. I'll apply your comments and submit my corrections ASAP. On 2012/05/30 18:46:18, yanivi wrote: > Thanks so much for sending this for review. We really appreciate your > contribution. Here are my comments. > > http://codereview.appspot.com/6242068/diff/1/google-oauth-client-appengine/.c... > File google-oauth-client-appengine/.classpath (right): > > http://codereview.appspot.com/6242068/diff/1/google-oauth-client-appengine/.c... > google-oauth-client-appengine/.classpath:1: <classpath> > If you don't mind, please don't include the changes to the non-java files in > this changeset. We'd prefer to keep using the Maven classpath container for > now. > > http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... > File > google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java > (right): > > http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... > google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:48: > public FileCredentialStore(File file) throws IOException { > please add a JsonFactory parameter to the constructor so the user may choose an > alternative JsonFactory > > http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... > google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:48: > public FileCredentialStore(File file) throws IOException { > please add JavaDoc that explains the parameters > > http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... > google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:53: > Preconditions.checkState(this.file.createNewFile(), > I will need to get this reviewed by an official security expert at Google, but > for now can you please chmod 600 on the file to reduce the chance of another > user accessing this file? > > For example: > > Runtime.getRuntime().exec(new String[] {"chmod", "600", > file.getAbsolutePath()}); > > http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... > google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:55: > } > else load the persisted credentials from the file > > http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... > google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:63: > generator = JACKSON_FACTORY.createJsonGenerator(Files.newWriter( > please call createJsonGenerator(new FileOutputStream(file), JsonEncoding.UTF8) > > http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... > google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:67: > throw new RuntimeException("Cannot store credentials on file: ", e); > Ouch. This is probably a bug that we don't allow throws IOException on store. > I opened a request for that: > > http://code.google.com/p/google-oauth-java-client/issues/detail?id=40 > > http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... > File > google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FilePersistedCredential.java > (right): > > http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... > google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FilePersistedCredential.java:26: > public class FilePersistedCredential extends GenericJson { > can you please make FilePersistedCredential and FilePersistedCredentials package > private? > > http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... > File > google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FilePersistedCredentials.java > (right): > > http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... > google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FilePersistedCredentials.java:59: > * user ID whose credential needs to be stored > loaded
Sign in to reply to this message.
http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... File google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:67: throw new RuntimeException("Cannot store credentials on file: ", e); On 2012/05/30 18:46:18, yanivi wrote: > Ouch. This is probably a bug that we don't allow throws IOException on store. > I opened a request for that: > > http://code.google.com/p/google-oauth-java-client/issues/detail?id=40 The bug has been fixed: http://code.google.com/p/google-oauth-java-client/issues/detail?id=40 If you sync to head you will no longer need to catch IOExceptions.
Sign in to reply to this message.
http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... File google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:35: public class FileCredentialStore implements CredentialStore { please add unit tests for this class http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:55: } On 2012/05/30 18:46:18, yanivi wrote: > else load the persisted credentials from the file may be able to use something like: credentials = JsonFactory.fromInputStream(new FileInputStream(file), FilePersistedCredentials.class);
Sign in to reply to this message.
Sign in to reply to this message.
Sign in to reply to this message.
http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... File google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:35: public class FileCredentialStore implements CredentialStore { On 2012/05/31 12:54:14, yanivi wrote: > please add unit tests for this class Done. http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:35: public class FileCredentialStore implements CredentialStore { On 2012/05/31 12:54:14, yanivi wrote: > please add unit tests for this class Done. http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:48: public FileCredentialStore(File file) throws IOException { On 2012/05/30 18:46:18, yanivi wrote: > please add a JsonFactory parameter to the constructor so the user may choose an > alternative JsonFactory Done. http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:53: Preconditions.checkState(this.file.createNewFile(), On 2012/05/30 18:46:18, yanivi wrote: > I will need to get this reviewed by an official security expert at Google, but > for now can you please chmod 600 on the file to reduce the chance of another > user accessing this file? > > For example: > > Runtime.getRuntime().exec(new String[] {"chmod", "600", > file.getAbsolutePath()}); Done. http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:55: } On 2012/05/30 18:46:18, yanivi wrote: > else load the persisted credentials from the file Done. http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:63: generator = JACKSON_FACTORY.createJsonGenerator(Files.newWriter( On 2012/05/30 18:46:18, yanivi wrote: > please call createJsonGenerator(new FileOutputStream(file), JsonEncoding.UTF8) Done. http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:67: throw new RuntimeException("Cannot store credentials on file: ", e); On 2012/05/30 18:46:18, yanivi wrote: > Ouch. This is probably a bug that we don't allow throws IOException on store. > I opened a request for that: > > http://code.google.com/p/google-oauth-java-client/issues/detail?id=40 Done. http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... File google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FilePersistedCredential.java (right): http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FilePersistedCredential.java:26: public class FilePersistedCredential extends GenericJson { I tried, but the unit tests need them public to instantiate them On 2012/05/30 18:46:18, yanivi wrote: > can you please make FilePersistedCredential and FilePersistedCredentials package > private? http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... File google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FilePersistedCredentials.java (right): http://codereview.appspot.com/6242068/diff/1/google-oauth-client/src/main/jav... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FilePersistedCredentials.java:59: * user ID whose credential needs to be stored On 2012/05/30 18:46:18, yanivi wrote: > loaded Done.
Sign in to reply to this message.
Sign in to reply to this message.
http://codereview.appspot.com/6242068/diff/9002/google-oauth-client/src/main/... File google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/9002/google-oauth-client/src/main/... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:68: .exec(new String[] { "chmod", "600", chmod won't work under Windows, is there a cross-platform way to accomplish this?
Sign in to reply to this message.
http://codereview.appspot.com/6242068/diff/9002/google-oauth-client/src/main/... File google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/9002/google-oauth-client/src/main/... google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java:68: .exec(new String[] { "chmod", "600", On 2012/06/04 13:11:53, jcgregorio_google wrote: > chmod won't work under Windows, is there a cross-platform way to accomplish > this? Good point. You can do: File.setReadable/Writable(false, false) File.setReadable/Writable(true, true) File.setExecutable(false, false) catch is that we can only do that in Java 6, whereas this artifact/jar google-oauth-client is supposed to be Java 5 compliant. So: I propose we create a new artifact/jar called google-oauth-client-java6 and move FileCredentialStore into there. I've reviewed this proposal with Ravi and he agrees with it. Sorry it would be more work for you, but strategically we are trying to drop support for Java 6, except for Android applications.
Sign in to reply to this message.
So I have to create another artifact/jar called google-oauth-client-java6 and move FileCredentialStore into it? Do I have to create another issue or it can be the same? On Mon, Jun 4, 2012 at 10:54 AM, <yanivi@google.com> wrote: > > http://codereview.appspot.com/**6242068/diff/9002/google-** > oauth-client/src/main/java/**com/google/api/client/auth/** > oauth2/FileCredentialStore.**java<http://codereview.appspot.com/6242068/diff/9002/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java> > File > google-oauth-client/src/main/**java/com/google/api/client/**auth/oauth2/** > FileCredentialStore.java > (right): > > http://codereview.appspot.com/**6242068/diff/9002/google-** > oauth-client/src/main/java/**com/google/api/client/auth/** > oauth2/FileCredentialStore.**java#newcode68<http://codereview.appspot.com/6242068/diff/9002/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java#newcode68> > google-oauth-client/src/main/**java/com/google/api/client/**auth/oauth2/** > FileCredentialStore.java:68: > .exec(new String[] { "chmod", "600", > On 2012/06/04 13:11:53, jcgregorio_google wrote: > >> chmod won't work under Windows, is there a cross-platform way to >> > accomplish > >> this? >> > > Good point. You can do: > File.setReadable/Writable(**false, false) > File.setReadable/Writable(**true, true) > File.setExecutable(false, false) > > catch is that we can only do that in Java 6, whereas this artifact/jar > google-oauth-client is supposed to be Java 5 compliant. > > So: I propose we create a new artifact/jar called > google-oauth-client-java6 and move FileCredentialStore into there. > > I've reviewed this proposal with Ravi and he agrees with it. Sorry it > would be more work for you, but strategically we are trying to drop > support for Java 6, except for Android applications. > > http://codereview.appspot.com/**6242068/<http://codereview.appspot.com/6242068/> > -- ------------------------------------------------------------------------ Rafael Naufal "A great pleasure in life is doing what people say you cannot do" -Walter Gagehot http://rafaelnaufal.com Twitter: http://twitter.com/rnaufal ------------------------------------------------------------------------
Sign in to reply to this message.
On 2012/06/04 15:00:26, rafael.naufal wrote: > So I have to create another artifact/jar called google-oauth-client-java6 > and move FileCredentialStore into it? Yes. Please follow the approach taken for google-oauth-client-appengine. Sorry to make you do this, but I think it's going to be the only way to make this happen realistically. > Do I have to create another issue or > it can be the same? No, feel free to reuse the same issue. Thanks again for your effort to make this happen! -- Yaniv > > On Mon, Jun 4, 2012 at 10:54 AM, <mailto:yanivi@google.com> wrote: > > > > > http://codereview.appspot.com/**6242068/diff/9002/google-** > > oauth-client/src/main/java/**com/google/api/client/auth/** > > > oauth2/FileCredentialStore.**java<http://codereview.appspot.com/6242068/diff/9002/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java> > > File > > google-oauth-client/src/main/**java/com/google/api/client/**auth/oauth2/** > > FileCredentialStore.java > > (right): > > > > http://codereview.appspot.com/**6242068/diff/9002/google-** > > oauth-client/src/main/java/**com/google/api/client/auth/** > > > oauth2/FileCredentialStore.**java#newcode68<http://codereview.appspot.com/6242068/diff/9002/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java#newcode68> > > google-oauth-client/src/main/**java/com/google/api/client/**auth/oauth2/** > > FileCredentialStore.java:68: > > .exec(new String[] { "chmod", "600", > > On 2012/06/04 13:11:53, jcgregorio_google wrote: > > > >> chmod won't work under Windows, is there a cross-platform way to > >> > > accomplish > > > >> this? > >> > > > > Good point. You can do: > > File.setReadable/Writable(**false, false) > > File.setReadable/Writable(**true, true) > > File.setExecutable(false, false) > > > > catch is that we can only do that in Java 6, whereas this artifact/jar > > google-oauth-client is supposed to be Java 5 compliant. > > > > So: I propose we create a new artifact/jar called > > google-oauth-client-java6 and move FileCredentialStore into there. > > > > I've reviewed this proposal with Ravi and he agrees with it. Sorry it > > would be more work for you, but strategically we are trying to drop > > support for Java 6, except for Android applications. > > > > > http://codereview.appspot.com/**6242068/%3Chttp://codereview.appspot.com/6242...> > > > > > > -- > ------------------------------------------------------------------------ > Rafael Naufal > "A great pleasure in life is doing what people say you cannot do" -Walter > Gagehot > http://rafaelnaufal.com > Twitter: http://twitter.com/rnaufal > ------------------------------------------------------------------------
Sign in to reply to this message.
Ok, just one doubt: if the file doesn't exist (the else clause) on the constructor, do I have to call File.setReadable(false, false) and File.setWritable(false, false)? Thanks, Rafael On Mon, Jun 4, 2012 at 12:32 PM, <yanivi@google.com> wrote: > On 2012/06/04 15:00:26, rafael.naufal wrote: > >> So I have to create another artifact/jar called >> > google-oauth-client-java6 > >> and move FileCredentialStore into it? >> > > Yes. Please follow the approach taken for > google-oauth-client-appengine. Sorry to make you do this, but I think > it's going to be the only way to make this happen realistically. > > > Do I have to create another issue or >> it can be the same? >> > > No, feel free to reuse the same issue. > > Thanks again for your effort to make this happen! > > -- Yaniv > > > On Mon, Jun 4, 2012 at 10:54 AM, <mailto:yanivi@google.com> wrote: >> > > > >> > http://codereview.appspot.com/****6242068/diff/9002/google-**<http://coderevi... >> > oauth-client/src/main/java/****com/google/api/client/auth/** >> > >> > > oauth2/FileCredentialStore.****java<http://codereview.** > appspot.com/6242068/diff/9002/**google-oauth-client/src/main/** > java/com/google/api/client/**auth/oauth2/**FileCredentialStore.java<http://codereview.appspot.com/6242068/diff/9002/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java> > > > >> > File >> > >> > google-oauth-client/src/main/****java/com/google/api/client/**** > auth/oauth2/** > >> > FileCredentialStore.java >> > (right): >> > >> > http://codereview.appspot.com/****6242068/diff/9002/google-**<http://coderevi... >> > oauth-client/src/main/java/****com/google/api/client/auth/** >> > >> > > oauth2/FileCredentialStore.****java#newcode68<http://** > codereview.appspot.com/**6242068/diff/9002/google-** > oauth-client/src/main/java/**com/google/api/client/auth/** > oauth2/FileCredentialStore.**java#newcode68<http://codereview.appspot.com/6242068/diff/9002/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/FileCredentialStore.java#newcode68> > > > >> > >> > google-oauth-client/src/main/****java/com/google/api/client/**** > auth/oauth2/** > >> > FileCredentialStore.java:68: >> > .exec(new String[] { "chmod", "600", >> > On 2012/06/04 13:11:53, jcgregorio_google wrote: >> > >> >> chmod won't work under Windows, is there a cross-platform way to >> >> >> > accomplish >> > >> >> this? >> >> >> > >> > Good point. You can do: >> > File.setReadable/Writable(****false, false) >> > File.setReadable/Writable(****true, true) >> >> > File.setExecutable(false, false) >> > >> > catch is that we can only do that in Java 6, whereas this >> > artifact/jar > >> > google-oauth-client is supposed to be Java 5 compliant. >> > >> > So: I propose we create a new artifact/jar called >> > google-oauth-client-java6 and move FileCredentialStore into there. >> > >> > I've reviewed this proposal with Ravi and he agrees with it. Sorry >> > it > >> > would be more work for you, but strategically we are trying to drop >> > support for Java 6, except for Android applications. >> > >> > >> > > http://codereview.appspot.com/****6242068/%3Chttp://** > codereview.appspot.com/**6242068/<http://codereview.appspot.com/**6242068/%3Chttp://codereview.appspot.com/6242068/> > > > > > >> > > > > -- >> > > ------------------------------**------------------------------** > ------------ > >> Rafael Naufal >> "A great pleasure in life is doing what people say you cannot do" >> > -Walter > >> Gagehot >> http://rafaelnaufal.com >> Twitter: http://twitter.com/rnaufal >> > > ------------------------------**------------------------------** > ------------ > > > > http://codereview.appspot.com/**6242068/<http://codereview.appspot.com/6242068/> > -- ------------------------------------------------------------------------ Rafael Naufal "A great pleasure in life is doing what people say you cannot do" -Walter Gagehot http://rafaelnaufal.com Twitter: http://twitter.com/rnaufal ------------------------------------------------------------------------
Sign in to reply to this message.
Sign in to reply to this message.
Sorry, I'm too busy to review in detail this week, but I wanted to give you one comment. I'll revisit this changeset in detail as soon as the 1.10 release is completed. http://codereview.appspot.com/6242068/diff/21001/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/21001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:34: * @since 1.10 Quick comment: we are starting to lock down the 1.10 release, which is planned for Monday. Please change it to 1.11. Thanks.
Sign in to reply to this message.
Sign in to reply to this message.
Sign in to reply to this message.
http://codereview.appspot.com/6242068/diff/21001/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/21001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:34: * @since 1.10 Ok, done! On 2012/06/06 18:53:01, yanivi wrote: > Quick comment: we are starting to lock down the 1.10 release, which is planned > for Monday. Please change it to 1.11. Thanks.
Sign in to reply to this message.
Sorry I haven't been able to find time to review this just because I'm preparing for my Google I/O presentation. Ravi, do you have time to do at least the first round of review? I also found a couple of Google internal implementations of FileCredentialStore. I think Rafael's implementation is probably better, but it is worth reviewing the other ones to see if there are any good ideas there.
Sign in to reply to this message.
http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-appengin... File google-oauth-client-appengine/.classpath (right): http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-appengin... google-oauth-client-appengine/.classpath:1: <classpath> As Yaniv mentioned before for google-oauth-client-appengine and google-oauth-client-servlet: "If you don't mind, please don't include the changes to the non-java files in this changeset. We'd prefer to keep using the Maven classpath container for now." http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/po... File google-oauth-client-java6/pom.xml (right): http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/po... google-oauth-client-java6/pom.xml:1: <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" Please also include a .classpath, .project and .settings for google-oauth-client-java6, they will be very similar to google-oauth-client but for Java6. http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/po... google-oauth-client-java6/pom.xml:7: <version>1.10.0-beta-SNAPSHOT</version> 1.11.0-beta-SNAPSHOT http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:103: } catch (IOException e) { Probably better to not catch and let the IOException be thrown. http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java (right): http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java:2: * Copyright (c) 2011 Google Inc. 2012 http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java (right): http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:2: * Copyright (c) 2011 Google Inc. 2012 http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:50: Preconditions.checkArgument(userId != null, "userId cannot be null"); Can instead do Preconditions.checkNotNull here and everywhere else. http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/package-info.java (right): http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/package-info.java:7: * http://www.apache.org/licenses/LICENSE-2.0na Remove trailing "na"
Sign in to reply to this message.
Sign in to reply to this message.
http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/po... File google-oauth-client-java6/pom.xml (right): http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/po... google-oauth-client-java6/pom.xml:1: <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" On 2012/06/20 13:45:39, rmistry wrote: > Please also include a .classpath, .project and .settings for > google-oauth-client-java6, they will be very similar to google-oauth-client but > for Java6. Done. http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/po... google-oauth-client-java6/pom.xml:7: <version>1.10.0-beta-SNAPSHOT</version> On 2012/06/20 13:45:39, rmistry wrote: > 1.11.0-beta-SNAPSHOT Done. http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:103: } catch (IOException e) { On 2012/06/20 13:45:39, rmistry wrote: > Probably better to not catch and let the IOException be thrown. Done. http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:103: } catch (IOException e) { On 2012/06/20 13:45:39, rmistry wrote: > Probably better to not catch and let the IOException be thrown. Done. http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java (right): http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java:2: * Copyright (c) 2011 Google Inc. On 2012/06/20 13:45:39, rmistry wrote: > 2012 Done. http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java (right): http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:50: Preconditions.checkArgument(userId != null, "userId cannot be null"); I've done Preconditions.checkArgument because of findbugs. If I do Preconditions.checkNotNull, findbugs would give some warnings. On 2012/06/20 13:45:39, rmistry wrote: > Can instead do Preconditions.checkNotNull here and everywhere else. http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/package-info.java (right): http://codereview.appspot.com/6242068/diff/24002/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/package-info.java:7: * http://www.apache.org/licenses/LICENSE-2.0na On 2012/06/20 13:45:39, rmistry wrote: > Remove trailing "na" Done.
Sign in to reply to this message.
Thanks for your patience Rafael! I'm back from Google I/O and I've finally had a chance to review your code. Unfortunately I'll be on vacation again for the rest of the week, but I'll be back next week on Monday to continue. Overall design looks correct, so all I have is annoying nitpicks. Also, there are unfortunately a few other files that need to be updated when adding a new artifact. Please take a look at Matthias' changeset for details: http://codereview.appspot.com/6311043/ Thanks again! http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.c... File google-oauth-client-java6/.classpath (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.c... google-oauth-client-java6/.classpath:4: <classpathentry kind="src" path="src/main/java" including="**/*.java"/> please move the src/main/java to the top just for convenience so it is listed first in the Eclipse package explorer http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.c... google-oauth-client-java6/.classpath:4: <classpathentry kind="src" path="src/main/java" including="**/*.java"/> remove the useless including/excluding attributes http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.c... google-oauth-client-java6/.classpath:6: <classpathentry kind="var" path="M2_REPO/commons-codec/commons-codec/1.6/commons-codec-1.6.jar"/> please remove all of the M2_REPO vars and instead just: <classpathentry kind="con" path="org.eclipse.m2e.MAVEN2_CLASSPATH_CONTAINER" /> http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.c... google-oauth-client-java6/.classpath:19: <classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER"/> Just to make sure it is 1.6: <classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-1.6" /> http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.p... File google-oauth-client-java6/.project (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.p... google-oauth-client-java6/.project:3: <comment>Internally developed code released as open source. NO_M2ECLIPSE_SUPPORT: Project files created with the maven-eclipse-plugin are not supported in M2Eclipse.</comment> please change comment to: <comment></comment> http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.p... google-oauth-client-java6/.project:5: <buildSpec> missing the maven build/nature: <buildSpec> <buildCommand> <name>org.eclipse.m2e.core.maven2Builder</name> <arguments> </arguments> </buildCommand> </buildSpec> <natures> <nature>org.eclipse.m2e.core.maven2Nature</nature> </natures> http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:57: * @throws IOException our style convention is to remove the @throws IOException just because IOException is basically an I/O exception and there isn't anything more specific we have to specify :) http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:66: if (this.file.exists()) { style: please remove "this." unless it is absolutely necessarily like on lines 62 and 63 similarly below http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:71: this.file.setExecutable(false); I had envision an implementation more like this: file.setReadable(false, false); file.setWritable(false, false); file.setExecutable(false, false); file.setReadable(true); file.setWritable(true); that way, it is only accessible to the owner, and it enabled writing to the file to the owner. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:82: Charset.forName("UTF-8")); Charsets.UTF_8 similarly below http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:84: generator.close(); can you please extract the code to write to file into a private method so we can avoid code duplication between this and the delete method? http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:121: FileInputStream is = null; better to initialize here: FileInputStream is = new FileInputStream(file); and then in the finally we are certain that "is != null" so can remove line 127 http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:131: Preconditions.checkArgument(this.credentials != null, remove this line since it cannot be null because otherwise an exception would have been thrown http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java:25: * @author Rafael Naufal * @since 1.11 http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java:73: public String getAccessToken() { [optional] remove these 3 unused methods http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:29: * @author Rafael Naufal * @since 1.11 http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:86: Preconditions.checkNotNull(fileCredential, perhaps remove this precondition since it is okay if trying to remove a user id that has already been deleted? e.g. if I call delete(userId) twice we should just quietly handle it http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:92: public Map<String, FilePersistedCredential> getCredentials() { remove unused method that exposes internal implementation http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/package-info.java (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/package-info.java:16: * OAuth 2.0 utilities that help simplify the authorization flow on Java6. "Java 6" http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... File google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStoreTest.java (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStoreTest.java:115: public void testNotDeleteCredentials() throws IOException { Please remove the throws IOException Description Resource Path Location Type The declared exception IOException is not actually thrown by the method testNotDeleteCredentials() from type FileCredentialStoreTest FileCredentialStoreTest.java /google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2 line 115 Java Problem The declared exception IOException is not actually thrown by the method testNotLoadCredentials() from type FileCredentialStoreTest FileCredentialStoreTest.java /google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2 line 86 Java Problem The declared exception IOException is not actually thrown by the method testParametersMustNotBeNull() from type FileCredentialStoreTest FileCredentialStoreTest.java /google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2 line 59 Java Problem
Sign in to reply to this message.
Hi Yanivi, I applied all your latest comments, but I have one doubt regarding the other files that need to be updated which you mentioned on your email. I've seen Mathias' changeset and noted these files that the google-oauth-client-java6 doesn't have: google-http-client-assembly/dependencies/protobuf-dependencies.html<http://codereview.appspot.com/6311043/patch/24001/25002> google-http-client-assembly/readme.html<http://codereview.appspot.com/6311043/patch/24001/25004> Are there any other files that need to be added or only the above ones? Thanks, Rafael On Tue, Jul 3, 2012 at 4:41 PM, <yanivi@google.com> wrote: > Thanks for your patience Rafael! I'm back from Google I/O and I've > finally had a chance to review your code. Unfortunately I'll be on > vacation again for the rest of the week, but I'll be back next week on > Monday to continue. > > Overall design looks correct, so all I have is annoying nitpicks. > > Also, there are unfortunately a few other files that need to be updated > when adding a new artifact. Please take a look at Matthias' changeset > for details: > > http://codereview.appspot.com/**6311043/<http://codereview.appspot.com/6311043/> > > Thanks again! > > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/.classpath<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.classpath> > File google-oauth-client-java6/.**classpath (right): > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/.classpath#**newcode4<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.classpath#newcode4> > google-oauth-client-java6/.**classpath:4: <classpathentry kind="src" > path="src/main/java" including="**/*.java"/> > please move the src/main/java to the top just for convenience so it is > listed first in the Eclipse package explorer > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/.classpath#**newcode4<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.classpath#newcode4> > google-oauth-client-java6/.**classpath:4: <classpathentry kind="src" > path="src/main/java" including="**/*.java"/> > remove the useless including/excluding attributes > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/.classpath#**newcode6<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.classpath#newcode6> > google-oauth-client-java6/.**classpath:6: <classpathentry kind="var" > path="M2_REPO/commons-codec/**commons-codec/1.6/commons-**codec-1.6.jar"/> > please remove all of the M2_REPO vars and instead just: > > <classpathentry kind="con" > path="org.eclipse.m2e.MAVEN2_**CLASSPATH_CONTAINER" /> > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/.classpath#**newcode19<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.classpath#newcode19> > google-oauth-client-java6/.**classpath:19: <classpathentry kind="con" > path="org.eclipse.jdt.**launching.JRE_CONTAINER"/> > Just to make sure it is 1.6: > > <classpathentry kind="con" > > path="org.eclipse.jdt.**launching.JRE_CONTAINER/org.** > eclipse.jdt.internal.debug.ui.**launcher.StandardVMType/**JavaSE-1.6" > /> > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/.project<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.project> > File google-oauth-client-java6/.**project (right): > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/.project#**newcode3<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.project#newcode3> > google-oauth-client-java6/.**project:3: <comment>Internally developed code > released as open source. NO_M2ECLIPSE_SUPPORT: Project files created > with the maven-eclipse-plugin are not supported in M2Eclipse.</comment> > please change comment to: > > <comment></comment> > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/.project#**newcode5<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.project#newcode5> > google-oauth-client-java6/.**project:5: <buildSpec> > missing the maven build/nature: > > <buildSpec> > <buildCommand> > <name>org.eclipse.m2e.core.**maven2Builder</name> > <arguments> > </arguments> > </buildCommand> > </buildSpec> > <natures> > <nature>org.eclipse.m2e.core.**maven2Nature</nature> > </natures> > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FileCredentialStore.java<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java> > File > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/FileCredentialStore.**java > (right): > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FileCredentialStore.java#**newcode57<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java#newcode57> > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/FileCredentialStore.**java:57: > * @throws IOException > our style convention is to remove the @throws IOException just because > IOException is basically an I/O exception and there isn't anything more > specific we have to specify :) > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FileCredentialStore.java#**newcode66<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java#newcode66> > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/FileCredentialStore.**java:66: > if (this.file.exists()) { > style: please remove "this." unless it is absolutely necessarily like on > lines 62 and 63 > > similarly below > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FileCredentialStore.java#**newcode71<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java#newcode71> > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/FileCredentialStore.**java:71: > this.file.setExecutable(false)**; > I had envision an implementation more like this: > > file.setReadable(false, false); > file.setWritable(false, false); > file.setExecutable(false, false); > file.setReadable(true); > file.setWritable(true); > > that way, it is only accessible to the owner, and it enabled writing to > the file to the owner. > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FileCredentialStore.java#**newcode82<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java#newcode82> > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/FileCredentialStore.**java:82: > Charset.forName("UTF-8")); > Charsets.UTF_8 > > similarly below > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FileCredentialStore.java#**newcode84<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java#newcode84> > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/FileCredentialStore.**java:84: > generator.close(); > can you please extract the code to write to file into a private method > so we can avoid code duplication between this and the delete method? > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FileCredentialStore.java#**newcode121<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java#newcode121> > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/FileCredentialStore.**java:121: > FileInputStream is = null; > better to initialize here: > > FileInputStream is = new FileInputStream(file); > > and then in the finally we are certain that "is != null" so can remove > line 127 > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FileCredentialStore.java#**newcode131<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java#newcode131> > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/FileCredentialStore.**java:131: > Preconditions.checkArgument(**this.credentials != null, > remove this line since it cannot be null because otherwise an exception > would have been thrown > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FilePersistedCredential.java<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java> > File > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/**FilePersistedCredential.java > (right): > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FilePersistedCredential.java#**newcode25<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java#newcode25> > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/**FilePersistedCredential.java:**25: > * @author Rafael Naufal > * @since 1.11 > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FilePersistedCredential.java#**newcode73<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java#newcode73> > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/**FilePersistedCredential.java:**73: > public String getAccessToken() { > [optional] remove these 3 unused methods > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FilePersistedCredentials.java<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java> > File > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/**FilePersistedCredentials.java > (right): > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FilePersistedCredentials.java#**newcode29<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java#newcode29> > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/**FilePersistedCredentials.java:** > 29: > * @author Rafael Naufal > * @since 1.11 > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FilePersistedCredentials.java#**newcode86<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java#newcode86> > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/**FilePersistedCredentials.java:** > 86: > Preconditions.checkNotNull(**fileCredential, > perhaps remove this precondition since it is okay if trying to remove a > user id that has already been deleted? > > e.g. if I call delete(userId) twice we should just quietly handle it > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FilePersistedCredentials.java#**newcode92<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java#newcode92> > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/**FilePersistedCredentials.java:** > 92: > public Map<String, FilePersistedCredential> getCredentials() { > remove unused method that exposes internal implementation > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**package-info.java<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/package-info.java> > File > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/package-info.java > (right): > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/main/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**package-info.java#newcode16<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/package-info.java#newcode16> > google-oauth-client-java6/src/**main/java/com/google/api/** > client/extensions/java6/auth/**oauth2/package-info.java:16: > * OAuth 2.0 utilities that help simplify the authorization flow on > Java6. > "Java 6" > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/test/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FileCredentialStoreTest.java<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStoreTest.java> > File > google-oauth-client-java6/src/**test/java/com/google/api/** > client/extensions/java6/auth/**oauth2/**FileCredentialStoreTest.java > (right): > > http://codereview.appspot.com/**6242068/diff/36001/google-** > oauth-client-java6/src/test/**java/com/google/api/client/** > extensions/java6/auth/oauth2/**FileCredentialStoreTest.java#**newcode115<http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStoreTest.java#newcode115> > google-oauth-client-java6/src/**test/java/com/google/api/** > client/extensions/java6/auth/**oauth2/**FileCredentialStoreTest.java:** > 115: > public void testNotDeleteCredentials() throws IOException { > Please remove the throws IOException > > Description Resource Path Location Type > The declared exception IOException is not actually thrown by the method > testNotDeleteCredentials() from type > FileCredentialStoreTest FileCredentialStoreTest.java > /google-oauth-client-java6/**src/test/java/com/google/api/** > client/extensions/java6/auth/**oauth2 line > 115 Java Problem > The declared exception IOException is not actually thrown by the method > testNotLoadCredentials() from type > FileCredentialStoreTest FileCredentialStoreTest.java > /google-oauth-client-java6/**src/test/java/com/google/api/** > client/extensions/java6/auth/**oauth2 line > 86 Java Problem > The declared exception IOException is not actually thrown by the method > testParametersMustNotBeNull() from type > FileCredentialStoreTest FileCredentialStoreTest.java > /google-oauth-client-java6/**src/test/java/com/google/api/** > client/extensions/java6/auth/**oauth2 line > 59 Java Problem > > http://codereview.appspot.com/**6242068/<http://codereview.appspot.com/6242068/> > -- ------------------------------------------------------------------------ Rafael Naufal "A great pleasure in life is doing what people say you cannot do" -Walter Gagehot http://rafaelnaufal.com Twitter: http://twitter.com/rnaufal ------------------------------------------------------------------------
Sign in to reply to this message.
On 2012/07/04 21:30:30, rafael.naufal wrote: > Hi Yanivi, > > I applied all your latest comments, but I have one doubt regarding the > other files that need to be updated which you mentioned on your email. > > I've seen Mathias' changeset and noted these files that the > google-oauth-client-java6 doesn't have: > > google-http-client-assembly/dependencies/protobuf-dependencies.html<http://codereview.appspot.com/6311043/patch/24001/25002> > google-http-client-assembly/readme.html<http://codereview.appspot.com/6311043/patch/24001/25004> > > Are there any other files that need to be added or only the above ones? > > Thanks, > > Rafael > Good question. I also see: google-http-client-assembly/.classpath google-http-client-assembly/pom.xml jdiff.xml pom.xml release.html We should probably do a better job of documenting exactly all of the places that need to be updated...
Sign in to reply to this message.
Ok, thanks Yanivi! The files you mentioned on your last email are created with Maven or is it by hand? About google-http-client-assembly/.**classpath google-http-client-assembly/**pom.xml I haven't updated them, so why do I have to modify them? The file release.html is only generated when a release is created, right? Do I need to generate a release before sending a patch for you to review it? How is the file jdiff.xml generated? Sorry, but I'm still in doubt about the set of other files that need to be updated. Thanks, Rafael On Mon, Jul 9, 2012 at 4:02 PM, <yanivi@google.com> wrote: > On 2012/07/04 21:30:30, rafael.naufal wrote: > >> Hi Yanivi, >> > > I applied all your latest comments, but I have one doubt regarding the >> other files that need to be updated which you mentioned on your email. >> > > I've seen Mathias' changeset and noted these files that the >> google-oauth-client-java6 doesn't have: >> > > > google-http-client-assembly/**dependencies/protobuf-**dependencies.html< > http://**codereview.appspot.com/**6311043/patch/24001/25002<http://codereview.appspot.com/6311043/patch/24001/25002> > > > > google-http-client-assembly/**readme.html<http://codereview.** > appspot.com/6311043/patch/**24001/25004<http://codereview.appspot.com/6311043/patch/24001/25004> > > > > > Are there any other files that need to be added or only the above >> > ones? > > Thanks, >> > > Rafael >> > > > Good question. I also see: > > google-http-client-assembly/.**classpath > google-http-client-assembly/**pom.xml > jdiff.xml > pom.xml > release.html > > We should probably do a better job of documenting exactly all of the > places that need to be updated... > > http://codereview.appspot.com/**6242068/<http://codereview.appspot.com/6242068/> > -- ------------------------------------------------------------------------ Rafael Naufal "A great pleasure in life is doing what people say you cannot do" -Walter Gagehot http://rafaelnaufal.com Twitter: http://twitter.com/rnaufal ------------------------------------------------------------------------
Sign in to reply to this message.
Ideally I'd like you to update them all now as part of this changeset (nothing is generated during release). But if you don't, it's not important. We can update them ourselves after you commit your changeset. So if you don't know how to update a file, don't worry about it. But to answer your questions, I think the only file that was generated is the *-dependencies.html file, and everything else was updated by hand. On Mon, Jul 9, 2012 at 3:37 PM, Rafael <rafael.naufal@gmail.com> wrote: > Ok, thanks Yanivi! > > The files you mentioned on your last email are created with Maven or is it > by hand? > > About > > google-http-client-assembly/.**classpath > google-http-client-assembly/**pom.xml > > I haven't updated them, so why do I have to modify them? > > The file release.html is only generated when a release is created, right? > Do I need to generate a release before sending a patch for you to review it? > > How is the file jdiff.xml generated? Sorry, but I'm still in doubt about > the set of other files that need to be updated. > > Thanks, > > Rafael > > On Mon, Jul 9, 2012 at 4:02 PM, <yanivi@google.com> wrote: > >> On 2012/07/04 21:30:30, rafael.naufal wrote: >> >>> Hi Yanivi, >>> >> >> I applied all your latest comments, but I have one doubt regarding the >>> other files that need to be updated which you mentioned on your email. >>> >> >> I've seen Mathias' changeset and noted these files that the >>> google-oauth-client-java6 doesn't have: >>> >> >> >> google-http-client-assembly/**dependencies/protobuf-**dependencies.html< >> http://**codereview.appspot.com/**6311043/patch/24001/25002<http://codereview.appspot.com/6311043/patch/24001/25002> >> > >> >> google-http-client-assembly/**readme.html<http://codereview.** >> appspot.com/6311043/patch/**24001/25004<http://codereview.appspot.com/6311043/patch/24001/25004> >> > >> >> >> Are there any other files that need to be added or only the above >>> >> ones? >> >> Thanks, >>> >> >> Rafael >>> >> >> >> Good question. I also see: >> >> google-http-client-assembly/.**classpath >> google-http-client-assembly/**pom.xml >> jdiff.xml >> pom.xml >> release.html >> >> We should probably do a better job of documenting exactly all of the >> places that need to be updated... >> >> http://codereview.appspot.com/**6242068/<http://codereview.appspot.com/6242068/> >> > > > > -- > ------------------------------------------------------------------------ > Rafael Naufal > "A great pleasure in life is doing what people say you cannot do" -Walter > Gagehot > http://rafaelnaufal.com > Twitter: http://twitter.com/rnaufal > ------------------------------------------------------------------------ >
Sign in to reply to this message.
How can I generate this *-dependencies.html file? On Mon, Jul 9, 2012 at 4:41 PM, Yaniv Inbar (יניב ענבר) <yanivi@google.com>wrote: > Ideally I'd like you to update them all now as part of this changeset > (nothing is generated during release). But if you don't, it's not > important. We can update them ourselves after you commit your changeset. > So if you don't know how to update a file, don't worry about it. But to > answer your questions, I think the only file that was generated is the > *-dependencies.html file, and everything else was updated by hand. > > > On Mon, Jul 9, 2012 at 3:37 PM, Rafael <rafael.naufal@gmail.com> wrote: > >> Ok, thanks Yanivi! >> >> The files you mentioned on your last email are created with Maven or is >> it by hand? >> >> About >> >> google-http-client-assembly/.**classpath >> google-http-client-assembly/**pom.xml >> >> I haven't updated them, so why do I have to modify them? >> >> The file release.html is only generated when a release is created, right? >> Do I need to generate a release before sending a patch for you to review it? >> >> How is the file jdiff.xml generated? Sorry, but I'm still in doubt about >> the set of other files that need to be updated. >> >> Thanks, >> >> Rafael >> >> On Mon, Jul 9, 2012 at 4:02 PM, <yanivi@google.com> wrote: >> >>> On 2012/07/04 21:30:30, rafael.naufal wrote: >>> >>>> Hi Yanivi, >>>> >>> >>> I applied all your latest comments, but I have one doubt regarding the >>>> other files that need to be updated which you mentioned on your email. >>>> >>> >>> I've seen Mathias' changeset and noted these files that the >>>> google-oauth-client-java6 doesn't have: >>>> >>> >>> >>> google-http-client-assembly/**dependencies/protobuf-**dependencies.html< >>> http://**codereview.appspot.com/**6311043/patch/24001/25002<http://codereview.appspot.com/6311043/patch/24001/25002> >>> > >>> >>> google-http-client-assembly/**readme.html<http://codereview.** >>> appspot.com/6311043/patch/**24001/25004<http://codereview.appspot.com/6311043/patch/24001/25004> >>> > >>> >>> >>> Are there any other files that need to be added or only the above >>>> >>> ones? >>> >>> Thanks, >>>> >>> >>> Rafael >>>> >>> >>> >>> Good question. I also see: >>> >>> google-http-client-assembly/.**classpath >>> google-http-client-assembly/**pom.xml >>> jdiff.xml >>> pom.xml >>> release.html >>> >>> We should probably do a better job of documenting exactly all of the >>> places that need to be updated... >>> >>> http://codereview.appspot.com/**6242068/<http://codereview.appspot.com/6242068/> >>> >> >> >> >> -- >> ------------------------------------------------------------------------ >> Rafael Naufal >> "A great pleasure in life is doing what people say you cannot do" -Walter >> Gagehot >> http://rafaelnaufal.com >> Twitter: http://twitter.com/rnaufal >> ------------------------------------------------------------------------ >> > > -- ------------------------------------------------------------------------ Rafael Naufal "A great pleasure in life is doing what people say you cannot do" -Walter Gagehot http://rafaelnaufal.com Twitter: http://twitter.com/rnaufal ------------------------------------------------------------------------
Sign in to reply to this message.
On 2012/07/10 20:39:40, rafael.naufal wrote: > How can I generate this *-dependencies.html file? > Details are here: http://code.google.com/p/google-oauth-java-client/source/browse/release.html#124 Thanks! > On Mon, Jul 9, 2012 at 4:41 PM, Yaniv Inbar (יניב ענבר) > <yanivi@google.com>wrote: > > > Ideally I'd like you to update them all now as part of this changeset > > (nothing is generated during release). But if you don't, it's not > > important. We can update them ourselves after you commit your changeset. > > So if you don't know how to update a file, don't worry about it. But to > > answer your questions, I think the only file that was generated is the > > *-dependencies.html file, and everything else was updated by hand. > > > > > > On Mon, Jul 9, 2012 at 3:37 PM, Rafael <mailto:rafael.naufal@gmail.com> wrote: > > > >> Ok, thanks Yanivi! > >> > >> The files you mentioned on your last email are created with Maven or is > >> it by hand? > >> > >> About > >> > >> google-http-client-assembly/.**classpath > >> google-http-client-assembly/**pom.xml > >> > >> I haven't updated them, so why do I have to modify them? > >> > >> The file release.html is only generated when a release is created, right? > >> Do I need to generate a release before sending a patch for you to review it? > >> > >> How is the file jdiff.xml generated? Sorry, but I'm still in doubt about > >> the set of other files that need to be updated. > >> > >> Thanks, > >> > >> Rafael > >> > >> On Mon, Jul 9, 2012 at 4:02 PM, <mailto:yanivi@google.com> wrote: > >> > >>> On 2012/07/04 21:30:30, rafael.naufal wrote: > >>> > >>>> Hi Yanivi, > >>>> > >>> > >>> I applied all your latest comments, but I have one doubt regarding the > >>>> other files that need to be updated which you mentioned on your email. > >>>> > >>> > >>> I've seen Mathias' changeset and noted these files that the > >>>> google-oauth-client-java6 doesn't have: > >>>> > >>> > >>> > >>> google-http-client-assembly/**dependencies/protobuf-**dependencies.html< > >>> > http://**codereview.appspot.com/**6311043/patch/24001/25002%3Chttp://codereview.appspot.com/6311043/patch/24001/25002> > >>> > > >>> > >>> google-http-client-assembly/**readme.html<http://codereview.** > >>> > appspot.com/6311043/patch/**24001/25004<http://codereview.appspot.com/6311043/patch/24001/25004> > >>> > > >>> > >>> > >>> Are there any other files that need to be added or only the above > >>>> > >>> ones? > >>> > >>> Thanks, > >>>> > >>> > >>> Rafael > >>>> > >>> > >>> > >>> Good question. I also see: > >>> > >>> google-http-client-assembly/.**classpath > >>> google-http-client-assembly/**pom.xml > >>> jdiff.xml > >>> pom.xml > >>> release.html > >>> > >>> We should probably do a better job of documenting exactly all of the > >>> places that need to be updated... > >>> > >>> > http://codereview.appspot.com/**6242068/%3Chttp://codereview.appspot.com/6242...> > >>> > >> > >> > >> > >> -- > >> ------------------------------------------------------------------------ > >> Rafael Naufal > >> "A great pleasure in life is doing what people say you cannot do" -Walter > >> Gagehot > >> http://rafaelnaufal.com > >> Twitter: http://twitter.com/rnaufal > >> ------------------------------------------------------------------------ > >> > > > > > > > -- > ------------------------------------------------------------------------ > Rafael Naufal > "A great pleasure in life is doing what people say you cannot do" -Walter > Gagehot > http://rafaelnaufal.com > Twitter: http://twitter.com/rnaufal > ------------------------------------------------------------------------
Sign in to reply to this message.
Sign in to reply to this message.
http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.c... File google-oauth-client-java6/.classpath (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.c... google-oauth-client-java6/.classpath:4: <classpathentry kind="src" path="src/main/java" including="**/*.java"/> On 2012/07/03 19:41:13, yanivi wrote: > please move the src/main/java to the top just for convenience so it is listed > first in the Eclipse package explorer Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.c... google-oauth-client-java6/.classpath:6: <classpathentry kind="var" path="M2_REPO/commons-codec/commons-codec/1.6/commons-codec-1.6.jar"/> On 2012/07/03 19:41:13, yanivi wrote: > please remove all of the M2_REPO vars and instead just: > > <classpathentry kind="con" > path="org.eclipse.m2e.MAVEN2_CLASSPATH_CONTAINER" /> Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.c... google-oauth-client-java6/.classpath:19: <classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER"/> On 2012/07/03 19:41:13, yanivi wrote: > Just to make sure it is 1.6: > > <classpathentry kind="con" > > path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-1.6" > /> Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.p... File google-oauth-client-java6/.project (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.p... google-oauth-client-java6/.project:3: <comment>Internally developed code released as open source. NO_M2ECLIPSE_SUPPORT: Project files created with the maven-eclipse-plugin are not supported in M2Eclipse.</comment> On 2012/07/03 19:41:13, yanivi wrote: > please change comment to: > > <comment></comment> Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.p... google-oauth-client-java6/.project:3: <comment>Internally developed code released as open source. NO_M2ECLIPSE_SUPPORT: Project files created with the maven-eclipse-plugin are not supported in M2Eclipse.</comment> On 2012/07/03 19:41:13, yanivi wrote: > please change comment to: > > <comment></comment> Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.p... google-oauth-client-java6/.project:5: <buildSpec> On 2012/07/03 19:41:13, yanivi wrote: > missing the maven build/nature: > > <buildSpec> > <buildCommand> > <name>org.eclipse.m2e.core.maven2Builder</name> > <arguments> > </arguments> > </buildCommand> > </buildSpec> > <natures> > <nature>org.eclipse.m2e.core.maven2Nature</nature> > </natures> Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/.p... google-oauth-client-java6/.project:5: <buildSpec> On 2012/07/03 19:41:13, yanivi wrote: > missing the maven build/nature: > > <buildSpec> > <buildCommand> > <name>org.eclipse.m2e.core.maven2Builder</name> > <arguments> > </arguments> > </buildCommand> > </buildSpec> > <natures> > <nature>org.eclipse.m2e.core.maven2Nature</nature> > </natures> Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:57: * @throws IOException On 2012/07/03 19:41:13, yanivi wrote: > our style convention is to remove the @throws IOException just because > IOException is basically an I/O exception and there isn't anything more specific > we have to specify :) Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:66: if (this.file.exists()) { On 2012/07/03 19:41:13, yanivi wrote: > style: please remove "this." unless it is absolutely necessarily like on lines > 62 and 63 > > similarly below Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:71: this.file.setExecutable(false); On 2012/07/03 19:41:13, yanivi wrote: > I had envision an implementation more like this: > > file.setReadable(false, false); > file.setWritable(false, false); > file.setExecutable(false, false); > file.setReadable(true); > file.setWritable(true); > > that way, it is only accessible to the owner, and it enabled writing to the file > to the owner. Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:82: Charset.forName("UTF-8")); On 2012/07/03 19:41:13, yanivi wrote: > Charsets.UTF_8 > > similarly below Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:84: generator.close(); On 2012/07/03 19:41:13, yanivi wrote: > can you please extract the code to write to file into a private method so we can > avoid code duplication between this and the delete method? Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:121: FileInputStream is = null; On 2012/07/03 19:41:13, yanivi wrote: > better to initialize here: > > FileInputStream is = new FileInputStream(file); > > and then in the finally we are certain that "is != null" so can remove line 127 Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:131: Preconditions.checkArgument(this.credentials != null, On 2012/07/03 19:41:13, yanivi wrote: > remove this line since it cannot be null because otherwise an exception would > have been thrown Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java:25: * @author Rafael Naufal On 2012/07/03 19:41:13, yanivi wrote: > * @since 1.11 Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java:25: * @author Rafael Naufal On 2012/07/03 19:41:13, yanivi wrote: > * @since 1.11 Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java:73: public String getAccessToken() { On 2012/07/03 19:41:13, yanivi wrote: > [optional] remove these 3 unused methods Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredential.java:73: public String getAccessToken() { On 2012/07/03 19:41:13, yanivi wrote: > [optional] remove these 3 unused methods Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:29: * @author Rafael Naufal On 2012/07/03 19:41:13, yanivi wrote: > * @since 1.11 Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:86: Preconditions.checkNotNull(fileCredential, On 2012/07/03 19:41:13, yanivi wrote: > perhaps remove this precondition since it is okay if trying to remove a user id > that has already been deleted? > > e.g. if I call delete(userId) twice we should just quietly handle it Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:92: public Map<String, FilePersistedCredential> getCredentials() { On 2012/07/03 19:41:13, yanivi wrote: > remove unused method that exposes internal implementation Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/package-info.java (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/package-info.java:16: * OAuth 2.0 utilities that help simplify the authorization flow on Java6. On 2012/07/03 19:41:13, yanivi wrote: > "Java 6" Done. http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... File google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStoreTest.java (right): http://codereview.appspot.com/6242068/diff/36001/google-oauth-client-java6/sr... google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStoreTest.java:115: public void testNotDeleteCredentials() throws IOException { On 2012/07/03 19:41:13, yanivi wrote: > Please remove the throws IOException > > Description Resource Path Location Type > The declared exception IOException is not actually thrown by the method > testNotDeleteCredentials() from type > FileCredentialStoreTest FileCredentialStoreTest.java /google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2 line > 115 Java Problem > The declared exception IOException is not actually thrown by the method > testNotLoadCredentials() from type > FileCredentialStoreTest FileCredentialStoreTest.java /google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2 line > 86 Java Problem > The declared exception IOException is not actually thrown by the method > testParametersMustNotBeNull() from type > FileCredentialStoreTest FileCredentialStoreTest.java /google-oauth-client-java6/src/test/java/com/google/api/client/extensions/java6/auth/oauth2 line > 59 Java Problem Done.
Sign in to reply to this message.
Only a few minor comments left, so we are very close to this being committed. Please also make this change to suppress the FindBugs error: diff -r e024970aea50 findbugs-exclude.xml --- a/findbugs-exclude.xml Mon Jun 11 16:39:47 2012 -0400 +++ b/findbugs-exclude.xml Wed Jul 11 10:57:03 2012 -0400 @@ -10,6 +10,10 @@ <!-- The following are known to be bugs and should be fixed --> <!-- The following are known to not be bugs and should not be fixed --> <And> + <Bug pattern="RV_RETURN_VALUE_IGNORED_BAD_PRACTICE"/> + <Class name="com.google.api.client.extensions.java6.auth.oauth2.FileCredentialStore"/> + </And> + <And> <Bug pattern="EI_EXPOSE_REP,EI_EXPOSE_REP2"/> <Class name="com.google.api.client.auth.jsontoken.JsonWebSignature"/> </And> By the way, I've gotten a request internally at Google for FileCredentialStore, so I'm prioritizing getting this changeset reviewed and committed ASAP. Thanks for your help! http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/.c... File google-oauth-client-java6/.classpath (right): http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/.c... google-oauth-client-java6/.classpath:7: path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-1.6" /> please also add <classpathentry kind="con" path="org.eclipse.m2e.MAVEN2_CLASSPATH_CONTAINER" /> http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/.p... File google-oauth-client-java6/.project (right): http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/.p... google-oauth-client-java6/.project:9: <name>org.eclipse.m2e.core.maven2Builder</name> please add <name>org.eclipse.jdt.core.javabuilder</name> http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/.p... google-oauth-client-java6/.project:15: <nature>org.eclipse.m2e.core.maven2Nature</nature> please add <nature>org.eclipse.jdt.core.javanature</nature> http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:37: public class FileCredentialStore implements CredentialStore { $mvn clean install /usr/local/google/users/yanivi/hg/google-oauth-java-client/default/default/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:24: warning: Wrong order for 'com.google.api.client.auth.oauth2.Credential' import. /usr/local/google/users/yanivi/hg/google-oauth-java-client/default/default/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:20: warning: Wrong order for 'com.google.api.client.auth.oauth2.Credential' import. imports order should be: import com.google.api.client.auth.oauth2.Credential; import com.google.api.client.auth.oauth2.CredentialStore; import com.google.api.client.json.JsonFactory; import com.google.api.client.json.JsonGenerator; import com.google.common.base.Charsets; import com.google.common.base.Preconditions; import java.io.File; import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:58: public FileCredentialStore(File file, JsonFactory jsonFactory) We are using 2-space indent in this project instead of 4-space indent. Please change it for consistency to make it easier for people to change each other's code. http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:75: public void store(String userId, Credential credential) throws IOException { Description Resource Path Location Type The method delete(String, Credential) of type FileCredentialStore should be tagged with @Override since it actually overrides a superinterface method FileCredentialStore.java /google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2 line 85 Java Problem The method load(String, Credential) of type FileCredentialStore should be tagged with @Override since it actually overrides a superinterface method FileCredentialStore.java /google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2 line 95 Java Problem The method store(String, Credential) of type FileCredentialStore should be tagged with @Override since it actually overrides a superinterface method FileCredentialStore.java /google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2 line 75 Java Problem http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:114: private void writeCredentials(String userId, Credential credential) [optional] We are using 100 space line length, not 80. So it would be nicer to move the throws into this line. http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:116: FileOutputStream fos = null; better to initialize here: FileOutputStream fos = new FileOutputStream(file); try { ... } finally { fos.close(); } http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java (right): http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:23: import com.google.common.base.Preconditions; imports order should be: import com.google.api.client.auth.oauth2.Credential; import com.google.api.client.json.GenericJson; import com.google.api.client.util.Key; import com.google.common.base.Preconditions; import java.util.HashMap; import java.util.Map;
Sign in to reply to this message.
Sign in to reply to this message.
Sign in to reply to this message.
http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/.c... File google-oauth-client-java6/.classpath (right): http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/.c... google-oauth-client-java6/.classpath:7: path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-1.6" /> On 2012/07/11 14:58:15, yanivi wrote: > please also add <classpathentry kind="con" > path="org.eclipse.m2e.MAVEN2_CLASSPATH_CONTAINER" /> Done. http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/.p... File google-oauth-client-java6/.project (right): http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/.p... google-oauth-client-java6/.project:9: <name>org.eclipse.m2e.core.maven2Builder</name> On 2012/07/11 14:58:15, yanivi wrote: > please add <name>org.eclipse.jdt.core.javabuilder</name> Done. http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/.p... google-oauth-client-java6/.project:15: <nature>org.eclipse.m2e.core.maven2Nature</nature> On 2012/07/11 14:58:15, yanivi wrote: > please add <nature>org.eclipse.jdt.core.javanature</nature> Done. http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java (right): http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:37: public class FileCredentialStore implements CredentialStore { On 2012/07/11 14:58:15, yanivi wrote: > $mvn clean install > > /usr/local/google/users/yanivi/hg/google-oauth-java-client/default/default/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:24: > warning: Wrong order for 'com.google.api.client.auth.oauth2.Credential' import. > /usr/local/google/users/yanivi/hg/google-oauth-java-client/default/default/google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:20: > warning: Wrong order for 'com.google.api.client.auth.oauth2.Credential' import. > > imports order should be: > > import com.google.api.client.auth.oauth2.Credential; > import com.google.api.client.auth.oauth2.CredentialStore; > import com.google.api.client.json.JsonFactory; > import com.google.api.client.json.JsonGenerator; > import com.google.common.base.Charsets; > import com.google.common.base.Preconditions; > > import java.io.File; > import java.io.FileInputStream; > import java.io.FileOutputStream; > import java.io.IOException; > import java.util.concurrent.locks.Lock; > import java.util.concurrent.locks.ReentrantLock; Done. http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:58: public FileCredentialStore(File file, JsonFactory jsonFactory) On 2012/07/11 14:58:15, yanivi wrote: > We are using 2-space indent in this project instead of 4-space indent. Please > change it for consistency to make it easier for people to change each other's > code. Done. http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:75: public void store(String userId, Credential credential) throws IOException { On 2012/07/11 14:58:15, yanivi wrote: > Description Resource Path Location Type > The method delete(String, Credential) of type FileCredentialStore should be > tagged with @Override since it actually overrides a superinterface > method FileCredentialStore.java /google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2 line > 85 Java Problem > The method load(String, Credential) of type FileCredentialStore should be tagged > with @Override since it actually overrides a superinterface > method FileCredentialStore.java /google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2 line > 95 Java Problem > The method store(String, Credential) of type FileCredentialStore should be > tagged with @Override since it actually overrides a superinterface > method FileCredentialStore.java /google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2 line > 75 Java Problem Done. http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:114: private void writeCredentials(String userId, Credential credential) On 2012/07/11 14:58:15, yanivi wrote: > [optional] We are using 100 space line length, not 80. So it would be nicer to > move the throws into this line. Done. http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FileCredentialStore.java:116: FileOutputStream fos = null; On 2012/07/11 14:58:15, yanivi wrote: > better to initialize here: > > FileOutputStream fos = new FileOutputStream(file); > try { > ... > > } finally { > fos.close(); > } Done. http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java (right): http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:23: import com.google.common.base.Preconditions; On 2012/07/11 14:58:15, yanivi wrote: > imports order should be: > > > import com.google.api.client.auth.oauth2.Credential; > import com.google.api.client.json.GenericJson; > import com.google.api.client.util.Key; > import com.google.common.base.Preconditions; > > import java.util.HashMap; > import java.util.Map; Done. http://codereview.appspot.com/6242068/diff/41003/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:23: import com.google.common.base.Preconditions; On 2012/07/11 14:58:15, yanivi wrote: > imports order should be: > > > import com.google.api.client.auth.oauth2.Credential; > import com.google.api.client.json.GenericJson; > import com.google.api.client.util.Key; > import com.google.common.base.Preconditions; > > import java.util.HashMap; > import java.util.Map; Done.
Sign in to reply to this message.
LGTM Congrats! Please commit using the directions below: http://code.google.com/p/google-oauth-java-client/wiki/BecomingAContributor#C... Let me know if you run into any problems committing. http://codereview.appspot.com/6242068/diff/54006/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java (right): http://codereview.appspot.com/6242068/diff/54006/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:37: new HashMap<String, FilePersistedCredential>(); [optional] Use: import com.google.common.collect.Maps; private Map<String, FilePersistedCredential> credentials = Maps.newHashMap();
Sign in to reply to this message.
Changes committed! http://code.google.com/p/google-oauth-java-client/source/browse/#hg%2Fgoogle-... http://codereview.appspot.com/6242068/diff/54006/google-oauth-client-java6/sr... File google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java (right): http://codereview.appspot.com/6242068/diff/54006/google-oauth-client-java6/sr... google-oauth-client-java6/src/main/java/com/google/api/client/extensions/java6/auth/oauth2/FilePersistedCredentials.java:37: new HashMap<String, FilePersistedCredential>(); On 2012/07/12 18:22:31, yanivi wrote: > [optional] Use: > > import com.google.common.collect.Maps; > > private Map<String, FilePersistedCredential> credentials = Maps.newHashMap(); Done.
Sign in to reply to this message.
|