Issue 827: api 1.17 & default: getEmailVerified now reads 'email_verified' field

MOE Sync

https://code.google.com/p/google-api-java-client/issues/detail?id=827

[peleyal, 2013-08-30 15:17:26]

I ran 
mvn clean install
mvn finbugs:check 
and 
mvn clirr:check

todo bien

[ngmiceli, 2013-08-30 15:31:17]

Please change the description to match what change is being exported: 

api 1.17 & default: getEmailVerified now reads 'email_verified' field

Don't forget to submit to both branches

LGTM

[peleyal, 2013-08-30 17:26:53]

Hi, 
I ran findbugs:check again and I encountered the following problem:

[INFO]
com.google.api.client.googleapis.auth.oauth2.GoogleIdToken$Payload.getEmailVerified()
has Boolean return type and returns explicit null
["com.google.api.client.googleapis.auth.oauth2.GoogleIdToken$Payload"] At
GoogleIdToken.java:[lines 91-322]



So, we changed the method to return Boolean, but in one of the cases we return
null. As a result unboxing will result in NullPointerException.

Findbugs full explanation:

NP: Method with Boolean return type returns explicit null
(NP_BOOLEAN_RETURN_NULL)

A method that returns either Boolean.TRUE, Boolean.FALSE or null is an accident
waiting to happen. This method can be invoked as though it returned a value of
type boolean, and the compiler will insert automatic unboxing of the Boolean
value. If a null value is returned, this will result in a NullPointerException.

NEED TO DECIDE WHAT DO TO. I don't understand this CL and is just a part of MOE
SYNC. Need to further investigate. Yaniv, why we need that behavior? Do we want
just to add it to findbugs-exclude file?

Thanks,
Let talk about it on Tuesday.

[ngmiceli, 2013-08-30 18:02:24]

Also, please note the related issue:
https://code.google.com/p/google-api-java-client/issues/detail?id=827

Add "Issue 827" to the description, and this codereview link to the issue.
Thanks

[yanivi, 2013-08-30 18:07:19]

Just suppress the Clirr check :)
It is there to prevent you from accidental backwards incompatible changes,
but in this case it is intentional.
-- Yaniv


On Fri, Aug 30, 2013 at 1:26 PM, <peleyal@google.com> wrote:

> Hi,
> I ran findbugs:check again and I encountered the following problem:
>
> [INFO]
> com.google.api.client.**googleapis.auth.oauth2.**GoogleIdToken$Payload.**
> getEmailVerified()
> has Boolean return type and returns explicit null
> ["com.google.api.client.**googleapis.auth.oauth2.**GoogleIdToken$Payload"]
> At GoogleIdToken.java:[lines 91-322]
>
>
>
> So, we changed the method to return Boolean, but in one of the cases we
> return null. As a result unboxing will result in NullPointerException.
>
> Findbugs full explanation:
>
> NP: Method with Boolean return type returns explicit null
> (NP_BOOLEAN_RETURN_NULL)
>
> A method that returns either Boolean.TRUE, Boolean.FALSE or null is an
> accident waiting to happen. This method can be invoked as though it
> returned a value of type boolean, and the compiler will insert automatic
> unboxing of the Boolean value. If a null value is returned, this will
> result in a NullPointerException.
>
> NEED TO DECIDE WHAT DO TO. I don't understand this CL and is just a part
> of MOE SYNC. Need to further investigate. Yaniv, why we need that
> behavior? Do we want just to add it to findbugs-exclude file?
>
> Thanks,
> Let talk about it on Tuesday.
>
>
https://codereview.appspot.**com/13431043/<https://codereview.appspot.com/134...
>

[ngmiceli, 2013-08-30 18:10:30]

This isn't a clirr check, but a findbugs. And the reasoning behind the bug
is logical. Anyone who does the following:
if(getEmailVerified())
will get a NullPointerException if it returns null. The user code is
totally plausible, and it seems really ugly to throw a NPE here.


