-
Notifications
You must be signed in to change notification settings - Fork 69
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(snippetgen): fix client streaming samples #1061
Conversation
client_streaming_forms = { | ||
types.CallingForm.RequestStreamingClient, | ||
types.CallingForm.RequestStreamingBidi, | ||
} | ||
|
||
if len(base_param_to_attrs) > 1 and calling_form in client_streaming_forms: | ||
raise types.InvalidRequestSetup( | ||
"Too many base parameters for client side streaming form" | ||
) | ||
|
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.
@software-dov 👋 I'm having trouble understanding this error, would you remember what this was written to prevent?
Some streaming method request messages have 2+ required fields (pubsub), so the current snippetgen logic adds two field
s to the dictionary. That causes this error to be raised. Are fields expected to be set differently for streaming?
# Request passed to `validate_and_transform_request`
[
{
"field":"subscription",
"value":"projects/{project}/subscriptions/{subscription}"
},
{
"field":"stream_ack_deadline_seconds",
"value":2813
}
]
I see two unit tests that raise this error, but I don't see any pre-existing tests showing a valid sample config for a client streaming method.
gapic-generator-python/tests/unit/samplegen/test_samplegen.py
Lines 1353 to 1402 in a8d2b3a
def test_single_request_client_streaming(dummy_api_schema, | |
calling_form=types.CallingForm.RequestStreamingClient): | |
# Each API client method really only takes one parameter: | |
# either a single protobuf message or an iterable of protobuf messages. | |
# With unary request methods, python lets us describe attributes as positional | |
# and keyword parameters, which simplifies request construction. | |
# The 'base' in the transformed request refers to an attribute, and the | |
# 'field's refer to sub-attributes. | |
# Client streaming and bidirectional streaming methods can't use this notation, | |
# and generate an exception if there is more than one 'base'. | |
input_type = DummyMessage( | |
fields={ | |
"cephalopod": DummyField( | |
message=DummyMessage( | |
fields={ | |
"order": DummyField( | |
message=DummyMessage(type="ORDER_TYPE") | |
) | |
}, | |
type="CEPHALOPOD_TYPE" | |
) | |
), | |
"gastropod": DummyField( | |
message=DummyMessage( | |
fields={ | |
"order": DummyField( | |
message=DummyMessage(type="ORDER_TYPE") | |
) | |
}, | |
type="GASTROPOD_TYPE" | |
) | |
) | |
}, | |
type="MOLLUSC_TYPE" | |
) | |
v = samplegen.Validator(DummyMethod(input=input_type), dummy_api_schema) | |
with pytest.raises(types.InvalidRequestSetup): | |
v.validate_and_transform_request( | |
types.CallingForm.RequestStreamingClient, | |
[ | |
{"field": "cephalopod.order", "value": "cephalopoda"}, | |
{"field": "gastropod.order", "value": "pulmonata"}, | |
], | |
) | |
def test_single_request_bidi_streaming(): | |
test_single_request_client_streaming( | |
types.CallingForm.RequestStreamingBidi) | |
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.
I really don't remember what was going on here. My guess is that I was mildly incorrect about the actual python method signature convention and I was trying to imply that we only wanted a single request to be put into a list...?
TL;DR: It's probably a bug or at least a misreading of the spec on my part.
This is ready for review, PTAL @software-dov @parthea. |
# 'logging_v2.TailLogEntriesRequest' objects | ||
# Here we create a generator that yields a single `request` for | ||
# demonstrative purposes. | ||
requests = [request] |
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.
One simpler alternative is:
requests = iter([requests])
I chose the generator over iter()
to nudge folks towards generating requests on demand.
Opinions/suggestions on the best way to present client streaming welcome!
Fixes #1014 and unblocks #1043.
NOTE: Some real world APIs expect the first request to pass a config (example) so the generated samples will not work out of the box. This will be addressed when the new sample config language is sorted out.