Skip to content
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

Merged
merged 7 commits into from
Nov 8, 2021

Conversation

busunkim96
Copy link
Contributor

@busunkim96 busunkim96 commented Nov 2, 2021

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.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 2, 2021
@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Nov 2, 2021
Comment on lines -554 to -560
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"
)

Copy link
Contributor Author

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 fields 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.

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)

Copy link
Contributor

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.

@busunkim96 busunkim96 marked this pull request as ready for review November 5, 2021 21:17
@busunkim96 busunkim96 requested review from a team as code owners November 5, 2021 21:17
@busunkim96
Copy link
Contributor Author

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]
Copy link
Contributor Author

@busunkim96 busunkim96 Nov 5, 2021

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

snippetgen: streaming methods have syntax errors
4 participants