On Fri, Aug 30, 2013 at 2:07 PM, Yaniv Inbar (יניב ענבר)
<yanivi@google.com>wrote:

> Just suppress the Clirr check :)
> It is there to prevent you from accidental backwards incompatible changes,
> but in this case it is intentional.
> -- Yaniv
>
>
> On Fri, Aug 30, 2013 at 1:26 PM, <peleyal@google.com> wrote:
>
>> Hi,
>> I ran findbugs:check again and I encountered the following problem:
>>
>> [INFO]
>> com.google.api.client.**googleapis.auth.oauth2.**GoogleIdToken$Payload.**
>> getEmailVerified()
>> has Boolean return type and returns explicit null
>> ["com.google.api.client.**googleapis.auth.oauth2.**
>> GoogleIdToken$Payload"]
>> At GoogleIdToken.java:[lines 91-322]
>>
>>
>>
>> So, we changed the method to return Boolean, but in one of the cases we
>> return null. As a result unboxing will result in NullPointerException.
>>
>> Findbugs full explanation:
>>
>> NP: Method with Boolean return type returns explicit null
>> (NP_BOOLEAN_RETURN_NULL)
>>
>> A method that returns either Boolean.TRUE, Boolean.FALSE or null is an
>> accident waiting to happen. This method can be invoked as though it
>> returned a value of type boolean, and the compiler will insert automatic
>> unboxing of the Boolean value. If a null value is returned, this will
>> result in a NullPointerException.
>>
>> NEED TO DECIDE WHAT DO TO. I don't understand this CL and is just a part
>> of MOE SYNC. Need to further investigate. Yaniv, why we need that
>> behavior? Do we want just to add it to findbugs-exclude file?
>>
>> Thanks,
>> Let talk about it on Tuesday.
>>
>>
https://codereview.appspot.**com/13431043/<https://codereview.appspot.com/134...
>>
>
>

[yanivi, 2013-09-03 12:56:11]

Sorry, you're right.  It should definitely be Boolean, not boolean.

On Friday, August 30, 2013, Nick Miceli wrote:

> This isn't a clirr check, but a findbugs. And the reasoning behind the bug
> is logical. Anyone who does the following:
> if(getEmailVerified())
> will get a NullPointerException if it returns null. The user code is
> totally plausible, and it seems really ugly to throw a NPE here.
>
>
> On Fri, Aug 30, 2013 at 2:07 PM, Yaniv Inbar (יניב ענבר) <
> yanivi@google.com <javascript:_e({}, 'cvml', 'yanivi@google.com');>>wrote:
>
>> Just suppress the Clirr check :)
>> It is there to prevent you from accidental backwards incompatible
>> changes, but in this case it is intentional.
>> -- Yaniv
>>
>>
>> On Fri, Aug 30, 2013 at 1:26 PM, <peleyal@google.com <javascript:_e({},
>> 'cvml', 'peleyal@google.com');>> wrote:
>>
>>> Hi,
>>> I ran findbugs:check again and I encountered the following problem:
>>>
>>> [INFO]
>>> com.google.api.client.**googleapis.auth.oauth2.**GoogleIdToken$Payload.*
>>> *getEmailVerified()
>>> has Boolean return type and returns explicit null
>>> ["com.google.api.client.**googleapis.auth.oauth2.**
>>> GoogleIdToken$Payload"]
>>> At GoogleIdToken.java:[lines 91-322]
>>>
>>>
>>>
>>> So, we changed the method to return Boolean, but in one of the cases we
>>> return null. As a result unboxing will result in NullPointerException.
>>>
>>> Findbugs full explanation:
>>>
>>> NP: Method with Boolean return type returns explicit null
>>> (NP_BOOLEAN_RETURN_NULL)
>>>
>>> A method that returns either Boolean.TRUE, Boolean.FALSE or null is an
>>> accident waiting to happen. This method can be invoked as though it
>>> returned a value of type boolean, and the compiler will insert automatic
>>> unboxing of the Boolean value. If a null value is returned, this will
>>> result in a NullPointerException.
>>>
>>> NEED TO DECIDE WHAT DO TO. I don't understand this CL and is just a part
>>> of MOE SYNC. Need to further investigate. Yaniv, why we need that
>>> behavior? Do we want just to add it to findbugs-exclude file?
>>>
>>> Thanks,
>>> Let talk about it on Tuesday.
>>>
>>>
https://codereview.appspot.**com/13431043/<https://codereview.appspot.com/134...
>>>
>>
>>
>

[peleyal, 2013-09-03 12:58:19]

So, should I just add it to findbugs excludes?
Why is it OK?

Eyal


On Tue, Sep 3, 2013 at 8:56 AM, Yaniv Inbar (יניב ענבר)
<yanivi@google.com>wrote:

> Sorry, you're right.  It should definitely be Boolean, not boolean.
>
> On Friday, August 30, 2013, Nick Miceli wrote:
>
>> This isn't a clirr check, but a findbugs. And the reasoning behind the
>> bug is logical. Anyone who does the following:
>> if(getEmailVerified())
>> will get a NullPointerException if it returns null. The user code is
>> totally plausible, and it seems really ugly to throw a NPE here.
>>
>>
>> On Fri, Aug 30, 2013 at 2:07 PM, Yaniv Inbar (יניב ענבר) <
>> yanivi@google.com> wrote:
>>
>>> Just suppress the Clirr check :)
>>> It is there to prevent you from accidental backwards incompatible
>>> changes, but in this case it is intentional.
>>> -- Yaniv
>>>
>>>
>>> On Fri, Aug 30, 2013 at 1:26 PM, <peleyal@google.com> wrote:
>>>
>>>> Hi,
>>>> I ran findbugs:check again and I encountered the following problem:
>>>>
>>>> [INFO]
>>>> com.google.api.client.**googleapis.auth.oauth2.**GoogleIdToken$Payload.
>>>> **getEmailVerified()
>>>> has Boolean return type and returns explicit null
>>>> ["com.google.api.client.**googleapis.auth.oauth2.**
>>>> GoogleIdToken$Payload"]
>>>> At GoogleIdToken.java:[lines 91-322]
>>>>
>>>>
>>>>
>>>> So, we changed the method to return Boolean, but in one of the cases we
>>>> return null. As a result unboxing will result in NullPointerException.
>>>>
>>>> Findbugs full explanation:
>>>>
>>>> NP: Method with Boolean return type returns explicit null
>>>> (NP_BOOLEAN_RETURN_NULL)
>>>>
>>>> A method that returns either Boolean.TRUE, Boolean.FALSE or null is an
>>>> accident waiting to happen. This method can be invoked as though it
>>>> returned a value of type boolean, and the compiler will insert automatic
>>>> unboxing of the Boolean value. If a null value is returned, this will
>>>> result in a NullPointerException.
>>>>
>>>> NEED TO DECIDE WHAT DO TO. I don't understand this CL and is just a part
>>>> of MOE SYNC. Need to further investigate. Yaniv, why we need that
>>>> behavior? Do we want just to add it to findbugs-exclude file?
>>>>
>>>> Thanks,
>>>> Let talk about it on Tuesday.
>>>>
>>>>
https://codereview.appspot.**com/13431043/<https://codereview.appspot.com/134...
>>>>
>>>
>>>
>>

[yanivi, 2013-09-03 13:08:21]

This is what we always do for JSON boolean values that might not exist in the
response.  If it is true or false it means the value was specified and we know
its value.  So null is the only correct way to represent the case where we don't
know the value.  NPE is the correct behavior because it represents a flaw in the
code, just like the way we handle the case of a null value being sent to a
parameter that shouldn't be null.  The correct way to write the application code
is:

if(getEmailVerified() != null && getEmailVerified())

[peleyal, 2013-09-03 14:45:46]

Two issues: 

1. I changed findbugs-exclude to exclude the Boolean issue.
2. Boolean.valueOf("5") doesn't throw any exception so I changed also the test.
It just returns false.

[yanivi, 2013-09-03 19:30:58]

https://codereview.appspot.com/13431043/diff/12001/findbugs-exclude.xml
File findbugs-exclude.xml (right):

https://codereview.appspot.com/13431043/diff/12001/findbugs-exclude.xml#newcode4
findbugs-exclude.xml:4: <!--
http://findbugs.sourceforge.net/bugDescriptions.html -->
can you please also add a link to
http://findbugs.sourceforge.net/manual/filter.html ?

https://codereview.appspot.com/13431043/diff/12001/findbugs-exclude.xml#newco...
findbugs-exclude.xml:66: <Class
name="com.google.api.client.googleapis.auth.oauth2.GoogleIdToken$Payload"/>
can we remove this line so we don't show this error on any classes?

https://codereview.appspot.com/13431043/diff/12001/google-api-client/src/main...
File
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdToken.java
(right):

https://codereview.appspot.com/13431043/diff/12001/google-api-client/src/main...
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdToken.java:222:
return Boolean.valueOf(emailVerified.toString());
why this change?  I don't think we should change it from (String) emailVerified

[peleyal, 2013-09-03 20:52:30]

https://codereview.appspot.com/13431043/diff/12001/findbugs-exclude.xml
File findbugs-exclude.xml (right):

https://codereview.appspot.com/13431043/diff/12001/findbugs-exclude.xml#newcode4
findbugs-exclude.xml:4: <!--
http://findbugs.sourceforge.net/bugDescriptions.html -->
On 2013/09/03 19:30:59, yanivi wrote:
> can you please also add a link to
> http://findbugs.sourceforge.net/manual/filter.html ?

Done.

https://codereview.appspot.com/13431043/diff/12001/findbugs-exclude.xml#newco...
findbugs-exclude.xml:66: <Class
name="com.google.api.client.googleapis.auth.oauth2.GoogleIdToken$Payload"/>
On 2013/09/03 19:30:59, yanivi wrote:
> can we remove this line so we don't show this error on any classes?

Done.

https://codereview.appspot.com/13431043/diff/12001/google-api-client/src/main...
File
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdToken.java
(right):

https://codereview.appspot.com/13431043/diff/12001/google-api-client/src/main...
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdToken.java:222:
return Boolean.valueOf(emailVerified.toString());
On 2013/09/03 19:30:59, yanivi wrote:
> why this change?  I don't think we should change it from (String)
emailVerified

Done.

[yanivi, 2013-09-03 23:00:29]

default branch: LGTM

1.17 branch: please also update clirr-ignored-differences.xml; otherwise:

[ERROR] com.google.api.client.googleapis.auth.oauth2.GoogleIdToken$Payload:
Return type of method 'public boolean getEmailVerified()' has been changed to
java.lang.Boolean
[ERROR] com.google.api.client.googleapis.auth.oauth2.GoogleIdToken$Payload:
Parameter 1 of 'public
com.google.api.client.googleapis.auth.oauth2.GoogleIdToken$Payload
setEmailVerified(boolean)' has changed its type to java.lang.Boolean

https://codereview.appspot.com/13431043/diff/27001/google-api-client/src/test...
File
google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdTokenTest.java
(right):

https://codereview.appspot.com/13431043/diff/27001/google-api-client/src/test...
google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdTokenTest.java:97:
}
For some reason I see a diff between this and patch set 1 on the last line. 
Normally I wouldn't care, but in this case this might break an equivalence with
the internal one.  Please figure out which one matches the internal one.

