Error handling improvements #2067

amanda-tarafa · 2022-03-03T17:14:24Z

As usual one commit at a time.

amanda-tarafa · 2022-03-03T17:20:30Z

Things that we can consider:

Add more information in GoogleApiException.Message if no message has been specified.
Even if a message was specified, attach more information to it. Not sure I like this option, I think we should respect whatever the messages calling code specifies.

jskeet

Generally fine - a few nits, but my only real concern is whether the default ToString() is now too big.

jskeet · 2022-03-04T09:56:51Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

        }

        /// <summary>Creates an API Service exception.</summary>
-        public GoogleApiException(string serviceName, string message) : this(serviceName, message, null) { }
+        public GoogleApiException(string serviceName, string message)


I think the formatting was actually clearer before, to be honest. not overly bothered though.

Changed : this(...) back to the previous line but left the { } on the next line, so it's clearer that the constructor is not doing anything itself.

jskeet · 2022-03-04T09:57:17Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

+            : this(serviceName, message, null)
+        { }
+
+        /// <summary>Creates an API Service exception.</summary>


Expand the comment to say "with no message" and maybe explain when the Message property will still have useful information?

jskeet · 2022-03-04T09:58:52Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

+
+        private string ServiceNameMessage => $"The service {serviceName} has thrown an exception.";
+
+        private string DefaultMessage =>


Unwrap to single line?

And maybe rename to "ServiceHttpAndErrorMessage" or something - "DefaultMessage" sounds like "the message you'd get if you didn't override the Message property" which is the opposite of what it means :)

Unwrapped done and renamed to CombinedMessage.

jskeet · 2022-03-04T10:00:13Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

