- Notifications
You must be signed in to change notification settings - Fork95
fix: resolve issue handling protobuf responses in rest streaming#604
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
c9e3277 to4a92058Compare4a92058 toad79bf0Compare
vchudnov-g left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A few minor comments, but LGTM.
google/api_core/rest_streaming.py Outdated
| returnself._response_message_cls.from_json(self._ready_objs.popleft()) | ||
| ifissubclass(self._response_message_cls,proto.Message): | ||
| returnself._response_message_cls.from_json(self._ready_objs.popleft()) | ||
| else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: for safety and future-proofing, maybe this should be anelif and we check forgoogle.protobuf.Message
(I realize we don't expect to ever have the second check fail if we get here....but "we don't expect" == "famous last words")
(not a blocker)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed in9f5b22f
tests/unit/test_rest_streaming.py Outdated
| responses= [EchoResponse(content="hello world"),EchoResponse(content="yes")] | ||
| @pytest.mark.parametrize( | ||
| "random_split,resp_message_is_proto_plus,response_type", | ||
| [(False,True,EchoResponse), (False,False,httpbody_pb2.HttpBody)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Given that you're not testing different combinations ofresp_message_is_proto_plus andresponse_type, and that you're already switching on the first to construct the responses (which you have to because they take different parameters): I suggest you don't parametrize onresponse_type and instead, in the function, do
ifresp_message_is_proto_plus:response_type=EchoResponseresponses= [EchoResponse(content="hello world"),EchoResponse(content="yes")]else:response_type=httpbody_pb2.HttpBodyresponses= [httpbody_pb2.HttpBody(content_type="hello world"),httpbody_pb2.HttpBody(content_type="yes"), ]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed in5b7a4ce
tests/unit/test_rest_streaming.py Outdated
| Song(title="another song",date_added=datetime.datetime(2021,12,17)), | ||
| ] | ||
| @pytest.mark.parametrize( | ||
| "random_split,resp_message_is_proto_plus,response_type", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same comment as above: since you have theif below already, don't parametrize onresponse_type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed in5b7a4ce
tests/unit/test_rest_streaming.py Outdated
| @pytest.mark.parametrize("random_split", [True,False]) | ||
| deftest_next_stress(random_split): | ||
| @pytest.mark.parametrize( | ||
| "random_split,resp_message_is_proto_plus,response_type", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ditto on parametrization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed in5b7a4ce
| ), | ||
| Song(title='\\{"key": ["value",]}\\',composer=composer_with_relateds), | ||
| ] | ||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ditto on parametrizing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed in5b7a4ce
…essage or google.protobuf.message.Message
parthea commentedFeb 13, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
/cherry-pick v1 |
* fix: resolve issue handling protobuf responses in rest streaming* raise ValueError if response_message_cls is not a subclass of proto.Message or google.protobuf.message.Message* remove response_type from pytest.mark.parametrize* 🦉 Updates from OwlBot post-processorSeehttps://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md* add test for ValueError in response_iterator._grab()---------Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
parthea commentedFeb 13, 2024
/cherry-pick v1 |
* fix: resolve issue handling protobuf responses in rest streaming* raise ValueError if response_message_cls is not a subclass of proto.Message or google.protobuf.message.Message* remove response_type from pytest.mark.parametrize* 🦉 Updates from OwlBot post-processorSeehttps://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md* add test for ValueError in response_iterator._grab()---------Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Towards b/311671723