[peleyal, 2013-09-03 23:33:34]

I just copy-pasted it from the internal
I'm going to push now on default + 1.17

[ngmiceli, 2013-09-03 23:35:50]

On 2013/09/03 23:33:34, peleyal wrote:
> I just copy-pasted it from the internal
> I'm going to push now on default + 1.17

This is what I tend to do:
hg commit
hg push
hg update 1.17
hg import --no-commit <patch from codereview>
hg commit
hg push

Rather than copy-pasta, as it ensures you're using the exact same change as was
LGTM'd

[peleyal, 2013-09-03 23:38:04]

On 2013/09/03 23:35:50, ngmiceli wrote:
> On 2013/09/03 23:33:34, peleyal wrote:
> > I just copy-pasted it from the internal
> > I'm going to push now on default + 1.17
> 
> This is what I tend to do:
> hg commit
> hg push
> hg update 1.17
> hg import --no-commit <patch from codereview>
> hg commit
> hg push
> 
> Rather than copy-pasta, as it ensures you're using the exact same change as
was
> LGTM'd

I meant I copy-pasted the last file
(google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdTokenTest.java)
and we still see some space diff....

Thanks Nick for your help :)

[peleyal, 2013-09-03 23:46:08]

I also didn't see the clirr error when I ran mvn clirr:check
I'm pushing to 1.17 now.


