oauth issue 73: call to setScopes(String...) with null result in NullPointerException (OAUTH)

[default + 1.15]

issue 73: call to setScopes(String...) with null result in NullPointerException

[yanivi, 2013-04-24 15:05:39]

https://codereview.appspot.com/8865045/diff/6001/google-oauth-client/src/main...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java
(right):

https://codereview.appspot.com/8865045/diff/6001/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:96:
private String scopes;
Unfortunately storing it as a String is very inconvenient.  I think it is better
to store it as a List to match what we've done for refreshListeners.  It would
mean having to break a backwards-incompatible change to getScopes() to instead
return List<String> and add a new setScopes(List<String>) and deprecate
setScopes(Iterable<String>).

[yanivi, 2013-04-24 15:10:44]

https://codereview.appspot.com/8865045/diff/6001/google-oauth-client/src/main...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java
(right):

https://codereview.appspot.com/8865045/diff/6001/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:697:
public final String getScopes() {
since we are making a backwards incompatible change to getScopes(), we should
add a new method like getScopesAsString() method that is implemented roughly
like this:

return scopes == null ? null : Joiner.on(' ').join(scopes);

That way existing users of getScopes() can simply use getScopesAsString()
instead.

[yanivi, 2013-04-26 13:12:31]

On 2013/04/24 15:10:44, yanivi wrote:
>
https://codereview.appspot.com/8865045/diff/6001/google-oauth-client/src/main...
> File
>
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java
> (right):
> 
>
https://codereview.appspot.com/8865045/diff/6001/google-oauth-client/src/main...
>
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:697:
> public final String getScopes() {
> since we are making a backwards incompatible change to getScopes(), we should
> add a new method like getScopesAsString() method that is implemented roughly
> like this:
> 
> return scopes == null ? null : Joiner.on(' ').join(scopes);
> 
> That way existing users of getScopes() can simply use getScopesAsString()
> instead.

Just realized I'm being silly: rather than List we should use Collection.  That
seems like a good compromise between being general purpose and enabling
functionality like size(), add(), remove(), contains(), clear(), etc.

[peleyal, 2013-04-26 15:40:07]

https://codereview.appspot.com/8865045/diff/6001/google-oauth-client/src/main...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java
(right):

https://codereview.appspot.com/8865045/diff/6001/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:96:
private String scopes;
On 2013/04/24 15:05:39, yanivi wrote:
> Unfortunately storing it as a String is very inconvenient.  I think it is
better
> to store it as a List to match what we've done for refreshListeners.  It would
> mean having to break a backwards-incompatible change to getScopes() to instead
> return List<String> and add a new setScopes(List<String>) and deprecate
> setScopes(Iterable<String>).

Done.

https://codereview.appspot.com/8865045/diff/6001/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:697:
public final String getScopes() {
Done. (but not in the builder, only in AuthorizationCodeFlow)
On 2013/04/24 15:10:44, yanivi wrote:
> since we are making a backwards incompatible change to getScopes(), we should
> add a new method like getScopesAsString() method that is implemented roughly
> like this:
> 
> return scopes == null ? null : Joiner.on(' ').join(scopes);
> 
> That way existing users of getScopes() can simply use getScopesAsString()
> instead.

[peleyal, 2013-04-30 17:14:36]

- I disabled clirr check in oauth as well (same ListArray bug when adding 7005
difference type)
- I changed refresh listener to Collection

[yanivi, 2013-05-01 15:41:01]

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java
(right):

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:96:
/** Collection of scopes or {@code null} for none. */
I think an empty list would be better for none.  see refreshlisteners

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:422:
Collection<String> scopes;
initialize to an empty arraylist like refreshListeners

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:432:
new ArrayList<CredentialRefreshListener>();
Lists.newArrayList();

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:687:
this.scopes = scopes == null ? null : Lists.newArrayList(scopes);
Lists.newArrayList() for null case

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:722:
this.scopes = scopes;
Preconditions.checkNotNull and update JavaDoc accordingly

alternatively, allow null parameter, but then assign Lists.newArrayList() to
scopes field

similarly for setRefreshListeners

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:731:
* with version 1.15 it returns a {@link List}.
Collection

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java
(right):

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java:165:
*
@since 1.15

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java:263:
public AuthorizationRequestUrl setScopes(Collection<String> scopes) {
@since 1.15

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:600:
new ArrayList<CredentialRefreshListener>();
Lists.newArrayList();

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:805:
public Builder setRefreshListeners(Collection<CredentialRefreshListener>
refreshListeners) {
for maximum backwards compatibility, keep setRefreshListeners(List) but mark it
deprecated

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:805:
public Builder setRefreshListeners(Collection<CredentialRefreshListener>
refreshListeners) {
@since 1.15

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenRequest.java
(right):

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenRequest.java:244:
*/
@since 1.15

https://codereview.appspot.com/8865045/diff/39001/pom.xml
File pom.xml (right):

https://codereview.appspot.com/8865045/diff/39001/pom.xml#newcode425
pom.xml:425: <!-- TODO(peleyal): enable this, but for now disable due to Clirr
ArrayList cast bug
[non-blocking] please file a bug to clirr-maven-plugin about this

[peleyal, 2013-05-01 19:14:03]

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java
(right):

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:96:
/** Collection of scopes or {@code null} for none. */
On 2013/05/01 15:41:01, yanivi wrote:
> I think an empty list would be better for none.  see refreshlisteners

Done.

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:422:
Collection<String> scopes;
On 2013/05/01 15:41:01, yanivi wrote:
> initialize to an empty arraylist like refreshListeners

Done.

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:432:
new ArrayList<CredentialRefreshListener>();
On 2013/05/01 15:41:01, yanivi wrote:
> Lists.newArrayList();

Done.

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:687:
this.scopes = scopes == null ? null : Lists.newArrayList(scopes);
On 2013/05/01 15:41:01, yanivi wrote:
> Lists.newArrayList() for null case

Done.

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:722:
this.scopes = scopes;
On 2013/05/01 15:41:01, yanivi wrote:
> Preconditions.checkNotNull and update JavaDoc accordingly
> 
> alternatively, allow null parameter, but then assign Lists.newArrayList() to
> scopes field
> 
> similarly for setRefreshListeners

Done.

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:731:
* with version 1.15 it returns a {@link List}.
On 2013/05/01 15:41:01, yanivi wrote:
> Collection

Done.

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java
(right):

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java:165:
*
On 2013/05/01 15:41:01, yanivi wrote:
> @since 1.15

Done.

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java:263:
public AuthorizationRequestUrl setScopes(Collection<String> scopes) {
On 2013/05/01 15:41:01, yanivi wrote:
> @since 1.15

Done.

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:600:
new ArrayList<CredentialRefreshListener>();
On 2013/05/01 15:41:01, yanivi wrote:
> Lists.newArrayList();

Done.

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:805:
public Builder setRefreshListeners(Collection<CredentialRefreshListener>
refreshListeners) {
On 2013/05/01 15:41:01, yanivi wrote:
> @since 1.15

Done.

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:805:
public Builder setRefreshListeners(Collection<CredentialRefreshListener>
refreshListeners) {
On 2013/05/01 15:41:01, yanivi wrote:
> for maximum backwards compatibility, keep setRefreshListeners(List) but mark
it
> deprecated

Done.

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenRequest.java
(right):

https://codereview.appspot.com/8865045/diff/39001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenRequest.java:244:
*/
On 2013/05/01 15:41:01, yanivi wrote:
> @since 1.15

Done.

https://codereview.appspot.com/8865045/diff/39001/pom.xml
File pom.xml (right):

https://codereview.appspot.com/8865045/diff/39001/pom.xml#newcode425
pom.xml:425: <!-- TODO(peleyal): enable this, but for now disable due to Clirr
ArrayList cast bug
http://jira.codehaus.org/browse/MCLIRR-56

On 2013/05/01 15:41:01, yanivi wrote:
> [non-blocking] please file a bug to clirr-maven-plugin about this

[yanivi, 2013-05-01 21:59:25]

almost there....

https://codereview.appspot.com/8865045/diff/78001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java
(right):

https://codereview.appspot.com/8865045/diff/78001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:683:
// this.scopes = (scopes == null) ? Lists.newArrayList() :
Lists.newArrayList(scopes);
here's the fix: Lists.<String>newArrayList()

please change it to that

https://codereview.appspot.com/8865045/diff/78001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):

https://codereview.appspot.com/8865045/diff/78001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:819:
public Builder setRefreshListeners(List<CredentialRefreshListener>
refreshListeners) {
Sorry to do this to you, but I realize now your backwards-incompatible change
from List to Collection was actually a better idea.  The problem is that if you
have code like this:

setRefreshListeners(Arrays.asList(......));

it will be marked as deprecated, but actually it's fine.  We're better off
making a backwards-incompatible change, and it's at least a source incompatible
change, even if it is a binary-incompatible change.

https://codereview.appspot.com/8865045/diff/78001/google-oauth-client/src/tes...
File
google-oauth-client/src/test/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlowTest.java
(right):

https://codereview.appspot.com/8865045/diff/78001/google-oauth-client/src/tes...
google-oauth-client/src/test/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlowTest.java:118:
if (scopes != null) {
instead change the caller to pass in something like Collections.emptySet()

[peleyal, 2013-05-02 12:48:10]

https://codereview.appspot.com/8865045/diff/78001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java
(right):

https://codereview.appspot.com/8865045/diff/78001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:683:
// this.scopes = (scopes == null) ? Lists.newArrayList() :
Lists.newArrayList(scopes);
On 2013/05/01 21:59:25, yanivi wrote:
> here's the fix: Lists.<String>newArrayList()
> 
> please change it to that

Done.

https://codereview.appspot.com/8865045/diff/78001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):

https://codereview.appspot.com/8865045/diff/78001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:819:
public Builder setRefreshListeners(List<CredentialRefreshListener>
refreshListeners) {
On 2013/05/01 21:59:25, yanivi wrote:
> Sorry to do this to you, but I realize now your backwards-incompatible change
> from List to Collection was actually a better idea.  The problem is that if
you
> have code like this:
> 
> setRefreshListeners(Arrays.asList(......));
> 
> it will be marked as deprecated, but actually it's fine.  We're better off
> making a backwards-incompatible change, and it's at least a source
incompatible
> change, even if it is a binary-incompatible change.

Done.

https://codereview.appspot.com/8865045/diff/78001/google-oauth-client/src/tes...
File
google-oauth-client/src/test/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlowTest.java
(right):

https://codereview.appspot.com/8865045/diff/78001/google-oauth-client/src/tes...
google-oauth-client/src/test/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlowTest.java:118:
if (scopes != null) {
On 2013/05/01 21:59:25, yanivi wrote:
> instead change the caller to pass in something like Collections.emptySet()

Done.

[yanivi, 2013-05-02 13:32:55]

LGTM