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.

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

fix: protobuf version not always getting set in headers #3322

Merged
merged 12 commits into from
Nov 7, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
package com.google.api.gax.core;

import com.google.api.core.InternalApi;
import com.google.api.gax.util.ClassWrapper;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.protobuf.Any;
Expand All @@ -48,8 +49,7 @@ public class GaxProperties {
private static final String DEFAULT_VERSION = "";
private static final String GAX_VERSION = getLibraryVersion(GaxProperties.class, "version.gax");
private static final String JAVA_VERSION = getRuntimeVersion();
private static final String PROTOBUF_VERSION =
getBundleVersion(Any.class).orElse(DEFAULT_VERSION);
private static final String PROTOBUF_VERSION = getProtobufVersion(new ClassWrapper(), Any.class);

private GaxProperties() {}

Expand Down Expand Up @@ -148,4 +148,30 @@ static Optional<String> getBundleVersion(Class<?> clazz) {
return Optional.empty();
}
}

/**
* Returns the Protobuf runtime version as reported by com.google.protobuf.RuntimeVersion, if
* class is available, otherwise by reading from MANIFEST file. If niether option is available
* defaults to protobuf version 3 as RuntimeVersion class is available in protobuf version 4+
*/
@VisibleForTesting
static String getProtobufVersion(ClassWrapper classWrapper, Class clazz) {
try {
Class<?> protobufRuntimeVersionClass =
classWrapper.forName("com.google.protobuf.RuntimeVersion");
return classWrapper.getFieldValue(protobufRuntimeVersionClass, "MAJOR")
+ "."
+ classWrapper.getFieldValue(protobufRuntimeVersionClass, "MINOR")
+ "."
+ classWrapper.getFieldValue(protobufRuntimeVersionClass, "PATCH");
} catch (ClassNotFoundException
| NoSuchFieldException
| IllegalAccessException
| SecurityException e) {
// If manifest file is not available default to protobuf generic version 3 as we know
// RuntimeVersion class is
// available in protobuf jar 4+.
return getBundleVersion(clazz).orElse("3");
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we are going to have two different format of protobuf versions (3.x.x vs 3)? Is it going to cause issues when we try to aggregate the metrics?

Copy link
Member

Choose a reason for hiding this comment

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

There might be 3.1.2, which provides precision. But there shouldn't be any 3.x.x.

We shouldn't be providing precision (significant digits) when we don't know them. We can fix any parsing logic as necessary.

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ private static String checkAndAppendProtobufVersionIfNecessary(
// TODO(b/366417603): appending protobuf version to existing client library token until resolved
Pattern pattern = Pattern.compile("(gccl|gapic)\\S*");
Matcher matcher = pattern.matcher(apiClientHeaderValue);
if (matcher.find()) {
if (matcher.find() && GaxProperties.getProtobufVersion() != null) {
return apiClientHeaderValue.substring(0, matcher.end())
+ "--"
+ PROTOBUF_HEADER_VERSION_KEY
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright 2024 Google LLC
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
* met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following disclaimer
* in the documentation and/or other materials provided with the
* distribution.
* * Neither the name of Google LLC nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
package com.google.api.gax.util;

/* Wrapper class for reflection {@link java.lang.Class} methods to enable unit testing. */
public class ClassWrapper {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's worth it to introduce a new public class just for unit testing in Gax. Can we move the wrapper methods to GaxProperties and make them package private?

Copy link
Contributor Author

@ldetmer ldetmer Nov 6, 2024

Choose a reason for hiding this comment

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

i hope I understood what you meant. I moved the class to package private so I could continue to mock, or did you meant do not unit test this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored to remove the wrapper class/methods at all. Let me know what you think!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, it looks good to me on the surface!

did you meant do not unit test this code

We should definitely do unit tests if possible. I see that the sonar coverage is complaining about the coverage now, can you please take a look at it and see if it makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for calling out, I missed a test case! Fixed


/* Wraps {@link java.lang.Class#forName} method */
public Class<?> forName(String name) throws ClassNotFoundException {
return Class.forName(name);
}

/* Consolidates retrieving a {@link java.lang.Field} on a {@link java.lang.Class} object via reflection and retrieving the value of that Field */
public Object getFieldValue(Class<?> clazz, String fieldName)
throws NoSuchFieldException, IllegalAccessException {
return clazz.getField(fieldName).get(null);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[
{
"name": "com.google.protobuf.RuntimeVersion",
"fields" : [
{ "name" : "MAJOR" },
{ "name" : "MINOR" },
{ "name" : "PATCH" }
]
}
]
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,13 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import com.google.api.gax.util.ClassWrapper;
import com.google.common.base.Strings;
import com.google.protobuf.Any;
import java.io.IOException;
import java.util.Optional;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -160,12 +165,8 @@ void testGetJavaRuntimeInfo_nullJavaVersion() {

@Test
public void testGetProtobufVersion() throws IOException {
Version version = readVersion(GaxProperties.getProtobufVersion());

assertTrue(version.major >= 3);
if (version.major == 3) {
assertTrue(version.minor >= 25);
}
assertTrue(
Pattern.compile("^\\d+\\.\\d+\\.\\d+").matcher(GaxProperties.getProtobufVersion()).find());
}

@Test
Expand All @@ -175,6 +176,52 @@ public void testGetBundleVersion_noManifestFile() throws IOException {
assertFalse(version.isPresent());
}

@Test
void testGetProtobufVersion_success() throws Exception {
ClassWrapper mockClassWrapper = mock(ClassWrapper.class);
when(mockClassWrapper.forName("com.google.protobuf.RuntimeVersion"))
.thenAnswer(invocationOnMock -> Class.class);
when(mockClassWrapper.getFieldValue(Class.class, "MAJOR")).thenReturn("2");
when(mockClassWrapper.getFieldValue(Class.class, "MINOR")).thenReturn("3");
when(mockClassWrapper.getFieldValue(Class.class, "PATCH")).thenReturn("4");

String version = GaxProperties.getProtobufVersion(mockClassWrapper, Any.class);

assertEquals("2.3.4", version);
}

@Test
void testGetProtobufVersion_classNotFoundException() throws Exception {
ClassWrapper mockClassWrapper = mock(ClassWrapper.class);
when(mockClassWrapper.forName("com.google.protobuf.RuntimeVersion"))
.thenThrow(new ClassNotFoundException(""));

String version = GaxProperties.getProtobufVersion(mockClassWrapper, Any.class);

assertTrue(Pattern.compile("^\\d+\\.\\d+\\.\\d+").matcher(version).find());
}

@Test
void testgetProtobufVersion_noSuchFieldException() throws Exception {
ClassWrapper mockClassWrapper = mock(ClassWrapper.class);
when(mockClassWrapper.getFieldValue(any(), any())).thenThrow(NoSuchFieldException.class);

String version = GaxProperties.getProtobufVersion(mockClassWrapper, Any.class);

assertTrue(Pattern.compile("^\\d+\\.\\d+\\.\\d+").matcher(version).find());
}

@Test
void testGetProtobufVersion_noManifest() throws Exception {
ClassWrapper mockClassWrapper = mock(ClassWrapper.class);
when(mockClassWrapper.forName("com.google.protobuf.RuntimeVersion"))
.thenThrow(new ClassNotFoundException(""));

String version = GaxProperties.getProtobufVersion(mockClassWrapper, GaxProperties.class);

assertEquals("3", version);
}

private Version readVersion(String version) {
assertTrue(Pattern.compile("^\\d+\\.\\d+\\.\\d+").matcher(version).find());
String[] versionComponents = version.split("\\.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ void testHttpJsonCompliance_userApiVersionSetSuccess() throws IOException {
@Test
void testGrpcCall_sendsCorrectApiClientHeader() {
Pattern defautlGrpcHeaderPattern =
Pattern.compile("gl-java/.* gapic/.*?--protobuf-.* gax/.* grpc/.* protobuf/.*");
Pattern.compile("gl-java/.* gapic/.*?--protobuf-\\d.* gax/.* grpc/.* protobuf/\\d.*");
grpcClient.echo(EchoRequest.newBuilder().build());
String headerValue = grpcInterceptor.metadata.get(API_CLIENT_HEADER_KEY);
assertTrue(defautlGrpcHeaderPattern.matcher(headerValue).matches());
Expand All @@ -250,7 +250,7 @@ void testGrpcCall_sendsCorrectApiClientHeader() {
@Test
void testHttpJson_sendsCorrectApiClientHeader() {
Pattern defautlHttpHeaderPattern =
Pattern.compile("gl-java/.* gapic/.*?--protobuf-.* gax/.* rest/ protobuf/.*");
Pattern.compile("gl-java/.* gapic/.*?--protobuf-\\d.* gax/.* rest/ protobuf/\\d.*");
httpJsonClient.echo(EchoRequest.newBuilder().build());
ArrayList<String> headerValues =
(ArrayList<String>)
Expand Down
Loading