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: resolve rare issue where ValueError is not raised when both request and flattened param are set #2258

Closed
wants to merge 4 commits into from

Conversation

parthea
Copy link
Contributor

@parthea parthea commented Nov 22, 2024

Fixes #2257

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Nov 22, 2024
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Nov 22, 2024
@parthea parthea marked this pull request as ready for review November 22, 2024 17:36
@parthea parthea requested a review from a team as a code owner November 22, 2024 17:36
@parthea parthea changed the title fix: resolve issue where ValueError is not raised if both request and flattened param are set fix: resolve rare issue where ValueError is not raised when both request and flattened param are set Nov 22, 2024
@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 22, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 22, 2024
@parthea parthea added owlbot:run Add this label to trigger the Owlbot post processor. process: review requested labels Nov 22, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 22, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Nov 22, 2024
@@ -327,7 +327,8 @@ async def sample_get_trigger():
# Create or coerce a protobuf request object.
# - Quick check: If we got a request object, we should *not* have
# gotten any keyword arguments that map to the request.
has_flattened_params = any([name])
flattened_params = [name]
Copy link
Contributor

@vchudnov-g vchudnov-g Nov 22, 2024

Choose a reason for hiding this comment

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

How is any(flattened_params) (what we had before) any different from len([param for param in flattened_params if param is not None]) > 0? IIUC any returns True if any of its elements evaluate to True, so removing the Nones (which evaluate to False anyway) would only make a difference if the resulting list has only non-None falsy-objects (where any would have returned False but len(...) > 0 would now return True). What would be a real example of such objects? Could we capture them in a golden file? (I scanned quickly and didn't see one.)

ref: https://docs.python.org/3/library/functions.html#any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we have non-None, falsy objects. The test_nested_messages.proto that was added to fragment tests is an example of a proto where encounter this problem

There is an example in the stack trace in #2257 (comment)

>           await client.my_method(
                test_nested_messages.MethodRequest(),
                some_message=test_nested_messages.SomeMessage(another_message=None),
            )

Specifically any([test_nested_messages.SomeMessage(another_message=None)]) evaluates to False however test_nested_messages.SomeMessage(another_message=None) is not None evaluates to True

See screen capture from local debugging of the generated fragment client.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I see. That's slightly surprising, but I guess the semantics are that if a message has nothing set, it evaluates as False? That would fit with what you're describing.

Thanks for the explanation!

@@ -327,7 +327,8 @@ async def sample_get_trigger():
# Create or coerce a protobuf request object.
# - Quick check: If we got a request object, we should *not* have
# gotten any keyword arguments that map to the request.
has_flattened_params = any([name])
flattened_params = [name]
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I see. That's slightly surprising, but I guess the semantics are that if a message has nothing set, it evaluates as False? That would fit with what you're describing.

Thanks for the explanation!

@parthea parthea added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 23, 2024
@parthea
Copy link
Contributor Author

parthea commented Nov 23, 2024

Adding do not merge while we discuss whether this could be considered a breaking change (and defer the fix until the next major version)

@parthea
Copy link
Contributor Author

parthea commented Nov 26, 2024

Let's revisit this PR in January

@parthea parthea closed this Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Indicates a pull request not ready for merge, due to either quality or timing. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated test_*_flattened_error test fails when the first field of a request message is a nested message
3 participants