Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: Add support for creating exceptions from an asynchronous response#688

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

Merged
ohmayr merged 11 commits intomainfromadd-support-for-mapping-rest-callables
Sep 10, 2024

Conversation

ohmayr
Copy link
Contributor

@ohmayrohmayr commentedAug 22, 2024
edited
Loading

This PR implements a_RestCall class to mapGoogleAPICallError to an asynchronous rest callable that raisesgoogle.auth.exceptions.TransportError.This wrapping is done via a helper methodrest_helpers_async.wrap_errors.

A new parameterkind is introduced inwrap_method which can be one ofgrpc_asyncio (Default Value) orrest_asyncio to distinguish between the type of callables so that we can apply the appropriate wrapping logic.

The property kind is exposed at the transport levelhere and will be configured when wrapping methods during the transport initializationhere.

As a follow up, a similar logic must be implemented for synchronous rest callables to avoid gRPC specific stack trace within a rest call.

RE: Updated Workflow:

I've removed the proposed_RestCall class after determining that it is not needed to wrap a rest callable and just adds unnecessary logic.

The new proposed workflow is as follows:

  • This PR still introduces a new parameterkind inwrap_method to determine the type of a callable i.e.grpc_asyncio orrest_asyncio.
  • Ifkind=="rest_asyncio", we skip the call togrpc_helpers_async.wrap_errors which is gRPC specific.

Essentially, this does the trick if we follow a similar pattern for error handling to what we're doing in a sync rest call.

We have been lazily creating and raising aGoogleAPICallError from arequests.Response within our GAPICs using thefrom_http_response methodhere. Therefore, we don't need to add the error handling logic to the callable beforehand (we're currently doing both the things for a sync rest call and should skip wrapping the callable beforehand there too).

This PR only handles the error handling logic for an async REST call. It also introduces a new helper i.e.format_http_response_error to create and raise aGoogleAPICallError for a more generic response since the existingfrom_http_response is specific to handling arequests response.

Alternate approach for the introduced helper:

The proposed implementation offormat_http_response_error takes in payload as an argument so that we don't need to await for it within the call and keep this function synchronous.

Alternatively, we can have an async version offrom_http_response that can be used to createGoogleAPICallError for async rest calls.

Follow Up:

As a follow up, we should skip wrapping sync rest callable withgrpc_helpers.wrap_errors (similar to what we're doing here) to avoid the unnecessary gRPC specific logic within a rest call stracktrace.

@product-auto-labelproduct-auto-labelbot added the size: mPull request size is medium. labelAug 22, 2024
@ohmayrohmayr changed the base branch fromadd-support-for-async-rest-streaming tomainAugust 23, 2024 06:41
@ohmayrohmayrforce-pushed theadd-support-for-mapping-rest-callables branch from98d205f to573413aCompareAugust 23, 2024 06:43
@ohmayrohmayr changed the titlefeat: add suport for mapping exceptions to rest callablesfeat: add support for creating exceptions from an asynchronous ResponseAug 23, 2024
@ohmayrohmayr changed the titlefeat: add support for creating exceptions from an asynchronous Responsefeat: Add support for creating exceptions from an asynchronous responseAug 23, 2024
@ohmayrohmayr marked this pull request as ready for reviewAugust 23, 2024 06:47
@ohmayrohmayr requested review froma team ascode ownersAugust 23, 2024 06:47
return message


def format_http_response_error(response, method, url, payload=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: add type hints
nit: add code comments to clarify the reason that we're moving away fromfrom_http_response

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

addressed with a comment.

Copy link
Contributor

@vchudnov-gvchudnov-g left a comment

Choose a reason for hiding this comment

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

Looks good. Just minor comments/questions. Please address@parthea's comments as well.

@@ -32,6 +32,7 @@ def wrap_method(
default_timeout=None,
default_compression=None,
client_info=client_info.DEFAULT_CLIENT_INFO,
kind="grpc",
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, and when we callwrap_method from the GAPIC, it will specify this parameter, right? So that will be in a follow-up PR in the generator repo?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

That's correct! Note to myself to update the default value togrpc_asyncio.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Update the PR description with the right default value.

ohmayr reacted with thumbs up emoji
@ohmayrohmayr added the do not mergeIndicates a pull request not ready for merge, due to either quality or timing. labelAug 29, 2024
@ohmayr
Copy link
ContributorAuthor

Adding ado not merge label until open comments are addressed and the default value is updated togrpc_asyncio.

@ohmayrohmayr removed the do not mergeIndicates a pull request not ready for merge, due to either quality or timing. labelSep 9, 2024
@daniel-sanche
Copy link
Contributor

Would this fix the issue ingoogleapis/gapic-generator-python#1764? Or is this something else?

@ohmayr
Copy link
ContributorAuthor

googleapis/gapic-generator-python#1764

This PR removes grpc code path from the stack trace when an error is raised from an asynchronous rest call.

@ohmayrohmayr merged commit1c4b0d0 intomainSep 10, 2024
33 checks passed
@ohmayrohmayr deleted the add-support-for-mapping-rest-callables branchSeptember 10, 2024 18:22
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@partheapartheaparthea left review comments

@vchudnov-gvchudnov-gvchudnov-g approved these changes

Labels
size: mPull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@ohmayr@daniel-sanche@parthea@vchudnov-g

[8]ページ先頭

©2009-2025 Movatter.jp