oauth issues 24, 41 & 56: OAuth 2.0 final standard and support password & client credentials

oauth issues 24, 41 & 56: OAuth 2.0 final standard and support password & client credentials

http://code.google.com/p/google-oauth-java-client/issues/detail?id=56
OAuth 2.0 final standard

http://code.google.com/p/google-oauth-java-client/issues/detail?id=41
OAuth 2.0 Client Credentials Grant Type

http://code.google.com/p/google-oauth-java-client/issues/detail?id=24
OAuth 2.0 Username and Password Grant Type

http://code.google.com/p/google-oauth-java-client/issues/detail?id=66
BearerToken.authorizationHeaderAccessMethod() should use getAuthorizationAsList

[ngmiceli, 2013-01-14 15:50:41]

Just a couple of thoughts regarding constructing AuthorizationCodeFlow.java

https://codereview.appspot.com/7060072/diff/3001/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/7060072/diff/3001/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:106:
public AuthorizationCodeFlow(AccessMethod method,
Since we're introducing this now, we're not easily going to be able to remove
this constructor in the future (deprecate in 1.15, can't remove 1.16?). Is this
the kind of constructor that changes often, or that you could see another
parameter needing to be added in the future? If so, perhaps we should only use
the "builder" or some other object-construction pattern rather than introducing
another constructor to the API
The flip side is if you have confidence that this contract will be the one we
want to stick with in the future. However, the two deprecated constructors below
give me worry

https://codereview.appspot.com/7060072/diff/3001/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:127:
protected AuthorizationCodeFlow(Builder builder) {
This isn't really the "Builder pattern" as I learned it.  It seems like you're
mostly delegating the responsibilities of the constructor to a sort of static
factory object. This doesn't really solve the problem of changing dependencies
though. If we want to change what the "constructor" takes in, we still have to
deprecate something- just a Builder constructor instead of the class'
constructor.
Depending on the use-case of the constructor I see two possible alternatives,
just worth considering before we lock ourselves into this on the 1.14 release.

If the constructor tends to take in similar combinations of parameters, we could
use the actual Builder pattern and make an abstract interface which is
implemented by a few concrete builders that enclose these use cases. Then anyone
can roll their own concrete implementation for alternate uses.

If most of the parameters end up as null anyway, and one tends to only specify a
couple of them, perhaps assigning the values to the Builder object via mutator
methods rather than the constructor would be more efficient. Then the user can
pick and choose any combination of parameters, and the amount of code will be
nicer to read (rather than null,null,null,object,"",null, etc). You seem to
already do something like this in Credential.java

Of course, if its neither of these, it gets a little trickier to motivate a
change...

[yanivi, 2013-01-14 19:09:41]

https://codereview.appspot.com/7060072/diff/3001/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/7060072/diff/3001/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:106:
public AuthorizationCodeFlow(AccessMethod method,
On 2013/01/14 15:50:42, ngmiceli wrote:
> Since we're introducing this now, we're not easily going to be able to remove
> this constructor in the future (deprecate in 1.15, can't remove 1.16?). Is
this
> the kind of constructor that changes often, or that you could see another
> parameter needing to be added in the future? If so, perhaps we should only use
> the "builder" or some other object-construction pattern rather than
introducing
> another constructor to the API
> The flip side is if you have confidence that this contract will be the one we
> want to stick with in the future. However, the two deprecated constructors
below
> give me worry

The style guideline is that there are two constructors: a public one with just
the basic required parameters and a protected one that takes Builder.  It will
be very rare that we'd need to add new constructors because it is rare to add
required parameters.

In this case, we have constructors that have too many parameters, and most
importantly the style guideline is that the constructor for the Builder should
match the constructor here.  So that's why I did it this way.

https://codereview.appspot.com/7060072/diff/3001/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeFlow.java:127:
protected AuthorizationCodeFlow(Builder builder) {
On 2013/01/14 15:50:42, ngmiceli wrote:
> This isn't really the "Builder pattern" as I learned it.  It seems like you're
> mostly delegating the responsibilities of the constructor to a sort of static
> factory object. This doesn't really solve the problem of changing dependencies
> though. If we want to change what the "constructor" takes in, we still have to
> deprecate something- just a Builder constructor instead of the class'
> constructor.
> Depending on the use-case of the constructor I see two possible alternatives,
> just worth considering before we lock ourselves into this on the 1.14 release.
> 
> If the constructor tends to take in similar combinations of parameters, we
could
> use the actual Builder pattern and make an abstract interface which is
> implemented by a few concrete builders that enclose these use cases. Then
anyone
> can roll their own concrete implementation for alternate uses.
> 
> If most of the parameters end up as null anyway, and one tends to only specify
a
> couple of them, perhaps assigning the values to the Builder object via mutator
> methods rather than the constructor would be more efficient. Then the user can
> pick and choose any combination of parameters, and the amount of code will be
> nicer to read (rather than null,null,null,object,"",null, etc). You seem to
> already do something like this in Credential.java
> 
> Of course, if its neither of these, it gets a little trickier to motivate a
> change...

No, because the Builder constructor only takes the basic required parameters. 
That set of required parameters is unlikely to change.  So our style guideline
would match your second proposal already, but with the advantage that we have
compile-time checking that required parameters are set.  That's something that
we've gotten specific feedback for in the past that compile-time checking of
required parameters was important.  If only provided mutators, we wouldn't be
able to do that.

[ngmiceli, 2013-01-14 21:19:36]

SGTM & LGTM
Thanks for taking the time to share the thought behind the new style with me

[yanivi, 2013-01-14 21:21:49]

(FYI waiting for review on https://codereview.appspot.com/7069069/ before
submitting)