Reworks the v0.4.1 release to be more sane#230
Reworks the v0.4.1 release to be more sane#230kevmoo wants to merge 3 commits intogoogleapis:mainfrom
Conversation
kevmoo
commented
Apr 7, 2026
kevmoo
commented
- Aligns the behavior of logging when one is on and not on Google Cloud
- Finalizes the stand-alone logger name to errorLoggingMiddleware
- errorLoggingMiddleware now handles all exception uniformly and WRT JSON responses
- Aligns the behavior of logging when one is on and not on Google Cloud - Finalizes the stand-alone logger name to errorLoggingMiddleware - errorLoggingMiddleware now handles all exception uniformly and WRT JSON responses
| ); | ||
| } catch (e, s) { | ||
| if (responseScenario == | ||
| _ResponseScenarios.nonHttpResponseError && |
There was a problem hiding this comment.
All exceptions are now caught, so this block is no longer needed!
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request renames httpResponseExceptionMiddleware to errorLoggingMiddleware and refactors error handling to provide standardized responses and enhanced logging. Feedback includes a critical requirement to rethrow HijackException for hijacked connections, and suggestions to improve error log clarity and response body formatting by including status codes and removing internal implementation prefixes.
| Handler exceptionHandler(Handler inner) => (request) async { | ||
| try { | ||
| return await inner(request); | ||
| } catch (error, stack) { |
There was a problem hiding this comment.
The middleware should not catch HijackException. Rethrowing it allows the underlying shelf server to handle the hijacked connection (e.g., for web sockets) correctly without attempting to send a standard HTTP response.
| } catch (error, stack) { | |
| } catch (error, stack) { | |
| if (error is HijackException) rethrow; | |
| final errorString = switch (error) { |
| HttpResponseException() => [ | ||
| if (error.innerError != null) | ||
| '${error.innerError} (${error.innerError.runtimeType})' | ||
| else | ||
| 'HTTP Exception: ${error.message} (${error.statusCode})', | ||
| if (error.status != null && error.status!.isNotEmpty) | ||
| 'Status: ${error.status}', | ||
| if (error.details != null && error.details!.isNotEmpty) | ||
| 'Details: ${error.details}', | ||
| ].join('\n'), |
There was a problem hiding this comment.
When an innerError is present, the current logging logic only logs the inner error's details, losing the context of the HttpResponseException itself (such as its message and status code). It is better to include both the high-level exception information and the underlying cause for better observability in stderr.
| HttpResponseException() => [ | |
| if (error.innerError != null) | |
| '${error.innerError} (${error.innerError.runtimeType})' | |
| else | |
| 'HTTP Exception: ${error.message} (${error.statusCode})', | |
| if (error.status != null && error.status!.isNotEmpty) | |
| 'Status: ${error.status}', | |
| if (error.details != null && error.details!.isNotEmpty) | |
| 'Details: ${error.details}', | |
| ].join('\n'), | |
| HttpResponseException() => [ | |
| 'HTTP Exception: ${error.message} (${error.statusCode})', | |
| if (error.innerError != null) | |
| 'Caused by: ${error.innerError} (${error.innerError.runtimeType})', | |
| if (error.status != null && error.status!.isNotEmpty) | |
| 'Status: ${error.status}', | |
| if (error.details != null && error.details!.isNotEmpty) | |
| 'Details: ${error.details}', | |
| ].join('\n'), |
| context: {_httpResponseExceptionKey: e, _exceptionStackTraceKey: stack}, | ||
| statusCode, | ||
| body: error is HttpResponseException | ||
| ? error.toString() |
There was a problem hiding this comment.
Using error.toString() for the plain text response body includes the HttpResponseException: prefix, which is an internal implementation detail. Using error.message directly provides a cleaner and more professional response for plain text clients.
| ? error.toString() | |
| body: error is HttpResponseException | |
| ? error.message | |
| : internalServerErrorMessage, |
| } | ||
| return buffer.toString(); | ||
| } | ||
| String toString() => 'HttpResponseException: $message'; |
There was a problem hiding this comment.
The new toString() implementation removes the HTTP status code, which makes the exception less informative when printed in logs or during debugging. While it is important to avoid leaking sensitive internal state (like innerError), the status code is public information and crucial for distinguishing between different error types (e.g., 400 vs 404) at a glance.
@override
String toString() => 'HttpResponseException ($statusCode): $message';| expect(ex.statusCode, 400); | ||
| expect(ex.message, 'Bad'); | ||
| expect(ex.toString(), 'Bad (400)'); | ||
| expect(ex.toString(), 'HttpResponseException: Bad'); |
There was a problem hiding this comment.
Update the test expectation to match the improved toString() format that includes the status code.
| expect(ex.toString(), 'HttpResponseException: Bad'); | |
| expect(ex.toString(), 'HttpResponseException (400): Bad'); |
| statusCode: 400, | ||
| contentType: anyOf(isNull, contains('text/plain')), | ||
| body: contains('minimal (400)'), | ||
| body: contains('HttpResponseException: minimal'), |
There was a problem hiding this comment.
Update the test expectation to match the cleaner plain text response body which should only contain the message without the class name prefix.
body: contains('minimal'),
|
|
||
| /// Convenience [Middleware] that handles logging depending on [projectId]. | ||
| /// | ||
| /// [projectId] is the optional Google Cloud Project ID used for trace |
There was a problem hiding this comment.
Can you clean up the writing a bit? You have three paragraphs about projectId.
Maybe something like:
If projectId is set then that project is used for trace correlation and the result will be a value from [cloudLoggingMiddleware]. If projectId is null, ...
| /// If [requestHeaders] indicate that JSON is expected, the response body will | ||
| /// be JSON. Otherwise, the response body will be a plain text string. | ||
| /// | ||
| /// ‼️ This method is VERY CAREFUL to not leak internal implementation details! |
There was a problem hiding this comment.
Co-authored-by: Brian Quinlan <bquinlan@google.com>
2 participants
