Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add HttpTransportFactory class, make credentials serializable, add tests #69

Merged
merged 8 commits into from
Oct 10, 2016
Merged

Add HttpTransportFactory class, make credentials serializable, add tests #69

merged 8 commits into from
Oct 10, 2016

Conversation

mziccard
Copy link
Contributor

No description provided.

@mziccard
Copy link
Contributor Author

This also updates the guava dependency from guava-jdk5:13.0 to guava:19.0 (see issue #68).

/cc @garrettjonesgoogle @anthmgoogle

garrettjonesgoogle
garrettjonesgoogle reviewed Sep 26, 2016
View reviewed changes
Copy link
Member

@garrettjonesgoogle garrettjonesgoogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have some very minor things. Thanks for the contribution!

appengine/java/com/google/auth/appengine/AppEngineCredentials.java
import com.google.appengine.api.appidentity.AppIdentityService;
import com.google.appengine.api.appidentity.AppIdentityService.GetAccessTokenResult;
import com.google.appengine.api.appidentity.AppIdentityServiceFactory;
import com.google.appengine.repackaged.com.google.common.collect.ImmutableSet;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

oauth2_http/java/com/google/auth/oauth2/ServiceAccountJwtAccessCredentials.java
// Allow clock to be overriden by test code
Clock clock = Clock.SYSTEM;
// Until we expose this to the users it can remain transient and non-serializable
transient Clock clock = Clock.SYSTEM;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

oauth2_http/java/com/google/auth/oauth2/UserCredentials.java
public String toString() {
return toStringHelper()
.add("clientId", clientId)
.add("clientSecret", clientSecret)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@mziccard
Copy link
Contributor Author

mziccard commented Sep 27, 2016 •
edited
Loading

Also related to #68 we should decide the java version we want to support adding something like the following to the pom.xml:

      <plugin>
        <artifactId>maven-compiler-plugin</artifactId>
        <version>3.5.1</version>
        <configuration>
          <source>1.7</source>
          <target>1.7</target>
          <encoding>UTF-8</encoding>
          <compilerArgument>-Xlint:unchecked</compilerArgument>
        </configuration>
      </plugin>

Right now we are not specifying any java version, which defaults us to 1.5. This is a very old version and prevents from using features like try-with-resources and diamond operator. Any reason we are targeting 1.5? Can we explicitly target 1.7 instead?

/cc @anthmgoogle @lesv @garrettjonesgoogle

@garrettjonesgoogle
Copy link
Member

The change LGTM. It might be good to have one more reviewer.

Regarding Java 1.5 vs 1.7 - @anthmgoogle should have more background on that than me.

anthmgoogle
anthmgoogle approved these changes Oct 5, 2016
View reviewed changes
Copy link
Contributor

@anthmgoogle anthmgoogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me from a product perspective.

I had some comments about the unit tests, but none are serious so no concerns with merging this in its current form, although some consideration or discussion of the comments may be good.

appengine/javatests/com/google/auth/appengine/AppEngineCredentialsTest.java
MockAppIdentityService appIdentity = new MockAppIdentityService();
GoogleCredentials credentials = new AppEngineCredentials(emptyScopes, appIdentity);
GoogleCredentials otherCredentials = new AppEngineCredentials(scopes, appIdentity);
assertFalse(credentials.hashCode() == otherCredentials.hashCode());

This comment was marked as spam.

Sign in to view

oauth2_http/javatests/com/google/auth/oauth2/BaseSerializationTest.java
public class BaseSerializationTest {

@SuppressWarnings("unchecked")
public <T> T serializeAndDeserialize(T obj) throws IOException, ClassNotFoundException {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

oauth2_http/javatests/com/google/auth/oauth2/ServiceAccountCredentialsTest.java
}

@Test
public void equals_false() throws IOException {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@mziccard
Copy link
Contributor Author

mziccard commented Oct 5, 2016 •
edited
Loading

@anthmgoogle Thanks for the comments. Are you fine with using guava 19.0 instead of guava-jdk5? Also, can we be explicit in targeting java 7 or are there requirements to support older versions? We could use try-with-resources and diamond operators to make our code look cleaner.

@anthmgoogle
Copy link
Contributor

@mziccard regarding Java versions and dependencies: The original plan with dependencies for this library was to be consistent with the google-oauth-java-client and related libraries, which were constrained to older versions of Java because of the need to work on Android. For example, 2 years ago you could not using Java compiler 1.7 for that reason and had to stick to 1.6. I don't know if there is any existing use on Android, or if these constraints still apply. To play it safe I would keep the compiler version at 1.6 unless we confirm that 1.7 works on Android now.

No concerns I'm aware of on the Guava version.

@lesv
Copy link
Contributor

lesv commented Oct 5, 2016

https://developer.android.com/guide/platform/j8-jack.html
I'm not recommending a change, just providing data.

@mziccard
Copy link
Contributor Author

mziccard commented Oct 6, 2016

From @lesv's link it seems that java7 should be safe to use. Also doing some digging in the library I found out it already uses classes introduced only since 1.7, see StandardCharsets usage, for instance.

@anthmgoogle
Copy link
Contributor

Cool. No concerns with these dependencies then.

It sounds like we need to tread more carefully with Java 8.

@mziccard
Copy link
Contributor Author

mziccard commented Oct 7, 2016

I added 4 commits that fix all the comments. PTAL.

@mziccard mziccard mentioned this pull request Oct 7, 2016
Closed
anthmgoogle
anthmgoogle approved these changes Oct 7, 2016
View reviewed changes
Copy link
Contributor

@anthmgoogle anthmgoogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed deltas. LGTM.

@mziccard
Copy link
Contributor Author

Can you guys merge this PR? I still don't have write access here. Write access would also allow me to setup Travis. Do you know who has the rights to add users to this repo?

@garrettjonesgoogle garrettjonesgoogle merged commit 905b597 into googleapis:master Oct 10, 2016
@anthmgoogle
Copy link
Contributor

I am one of the admins. I have given you admin access.

@mziccard
Copy link
Contributor Author

Thank you @anthmgoogle

This was referenced Oct 28, 2016
Closed
Closed
This was referenced Jun 19, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garrettjonesgoogle garrettjonesgoogle garrettjonesgoogle left review comments

@anthmgoogle anthmgoogle anthmgoogle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mziccard @garrettjonesgoogle @anthmgoogle @lesv