feat: update remove HTTP Response body from context

Author: vaind

dart/lib/src/http_client/failed_request_client.dart

@@ -66,7 +66,6 @@ import 'sentry_http_client.dart';
 class FailedRequestClient extends BaseClient {
   FailedRequestClient({
     this.maxRequestBodySize = MaxRequestBodySize.never,
-    this.maxResponseBodySize = MaxResponseBodySize.never,
     this.failedRequestStatusCodes = const [],
     this.captureFailedRequests = true,
     this.sendDefaultPii = false,
@@ -89,10 +88,6 @@ class FailedRequestClient extends BaseClient {
   /// This does not change whether an event is captured.
   final MaxRequestBodySize maxRequestBodySize;
 
-  /// Configures up to which size response bodies should be included in events.
-  /// This does not change whether an event is captured.
-  final MaxResponseBodySize maxResponseBodySize;
-
   /// Describes which HTTP status codes should be considered as a failed
   /// requests.
   ///
@@ -196,7 +191,9 @@ class FailedRequestClient extends BaseClient {
       event.contexts.response = SentryResponse(
         headers: sendDefaultPii ? response.headers : null,
         redirected: response.isRedirect,
-        body: sendDefaultPii ? null : null, // TODO response.stream
+        // Body not captured yet.
+        // See https://github.com/getsentry/sentry-dart/pull/1093#issuecomment-1293056244
+        body: null,
         statusCode: response.statusCode,
         status: response.reasonPhrase,
       );

dart/lib/src/http_client/sentry_http_client.dart

@@ -81,7 +81,6 @@ class SentryHttpClient extends BaseClient {
     Hub? hub,
     bool recordBreadcrumbs = true,
     MaxRequestBodySize maxRequestBodySize = MaxRequestBodySize.never,
-    MaxResponseBodySize maxResponseBodySize = MaxResponseBodySize.never,
     List<SentryStatusCode> failedRequestStatusCodes = const [],
     bool captureFailedRequests = false,
     bool sendDefaultPii = false,
@@ -95,7 +94,6 @@ class SentryHttpClient extends BaseClient {
       failedRequestStatusCodes: failedRequestStatusCodes,
       captureFailedRequests: captureFailedRequests,
       maxRequestBodySize: maxRequestBodySize,
-      maxResponseBodySize: maxResponseBodySize,
       sendDefaultPii: sendDefaultPii,
       hub: _hub,
       client: innerClient,

dart/lib/src/protocol/sentry_response.dart

@@ -54,7 +54,10 @@ class SentryResponse {
   factory SentryResponse.fromJson(Map<String, dynamic> json) {
     return SentryResponse(
       url: json['url'],
-      headers: json['headers'],
+      headers: json.containsKey('headers')
+          ? (json['headers'] as Map<String, dynamic>)
+              .map((key, value) => MapEntry(key, value as String))
+          : null,
       other: json['other'],
       body: json['body'],
       statusCode: json['status_code'],

dart/test/http_client/failed_request_client_test.dart

@@ -30,7 +30,7 @@ void main() {
       expect(fixture.transport.calls, 0);
     });
 
-    test('exception gets reported if client throws', () async {
+    test('event reported if client throws', () async {
       final sut = fixture.getSut(
         client: createThrowingClient(),
         captureFailedRequests: true,
@@ -61,9 +61,12 @@ void main() {
       expect(request?.other.keys.contains('duration'), true);
       // ignore: deprecated_member_use_from_same_package
       expect(request?.other.keys.contains('content_length'), true);
+
+      // Response is not captured in case of exception
+      expect(eventCall.contexts.response, isNull);
     });
 
-    test('exception gets not reported if disabled', () async {
+    test('event not reported if disabled', () async {
       final sut = fixture.getSut(
         client: createThrowingClient(),
         captureFailedRequests: false,
@@ -77,9 +80,10 @@ void main() {
       expect(fixture.transport.calls, 0);
     });
 
-    test('exception gets reported if bad status code occurs', () async {
+    test('event reported if bad status code occurs', () async {
       final sut = fixture.getSut(
-        client: fixture.getClient(statusCode: 404, reason: 'Not Found'),
+        client: fixture.getClient(
+            statusCode: 404, reason: 'Not Found', headers: {'lorem': 'ipsum'}),
         badStatusCodes: [SentryStatusCode(404)],
       );
 
@@ -114,6 +118,15 @@ void main() {
       expect(request?.other.keys.contains('duration'), true);
       // ignore: deprecated_member_use_from_same_package
       expect(request?.other.keys.contains('content_length'), true);
+
+      final response = eventCall.contexts.response!;
+      expect(response.body, isNull);
+      expect(response.statusCode, 404);
+      expect(response.status, 'Not Found');
+      expect(response.headers, equals({'lorem': 'ipsum'}));
+      expect(response.other, isEmpty);
+      expect(response.redirected, false);
+      expect(response.url, isNull);
     });
 
     test(
@@ -160,6 +173,7 @@ void main() {
       expect(event.request?.headers.isEmpty, true);
       expect(event.request?.cookies, isNull);
       expect(event.request?.data, isNull);
+      expect(event.contexts.response?.headers.isEmpty, true);
     });
 
     test('pii is not send on invalid status code', () async {
@@ -176,6 +190,8 @@ void main() {
       expect(fixture.transport.calls, 1);
       expect(event.request?.headers.isEmpty, true);
       expect(event.request?.cookies, isNull);
+      expect(event.request?.data, isNull);
+      expect(event.contexts.response?.headers.isEmpty, true);
     });
 
     test('request body is included according to $MaxRequestBodySize', () async {
@@ -225,56 +241,6 @@ void main() {
         expect(capturedRequest?.data, scenario.matcher);
       }
     });
-
-    test('response body is included according to $MaxResponseBodySize',
-        () async {
-      final scenarios = [
-        // never
-        MaxBodySizeTestConfig(MaxResponseBodySize.never, 0, false),
-        MaxBodySizeTestConfig(MaxResponseBodySize.never, 4001, false),
-        MaxBodySizeTestConfig(MaxResponseBodySize.never, 10001, false),
-        // always
-        MaxBodySizeTestConfig(MaxResponseBodySize.always, 0, true),
-        MaxBodySizeTestConfig(MaxResponseBodySize.always, 4001, true),
-        MaxBodySizeTestConfig(MaxResponseBodySize.always, 10001, true),
-        // small
-        MaxBodySizeTestConfig(MaxResponseBodySize.small, 0, true),
-        MaxBodySizeTestConfig(MaxResponseBodySize.small, 4000, true),
-        MaxBodySizeTestConfig(MaxResponseBodySize.small, 4001, false),
-        // medium
-        MaxBodySizeTestConfig(MaxResponseBodySize.medium, 0, true),
-        MaxBodySizeTestConfig(MaxResponseBodySize.medium, 4001, true),
-        MaxBodySizeTestConfig(MaxResponseBodySize.medium, 10000, true),
-        MaxBodySizeTestConfig(MaxResponseBodySize.medium, 10001, false),
-      ];
-
-      const errCode = 500;
-      const errReason = 'Internal Server Error';
-
-      for (final scenario in scenarios) {
-        fixture.transport.reset();
-
-        final sut = fixture.getSut(
-          client: MockClient((request) => Future.value(Response(
-              ' ' * scenario.contentLength, errCode,
-              reasonPhrase: errReason))),
-          badStatusCodes: [SentryStatusCode(errCode)],
-          captureFailedRequests: true,
-          maxResponseBodySize: scenario.maxBodySize,
-        );
-
-        final response = await sut.send(Request('GET', requestUri));
-        expect(response.statusCode, errCode);
-        expect(response.reasonPhrase, errReason);
-
-        expect(fixture.transport.calls, 1);
-
-        final eventCall = fixture.transport.events.first;
-        final capturedResponse = eventCall.contexts.response;
-        expect(capturedResponse, isNotNull);
-        expect(capturedResponse?.body, scenario.matcher);
-      }
-    });
   });
 }
 
@@ -302,7 +268,6 @@ class Fixture {
     MockClient? client,
     bool captureFailedRequests = false,
     MaxRequestBodySize maxRequestBodySize = MaxRequestBodySize.small,
-    MaxResponseBodySize maxResponseBodySize = MaxResponseBodySize.small,
     List<SentryStatusCode> badStatusCodes = const [],
     bool sendDefaultPii = true,
   }) {
@@ -313,15 +278,17 @@ class Fixture {
       captureFailedRequests: captureFailedRequests,
       failedRequestStatusCodes: badStatusCodes,
       maxRequestBodySize: maxRequestBodySize,
-      maxResponseBodySize: maxResponseBodySize,
       sendDefaultPii: sendDefaultPii,
     );
   }
 
-  MockClient getClient({int statusCode = 200, String? reason}) {
+  MockClient getClient(
+      {int statusCode = 200,
+      String? reason,
+      Map<String, String> headers = const {}}) {
     return MockClient((request) async {
       expect(request.url, requestUri);
-      return Response('', statusCode, reasonPhrase: reason);
+      return Response('', statusCode, reasonPhrase: reason, headers: headers);
     });
   }
 }

dart/test/mocks/mock_transport.dart

@@ -1,23 +1,38 @@
 import 'dart:convert';
 
 import 'package:sentry/sentry.dart';
+import 'package:test/expect.dart';
 
 class MockTransport implements Transport {
   List<SentryEnvelope> envelopes = [];
   List<SentryEvent> events = [];
-  int calls = 0;
+
+  int _calls = 0;
+  String _exceptions = '';
+
+  int get calls {
+    expect(_exceptions, isEmpty);
+    return _calls;
+  }
 
   bool called(int calls) {
     return calls == calls;
   }
 
   @override
   Future<SentryId> send(SentryEnvelope envelope) async {
-    calls++;
+    _calls++;
 
-    envelopes.add(envelope);
-    final event = await _eventFromEnvelope(envelope);
-    events.add(event);
+    // Exception here would be swallowed by Sentry, making it hard to find test
+    // failure causes. Instead, we log them and check on access to [calls].
+    try {
+      envelopes.add(envelope);
+      final event = await _eventFromEnvelope(envelope);
+      events.add(event);
+    } catch (e, stack) {
+      _exceptions += "$e\n$stack\n\n";
+      rethrow;
+    }
 
     return envelope.header.eventId ?? SentryId.empty();
   }
@@ -31,6 +46,10 @@ class MockTransport implements Transport {
     final envelopeMap = envelopeItemJson as Map<String, dynamic>;
     final requestJson = envelopeMap['request'] as Map<String, dynamic>?;
 
+    // TODO the following code should really be part of fromJson() that handle those keys.
+    // JSON being Map<String, dynamic> is nothing out of ordinary.
+    // See [SentryResponse.fromJson()] as an example.
+
     // '_InternalLinkedHashMap<dynamic, dynamic>' is not a subtype of type 'Map<String, String>'
     final headersMap = requestJson?['headers'] as Map<String, dynamic>?;
     final newHeadersMap = <String, String>{};
@@ -56,7 +75,7 @@ class MockTransport implements Transport {
   void reset() {
     envelopes.clear();
     events.clear();
-    calls = 0;
+    _calls = 0;
   }
 }