pw_rpc: Handle duplicate generated proto message types
- google.protobuf.message_factory may generate new, unique message types
rather than reusing previously generated classes. The new types are
essentially identical to others and share the same descriptor
instance. When working with pw_rpc methods, accept any message type
that shares the same descriptors.
- Move pw_rpc.descriptors.Method tests to a new file and expand them to
cover this case.
Change-Id: I46a7823e086c0bbf26ecd5db3017bed0f7ae27e0
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/27160
Commit-Queue: Wyatt Hepler <hepler@google.com>
Pigweed-Auto-Submit: Wyatt Hepler <hepler@google.com>
Reviewed-by: Alexei Frolov <frolv@google.com>
diff --git a/pw_rpc/py/BUILD.gn b/pw_rpc/py/BUILD.gn
index adab838..6f0baa6 100644
--- a/pw_rpc/py/BUILD.gn
+++ b/pw_rpc/py/BUILD.gn
@@ -36,6 +36,7 @@
tests = [
"callback_client_test.py",
"client_test.py",
+ "descriptors_test.py",
"ids_test.py",
"packets_test.py",
]
diff --git a/pw_rpc/py/client_test.py b/pw_rpc/py/client_test.py
index edf5db8..097e0e9 100755
--- a/pw_rpc/py/client_test.py
+++ b/pw_rpc/py/client_test.py
@@ -204,23 +204,6 @@
with self.assertRaises(KeyError):
self._client.method('nothing.Good')
- def test_get_request_with_both_message_and_kwargs(self):
- method = self._client.services['pw.test2.Alpha'].methods['Unary']
-
- with self.assertRaisesRegex(TypeError, r'either'):
- method.get_request(method.request_type(), {'magic_number': 1.0})
-
- def test_get_request_with_wrong_type(self):
- method = self._client.services['pw.test2.Alpha'].methods['Unary']
- with self.assertRaisesRegex(TypeError, r'pw\.test2\.Request'):
- method.get_request('a str!', {})
-
- def test_get_request_with_incorrect_message_type(self):
- msg = self._protos.packages.pw.test1.AnotherMessage()
- with self.assertRaisesRegex(TypeError, r'pw\.test1\.SomeMessage'):
- self._client.services['pw.test1.PublicService'].methods[
- 'SomeUnary'].get_request(msg, {})
-
def test_process_packet_invalid_proto_data(self):
self.assertIs(self._client.process_packet(b'NOT a packet!'),
Status.DATA_LOSS)
diff --git a/pw_rpc/py/descriptors_test.py b/pw_rpc/py/descriptors_test.py
new file mode 100644
index 0000000..516d5e3
--- /dev/null
+++ b/pw_rpc/py/descriptors_test.py
@@ -0,0 +1,91 @@
+# Copyright 2020 The Pigweed Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may obtain a copy of
+# the License at
+#
+# https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations under
+# the License.
+"""Tests classes in pw_rpc.descriptors."""
+
+import unittest
+
+from google.protobuf.message_factory import MessageFactory
+
+from pw_protobuf_compiler import python_protos
+from pw_rpc import descriptors
+
+TEST_PROTO = """\
+syntax = "proto3";
+
+package pw.test1;
+
+message SomeMessage {
+ uint32 magic_number = 1;
+}
+
+message AnotherMessage {
+ enum Result {
+ FAILED = 0;
+ FAILED_MISERABLY = 1;
+ I_DONT_WANT_TO_TALK_ABOUT_IT = 2;
+ }
+
+ Result result = 1;
+ string payload = 2;
+}
+
+service PublicService {
+ rpc SomeUnary(SomeMessage) returns (AnotherMessage) {}
+ rpc SomeServerStreaming(SomeMessage) returns (stream AnotherMessage) {}
+ rpc SomeClientStreaming(stream SomeMessage) returns (AnotherMessage) {}
+ rpc SomeBidiStreaming(stream SomeMessage) returns (stream AnotherMessage) {}
+}
+"""
+
+
+class MethodTest(unittest.TestCase):
+ """Tests pw_rpc.Method."""
+ def setUp(self):
+ module, = python_protos.compile_and_import_strings([TEST_PROTO])
+ service = descriptors.Service.from_descriptor(
+ module.DESCRIPTOR.services_by_name['PublicService'])
+ self._method = service.methods['SomeUnary']
+
+ def test_get_request_with_both_message_and_kwargs(self):
+ with self.assertRaisesRegex(TypeError, r'either'):
+ self._method.get_request(self._method.request_type(),
+ {'magic_number': 1})
+
+ def test_get_request_with_wrong_type(self):
+ with self.assertRaisesRegex(TypeError, r'pw\.test1\.SomeMessage'):
+ self._method.get_request('a str!', {})
+
+ def test_get_request_with_different_message_type(self):
+ msg = self._method.response_type()
+ with self.assertRaisesRegex(TypeError, r'pw\.test1\.SomeMessage'):
+ self._method.get_request(msg, {})
+
+ def test_get_request_with_different_copy_of_same_message_class(self):
+ some_message_clone = MessageFactory(
+ self._method.request_type.DESCRIPTOR.file.pool).GetPrototype(
+ self._method.request_type.DESCRIPTOR)
+
+ msg = some_message_clone()
+
+ # Protobuf classes obtained with a MessageFactory may or may not be a
+ # unique type, but will always use the same descriptor instance.
+ self.assertIsInstance(msg, some_message_clone)
+ self.assertIs(msg.DESCRIPTOR, self._method.request_type.DESCRIPTOR)
+
+ result = self._method.get_request(msg, {})
+ self.assertIs(result, msg)
+
+
+if __name__ == '__main__':
+ unittest.main()
diff --git a/pw_rpc/py/pw_rpc/descriptors.py b/pw_rpc/py/pw_rpc/descriptors.py
index c6bb2e2..7545c6d 100644
--- a/pw_rpc/py/pw_rpc/descriptors.py
+++ b/pw_rpc/py/pw_rpc/descriptors.py
@@ -20,7 +20,9 @@
Iterator, Tuple, TypeVar, Union)
from google.protobuf import descriptor_pb2, message_factory
-from google.protobuf.descriptor import FieldDescriptor, MethodDescriptor
+from google.protobuf.descriptor import (FieldDescriptor, MethodDescriptor,
+ ServiceDescriptor)
+from google.protobuf.message import Message
from pw_protobuf_compiler import python_protos
from pw_rpc import ids
@@ -38,7 +40,7 @@
@dataclass(frozen=True, eq=False)
class Service:
"""Describes an RPC service."""
- _descriptor: MethodDescriptor
+ _descriptor: ServiceDescriptor
id: int
methods: 'Methods'
@@ -55,8 +57,9 @@
return self._descriptor.file.package
@classmethod
- def from_descriptor(cls, descriptor):
- service = cls(descriptor, ids.calculate(descriptor.full_name), None)
+ def from_descriptor(cls, descriptor: ServiceDescriptor) -> 'Service':
+ service = cls(descriptor, ids.calculate(descriptor.full_name),
+ None) # type: ignore[arg-type]
object.__setattr__(
service, 'methods',
Methods(
@@ -139,6 +142,16 @@
yield f'{field.name}={value}'
+def _message_is_type(proto, expected_type) -> bool:
+ """Returns true if the protobuf instance is the expected type."""
+ # Getting protobuf classes from google.protobuf.message_factory may create a
+ # new, unique generated proto class. Any generated classes for a particular
+ # proto message share the same MessageDescriptor instance and are
+ # interchangeable, so check the descriptors in addition to the types.
+ return isinstance(proto, expected_type) or (isinstance(
+ proto, Message) and proto.DESCRIPTOR is expected_type.DESCRIPTOR)
+
+
@dataclass(frozen=True, eq=False)
class Method:
"""Describes a method in a service."""
@@ -217,7 +230,7 @@
if proto is None:
return self.request_type(**proto_kwargs)
- if not isinstance(proto, self.request_type):
+ if not _message_is_type(proto, self.request_type):
try:
bad_type = proto.DESCRIPTOR.full_name
except AttributeError:
