Skip to content

Commit f9fccec

Browse files
chingor13chingor13
authored
Fix possible NPE when missing os.version system property (googleapis#1210)
* Add tests for missing os.version system property * Only append the OS name/version if we can get them from the system properties * Fix test allow testing different system properties * Fix tests * Fix the reset of system properties * Remove unused import * undo change to AsesrtHeaderTransport * Fix codestyle
1 parent 13407f9 commit f9fccec

2 files changed

Lines changed: 77 additions & 16 deletions

File tree

‎google-api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequest.java‎

Lines changed: 39 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@
1212

1313
package com.google.api.client.googleapis.services;
1414

15+
import static com.google.common.base.StandardSystemProperty.OS_NAME;
16+
import static com.google.common.base.StandardSystemProperty.OS_VERSION;
17+
1518
import com.google.api.client.googleapis.GoogleUtils;
1619
import com.google.api.client.googleapis.MethodOverride;
1720
import com.google.api.client.googleapis.batch.BatchCallback;
@@ -125,7 +128,10 @@ protected AbstractGoogleClientRequest(AbstractGoogleClient abstractGoogleClient,
125128
requestHeaders.setUserAgent(USER_AGENT_SUFFIX);
126129
}
127130
// Set the header for the Api Client version (Java and OS version)
128-
requestHeaders.set(API_VERSION_HEADER, ApiClientVersion.build(abstractGoogleClient));
131+
requestHeaders.set(
132+
API_VERSION_HEADER,
133+
ApiClientVersion.getDefault().build(abstractGoogleClient.getClass().getSimpleName())
134+
);
129135
}
130136

131137
/**
@@ -135,21 +141,34 @@ protected AbstractGoogleClientRequest(AbstractGoogleClient abstractGoogleClient,
135141
* See <a href="https://cloud.google.com/apis/docs/system-parameters"></a>
136142
*
137143
*/
138-
private static class ApiClientVersion {
139-
private static final String JAVA_VERSION = getJavaVersion();
140-
private static final String OS_NAME = formatName(System.getProperty("os.name"));
141-
private static final String OS_VERSION = formatSemver(System.getProperty("os.version"));
142-
143-
private static String build(AbstractGoogleClient client) {
144-
// TODO(chingor): add the API version from the generated client
145-
return String.format(
146-
"java/%s http-google-%s/%s %s/%s",
147-
JAVA_VERSION,
148-
formatName(client.getClass().getSimpleName()),
149-
formatSemver(GoogleUtils.VERSION),
150-
OS_NAME,
151-
OS_VERSION
152-
);
144+
static class ApiClientVersion {
145+
private static final ApiClientVersion DEFAULT_VERSION = new ApiClientVersion();
146+
private final String headerTemplate;
147+
148+
ApiClientVersion() {
149+
this(getJavaVersion(), OS_NAME.value(), OS_VERSION.value(), GoogleUtils.VERSION);
150+
}
151+
152+
ApiClientVersion(String javaVersion, String osName, String osVersion, String clientVersion) {
153+
StringBuilder sb = new StringBuilder("java/");
154+
sb.append(formatSemver(javaVersion));
155+
sb.append(" http-google-%s/");
156+
sb.append(formatSemver(clientVersion));
157+
if (osName != null && osVersion != null) {
158+
sb.append(" ");
159+
sb.append(formatName(osName));
160+
sb.append("/");
161+
sb.append(formatSemver(osVersion));
162+
}
163+
this.headerTemplate = sb.toString();
164+
}
165+
166+
String build(String clientName) {
167+
return String.format(headerTemplate, formatName(clientName));
168+
}
169+
170+
private static ApiClientVersion getDefault() {
171+
return DEFAULT_VERSION;
153172
}
154173

155174
private static String getJavaVersion() {
@@ -168,6 +187,10 @@ private static String formatName(String name) {
168187
}
169188

170189
private static String formatSemver(String version) {
190+
if (version == null) {
191+
return null;
192+
}
193+
171194
// Take only the semver version: x.y.z-a_b_c -> x.y.z
172195
Matcher m = Pattern.compile("(\\d+\\.\\d+\\.\\d+).*").matcher(version);
173196
if (m.find()) {

‎google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientRequestTest.java‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
package com.google.api.client.googleapis.services;
1414

15+
import com.google.api.client.googleapis.services.AbstractGoogleClientRequest.ApiClientVersion;
1516
import com.google.api.client.googleapis.testing.services.MockGoogleClient;
1617
import com.google.api.client.googleapis.testing.services.MockGoogleClientRequest;
1718
import com.google.api.client.http.EmptyContent;
@@ -50,6 +51,26 @@ public class AbstractGoogleClientRequestTest extends TestCase {
5051
"{\"error\":{\"code\":401,\"errors\":[{\"domain\":\"global\","
5152
+ "\"location\":\"Authorization\",\"locationType\":\"header\","
5253
+ "\"message\":\"me\",\"reason\":\"authError\"}],\"message\":\"me\"}}";
54+
private String originalOsName;
55+
private String originalOsVersion;
56+
57+
@Override
58+
protected void setUp() throws Exception {
59+
super.setUp();
60+
61+
// save the original system properties so we can mock them out
62+
this.originalOsName = System.getProperty("os.name");
63+
this.originalOsVersion = System.getProperty("os.version");
64+
}
65+
66+
@Override
67+
protected void tearDown() throws Exception {
68+
// restore the original system properties
69+
System.setProperty("os.name", originalOsName);
70+
System.setProperty("os.version", originalOsVersion);
71+
72+
super.tearDown();
73+
}
5374

5475
public void testExecuteUnparsed_error() throws Exception {
5576
HttpTransport transport = new MockHttpTransport() {
@@ -208,6 +229,23 @@ public void testSetsApiClientHeader() throws Exception {
208229
request.executeUnparsed();
209230
}
210231

232+
public void testSetsApiClientHeaderWithOsVersion() throws Exception {
233+
System.setProperty("os.name", "My OS");
234+
System.setProperty("os.version", "1.2.3");
235+
236+
String version = new ApiClientVersion().build("My Client");
237+
assertTrue("Api version should contain the os version", version.matches(".* my-os/1.2.3"));
238+
}
239+
240+
public void testSetsApiClientHeaderWithoutOsVersion() throws Exception {
241+
System.setProperty("os.name", "My OS");
242+
System.clearProperty("os.version");
243+
assertNull(System.getProperty("os.version"));
244+
245+
String version = new ApiClientVersion().build("My Client");
246+
assertFalse("Api version should not contain the os version", version.matches(".*my-os.*"));
247+
}
248+
211249
private class AssertHeaderTransport extends MockHttpTransport {
212250
String expectedHeader;
213251
String expectedHeaderValue;

0 commit comments

Comments
 (0)