diff --git a/pkgs/google_cloud/CHANGELOG.md b/pkgs/google_cloud/CHANGELOG.md index aecfc334..1645dc12 100644 --- a/pkgs/google_cloud/CHANGELOG.md +++ b/pkgs/google_cloud/CHANGELOG.md @@ -2,9 +2,9 @@ ### `http_serving.dart` -- Introduces `HttpResponseException` and `httpResponseExceptionMiddleware`. +- Introduces `HttpResponseException` and `errorLoggingMiddleware`. - `BadRequestException` and `badRequestMiddleware` are deprecated in favor of - `HttpResponseException` and `httpResponseExceptionMiddleware`, respectively. + `HttpResponseException` and `errorLoggingMiddleware`, respectively. (They are aliased for backward compatibility.) - Expanded `HttpResponseException` to support structured error reporting as per AIP-193. @@ -26,7 +26,7 @@ - `gatewayTimeout` (504) - Added default `status` values for factories that map 1:1 to gRPC status codes (e.g., `unauthorized` defaults to `'UNAUTHENTICATED'`). -- Updated `httpResponseExceptionMiddleware` to leverage +- Updated `errorLoggingMiddleware` to leverage `HttpResponseException.toJson()` for JSON responses, returning a standard Google Cloud error payload. - Updated plain text errors to use `HttpResponseException.toString()`. diff --git a/pkgs/google_cloud/lib/http_serving.dart b/pkgs/google_cloud/lib/http_serving.dart index b0233590..039f7189 100644 --- a/pkgs/google_cloud/lib/http_serving.dart +++ b/pkgs/google_cloud/lib/http_serving.dart @@ -43,7 +43,7 @@ /// {@canonicalFor http_serving.cloudLoggingMiddleware} /// {@canonicalFor http_serving.createLoggingMiddleware} /// {@canonicalFor http_serving.currentLogger} -/// {@canonicalFor http_serving.httpResponseExceptionMiddleware} +/// {@canonicalFor http_serving.errorLoggingMiddleware} /// {@canonicalFor http_serving.BadRequestException} /// {@canonicalFor http_serving.HttpResponseException} /// {@canonicalFor http_serving.listenPortFromEnvironment} @@ -61,7 +61,7 @@ export 'src/serving/http_logging.dart' cloudLoggingMiddleware, createLoggingMiddleware, currentLogger, - httpResponseExceptionMiddleware; + errorLoggingMiddleware; export 'src/serving/http_response_exception.dart' show // ignore: deprecated_member_use_from_same_package diff --git a/pkgs/google_cloud/lib/src/serving/http_logging.dart b/pkgs/google_cloud/lib/src/serving/http_logging.dart index b7ee48af..ecd4b799 100644 --- a/pkgs/google_cloud/lib/src/serving/http_logging.dart +++ b/pkgs/google_cloud/lib/src/serving/http_logging.dart @@ -16,7 +16,7 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io'; -import 'package:io/ansi.dart'; +import 'package:meta/meta.dart'; import 'package:shelf/shelf.dart'; import '../constants.dart'; @@ -28,92 +28,140 @@ import 'trace_context_data.dart'; export '../structured_logging.dart'; -const _httpResponseExceptionKey = 'google_cloud.response_exception'; -const _exceptionStackTraceKey = 'google_cloud.bad_stack_trace'; +/// The default message to use for internal server errors. +/// +/// This is used when an exception is thrown that is not an +/// [HttpResponseException]. +@internal +const internalServerErrorMessage = 'Internal Server Error'; /// Convenience [Middleware] that handles logging depending on [projectId]. /// /// [projectId] is the optional Google Cloud Project ID used for trace /// correlation. /// -/// If [projectId] is `null`, returns [Middleware] composed of [logRequests] and -/// [httpResponseExceptionMiddleware]. +/// If [projectId] is `null`, returns [errorLoggingMiddleware]. /// /// If [projectId] is provided, returns the value from [cloudLoggingMiddleware]. Middleware createLoggingMiddleware({String? projectId}) => projectId == null - ? _errorWriter - .addMiddleware(logRequests()) - .addMiddleware(_handleResponseException) + ? _handleResponseException : cloudLoggingMiddleware(projectId); -@Deprecated('use httpResponseExceptionMiddleware instead') -Middleware get badRequestMiddleware => httpResponseExceptionMiddleware; - -/// Adds logic which catches [HttpResponseException], logs details to [stderr] -/// and returns a corresponding [Response]. -Middleware get httpResponseExceptionMiddleware => _handleResponseException; - -Handler _handleResponseException(Handler innerHandler) => (request) async { - try { - final response = await innerHandler(request); - return response; - } on HttpResponseException catch (error, stack) { - return _responseFromException(error, stack, request.headers); - } -}; - -Handler _errorWriter(Handler innerHandler) => (request) async { - final response = await innerHandler(request); - - final error = - response.context[_httpResponseExceptionKey] as HttpResponseException?; - - if (error != null) { - final stack = response.context[_exceptionStackTraceKey] as StackTrace?; - final output = [ - error, - if (error.innerError != null) - '${error.innerError} (${error.innerError.runtimeType})', - if (error.innerStack ?? stack case final s?) formatStackTrace(s), - ]; +@Deprecated('use errorLoggingMiddleware instead') +Middleware get badRequestMiddleware => errorLoggingMiddleware; - final bob = output - .expand((e) => LineSplitter.split('$e'.trim())) - .join('\n'); - - stderr.writeln(lightRed.wrap(bob)); - } - return response; -}; +/// Wraps the [logRequests] middleware and catches exceptions and logs them to +/// stderr. +/// +/// Caught exceptions are logged as normal text to [stderr]. +/// +/// {@template exceptionResponseMapping} +/// All errors that are thrown in the context of the handler are caught and +/// and logged with an appopriate response sent to the caller. +/// +/// The HTTP status code sent depends on the exception type. +/// +/// - [HttpResponseException] (including [BadRequestException]) cause a response +/// with the status code of the exception. +/// - Other exceptions cause a response with a status code of 500 and the +/// default message "Internal Server Error". +/// +/// The response body will be JSON if the request headers indicate that JSON +/// is expected, otherwise it will be a plain text string. +/// {@endtemplate} +Middleware get errorLoggingMiddleware => _handleResponseException; + +Handler _handleResponseException(Handler innerHandler) { + Handler exceptionHandler(Handler inner) => (request) async { + try { + return await inner(request); + } catch (error, stack) { + 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'), + _ => '$error (${error.runtimeType})', + }; + + final theStack = error is HttpResponseException + ? error.innerStack ?? stack + : stack; + + final text = [ + errorString, + formatStackTrace(theStack), + ].expand((e) => LineSplitter.split('$e'.trim())).join('\n'); + + stderr.writeln(text); + return _responseFromException(error, stack, request.headers); + } + }; + return logRequests()(exceptionHandler(innerHandler)); +} +/// Creates a [Response] from [error] and [stack]. +/// +/// 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! +/// +/// If the error is an [HttpResponseException], the `toString` and `toJson` +/// methods are used, but they have been carefully written to not leak internal +/// implementation details. Response _responseFromException( - HttpResponseException e, + Object error, StackTrace stack, [ Map? requestHeaders, ]) { + final statusCode = error is HttpResponseException ? error.statusCode : 500; + if (requestHeaders != null && shouldSendJsonResponse(requestHeaders)) { + final jsonBody = error is HttpResponseException + // ‼️ Note we only send the `toJson` of HttpResponseException because + // it's been vetted to be safe. + ? error.toJson() + : { + 'error': { + 'code': statusCode, + 'message': internalServerErrorMessage, + 'status': 'INTERNAL', + }, + }; + return Response( - e.statusCode, - body: jsonEncode(e.toJson()), + statusCode, + body: jsonEncode(jsonBody), headers: {HttpHeaders.contentTypeHeader: 'application/json'}, - context: {_httpResponseExceptionKey: e, _exceptionStackTraceKey: stack}, ); } return Response( - e.statusCode, - body: e.toString(), - context: {_httpResponseExceptionKey: e, _exceptionStackTraceKey: stack}, + statusCode, + body: error is HttpResponseException + // ‼️ Note we only send the `toString` of HttpResponseException because + // it's been vetted to be safe. + ? error.toString() + : internalServerErrorMessage, ); } -/// Return [Middleware] that logs errors using Google Cloud structured logs and -/// returns the correct response. +/// Return [Middleware] that handles exceptions and generates Google Cloud +/// structured logs. /// /// [projectId] is the Google Cloud Project ID used for trace correlation. /// -/// Log messages of type [Map] are logged as structured logs (`jsonPayload`); -/// all other logs messages are logged as text logs (`textPayload`). +/// Logs messages sent to [currentLogger] and calls to [print] are formatted +/// to include trace correlation. +/// +/// {@macro exceptionResponseMapping} Middleware cloudLoggingMiddleware(String projectId) { Handler hostedLoggingMiddleware(Handler innerHandler) => (request) async { // Add log correlation to nest all log messages beneath request log in @@ -152,18 +200,34 @@ Middleware cloudLoggingMiddleware(String projectId) { completer.completeError(error, stackTrace); } - final logContentString = error is HttpResponseException - ? createErrorLogEntryFromRequest( - error, - error.innerStack ?? stackTrace, - error.statusCode >= 500 ? LogSeverity.error : LogSeverity.warning, - extraPayload: error.toJson(), - ) - : createErrorLogEntryFromRequest( - error, - stackTrace, - LogSeverity.error, - ); + final mainErrorString = switch (error) { + HttpResponseException(innerError: final innerError?) => + '$error — Caused by: $innerError (${innerError.runtimeType})', + _ => '$error', + }; + + final extraPayload = error is HttpResponseException + ? { + ...error.toJson(), + if (error.innerError != null) + 'inner_error': '${error.innerError}', + if (error.innerStack != null) + 'inner_stack_trace': formatStackTrace(error.innerStack!), + } + : null; + + final logSeverity = error is HttpResponseException + ? (error.statusCode >= 500 ? LogSeverity.error : LogSeverity.warning) + : LogSeverity.error; + + final logContentString = createErrorLogEntryFromRequest( + mainErrorString, + error is HttpResponseException + ? error.innerStack ?? stackTrace + : stackTrace, + logSeverity, + extraPayload: extraPayload, + ); // Serialize to a JSON string and output. parent.print(self, logContentString); @@ -172,9 +236,11 @@ Middleware cloudLoggingMiddleware(String projectId) { return; } - final response = error is HttpResponseException - ? _responseFromException(error, stackTrace, request.headers) - : Response.internalServerError(); + final response = _responseFromException( + error, + stackTrace, + request.headers, + ); completer.complete(response); } diff --git a/pkgs/google_cloud/lib/src/serving/http_response_exception.dart b/pkgs/google_cloud/lib/src/serving/http_response_exception.dart index a0f341dc..996e9bc8 100644 --- a/pkgs/google_cloud/lib/src/serving/http_response_exception.dart +++ b/pkgs/google_cloud/lib/src/serving/http_response_exception.dart @@ -12,21 +12,25 @@ // See the License for the specific language governing permissions and // limitations under the License. -import 'package:shelf/shelf.dart'; +/// @docImport 'http_logging.dart'; +library; -import 'http_logging.dart'; +import 'package:shelf/shelf.dart'; @Deprecated('use HttpResponseException instead') typedef BadRequestException = HttpResponseException; /// When thrown in a [Handler], configured with -/// [httpResponseExceptionMiddleware] or similar, causes a response with +/// [createLoggingMiddleware] or similar, causes a response with /// [statusCode] to be returned. /// /// [statusCode] must be >= `400` and <= `599`. /// /// [message] is used as the body of the response send to the requester. /// +/// [details] can be used to provide additional debugging information which is +/// included in logs, but not sent to the requester. +/// /// If provided, [innerError] and [innerStack] can be used to provide additional /// debugging information which is included in logs, but not sent to the /// requester. @@ -271,16 +275,17 @@ class HttpResponseException implements Exception { details: details, ); + // ‼️ DO NOT INCLUDE innerError or innerStack in toString(). + // + // This data is presented to the end user and may contain sensitive + // information. @override - String toString() { - final buffer = StringBuffer('$message ($statusCode)'); - if (status != null && status!.isNotEmpty) buffer.write(' [$status]'); - if (details != null && details!.isNotEmpty) { - buffer.write(' Details: $details'); - } - return buffer.toString(); - } + String toString() => 'HttpResponseException: $message'; + // ‼️ DO NOT INCLUDE innerError or innerStack in toJson(). + // + // This data is presented to the end user and may contain sensitive + // information. /// Returns a JSON representation of the error, suitable for including in a /// response body. Map toJson() => { diff --git a/pkgs/google_cloud/pubspec.yaml b/pkgs/google_cloud/pubspec.yaml index be80b847..8ee77fd5 100644 --- a/pkgs/google_cloud/pubspec.yaml +++ b/pkgs/google_cloud/pubspec.yaml @@ -11,7 +11,6 @@ environment: dependencies: collection: ^1.19.0 http: ^1.1.0 - io: ^1.0.3 meta: ^1.18.1 shelf: ^1.4.0 stack_trace: ^1.11.0 diff --git a/pkgs/google_cloud/test/http_response_exception_test.dart b/pkgs/google_cloud/test/http_response_exception_test.dart index aac876a4..b8272cb1 100644 --- a/pkgs/google_cloud/test/http_response_exception_test.dart +++ b/pkgs/google_cloud/test/http_response_exception_test.dart @@ -35,7 +35,7 @@ void main() { final ex = HttpResponseException(400, 'Bad'); expect(ex.statusCode, 400); expect(ex.message, 'Bad'); - expect(ex.toString(), 'Bad (400)'); + expect(ex.toString(), 'HttpResponseException: Bad'); }); test('invalid status code low', () { diff --git a/pkgs/google_cloud/test/logging_test.dart b/pkgs/google_cloud/test/logging_test.dart index edcfb84b..1cdc953c 100644 --- a/pkgs/google_cloud/test/logging_test.dart +++ b/pkgs/google_cloud/test/logging_test.dart @@ -54,24 +54,11 @@ void main() { print: (_, _, _, String line) => stdoutLines.add(line), ), () async { - try { - response = await handler( - responseType.toRequest( - responseScenario == _ResponseScenarios.successfulWithLogs, - ), - ); - } catch (e, s) { - if (responseScenario == - _ResponseScenarios.nonHttpResponseError && - environment == _Environment.normal) { - stderrLines - ..add('Exception: $e') - ..add(s.toString()); - response = Response.internalServerError(); - } else { - rethrow; - } - } + response = await handler( + responseType.toRequest( + responseScenario == _ResponseScenarios.successfulWithLogs, + ), + ); }, ), stdout: () => _MockStdout(stdoutLines), @@ -147,7 +134,7 @@ TypeMatcher _responseScenarioFactory( _responseMatcher( statusCode: 400, contentType: anyOf(isNull, contains('text/plain')), - body: contains('minimal (400)'), + body: contains('HttpResponseException: minimal'), ), (_ResponseScenarios.httpResponseErrorMinimal, _ResponseType.json) => _responseMatcher( @@ -161,7 +148,7 @@ TypeMatcher _responseScenarioFactory( _responseMatcher( statusCode: 400, contentType: anyOf(isNull, contains('text/plain')), - body: contains('with details (400)'), + body: contains('HttpResponseException: with details'), ), (_ResponseScenarios.httpResponseErrorWithDetails, _ResponseType.json) => _responseMatcher( @@ -190,8 +177,14 @@ TypeMatcher _responseScenarioFactory( (_ResponseScenarios.nonHttpResponseError, _ResponseType.json) => _responseMatcher( statusCode: 500, - contentType: anyOf(isNull, contains('text/plain')), - body: contains(internalServerErrorMessage), + contentType: contains('application/json'), + body: _jsonStringMatcher({ + 'error': { + 'code': 500, + 'message': internalServerErrorMessage, + 'status': 'INTERNAL', + }, + }), ), (_ResponseScenarios.successfulWithLogs, _) => _responseMatcher( statusCode: 200, @@ -206,7 +199,7 @@ TypeMatcher _responseScenarioFactory( ) => switch ((responseScenario, environment)) { (_ResponseScenarios.httpResponseErrorMinimal, _Environment.cloud) => ( stdout: _jsonStringMatcher({ - 'message': 'minimal (400)', + 'message': 'HttpResponseException: minimal', 'severity': 'WARNING', 'error': {'code': 400, 'message': 'minimal'}, 'stack_trace': _stackTraceMatcher, @@ -216,7 +209,7 @@ TypeMatcher _responseScenarioFactory( ), (_ResponseScenarios.httpResponseErrorWithDetails, _Environment.cloud) => ( stdout: _jsonStringMatcher({ - 'message': startsWith('with details (400)'), + 'message': startsWith('HttpResponseException: with details'), 'severity': 'WARNING', 'error': { 'code': 400, @@ -229,6 +222,8 @@ TypeMatcher _responseScenarioFactory( }, ], }, + 'inner_error': 'Invalid argument(s): inner error', + 'inner_stack_trace': _stackTraceMatcher, 'stack_trace': _stackTraceMatcher, 'logging.googleapis.com/sourceLocation': isA>(), }), @@ -245,18 +240,21 @@ TypeMatcher _responseScenarioFactory( ), (_ResponseScenarios.httpResponseErrorMinimal, _Environment.normal) => ( stdout: endsWith('[400] /'), - stderr: allOf([contains('minimal (400)'), _stackTraceMatcher]), + stderr: allOf([ + contains('HTTP Exception: minimal (400)'), + _stackTraceMatcher, + ]), ), (_ResponseScenarios.httpResponseErrorWithDetails, _Environment.normal) => ( stdout: endsWith('[400] /'), stderr: allOf([ contains('Invalid argument(s): inner error (ArgumentError)'), - contains('[BAD_REQUEST]'), + contains('Status: BAD_REQUEST'), _stackTraceMatcher, ]), ), (_ResponseScenarios.nonHttpResponseError, _Environment.normal) => ( - stdout: contains('[ERROR]'), + stdout: endsWith('[500] /'), stderr: allOf([contains('Exception: non http error'), _stackTraceMatcher]), ), (_ResponseScenarios.successfulWithLogs, _Environment.cloud) => ( @@ -328,12 +326,10 @@ TypeMatcher _responseMatcher({ Matcher _jsonStringMatcher(Object expected) => isA().having( (output) => jsonDecode(output) as Map, 'decoded as a JSON map', - equals(expected), + expected, ); -final _stackTraceMatcher = matches( - RegExp(r'test/logging_test.dart[: ]\d+:\d+'), -); +final _stackTraceMatcher = matches(RegExp(r'test/logging_test.dart \d+:\d+')); Future _throwHttpResponseMinimal(_) => throw HttpResponseException(400, 'minimal');