From 3e6cc1487c8bfe3abe542d2aa5c1bc3779503740 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Mon, 29 Oct 2018 16:28:26 -0700 Subject: [PATCH 1/8] Add tests for missing os.version system property --- .../AbstractGoogleClientRequestTest.java | 50 ++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java b/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java index cb25bc00c..c6bc5628d 100644 --- a/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java +++ b/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java @@ -32,6 +32,7 @@ import com.google.api.client.util.StringUtils; import java.io.IOException; import java.util.Arrays; +import java.util.Properties; import junit.framework.TestCase; /** @@ -50,6 +51,23 @@ public class AbstractGoogleClientRequestTest extends TestCase { "{\"error\":{\"code\":401,\"errors\":[{\"domain\":\"global\"," + "\"location\":\"Authorization\",\"locationType\":\"header\"," + "\"message\":\"me\",\"reason\":\"authError\"}],\"message\":\"me\"}}"; + private Properties originalProperties; + + @Override + protected void setUp() throws Exception { + super.setUp(); + + // save the original system properties so we can mock them out + this.originalProperties = System.getProperties(); + } + + @Override + protected void tearDown() throws Exception { + // restore the original system properties + System.setProperties(originalProperties); + + super.tearDown(); + } public void testExecuteUnparsed_error() throws Exception { HttpTransport transport = new MockHttpTransport() { @@ -208,13 +226,42 @@ public void testSetsApiClientHeader() throws Exception { request.executeUnparsed(); } + public void testSetsApiClientHeaderWithOsVersion() throws Exception { + System.setProperty("os.name", "My OS"); + System.setProperty("os.version", "1.2.3"); + HttpTransport transport = new AssertHeaderTransport("X-Goog-Api-Client", ".* my-os/1.2.3"); + MockGoogleClient client = new MockGoogleClient.Builder( + transport, ROOT_URL, SERVICE_PATH, JSON_OBJECT_PARSER, null).build(); + MockGoogleClientRequest request = + new MockGoogleClientRequest(client, HttpMethods.GET, URI_TEMPLATE, null, Void.class); + request.executeUnparsed(); + } + + public void testSetsApiClientHeaderWithoutOsVersion() throws Exception { + System.setProperty("os.name", "My OS"); + System.clearProperty("os.version"); + + HttpTransport transport = new AssertHeaderTransport("X-Goog-Api-Client", ".*my-os.*", false); + MockGoogleClient client = new MockGoogleClient.Builder( + transport, ROOT_URL, SERVICE_PATH, JSON_OBJECT_PARSER, null).build(); + MockGoogleClientRequest request = + new MockGoogleClientRequest(client, HttpMethods.GET, URI_TEMPLATE, null, Void.class); + request.executeUnparsed(); + } + private class AssertHeaderTransport extends MockHttpTransport { String expectedHeader; String expectedHeaderValue; + boolean expected; AssertHeaderTransport(String header, String value) { + this(header, value, true); + } + + AssertHeaderTransport(String header, String value, boolean match) { expectedHeader = header; expectedHeaderValue = value; + expected = match; } @Override @@ -223,12 +270,13 @@ public LowLevelHttpRequest buildRequest(String method, String url) throws IOExce @Override public LowLevelHttpResponse execute() throws IOException { String firstHeader = getFirstHeaderValue(expectedHeader); - assertTrue( + assertEquals( String.format( "Expected header value to match %s, instead got %s.", expectedHeaderValue, firstHeader ), + expected, firstHeader.matches(expectedHeaderValue) ); return new MockLowLevelHttpResponse(); From 342c608b96d9ffac5861fb0be78e95b93ad7d88f Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Mon, 29 Oct 2018 16:34:08 -0700 Subject: [PATCH 2/8] Only append the OS name/version if we can get them from the system properties --- .../services/AbstractGoogleClientRequest.java | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java b/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java index d17bf1b10..c3e0b266d 100644 --- a/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java +++ b/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java @@ -139,16 +139,14 @@ private static class ApiClientVersion { private static final String JAVA_VERSION = getJavaVersion(); private static final String OS_NAME = formatName(System.getProperty("os.name")); private static final String OS_VERSION = formatSemver(System.getProperty("os.version")); + private static final String HEADER_TEMPLATE = buildHeaderTemplate(); private static String build(AbstractGoogleClient client) { // TODO(chingor): add the API version from the generated client return String.format( - "java/%s http-google-%s/%s %s/%s", - JAVA_VERSION, + HEADER_TEMPLATE, formatName(client.getClass().getSimpleName()), - formatSemver(GoogleUtils.VERSION), - OS_NAME, - OS_VERSION + formatSemver(GoogleUtils.VERSION) ); } @@ -168,6 +166,10 @@ private static String formatName(String name) { } private static String formatSemver(String version) { + if (version == null) { + return null; + } + // Take only the semver version: x.y.z-a_b_c -> x.y.z Matcher m = Pattern.compile("(\\d+\\.\\d+\\.\\d+).*").matcher(version); if (m.find()) { @@ -176,6 +178,19 @@ private static String formatSemver(String version) { return version; } } + + private static String buildHeaderTemplate() { + StringBuilder sb = new StringBuilder("java/"); + sb.append(JAVA_VERSION); + sb.append(" http-google-%s/%s"); + if (OS_NAME != null && OS_VERSION != null) { + sb.append(" "); + sb.append(OS_NAME); + sb.append("/"); + sb.append(OS_VERSION); + } + return sb.toString(); + } } /** Returns whether to disable GZip compression of HTTP content. */ From 9dacfe8c3e7ce76ea64492ec87ce53165ae8cc9d Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Mon, 29 Oct 2018 16:53:08 -0700 Subject: [PATCH 3/8] Fix test allow testing different system properties --- .../services/AbstractGoogleClientRequest.java | 6 ++++-- .../AbstractGoogleClientRequestTest.java | 19 +++++++++++-------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java b/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java index c3e0b266d..ee794dfb2 100644 --- a/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java +++ b/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java @@ -35,6 +35,7 @@ import com.google.api.client.util.GenericData; import com.google.api.client.util.Preconditions; +import com.google.common.annotations.VisibleForTesting; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -135,13 +136,14 @@ protected AbstractGoogleClientRequest(AbstractGoogleClient abstractGoogleClient, * See * */ - private static class ApiClientVersion { + static class ApiClientVersion { private static final String JAVA_VERSION = getJavaVersion(); private static final String OS_NAME = formatName(System.getProperty("os.name")); private static final String OS_VERSION = formatSemver(System.getProperty("os.version")); private static final String HEADER_TEMPLATE = buildHeaderTemplate(); - private static String build(AbstractGoogleClient client) { + @VisibleForTesting + static String build(AbstractGoogleClient client) { // TODO(chingor): add the API version from the generated client return String.format( HEADER_TEMPLATE, diff --git a/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java b/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java index c6bc5628d..5f7ca36f6 100644 --- a/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java +++ b/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java @@ -12,6 +12,7 @@ package com.google.api.client.googleapis.services; +import com.google.api.client.googleapis.services.AbstractGoogleClientRequest.ApiClientVersion; import com.google.api.client.googleapis.testing.services.MockGoogleClient; import com.google.api.client.googleapis.testing.services.MockGoogleClientRequest; import com.google.api.client.http.EmptyContent; @@ -229,24 +230,26 @@ public void testSetsApiClientHeader() throws Exception { public void testSetsApiClientHeaderWithOsVersion() throws Exception { System.setProperty("os.name", "My OS"); System.setProperty("os.version", "1.2.3"); - HttpTransport transport = new AssertHeaderTransport("X-Goog-Api-Client", ".* my-os/1.2.3"); + + HttpTransport transport = new MockHttpTransport(); MockGoogleClient client = new MockGoogleClient.Builder( transport, ROOT_URL, SERVICE_PATH, JSON_OBJECT_PARSER, null).build(); - MockGoogleClientRequest request = - new MockGoogleClientRequest(client, HttpMethods.GET, URI_TEMPLATE, null, Void.class); - request.executeUnparsed(); + + String version = ApiClientVersion.build(client); + assertTrue("Api version should contain the os version", version.matches(".* my-os/1.2.3")); } public void testSetsApiClientHeaderWithoutOsVersion() throws Exception { System.setProperty("os.name", "My OS"); System.clearProperty("os.version"); + assertNull(System.getProperty("os.version")); - HttpTransport transport = new AssertHeaderTransport("X-Goog-Api-Client", ".*my-os.*", false); + HttpTransport transport = new MockHttpTransport(); MockGoogleClient client = new MockGoogleClient.Builder( transport, ROOT_URL, SERVICE_PATH, JSON_OBJECT_PARSER, null).build(); - MockGoogleClientRequest request = - new MockGoogleClientRequest(client, HttpMethods.GET, URI_TEMPLATE, null, Void.class); - request.executeUnparsed(); + + String version = ApiClientVersion.build(client); + assertFalse("Api version should not contain the os version", version.matches(".*my-os.*")); } private class AssertHeaderTransport extends MockHttpTransport { From d0c87aad2739874f5ecd03025aa3b3104dd46dc8 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Tue, 30 Oct 2018 14:14:15 -0700 Subject: [PATCH 4/8] Fix tests --- .../services/AbstractGoogleClientRequest.java | 60 +++++++++++-------- .../AbstractGoogleClientRequestTest.java | 12 +--- 2 files changed, 37 insertions(+), 35 deletions(-) diff --git a/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java b/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java index ee794dfb2..0dde675ce 100644 --- a/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java +++ b/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java @@ -126,7 +126,10 @@ protected AbstractGoogleClientRequest(AbstractGoogleClient abstractGoogleClient, requestHeaders.setUserAgent(USER_AGENT_SUFFIX); } // Set the header for the Api Client version (Java and OS version) - requestHeaders.set(API_VERSION_HEADER, ApiClientVersion.build(abstractGoogleClient)); + requestHeaders.set( + API_VERSION_HEADER, + ApiClientVersion.getDefault().build(abstractGoogleClient.getClass().getSimpleName()) + ); } /** @@ -137,19 +140,39 @@ protected AbstractGoogleClientRequest(AbstractGoogleClient abstractGoogleClient, * */ static class ApiClientVersion { - private static final String JAVA_VERSION = getJavaVersion(); - private static final String OS_NAME = formatName(System.getProperty("os.name")); - private static final String OS_VERSION = formatSemver(System.getProperty("os.version")); - private static final String HEADER_TEMPLATE = buildHeaderTemplate(); + private static final ApiClientVersion DEFAULT_VERSION = new ApiClientVersion(); + private String headerTemplate; + + ApiClientVersion() { + this( + getJavaVersion(), + System.getProperty("os.name"), + System.getProperty("os.version"), + GoogleUtils.VERSION + ); + } - @VisibleForTesting - static String build(AbstractGoogleClient client) { + ApiClientVersion(String javaVersion, String osName, String osVersion, String clientVersion) { + StringBuilder sb = new StringBuilder("java/"); + sb.append(formatSemver(javaVersion)); + sb.append(" http-google-%s/"); + sb.append(formatSemver(clientVersion)); + if (osName != null && osVersion != null) { + sb.append(" "); + sb.append(formatName(osName)); + sb.append("/"); + sb.append(formatSemver(osVersion)); + } + this.headerTemplate = sb.toString(); + } + + public String build(String clientName) { // TODO(chingor): add the API version from the generated client - return String.format( - HEADER_TEMPLATE, - formatName(client.getClass().getSimpleName()), - formatSemver(GoogleUtils.VERSION) - ); + return String.format(headerTemplate, formatName(clientName)); + } + + private static ApiClientVersion getDefault() { + return DEFAULT_VERSION; } private static String getJavaVersion() { @@ -180,19 +203,6 @@ private static String formatSemver(String version) { return version; } } - - private static String buildHeaderTemplate() { - StringBuilder sb = new StringBuilder("java/"); - sb.append(JAVA_VERSION); - sb.append(" http-google-%s/%s"); - if (OS_NAME != null && OS_VERSION != null) { - sb.append(" "); - sb.append(OS_NAME); - sb.append("/"); - sb.append(OS_VERSION); - } - return sb.toString(); - } } /** Returns whether to disable GZip compression of HTTP content. */ diff --git a/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java b/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java index 5f7ca36f6..53131d493 100644 --- a/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java +++ b/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java @@ -231,11 +231,7 @@ public void testSetsApiClientHeaderWithOsVersion() throws Exception { System.setProperty("os.name", "My OS"); System.setProperty("os.version", "1.2.3"); - HttpTransport transport = new MockHttpTransport(); - MockGoogleClient client = new MockGoogleClient.Builder( - transport, ROOT_URL, SERVICE_PATH, JSON_OBJECT_PARSER, null).build(); - - String version = ApiClientVersion.build(client); + String version = new ApiClientVersion().build("My Client"); assertTrue("Api version should contain the os version", version.matches(".* my-os/1.2.3")); } @@ -244,11 +240,7 @@ public void testSetsApiClientHeaderWithoutOsVersion() throws Exception { System.clearProperty("os.version"); assertNull(System.getProperty("os.version")); - HttpTransport transport = new MockHttpTransport(); - MockGoogleClient client = new MockGoogleClient.Builder( - transport, ROOT_URL, SERVICE_PATH, JSON_OBJECT_PARSER, null).build(); - - String version = ApiClientVersion.build(client); + String version = new ApiClientVersion().build("My Client"); assertFalse("Api version should not contain the os version", version.matches(".*my-os.*")); } From 212459c942fa4f4ad5fa78fc27c88a0796bbb68e Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Tue, 30 Oct 2018 14:40:57 -0700 Subject: [PATCH 5/8] Fix the reset of system properties --- .../googleapis/services/AbstractGoogleClientRequest.java | 4 +--- .../services/AbstractGoogleClientRequestTest.java | 9 ++++++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java b/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java index 0dde675ce..8e5668feb 100644 --- a/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java +++ b/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java @@ -35,7 +35,6 @@ import com.google.api.client.util.GenericData; import com.google.api.client.util.Preconditions; -import com.google.common.annotations.VisibleForTesting; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -166,8 +165,7 @@ static class ApiClientVersion { this.headerTemplate = sb.toString(); } - public String build(String clientName) { - // TODO(chingor): add the API version from the generated client + String build(String clientName) { return String.format(headerTemplate, formatName(clientName)); } diff --git a/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java b/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java index 53131d493..6a7f044dc 100644 --- a/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java +++ b/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java @@ -52,20 +52,23 @@ public class AbstractGoogleClientRequestTest extends TestCase { "{\"error\":{\"code\":401,\"errors\":[{\"domain\":\"global\"," + "\"location\":\"Authorization\",\"locationType\":\"header\"," + "\"message\":\"me\",\"reason\":\"authError\"}],\"message\":\"me\"}}"; - private Properties originalProperties; + private String originalOsName; + private String originalOsVersion; @Override protected void setUp() throws Exception { super.setUp(); // save the original system properties so we can mock them out - this.originalProperties = System.getProperties(); + this.originalOsName = System.getProperty("os.name"); + this.originalOsVersion = System.getProperty("os.version"); } @Override protected void tearDown() throws Exception { // restore the original system properties - System.setProperties(originalProperties); + System.setProperty("os.name", originalOsName); + System.setProperty("os.version", originalOsVersion); super.tearDown(); } From 2b6120b05f84af767dc33b322655a6dc89101342 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Tue, 30 Oct 2018 14:53:08 -0700 Subject: [PATCH 6/8] Remove unused import --- .../googleapis/services/AbstractGoogleClientRequestTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java b/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java index 6a7f044dc..0cd82672d 100644 --- a/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java +++ b/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java @@ -33,7 +33,6 @@ import com.google.api.client.util.StringUtils; import java.io.IOException; import java.util.Arrays; -import java.util.Properties; import junit.framework.TestCase; /** From 43404c668e9f3f8c36741ae9f29162332dfb9431 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Tue, 30 Oct 2018 14:55:23 -0700 Subject: [PATCH 7/8] undo change to AsesrtHeaderTransport --- .../services/AbstractGoogleClientRequestTest.java | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java b/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java index 0cd82672d..58c5320da 100644 --- a/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java +++ b/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java @@ -249,16 +249,10 @@ public void testSetsApiClientHeaderWithoutOsVersion() throws Exception { private class AssertHeaderTransport extends MockHttpTransport { String expectedHeader; String expectedHeaderValue; - boolean expected; AssertHeaderTransport(String header, String value) { - this(header, value, true); - } - - AssertHeaderTransport(String header, String value, boolean match) { expectedHeader = header; expectedHeaderValue = value; - expected = match; } @Override @@ -267,13 +261,12 @@ public LowLevelHttpRequest buildRequest(String method, String url) throws IOExce @Override public LowLevelHttpResponse execute() throws IOException { String firstHeader = getFirstHeaderValue(expectedHeader); - assertEquals( + assertTrue( String.format( "Expected header value to match %s, instead got %s.", expectedHeaderValue, firstHeader ), - expected, firstHeader.matches(expectedHeaderValue) ); return new MockLowLevelHttpResponse(); From b147dbf223b389afa9e6efc53001e8ca29cb2b29 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Fri, 2 Nov 2018 10:55:31 -0700 Subject: [PATCH 8/8] Fix codestyle --- .../services/AbstractGoogleClientRequest.java | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java b/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java index 8e5668feb..5a23f315c 100644 --- a/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java +++ b/google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java @@ -12,6 +12,9 @@ package com.google.api.client.googleapis.services; +import static com.google.common.base.StandardSystemProperty.OS_NAME; +import static com.google.common.base.StandardSystemProperty.OS_VERSION; + import com.google.api.client.googleapis.GoogleUtils; import com.google.api.client.googleapis.MethodOverride; import com.google.api.client.googleapis.batch.BatchCallback; @@ -140,15 +143,10 @@ protected AbstractGoogleClientRequest(AbstractGoogleClient abstractGoogleClient, */ static class ApiClientVersion { private static final ApiClientVersion DEFAULT_VERSION = new ApiClientVersion(); - private String headerTemplate; + private final String headerTemplate; ApiClientVersion() { - this( - getJavaVersion(), - System.getProperty("os.name"), - System.getProperty("os.version"), - GoogleUtils.VERSION - ); + this(getJavaVersion(), OS_NAME.value(), OS_VERSION.value(), GoogleUtils.VERSION); } ApiClientVersion(String javaVersion, String osName, String osVersion, String clientVersion) {