On Tue, Sep 3, 2013 at 7:38 PM, <peleyal@google.com> wrote:

> On 2013/09/03 23:35:50, ngmiceli wrote:
>
>> On 2013/09/03 23:33:34, peleyal wrote:
>> > I just copy-pasted it from the internal
>> > I'm going to push now on default + 1.17
>>
>
>  This is what I tend to do:
>> hg commit
>> hg push
>> hg update 1.17
>> hg import --no-commit <patch from codereview>
>> hg commit
>> hg push
>>
>
>  Rather than copy-pasta, as it ensures you're using the exact same
>>
> change as was
>
>> LGTM'd
>>
>
> I meant I copy-pasted the last file
> (google-api-client/src/test/**java/com/google/api/client/**
> googleapis/auth/oauth2/**GoogleIdTokenTest.java)
> and we still see some space diff....
>
> Thanks Nick for your help :)
>
>
https://codereview.appspot.**com/13431043/<https://codereview.appspot.com/134...
>

[yanivi, 2013-09-03 23:51:17]

Please fix the Clirr error in a separate changeset then:

cd google-api-client
mvn -q clirr:check

[ERROR] com.google.api.client.googleapis.auth.oauth2.GoogleIdToken$Payload:
Return type of method 'public boolean getEmailVerified()' has been changed to
java.lang.Boolean
[ERROR] com.google.api.client.googleapis.auth.oauth2.GoogleIdToken$Payload:
Parameter 1 of 'public
com.google.api.client.googleapis.auth.oauth2.GoogleIdToken$Payload
setEmailVerified(boolean)' has changed its type to java.lang.Boolean

[peleyal, 2013-09-04 13:10:19]

Added a new CL: https://codereview.appspot.com/13525043/ for that.


On Tue, Sep 3, 2013 at 7:51 PM, <yanivi@google.com> wrote:

> Please fix the Clirr error in a separate changeset then:
>
> cd google-api-client
> mvn -q clirr:check
>
>
> [ERROR]
> com.google.api.client.**googleapis.auth.oauth2.**GoogleIdToken$Payload:
> Return type of method 'public boolean getEmailVerified()' has been
> changed to java.lang.Boolean
> [ERROR]
> com.google.api.client.**googleapis.auth.oauth2.**GoogleIdToken$Payload:
> Parameter 1 of 'public
> com.google.api.client.**googleapis.auth.oauth2.**GoogleIdToken$Payload
> setEmailVerified(boolean)' has changed its type to java.lang.Boolean
>
>
>
https://codereview.appspot.**com/13431043/<https://codereview.appspot.com/134...
>