@@ -26,6 +26,7 @@ namespace Google
    public class GoogleApiException : Exception
    {
        private readonly string serviceName;
+        private readonly string message;


As discussed, this can probably be a boolean just to indicate whether a message was supplied.

And let's give it a leading underscore, which won't be inconsistent if we take other suggestions...

jskeet · 2022-03-04T10:01:30Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

@@ -26,6 +26,7 @@ namespace Google
    public class GoogleApiException : Exception
    {
        private readonly string serviceName;


Let's remove this and make ServiceName a read-only autoprop. That remove the unconventional field name and makes the code feel more modern :)

Ah yes, done.

jskeet · 2022-03-04T10:20:45Z

Src/Support/Google.Apis/Requests/BatchRequest.cs

+                        // Parse the error from the current response.
+                        error = await service.DeserializeError(responseMessage).ConfigureAwait(false);
+                    }
+                    catch(GoogleApiException ex) when (!(ex.Error is null))


Nit: add space. Oh for later versions of C# with "is not null". (Or just use ex.Error is object for now.)

jskeet · 2022-03-04T10:23:12Z

Src/Support/Google.Apis/Responses/HttpResponseMessageExtensions.cs

+    {
+        internal static async Task<RequestError> DeserializeErrorAsync(this HttpResponseMessage response, string name, ISerializer serializer)
+        {
+            if (response?.Content is null)


Add a comment here?

jskeet · 2022-03-04T10:23:37Z

Src/Support/Google.Apis/Responses/HttpResponseMessageExtensions.cs

+            }
+            catch (JsonException ex)
+            {
+                throw new GoogleApiException(name, null, ex)


Maybe use a named argument for the second one (I assume it's message).

Done, yes it's the message

jskeet · 2022-03-04T10:23:57Z

Src/Support/Google.Apis/Responses/HttpResponseMessageExtensions.cs

+                    Error = new RequestError { ErrorResponseContent = responseText }
+                };
+            }
+            if (error == null)


is instead of ==, if you've got the energy to change it

jskeet · 2022-03-04T10:24:38Z

Src/Support/Google.Apis/Responses/HttpResponseMessageExtensions.cs

+{
+    internal static class HttpResponseMessageExtensions
+    {
+        internal static async Task<RequestError> DeserializeErrorAsync(this HttpResponseMessage response, string name, ISerializer serializer)


I think it would be good to add a comment to explain the situations in which this throws an exception instead of returning an error.

amanda-tarafa

Jon, all comments adressed on the latest commit.

For trying to reduce the size of the ToString() I'm only printing the raw content if we couldn't parse any of it as an ErrorResponse instead of always.

I'm taking a look at BQ and Storage now and see what direct improvements we can do there.

amanda-tarafa · 2022-03-05T00:19:25Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

@@ -26,6 +26,7 @@ namespace Google
    public class GoogleApiException : Exception
    {
        private readonly string serviceName;


Ah yes, done.

amanda-tarafa · 2022-03-05T00:21:47Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

@@ -26,6 +26,7 @@ namespace Google
    public class GoogleApiException : Exception
    {
        private readonly string serviceName;
+        private readonly string message;


amanda-tarafa · 2022-03-05T00:29:15Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

        }

        /// <summary>Creates an API Service exception.</summary>
-        public GoogleApiException(string serviceName, string message) : this(serviceName, message, null) { }
+        public GoogleApiException(string serviceName, string message)


Changed : this(...) back to the previous line but left the { } on the next line, so it's clearer that the constructor is not doing anything itself.

amanda-tarafa · 2022-03-05T00:29:31Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

+            : this(serviceName, message, null)
+        { }
+
+        /// <summary>Creates an API Service exception.</summary>


amanda-tarafa · 2022-03-05T00:35:11Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

+
+        private string ServiceNameMessage => $"The service {serviceName} has thrown an exception.";
+
+        private string DefaultMessage =>


Unwrapped done and renamed to CombinedMessage.

amanda-tarafa · 2022-03-05T00:49:26Z

Src/Support/Google.Apis/Requests/BatchRequest.cs

+                        // Parse the error from the current response.
+                        error = await service.DeserializeError(responseMessage).ConfigureAwait(false);
+                    }
+                    catch(GoogleApiException ex) when (!(ex.Error is null))


amanda-tarafa · 2022-03-05T00:51:55Z

Src/Support/Google.Apis/Responses/HttpResponseMessageExtensions.cs

+                    Error = new RequestError { ErrorResponseContent = responseText }
+                };
+            }
+            if (error == null)


amanda-tarafa · 2022-03-05T00:52:35Z

Src/Support/Google.Apis/Responses/HttpResponseMessageExtensions.cs

+            }
+            catch (JsonException ex)
+            {
+                throw new GoogleApiException(name, null, ex)


Done, yes it's the message

amanda-tarafa · 2022-03-05T01:00:13Z

Src/Support/Google.Apis/Responses/HttpResponseMessageExtensions.cs

+{
+    internal static class HttpResponseMessageExtensions
+    {
+        internal static async Task<RequestError> DeserializeErrorAsync(this HttpResponseMessage response, string name, ISerializer serializer)


amanda-tarafa · 2022-03-05T01:09:26Z

Src/Support/Google.Apis/Responses/HttpResponseMessageExtensions.cs

+    {
+        internal static async Task<RequestError> DeserializeErrorAsync(this HttpResponseMessage response, string name, ISerializer serializer)
+        {
+            if (response?.Content is null)


jskeet · 2022-03-07T17:04:20Z

Will look tomorrow...

jskeet

A few nits, but they could be dealt with separately if you'd prefer

jskeet · 2022-03-08T06:58:26Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

-            serviceName.ThrowIfNull("serviceName");
-            this.serviceName = serviceName;
-            this.message = message;
+            ServiceName = serviceName.ThrowIfNull("serviceName");


Nit: use nameof.

jskeet · 2022-03-08T06:58:35Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

-            this.serviceName = serviceName;
-            this.message = message;
+            ServiceName = serviceName.ThrowIfNull("serviceName");
+            _hasMessage = !(message is null);


message is object?

jskeet · 2022-03-08T06:59:59Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

-            : $"Raw error details are: {Error.ErrorResponseContent}";
+            : (Error.IsOnlyRawContent
+                ? $"No JSON error details were specified.{Environment.NewLine}" +
+                    (Error.ErrorResponseContent.Trim() == ""


Use string.IsNullOrWhitespace?

Ha, yes, of course, done.

jskeet · 2022-03-08T07:00:50Z

Src/Support/Google.Apis.Tests/Apis/Requests/ClientServiceRequestTest.cs

-                string exToString = exception.ToString();
-                Assert.Contains(ErrorMessageHanlder.ExpectedError, exToString);
-                Assert.Contains(ErrorMessageHanlder.ErrorContent, exToString);
+                Assert.Contains(ErrorMessageHanlder.ExpectedError, exception.ToString());


Nit: at some point, maybe not in this PR, let's fix ErrorMessageHanlder to ErrorMessageHandler.

I'm adding a commit for this.

jskeet · 2022-03-08T07:01:24Z

Src/Support/Google.Apis/Responses/HttpResponseMessageExtensions.cs

+        /// </list>
+        /// Any exception thrown while reading the <paramref name="response"/> <see cref="HttpResponseMessage.Content"/>
+        /// will be bubbled up.
+        /// Otherwie this method will returned the deserialized <see cref="RequestError"/>.


Nit: Otherwie => Otherwise

Done, and also returned-> return

In particular improves GoogleApiExcetion error messages and string representation by including more information. In cases when then response does not contain a valid JSON body, the error message has changed entirely to be more explicit. This affects normal, batch, and media upload and download requests. Closes googleapis#2063. Towards googleapis/google-cloud-dotnet#8206 BREAKING CHANGE: If code was depending on exact exception message or string representation content.

amanda-tarafa

All latest nits addressed.

I've squashed all the commits into one, and added a new commit fixing the test class name typo.

amanda-tarafa · 2022-03-09T10:52:23Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

-            serviceName.ThrowIfNull("serviceName");
-            this.serviceName = serviceName;
-            this.message = message;
+            ServiceName = serviceName.ThrowIfNull("serviceName");


amanda-tarafa · 2022-03-09T10:52:55Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

-            this.serviceName = serviceName;
-            this.message = message;
+            ServiceName = serviceName.ThrowIfNull("serviceName");
+            _hasMessage = !(message is null);


amanda-tarafa · 2022-03-09T10:53:26Z

Src/Support/Google.Apis.Core/GoogleApiException.cs

-            : $"Raw error details are: {Error.ErrorResponseContent}";
+            : (Error.IsOnlyRawContent
+                ? $"No JSON error details were specified.{Environment.NewLine}" +
+                    (Error.ErrorResponseContent.Trim() == ""


Ha, yes, of course, done.

amanda-tarafa · 2022-03-09T10:54:47Z

Src/Support/Google.Apis.Tests/Apis/Requests/ClientServiceRequestTest.cs

-                string exToString = exception.ToString();
-                Assert.Contains(ErrorMessageHanlder.ExpectedError, exToString);
-                Assert.Contains(ErrorMessageHanlder.ErrorContent, exToString);
+                Assert.Contains(ErrorMessageHanlder.ExpectedError, exception.ToString());


I'm adding a commit for this.

amanda-tarafa · 2022-03-09T10:55:57Z

Src/Support/Google.Apis/Responses/HttpResponseMessageExtensions.cs

+        /// </list>
+        /// Any exception thrown while reading the <paramref name="response"/> <see cref="HttpResponseMessage.Content"/>
+        /// will be bubbled up.
+        /// Otherwie this method will returned the deserialized <see cref="RequestError"/>.


Done, and also returned-> return

Features - [googleapis#2067] BREAKING. Improve error handling for failed requests. BREAKING CHANGE: If calling code depended on exact GoogleApiException message or string representation. - [googleapis#2086] Makes `LocalServerCodeReceiver` partially extendable.

Features - [#2067] BREAKING. Improve error handling for failed requests. BREAKING CHANGE: If calling code depended on exact GoogleApiException message or string representation. - [#2086] Makes `LocalServerCodeReceiver` partially extendable.

amanda-tarafa requested a review from jskeet March 3, 2022 17:14

amanda-tarafa requested a review from a team as a code owner March 3, 2022 17:14

amanda-tarafa self-assigned this Mar 3, 2022

This was referenced Mar 3, 2022

GoogleApiException: "Error response is null" - request to include status code in message #2063

Closed

BigQuery.V2 Does Not Report Insert Errors googleapis/google-cloud-dotnet#8206

Closed

jskeet approved these changes Mar 4, 2022

View reviewed changes

amanda-tarafa force-pushed the error-handling branch from 641310f to c244dd2 Compare March 7, 2022 16:36

amanda-tarafa commented Mar 7, 2022

View reviewed changes

jskeet approved these changes Mar 8, 2022

View reviewed changes

amanda-tarafa mentioned this pull request Mar 8, 2022

feat!: Improves error handling. googleapis/google-cloud-dotnet#8228

Merged

amanda-tarafa added 2 commits March 9, 2022 14:07

chore: Fix typo in class name use for testing only.

449d1c1

amanda-tarafa force-pushed the error-handling branch from c244dd2 to 449d1c1 Compare March 9, 2022 14:10

amanda-tarafa commented Mar 9, 2022

View reviewed changes

amanda-tarafa merged commit 92c58dd into googleapis:main Mar 9, 2022

amanda-tarafa deleted the error-handling branch March 9, 2022 14:23

amanda-tarafa mentioned this pull request Mar 25, 2022

Update version: 1.56.0 -> 1.57.0 #2087

Merged


		private string ServiceNameMessage => $"The service {serviceName} has thrown an exception.";

		private string DefaultMessage =>

Error handling improvements #2067

Error handling improvements #2067

Conversation

amanda-tarafa commented Mar 3, 2022

amanda-tarafa commented Mar 3, 2022 • edited Loading

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet commented Mar 7, 2022

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa commented Mar 3, 2022 •

edited

Loading