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

diregapic: errors that appear in the Operation type should be returned by Poll #1320

Closed
2 tasks
noahdietz opened this issue May 3, 2023 · 1 comment · Fixed by #1323 or #1324
Closed
2 tasks

diregapic: errors that appear in the Operation type should be returned by Poll #1320

noahdietz opened this issue May 3, 2023 · 1 comment · Fixed by #1323 or #1324
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@noahdietz
Copy link
Collaborator

googleapis/google-cloud-go#7876 points out that when an Operation actually resulted in an error, the client isn't surfacing that to the users as an error. It should do this.

The tricky part is that the Error type is bespoke, and doesn't align with either error type traditional APIs returned googleapi.Error and google.rpc.Status.

So the Operation type in compute.proto has its code and message fields annotated with ERROR_CODE and ERROR_MESSAGE google.cloud.operation_field annotations, which we can use to create an error. I've copied them below to save loading a large proto file:

  // [Output Only] If errors are generated during processing of the operation, this field will be populated.
  optional Error error = 96784904;

  // [Output Only] If the operation fails, this field contains the HTTP error message that was returned, such as `NOT FOUND`.
  optional string http_error_message = 202521945 [(google.cloud.operation_field) = ERROR_MESSAGE];

  // [Output Only] If the operation fails, this field contains the HTTP error status code that was returned. For example, a `404` means the resource was not found.
  optional int32 http_error_status_code = 312345196 [(google.cloud.operation_field) = ERROR_CODE];

We can create a googleapi.Error/apierror.APIError from that information, but those will be very very limited without the Error error field info.

To help with this, we may also want to consider adding some manual code to extend the generated operation handles that will extract the extra error details from Error error.

  • required: return an error based on if the ERROR_CODE and/or ERROR_MESSAGE fields are populated with non-default values (0 and "" respectively)
  • nice to have: some sort of helper code to extract the Error error information. This is likely required to be manually added.
@noahdietz noahdietz added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels May 3, 2023
@noahdietz
Copy link
Collaborator Author

Without a concrete solution for generically identifying the field containing error details, and then the even how to being navigating said bespoke field to extract the details, I think we can have the generator simply look for a field named error on the operation type, and include its contents as a string like fmt.Sprintf("%v"). This sucks because its not concrete, typed information like what appears in google.rpc.Status, but at least it will be included in logs of the error we return.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
2 participants