FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: resolve issue handling protobuf responses in rest streaming by parthea · Pull Request #604 · googleapis/python-api-core · GitHub

This repository was archived by the owner on Feb 23, 2026. It is now read-only.
/ python-api-core Public archive

fix: resolve issue handling protobuf responses in rest streaming - #604

Merged
parthea merged 5 commits into
mainfrom
fix-rest-streaming
Feb 13, 2024
ConversationCommits (5)ChecksFiles changed
Merged

fix: resolve issue handling protobuf responses in rest streaming#604
parthea merged 5 commits into
mainfrom
fix-rest-streaming

Conversation

parthea commented Feb 12, 2024

Copy link
Copy Markdown
Contributor

Towards b/311671723

product-auto-label Bot added the size: m Pull request size is medium. label Feb 12, 2024
parthea marked this pull request as ready for review February 12, 2024 21:01
parthea requested review from a team February 12, 2024 21:01
vchudnov-g approved these changes Feb 13, 2024
View reviewed changes

vchudnov-g left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A few minor comments, but LGTM.

Comment thread google/api_core/rest_streaming.py Outdated
return self._response_message_cls.from_json(self._ready_objs.popleft())
if issubclass(self._response_message_cls, proto.Message):
return self._response_message_cls.from_json(self._ready_objs.popleft())
else:

vchudnov-g Feb 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: for safety and future-proofing, maybe this should be an elif and we check for google.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)

parthea Feb 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed in 9f5b22f

Comment thread 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)],

vchudnov-g Feb 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Given that you're not testing different combinations of resp_message_is_proto_plus and response_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 on response_type and instead, in the function, do

if resp_message_is_proto_plus:
  response_type = EchoResponse
  responses = [EchoResponse(content="hello world"), EchoResponse(content="yes")]
else:
  response_type = httpbody_pb2.HttpBody
  responses = [
            httpbody_pb2.HttpBody(content_type="hello world"),
            httpbody_pb2.HttpBody(content_type="yes"),
        ]

parthea Feb 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed in 5b7a4ce

Comment thread 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",

vchudnov-g Feb 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Same comment as above: since you have the if below already, don't parametrize on response_type

parthea Feb 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed in 5b7a4ce

Comment thread tests/unit/test_rest_streaming.py Outdated
@pytest.mark.parametrize("random_split", [True, False])
def test_next_stress(random_split):
@pytest.mark.parametrize(
"random_split,resp_message_is_proto_plus,response_type",

vchudnov-g Feb 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ditto on parametrization

parthea Feb 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed in 5b7a4ce

Comment thread tests/unit/test_rest_streaming.py
),
Song(title='\\{"key": ["value",]}\\', composer=composer_with_relateds),
]
@pytest.mark.parametrize(

vchudnov-g Feb 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ditto on parametrizing

parthea Feb 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed in 5b7a4ce

product-auto-label Bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Feb 13, 2024
parthea merged commit bcebc92 into main Feb 13, 2024
parthea deleted the fix-rest-streaming branch February 13, 2024 14:59
release-please Bot mentioned this pull request Feb 13, 2024
Merged
This was referenced Feb 13, 2024
Closed
Merged

parthea commented Feb 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

/cherry-pick v1

parthea added a commit that referenced this pull request Feb 13, 2024
fix: resolve issue handling protobuf responses in rest streaming (#604)
65b760e 
* 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-processor

See https://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 restored the fix-rest-streaming branch February 13, 2024 16:50
parthea deleted the fix-rest-streaming branch February 13, 2024 17:02

parthea commented Feb 13, 2024

Copy link
Copy Markdown
Contributor Author

/cherry-pick v1

gcp-cherry-pick-bot Bot pushed a commit that referenced this pull request Feb 13, 2024
fix: resolve issue handling protobuf responses in rest streaming (#604)
fde7316 
* 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-processor

See https://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>
gcp-cherry-pick-bot Bot mentioned this pull request Feb 13, 2024
Merged
parthea mentioned this pull request Feb 29, 2024
Closed
release-please Bot mentioned this pull request May 30, 2025
Closed
release-please Bot mentioned this pull request May 30, 2025
Closed
daniel-sanche mentioned this pull request Mar 17, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
vchudnov-g vchudnov-g approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

vchudnov-g

ohmayr

Labels

size: l Pull request size is large.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL