Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(24)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 10053044: api issue 793: New GooglePublicCerts and improvements to GoogleIdTokenVerifier (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 4 months ago by yanivi
Modified:
13 years, 2 months ago
Reviewers:
peleyal
CC:
ngmiceli
Base URL:
https://code.google.com/p/google-api-java-client/
Visibility:
Public.

Description

api issue 793: New GooglePublicCerts and improvements to GoogleIdTokenVerifier https://code.google.com/p/google-api-java-client/issues/detail?id=793

Patch Set 1 #

Patch Set 2 : name improvements & fix javadoc #

Patch Set 3 : minor #

Patch Set 4 : sync #

Patch Set 5 : update to 1.17 #

Patch Set 6 : minor #

Total comments: 18

Patch Set 7 : fixes #

Total comments: 1

Patch Set 8 : rename to GooglePublicKeysManager #

Messages

Total messages: 10
yanivi
13 years, 4 months ago (2013-06-05 19:58:23 UTC) #1
yanivi
13 years, 4 months ago (2013-06-06 14:32:24 UTC) #2
yanivi
friendly ping
13 years, 4 months ago (2013-06-11 11:52:45 UTC) #3
yanivi
friendly ping
13 years, 4 months ago (2013-06-17 17:38:58 UTC) #4
yanivi
ping
13 years, 4 months ago (2013-06-20 16:52:14 UTC) #5
yanivi
ping :)
13 years, 2 months ago (2013-08-09 19:09:55 UTC) #6
peleyal
Sorry that it took me forever. Hope that my comments make sense. Have a great ...
13 years, 2 months ago (2013-08-17 00:28:21 UTC) #7
yanivi
https://codereview.appspot.com/10053044/diff/37001/google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdTokenVerifier.java File google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdTokenVerifier.java (left): https://codereview.appspot.com/10053044/diff/37001/google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdTokenVerifier.java#oldcode127 google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdTokenVerifier.java:127: transport = builder.transport; On 2013/08/17 00:28:21, peleyal wrote: > ...
13 years, 2 months ago (2013-08-19 16:24:27 UTC) #8
peleyal
LGTM (sorry for the months of delay) Just notice my optional comments. https://codereview.appspot.com/10053044/diff/37001/google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdTokenVerifier.java File google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdTokenVerifier.java ...
13 years, 2 months ago (2013-08-20 20:07:31 UTC) #9
yanivi
13 years, 2 months ago (2013-08-20 21:16:05 UTC) #10 
Submitting now...

On Tuesday, August 20, 2013, wrote:

> LGTM (sorry for the months of delay)
> Just notice my optional comments.
>
>
>
> https://codereview.appspot.**com/10053044/diff/37001/**
> google-api-client/src/main/**java/com/google/api/client/**
>
googleapis/auth/oauth2/**GoogleIdTokenVerifier.java<https://codereview.appspot.com/10053044/diff/37001/google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdTokenVerifier.java>
> File
> google-api-client/src/main/**java/com/google/api/client/**
> googleapis/auth/oauth2/**GoogleIdTokenVerifier.java
> (right):
>
> https://codereview.appspot.**com/10053044/diff/37001/**
> google-api-client/src/main/**java/com/google/api/client/**
>
googleapis/auth/oauth2/**GoogleIdTokenVerifier.java#**newcode86<https://codereview.appspot.com/10053044/diff/37001/google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleIdTokenVerifier.java#newcode86>
> google-api-client/src/main/**java/com/google/api/client/**
> googleapis/auth/oauth2/**GoogleIdTokenVerifier.java:86:
> * Returns the Google public certificates manager.
> [optional]
> I don't like this name, can you rephrase?
>

Done: renamed  to GooglePublicKeysManager

On 2013/08/19 16:24:27, yanivi wrote:
>
>> On 2013/08/17 00:28:21, peleyal wrote:
>> > manager? where did that come from?
>>
>
>  It is initialized inside the Builder.
>>
>
> https://codereview.appspot.**com/10053044/diff/37001/**
> google-api-client/src/main/**java/com/google/api/client/**
>
googleapis/auth/oauth2/**GooglePublicCerts.java<https://codereview.appspot.com/10053044/diff/37001/google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GooglePublicCerts.java>
> File
> google-api-client/src/main/**java/com/google/api/client/**
> googleapis/auth/oauth2/**GooglePublicCerts.java
> (right):
>
> https://codereview.appspot.**com/10053044/diff/37001/**
> google-api-client/src/main/**java/com/google/api/client/**
>
googleapis/auth/oauth2/**GooglePublicCerts.java#**newcode90<https://codereview.appspot.com/10053044/diff/37001/google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GooglePublicCerts.java#newcode90>
> google-api-client/src/main/**java/com/google/api/client/**
> googleapis/auth/oauth2/**GooglePublicCerts.java:90:
> public GooglePublicCerts(**HttpTransport transport, JsonFactory
> jsonFactory) {
> [optional]
> Don't like it, but ACK.
> I prefer to have here only one constructor that uses the builder. I'm
> pretty sure we are doing so in some classes in the library (but I'm not
> sure... you know better than I'm)


Again, I'm following standard style, and if any classes aren't following it
then they should be updated to follow the latest standard style.

On 2013/08/19 16:24:27, yanivi wrote:
>
>> On 2013/08/17 00:28:21, peleyal wrote:
>> > Why you want to provide this constructor, isn't it enough to have
>>
> just the
>
>> > protected GooglePublicCerts(Builder builder) ?
>>
>
>  This is the builder pattern style we are using throughout the library.
>>
>  We
>
>> provide a constructor with just the required parameters, and then
>>
> provide a
>
>> builder-based constructor if you want to specify optional parameters.
>>
>
> https://codereview.appspot.**com/10053044/diff/44001/**
> google-api-client/src/test/**java/com/google/api/client/**
>
googleapis/auth/oauth2/**GooglePublicCertsTest.java<https://codereview.appspot.com/10053044/diff/44001/google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/GooglePublicCertsTest.java>
> File
> google-api-client/src/test/**java/com/google/api/client/**
> googleapis/auth/oauth2/**GooglePublicCertsTest.java
> (right):
>
> https://codereview.appspot.**com/10053044/diff/44001/**
> google-api-client/src/test/**java/com/google/api/client/**
>
googleapis/auth/oauth2/**GooglePublicCertsTest.java#**newcode124<https://codereview.appspot.com/10053044/diff/44001/google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/GooglePublicCertsTest.java#newcode124>
> google-api-client/src/test/**java/com/google/api/client/**
> googleapis/auth/oauth2/**GooglePublicCertsTest.java:**124:
> new HttpHeaders().setAge(345L).**setCacheControl("max-age=" + MAX_AGE)));
> [optional]
> I prefer 345L to be a const too, because it's shared in 3 different
> place in this method.
> Also consdier to define
>
> const int EXPECTED_TIME = 12000;
> const int AGE = 345L;
>
> assertEquals(EXPECTED_TIME, certs.getCacheTimeInSec(
> new HttpHeaders().setAge(AGE).**setCacheControl("max-age=" + MAX_AGE)));
>
> It looks better to me.
>

I disagree when it comes to unit tests.  I like seeing the numbers
directly.  It makes the test more readable in this case.


>
>
https://codereview.appspot.**com/10053044/<https://codereview.appspot.com/100...
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b