pw_rpc: Avoid reflection in Java client
The pw_rpc Java client currently requires manual service declarations,
since codegen has not yet been implemented. Previously, service
declarations used protobuf class objects. Later, their parser() methods
were called using reflection.
Relying on reflection is not ideal. It breaks when optimizers like R8
are used, since the parser() method is not actually called anywhere.
This change updates Service declarations to accept protobuf parsers
instead of class objects.
Bug: b/293361955
Change-Id: Idec52db5ba429861c73a3886c9214001a2b264bb
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/162930
Pigweed-Auto-Submit: Wyatt Hepler <hepler@google.com>
Reviewed-by: Alexei Frolov <frolv@google.com>
Commit-Queue: Wyatt Hepler <hepler@google.com>
diff --git a/pw_rpc/java/main/dev/pigweed/pw_rpc/Method.java b/pw_rpc/java/main/dev/pigweed/pw_rpc/Method.java
index 148b78f..24acec9 100644
--- a/pw_rpc/java/main/dev/pigweed/pw_rpc/Method.java
+++ b/pw_rpc/java/main/dev/pigweed/pw_rpc/Method.java
@@ -37,9 +37,9 @@
public abstract Type type();
- abstract Class<? extends MessageLite> request();
+ abstract Parser<? extends MessageLite> requestParser();
- abstract Class<? extends MessageLite> response();
+ abstract Parser<? extends MessageLite> responseParser();
final int id() {
return Ids.calculate(name());
@@ -57,11 +57,11 @@
return createFullName(service().name(), name());
}
- public static String createFullName(String serviceName, String methodName) {
+ static String createFullName(String serviceName, String methodName) {
return serviceName + '/' + methodName;
}
- /* package */ static Builder builder() {
+ static Builder builder() {
return new AutoValue_Method.Builder();
}
@@ -79,34 +79,16 @@
abstract Builder setName(String value);
- abstract Builder setRequest(Class<? extends MessageLite> value);
+ abstract Builder setRequestParser(Parser<? extends MessageLite> parser);
- abstract Builder setResponse(Class<? extends MessageLite> value);
+ abstract Builder setResponseParser(Parser<? extends MessageLite> parser);
abstract Method build();
}
/** Decodes a response payload according to the method's response type. */
final MessageLite decodeResponsePayload(ByteString data) throws InvalidProtocolBufferException {
- return decodeProtobuf(response(), data);
- }
-
- /** Deserializes a protobuf using the provided class. */
- @SuppressWarnings("unchecked")
- static MessageLite decodeProtobuf(Class<? extends MessageLite> messageType, ByteString data)
- throws InvalidProtocolBufferException {
- try {
- Parser<? extends MessageLite> parser =
- (Parser<? extends MessageLite>) messageType.getMethod("parser").invoke(null);
- return parser.parseFrom(data);
- } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
- throw new LinkageError(
- String.format("Service method was created with %s, which does not have parser() method; "
- + "either the class is not a generated protobuf class "
- + "or the parser() method was optimized out (see b/293361955)",
- messageType),
- e);
- }
+ return responseParser().parseFrom(data);
}
/** Which type of RPC this is: unary or server/client/bidirectional streaming. */
diff --git a/pw_rpc/java/main/dev/pigweed/pw_rpc/Service.java b/pw_rpc/java/main/dev/pigweed/pw_rpc/Service.java
index 6b90206..afaaf40 100644
--- a/pw_rpc/java/main/dev/pigweed/pw_rpc/Service.java
+++ b/pw_rpc/java/main/dev/pigweed/pw_rpc/Service.java
@@ -16,6 +16,8 @@
import com.google.common.collect.ImmutableMap;
import com.google.protobuf.MessageLite;
+import com.google.protobuf.Parser;
+import java.lang.reflect.InvocationTargetException;
import java.util.Arrays;
import java.util.stream.Collectors;
@@ -42,11 +44,11 @@
return id;
}
- public ImmutableMap<Integer, Method> methods() {
+ ImmutableMap<Integer, Method> methods() {
return methods;
}
- public final Method method(String name) {
+ final Method method(String name) {
return methods().get(Ids.calculate(name));
}
@@ -55,43 +57,137 @@
return name();
}
- // TODO(b/293361955): Method declarations should use ProtobufClass.parser()
- // instead of the class object so that the parser() method is referenced,
- // preventing it from being optimized out.
+ // TODO(b/293361955): Remove deprecated methods.
+ /**
+ * Declares a unary service method.
+ *
+ * @param name The method name within the service, e.g. "MyMethod" for my_pkg.MyService.MyMethod.
+ * @param request Parser for the request protobuf, e.g. MyRequestProto.parser()
+ * @param response Parser for the response protobuf, e.g. MyResponseProto.parser()
+ * @return Method.Builder, for internal use by the Service class only
+ */
public static Method.Builder unaryMethod(
- String name, Class<? extends MessageLite> request, Class<? extends MessageLite> response) {
+ String name, Parser<? extends MessageLite> request, Parser<? extends MessageLite> response) {
return Method.builder()
.setType(Method.Type.UNARY)
.setName(name)
- .setRequest(request)
- .setResponse(response);
+ .setRequestParser(request)
+ .setResponseParser(response);
}
- public static Method.Builder serverStreamingMethod(
+ /**
+ * Declares a unary service method.
+ *
+ * @deprecated Pass `ProtobufType.parser()` instead of `ProtobufType.class`.
+ */
+ @Deprecated
+ public static Method.Builder unaryMethod(
String name, Class<? extends MessageLite> request, Class<? extends MessageLite> response) {
+ return unaryMethod(name, getParser(request), getParser(response));
+ }
+
+ /**
+ * Declares a server streaming service method.
+ *
+ * @param name The method name within the service, e.g. "MyMethod" for my_pkg.MyService.MyMethod.
+ * @param request Parser for the request protobuf, e.g. MyRequestProto.parser()
+ * @param response Parser for the response protobuf, e.g. MyResponseProto.parser()
+ * @return Method.Builder, for internal use by the Service class only
+ */
+ public static Method.Builder serverStreamingMethod(
+ String name, Parser<? extends MessageLite> request, Parser<? extends MessageLite> response) {
return Method.builder()
.setType(Method.Type.SERVER_STREAMING)
.setName(name)
- .setRequest(request)
- .setResponse(response);
+ .setRequestParser(request)
+ .setResponseParser(response);
}
- public static Method.Builder clientStreamingMethod(
+ /**
+ * Declares a server streaming service method.
+ *
+ * @deprecated Pass `ProtobufType.parser()` instead of `ProtobufType.class`.
+ */
+ @Deprecated
+ public static Method.Builder serverStreamingMethod(
String name, Class<? extends MessageLite> request, Class<? extends MessageLite> response) {
+ return serverStreamingMethod(name, getParser(request), getParser(response));
+ }
+
+ /**
+ * Declares a client streaming service method.
+ *
+ * @param name The method name within the service, e.g. "MyMethod" for my_pkg.MyService.MyMethod.
+ * @param request Parser for the request protobuf, e.g. MyRequestProto.parser()
+ * @param response Parser for the response protobuf, e.g. MyResponseProto.parser()
+ * @return Method.Builder, for internal use by the Service class only
+ */
+ public static Method.Builder clientStreamingMethod(
+ String name, Parser<? extends MessageLite> request, Parser<? extends MessageLite> response) {
return Method.builder()
.setType(Method.Type.CLIENT_STREAMING)
.setName(name)
- .setRequest(request)
- .setResponse(response);
+ .setRequestParser(request)
+ .setResponseParser(response);
}
- public static Method.Builder bidirectionalStreamingMethod(
+ /**
+ * Declares a client streaming service method.
+ *
+ * @deprecated Pass `ProtobufType.parser()` instead of `ProtobufType.class`.
+ */
+ @Deprecated
+ public static Method.Builder clientStreamingMethod(
String name, Class<? extends MessageLite> request, Class<? extends MessageLite> response) {
+ return clientStreamingMethod(name, getParser(request), getParser(response));
+ }
+
+ /**
+ * Declares a bidirectional streaming service method.
+ *
+ * @param name The method name within the service, e.g. "MyMethod" for my_pkg.MyService.MyMethod.
+ * @param request Parser for the request protobuf, e.g. MyRequestProto.parser()
+ * @param response Parser for the response protobuf, e.g. MyResponseProto.parser()
+ * @return Method.Builder, for internal use by the Service class only
+ */
+ public static Method.Builder bidirectionalStreamingMethod(
+ String name, Parser<? extends MessageLite> request, Parser<? extends MessageLite> response) {
return Method.builder()
.setType(Method.Type.BIDIRECTIONAL_STREAMING)
.setName(name)
- .setRequest(request)
- .setResponse(response);
+ .setRequestParser(request)
+ .setResponseParser(response);
+ }
+
+ /**
+ * Declares a bidirectional streaming service method.
+ *
+ * @deprecated Pass `ProtobufType.parser()` instead of `ProtobufType.class`.
+ */
+ @Deprecated
+ public static Method.Builder bidirectionalStreamingMethod(
+ String name, Class<? extends MessageLite> request, Class<? extends MessageLite> response) {
+ return bidirectionalStreamingMethod(name, getParser(request), getParser(response));
+ }
+
+ /**
+ * Gets the Parser from a protobuf class using reflection.
+ *
+ * This function is provided for backwards compatibility with the deprecated service API that
+ * takes a class object instead of a protobuf parser object.
+ */
+ @SuppressWarnings("unchecked")
+ private static Parser<? extends MessageLite> getParser(Class<? extends MessageLite> messageType) {
+ try {
+ return (Parser<? extends MessageLite>) messageType.getMethod("parser").invoke(null);
+ } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
+ throw new LinkageError(
+ String.format("Service method was created with %s, which does not have parser() method; "
+ + "either the class is not a generated protobuf class "
+ + "or the parser() method was optimized out (see b/293361955)",
+ messageType),
+ e);
+ }
}
}
diff --git a/pw_rpc/java/test/dev/pigweed/pw_rpc/ClientTest.java b/pw_rpc/java/test/dev/pigweed/pw_rpc/ClientTest.java
index b3e3198..de7f12e 100644
--- a/pw_rpc/java/test/dev/pigweed/pw_rpc/ClientTest.java
+++ b/pw_rpc/java/test/dev/pigweed/pw_rpc/ClientTest.java
@@ -43,11 +43,13 @@
@Rule public final MockitoRule mockito = MockitoJUnit.rule();
private static final Service SERVICE = new Service("pw.rpc.test1.TheTestService",
- Service.unaryMethod("SomeUnary", SomeMessage.class, AnotherMessage.class),
- Service.serverStreamingMethod("SomeServerStreaming", SomeMessage.class, AnotherMessage.class),
- Service.clientStreamingMethod("SomeClientStreaming", SomeMessage.class, AnotherMessage.class),
+ Service.unaryMethod("SomeUnary", SomeMessage.parser(), AnotherMessage.parser()),
+ Service.serverStreamingMethod(
+ "SomeServerStreaming", SomeMessage.parser(), AnotherMessage.parser()),
+ Service.clientStreamingMethod(
+ "SomeClientStreaming", SomeMessage.parser(), AnotherMessage.parser()),
Service.bidirectionalStreamingMethod(
- "SomeBidiStreaming", SomeMessage.class, AnotherMessage.class));
+ "SomeBidiStreaming", SomeMessage.parser(), AnotherMessage.parser()));
private static final Method UNARY_METHOD = SERVICE.method("SomeUnary");
private static final Method SERVER_STREAMING_METHOD = SERVICE.method("SomeServerStreaming");
@@ -135,7 +137,7 @@
InvalidRpcServiceException.class, () -> client.method(CHANNEL_ID, "abc.Service/Method"));
Service service = new Service("throwaway.NotRealService",
- Service.unaryMethod("NotAnRpc", SomeMessage.class, AnotherMessage.class));
+ Service.unaryMethod("NotAnRpc", SomeMessage.parser(), AnotherMessage.parser()));
assertThrows(InvalidRpcServiceException.class,
() -> client.method(CHANNEL_ID, service.method("NotAnRpc")));
}
@@ -417,4 +419,34 @@
assertThrows(InvalidRpcChannelException.class,
() -> client.openChannel(new Channel(CHANNEL_ID, packet -> {})));
}
+
+ @Test
+ public void serviceDeclaration_deprecatedClassBasedEquivalentToParserBased() {
+ final Service declaredWithClassObjects = new Service(SERVICE.name(),
+ Service.unaryMethod("SomeUnary", SomeMessage.parser(), AnotherMessage.parser()),
+ Service.serverStreamingMethod(
+ "SomeServerStreaming", SomeMessage.parser(), AnotherMessage.parser()),
+ Service.clientStreamingMethod(
+ "SomeClientStreaming", SomeMessage.parser(), AnotherMessage.parser()),
+ Service.bidirectionalStreamingMethod(
+ "SomeBidiStreaming", SomeMessage.parser(), AnotherMessage.parser()));
+
+ assertThat(declaredWithClassObjects.method("SomeUnary").responseParser())
+ .isSameInstanceAs(declaredWithClassObjects.method("SomeUnary").responseParser());
+ assertThat(declaredWithClassObjects.method("SomeServerStreaming").responseParser())
+ .isSameInstanceAs(declaredWithClassObjects.method("SomeServerStreaming").responseParser());
+ assertThat(declaredWithClassObjects.method("SomeClientStreaming").responseParser())
+ .isSameInstanceAs(declaredWithClassObjects.method("SomeClientStreaming").responseParser());
+ assertThat(declaredWithClassObjects.method("SomeBidiStreaming").responseParser())
+ .isSameInstanceAs(declaredWithClassObjects.method("SomeBidiStreaming").responseParser());
+
+ assertThat(declaredWithClassObjects.method("SomeUnary").requestParser())
+ .isSameInstanceAs(declaredWithClassObjects.method("SomeUnary").requestParser());
+ assertThat(declaredWithClassObjects.method("SomeServerStreaming").requestParser())
+ .isSameInstanceAs(declaredWithClassObjects.method("SomeServerStreaming").requestParser());
+ assertThat(declaredWithClassObjects.method("SomeClientStreaming").requestParser())
+ .isSameInstanceAs(declaredWithClassObjects.method("SomeClientStreaming").requestParser());
+ assertThat(declaredWithClassObjects.method("SomeBidiStreaming").requestParser())
+ .isSameInstanceAs(declaredWithClassObjects.method("SomeBidiStreaming").requestParser());
+ }
}
diff --git a/pw_rpc/java/test/dev/pigweed/pw_rpc/EndpointTest.java b/pw_rpc/java/test/dev/pigweed/pw_rpc/EndpointTest.java
index b62aca8..041f693 100644
--- a/pw_rpc/java/test/dev/pigweed/pw_rpc/EndpointTest.java
+++ b/pw_rpc/java/test/dev/pigweed/pw_rpc/EndpointTest.java
@@ -36,11 +36,13 @@
@Rule public final MockitoRule mockito = MockitoJUnit.rule();
private static final Service SERVICE = new Service("pw.rpc.test1.TheTestService",
- Service.unaryMethod("SomeUnary", SomeMessage.class, SomeMessage.class),
- Service.serverStreamingMethod("SomeServerStreaming", SomeMessage.class, SomeMessage.class),
- Service.clientStreamingMethod("SomeClientStreaming", SomeMessage.class, SomeMessage.class),
+ Service.unaryMethod("SomeUnary", SomeMessage.parser(), SomeMessage.parser()),
+ Service.serverStreamingMethod(
+ "SomeServerStreaming", SomeMessage.parser(), SomeMessage.parser()),
+ Service.clientStreamingMethod(
+ "SomeClientStreaming", SomeMessage.parser(), SomeMessage.parser()),
Service.bidirectionalStreamingMethod(
- "SomeBidiStreaming", SomeMessage.class, SomeMessage.class));
+ "SomeBidiStreaming", SomeMessage.parser(), SomeMessage.parser()));
private static final Method METHOD = SERVICE.method("SomeUnary");
diff --git a/pw_rpc/java/test/dev/pigweed/pw_rpc/FutureCallTest.java b/pw_rpc/java/test/dev/pigweed/pw_rpc/FutureCallTest.java
index a1f1199..ca71962 100644
--- a/pw_rpc/java/test/dev/pigweed/pw_rpc/FutureCallTest.java
+++ b/pw_rpc/java/test/dev/pigweed/pw_rpc/FutureCallTest.java
@@ -41,10 +41,10 @@
@Rule public final MockitoRule mockito = MockitoJUnit.rule();
private static final Service SERVICE = new Service("pw.rpc.test1.TheTestService",
- Service.unaryMethod("SomeUnary", SomeMessage.class, AnotherMessage.class),
- Service.clientStreamingMethod("SomeClient", SomeMessage.class, AnotherMessage.class),
+ Service.unaryMethod("SomeUnary", SomeMessage.parser(), AnotherMessage.parser()),
+ Service.clientStreamingMethod("SomeClient", SomeMessage.parser(), AnotherMessage.parser()),
Service.bidirectionalStreamingMethod(
- "SomeBidirectional", SomeMessage.class, AnotherMessage.class));
+ "SomeBidirectional", SomeMessage.parser(), AnotherMessage.parser()));
private static final Method METHOD = SERVICE.method("SomeUnary");
private static final int CHANNEL_ID = 555;
diff --git a/pw_rpc/java/test/dev/pigweed/pw_rpc/StreamObserverCallTest.java b/pw_rpc/java/test/dev/pigweed/pw_rpc/StreamObserverCallTest.java
index 82d3151..d1260de 100644
--- a/pw_rpc/java/test/dev/pigweed/pw_rpc/StreamObserverCallTest.java
+++ b/pw_rpc/java/test/dev/pigweed/pw_rpc/StreamObserverCallTest.java
@@ -32,10 +32,10 @@
@Rule public final MockitoRule mockito = MockitoJUnit.rule();
private static final Service SERVICE = new Service("pw.rpc.test1.TheTestService",
- Service.unaryMethod("SomeUnary", SomeMessage.class, AnotherMessage.class),
- Service.clientStreamingMethod("SomeClient", SomeMessage.class, AnotherMessage.class),
+ Service.unaryMethod("SomeUnary", SomeMessage.parser(), AnotherMessage.parser()),
+ Service.clientStreamingMethod("SomeClient", SomeMessage.parser(), AnotherMessage.parser()),
Service.bidirectionalStreamingMethod(
- "SomeBidirectional", SomeMessage.class, AnotherMessage.class));
+ "SomeBidirectional", SomeMessage.parser(), AnotherMessage.parser()));
private static final Method METHOD = SERVICE.method("SomeUnary");
private static final int CHANNEL_ID = 555;
diff --git a/pw_rpc/java/test/dev/pigweed/pw_rpc/StreamObserverMethodClientTest.java b/pw_rpc/java/test/dev/pigweed/pw_rpc/StreamObserverMethodClientTest.java
index 36102f4..a1bc59c 100644
--- a/pw_rpc/java/test/dev/pigweed/pw_rpc/StreamObserverMethodClientTest.java
+++ b/pw_rpc/java/test/dev/pigweed/pw_rpc/StreamObserverMethodClientTest.java
@@ -34,11 +34,13 @@
public final class StreamObserverMethodClientTest {
private static final Service SERVICE = new Service("pw.rpc.test1.TheTestService",
- Service.unaryMethod("SomeUnary", SomeMessage.class, AnotherMessage.class),
- Service.serverStreamingMethod("SomeServerStreaming", SomeMessage.class, AnotherMessage.class),
- Service.clientStreamingMethod("SomeClientStreaming", SomeMessage.class, AnotherMessage.class),
+ Service.unaryMethod("SomeUnary", SomeMessage.parser(), AnotherMessage.parser()),
+ Service.serverStreamingMethod(
+ "SomeServerStreaming", SomeMessage.parser(), AnotherMessage.parser()),
+ Service.clientStreamingMethod(
+ "SomeClientStreaming", SomeMessage.parser(), AnotherMessage.parser()),
Service.bidirectionalStreamingMethod(
- "SomeBidirectionalStreaming", SomeMessage.class, AnotherMessage.class));
+ "SomeBidirectionalStreaming", SomeMessage.parser(), AnotherMessage.parser()));
@Rule public final MockitoRule mockito = MockitoJUnit.rule();
@@ -212,7 +214,8 @@
@Test
public void invalidService_throwsException() {
Service otherService = new Service("something.Else",
- Service.clientStreamingMethod("ClientStream", SomeMessage.class, AnotherMessage.class));
+ Service.clientStreamingMethod(
+ "ClientStream", SomeMessage.parser(), AnotherMessage.parser()));
MethodClient methodClient = new MethodClient(
client, channel.id(), otherService.method("ClientStream"), defaultObserver);
@@ -222,13 +225,13 @@
@Test
public void invalidMethod_throwsException() {
Service serviceWithDifferentUnaryMethod = new Service("pw.rpc.test1.TheTestService",
- Service.unaryMethod("SomeUnary", AnotherMessage.class, AnotherMessage.class),
+ Service.unaryMethod("SomeUnary", AnotherMessage.parser(), AnotherMessage.parser()),
Service.serverStreamingMethod(
- "SomeServerStreaming", SomeMessage.class, AnotherMessage.class),
+ "SomeServerStreaming", SomeMessage.parser(), AnotherMessage.parser()),
Service.clientStreamingMethod(
- "SomeClientStreaming", SomeMessage.class, AnotherMessage.class),
+ "SomeClientStreaming", SomeMessage.parser(), AnotherMessage.parser()),
Service.bidirectionalStreamingMethod(
- "SomeBidirectionalStreaming", SomeMessage.class, AnotherMessage.class));
+ "SomeBidirectionalStreaming", SomeMessage.parser(), AnotherMessage.parser()));
MethodClient methodClient = new MethodClient(
client, 999, serviceWithDifferentUnaryMethod.method("SomeUnary"), defaultObserver);
diff --git a/pw_rpc/java/test/dev/pigweed/pw_rpc/TestClient.java b/pw_rpc/java/test/dev/pigweed/pw_rpc/TestClient.java
index a58d519..5e989a7 100644
--- a/pw_rpc/java/test/dev/pigweed/pw_rpc/TestClient.java
+++ b/pw_rpc/java/test/dev/pigweed/pw_rpc/TestClient.java
@@ -175,9 +175,10 @@
private <T extends MessageLite> T parseRequestPayload(Class<T> payloadType, RpcPacket packet) {
try {
- return payloadType.cast(Method.decodeProtobuf(
- client.method(CHANNEL_ID, packet.getServiceId(), packet.getMethodId()).method().request(),
- packet.getPayload()));
+ return payloadType.cast(client.method(CHANNEL_ID, packet.getServiceId(), packet.getMethodId())
+ .method()
+ .requestParser()
+ .parseFrom(packet.getPayload()));
} catch (InvalidProtocolBufferException e) {
throw new AssertionError("Decoding sent packet payload failed", e);
}
