diff --git a/CHANGELOG.md b/CHANGELOG.md index a67457b5e3..b4e8ef92fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased +* [Dio] Ref: Replace FailedRequestAdapter with FailedRequestInterceptor (#728) * Fix: Add missing return values - dart analyzer (#742) # 6.3.0 diff --git a/dio/README.md b/dio/README.md index 0624e0009c..a01ab085bb 100644 --- a/dio/README.md +++ b/dio/README.md @@ -22,7 +22,9 @@ Integration for the [`dio`](https://pub.dev/packages/dio) package. - Follow the installing instructions on [pub.dev](https://pub.dev/packages/sentry/install). -- Initialize the Sentry SDK using the DSN issued by Sentry.io: +- Initialize the Sentry SDK using the DSN issued by Sentry.io. + +- Call `dio.addSentry()`. This *must* be the last initialization step of the Dio setup, otherwise your configuration of Dio might overwrite the Sentry configuration. ```dart import 'package:sentry/sentry.dart'; @@ -39,14 +41,13 @@ Future main() async { void initDio() { final dio = Dio(); - // Make sure this is the last initialization method, - // otherwise you might override Sentrys configuration. + /// This *must* be the last initialization step of the Dio setup, otherwise + /// your configuration of Dio might overwrite the Sentry configuration. dio.addSentry(...); } ``` -Dependending on you configuration, this can add performance tracing, -http breadcrumbs and automatic recording of bad http requests. +Depending on your configuration, this adds performance tracing and http breadcrumbs. Also, exceptions from invalid http status codes or parsing exceptions are automatically captured. #### Resources diff --git a/dio/lib/src/failed_request_client_adapter.dart b/dio/lib/src/failed_request_client_adapter.dart deleted file mode 100644 index 706f7ee711..0000000000 --- a/dio/lib/src/failed_request_client_adapter.dart +++ /dev/null @@ -1,165 +0,0 @@ -// ignore_for_file: strict_raw_type - -import 'dart:typed_data'; - -import 'package:dio/dio.dart'; -import 'package:meta/meta.dart'; -import 'package:sentry/sentry.dart'; - -/// A [Dio](https://pub.dev/packages/dio)-package compatible HTTP client adapter -/// which records events for failed requests. -/// -/// Configured with default values, this captures requests which throw an -/// exception. -/// This can be for example for the following reasons: -/// - In an browser environment this can be requests which fail because of CORS. -/// - In an mobile or desktop application this can be requests which failed -/// because the connection was interrupted. -/// -/// Additionally you can configure specific HTTP response codes to be considered -/// as a failed request. In the following example, the status codes 404 and 500 -/// are considered a failed request. -/// -/// Remarks: -/// If this client is used as a wrapper, a call to close also closes the -/// given client. -@experimental -class FailedRequestClientAdapter extends HttpClientAdapter { - // ignore: public_member_api_docs - FailedRequestClientAdapter({ - required HttpClientAdapter client, - this.maxRequestBodySize = MaxRequestBodySize.never, - this.failedRequestStatusCodes = const [], - this.captureFailedRequests = true, - this.sendDefaultPii = false, - Hub? hub, - }) : _hub = hub ?? HubAdapter(), - _client = client; - - final HttpClientAdapter _client; - final Hub _hub; - - /// Configures wether to record exceptions for failed requests. - /// Examples for captures exceptions are: - /// - In an browser environment this can be requests which fail because of CORS. - /// - In an mobile or desktop application this can be requests which failed - /// because the connection was interrupted. - final bool captureFailedRequests; - - /// Configures up to which size request bodies should be included in events. - /// This does not change wether an event is captured. - final MaxRequestBodySize maxRequestBodySize; - - /// Describes which HTTP status codes should be considered as a failed - /// requests. - /// - /// Per default no status code is considered a failed request. - final List failedRequestStatusCodes; - - /// Configures wether default PII is enabled for this client adapter. - final bool sendDefaultPii; - - @override - Future fetch( - RequestOptions options, - Stream? requestStream, - Future? cancelFuture, - ) async { - int? statusCode; - Object? exception; - StackTrace? stackTrace; - - final stopwatch = Stopwatch(); - stopwatch.start(); - - try { - final response = - await _client.fetch(options, requestStream, cancelFuture); - statusCode = response.statusCode; - return response; - } catch (e, st) { - exception = e; - stackTrace = st; - rethrow; - } finally { - stopwatch.stop(); - - // If captureFailedRequests is true, there statusCode is null. - // So just one of these blocks can be called. - - if (captureFailedRequests && exception != null) { - await _captureEvent( - exception: exception, - stackTrace: stackTrace, - options: options, - requestDuration: stopwatch.elapsed, - ); - } else if (failedRequestStatusCodes.containsStatusCode(statusCode)) { - final message = - 'Event was captured because the request status code was $statusCode'; - final httpException = SentryHttpClientError(message); - - // Capture an exception if the status code is considered bad - await _captureEvent( - exception: exception ?? httpException, - options: options, - reason: message, - requestDuration: stopwatch.elapsed, - ); - } - } - } - - @override - void close({bool force = false}) => _client.close(force: force); - - // See https://develop.sentry.dev/sdk/event-payloads/request/ - Future _captureEvent({ - required Object? exception, - StackTrace? stackTrace, - String? reason, - required Duration requestDuration, - required RequestOptions options, - }) async { - // As far as I can tell there's no way to get the uri without the query part - // so we replace it with an empty string. - final urlWithoutQuery = options.uri.replace(query: '').toString(); - - final query = options.uri.query.isEmpty ? null : options.uri.query; - - final headers = options.headers - .map((key, dynamic value) => MapEntry(key, value?.toString() ?? '')); - - final sentryRequest = SentryRequest( - method: options.method, - headers: sendDefaultPii ? headers : null, - url: urlWithoutQuery, - queryString: query, - cookies: sendDefaultPii ? options.headers['Cookie']?.toString() : null, - other: { - 'duration': requestDuration.toString(), - }, - ); - - final mechanism = Mechanism( - type: 'SentryHttpClient', - description: reason, - ); - final throwableMechanism = ThrowableMechanism(mechanism, exception); - - final event = SentryEvent( - throwable: throwableMechanism, - request: sentryRequest, - ); - await _hub.captureEvent(event, stackTrace: stackTrace); - } -} - -extension _ListX on List { - bool containsStatusCode(int? statusCode) { - if (statusCode == null) { - return false; - } - return any((element) => element.isInRange(statusCode)); - } -} diff --git a/dio/lib/src/failed_request_interceptor.dart b/dio/lib/src/failed_request_interceptor.dart new file mode 100644 index 0000000000..41b0d64a90 --- /dev/null +++ b/dio/lib/src/failed_request_interceptor.dart @@ -0,0 +1,21 @@ +// ignore_for_file: public_member_api_docs + +import 'package:dio/dio.dart'; +import 'package:sentry/sentry.dart'; + +class FailedRequestInterceptor extends Interceptor { + FailedRequestInterceptor({Hub? hub}) : _hub = hub ?? HubAdapter(); + + final Hub _hub; + + @override + void onError( + DioError err, + ErrorInterceptorHandler handler, + ) { + _hub.getSpan()?.throwable = err; + _hub.captureException(err); + + handler.next(err); + } +} diff --git a/dio/lib/src/sentry_dio_client_adapter.dart b/dio/lib/src/sentry_dio_client_adapter.dart index 35e9a4ba3a..420ace5cac 100644 --- a/dio/lib/src/sentry_dio_client_adapter.dart +++ b/dio/lib/src/sentry_dio_client_adapter.dart @@ -5,7 +5,6 @@ import 'dart:typed_data'; import 'package:dio/dio.dart'; import 'package:meta/meta.dart'; import 'package:sentry/sentry.dart'; -import 'failed_request_client_adapter.dart'; import 'tracing_client_adapter.dart'; import 'breadcrumb_client_adapter.dart'; @@ -54,24 +53,11 @@ class SentryDioClientAdapter extends HttpClientAdapter { Hub? hub, bool recordBreadcrumbs = true, bool networkTracing = true, - MaxRequestBodySize maxRequestBodySize = MaxRequestBodySize.never, - List failedRequestStatusCodes = const [], - bool captureFailedRequests = false, - bool sendDefaultPii = false, }) { _hub = hub ?? HubAdapter(); var innerClient = client; - innerClient = FailedRequestClientAdapter( - failedRequestStatusCodes: failedRequestStatusCodes, - captureFailedRequests: captureFailedRequests, - maxRequestBodySize: maxRequestBodySize, - sendDefaultPii: sendDefaultPii, - hub: _hub, - client: innerClient, - ); - if (networkTracing) { innerClient = TracingClientAdapter(client: innerClient, hub: _hub); } diff --git a/dio/lib/src/sentry_dio_extension.dart b/dio/lib/src/sentry_dio_extension.dart index 1633b6c955..41406fa7e8 100644 --- a/dio/lib/src/sentry_dio_extension.dart +++ b/dio/lib/src/sentry_dio_extension.dart @@ -1,6 +1,7 @@ import 'package:dio/dio.dart'; import 'package:meta/meta.dart'; import 'package:sentry/sentry.dart'; +import 'failed_request_interceptor.dart'; import 'sentry_transformer.dart'; import 'sentry_dio_client_adapter.dart'; @@ -19,15 +20,17 @@ extension SentryDioExtension on Dio { bool captureFailedRequests = false, bool sendDefaultPii = false, }) { + if (captureFailedRequests) { + // Add FailedRequestInterceptor at index 0, so it's the first interceptor. + // This ensures that it is called and not skipped by any previous interceptor. + interceptors.insert(0, FailedRequestInterceptor()); + } + // intercept http requests httpClientAdapter = SentryDioClientAdapter( client: httpClientAdapter, recordBreadcrumbs: recordBreadcrumbs, networkTracing: networkTracing, - maxRequestBodySize: maxRequestBodySize, - failedRequestStatusCodes: failedRequestStatusCodes, - captureFailedRequests: captureFailedRequests, - sendDefaultPii: sendDefaultPii, ); // intercept transformations diff --git a/dio/pubspec.yaml b/dio/pubspec.yaml index 2eae725dde..0d58cbd029 100644 --- a/dio/pubspec.yaml +++ b/dio/pubspec.yaml @@ -19,8 +19,7 @@ dev_dependencies: yaml: ^3.1.0 # needed for version match (code and pubspec) coverage: ^1.0.3 mockito: ^5.0.16 - http: ^0.13.0 - + dependency_overrides: sentry: path: ../dart diff --git a/dio/test/failed_request_client_adapter_test.dart b/dio/test/failed_request_client_adapter_test.dart deleted file mode 100644 index f9a1742882..0000000000 --- a/dio/test/failed_request_client_adapter_test.dart +++ /dev/null @@ -1,271 +0,0 @@ -import 'package:dio/dio.dart'; -import 'package:http/http.dart'; -import 'package:mockito/mockito.dart'; -import 'package:sentry/sentry.dart'; -import 'package:sentry/src/http_client/failed_request_client.dart'; -import 'package:sentry_dio/src/failed_request_client_adapter.dart'; -import 'package:test/test.dart'; - -import 'mocks.dart'; -import 'mocks/mock_http_client_adapter.dart'; -import 'mocks/mock_hub.dart'; -import 'mocks/mock_transport.dart'; - -final requestUri = Uri.parse('https://example.com?foo=bar'); -final requestOptions = '?foo=bar'; - -void main() { - group(FailedRequestClientAdapter, () { - late Fixture fixture; - - setUp(() { - fixture = Fixture(); - }); - - test('no captured events when everything goes well', () async { - final sut = fixture.getSut( - client: fixture.getClient(statusCode: 200, reason: 'OK'), - ); - - final response = await sut.get(requestOptions); - expect(response.statusCode, 200); - - expect(fixture.transport.calls, 0); - }); - - test('exception gets reported if client throws', () async { - final sut = fixture.getSut( - client: createThrowingClient(), - captureFailedRequests: true, - ); - - await expectLater( - () async => await sut.get( - requestOptions, - options: Options(headers: {'Cookie': 'foo=bar'}), - ), - throwsException, - ); - - expect(fixture.transport.calls, 1); - - final eventCall = fixture.transport.events.first; - final exception = eventCall.exceptions?.first; - final mechanism = exception?.mechanism; - - expect(exception?.stackTrace, isNotNull); - expect(mechanism?.type, 'SentryHttpClient'); - - final request = eventCall.request; - expect(request, isNotNull); - expect(request?.method, 'GET'); - expect(request?.url, 'https://example.com?'); - expect(request?.queryString, 'foo=bar'); - expect(request?.cookies, 'foo=bar'); - expect(request?.headers, {'Cookie': 'foo=bar'}); - expect(request?.other.keys.contains('duration'), true); - expect(request?.other.keys.contains('content_length'), false); - }); - - test('exception gets not reported if disabled', () async { - final sut = fixture.getSut( - client: createThrowingClient(), - captureFailedRequests: false, - ); - - await expectLater( - () async => await sut.get( - requestOptions, - options: Options(headers: {'Cookie': 'foo=bar'}), - ), - throwsException, - ); - - expect(fixture.transport.calls, 0); - }); - - test('exception gets reported if bad status code occurs', () async { - final sut = fixture.getSut( - client: fixture.getClient(statusCode: 404, reason: 'Not Found'), - badStatusCodes: [SentryStatusCode(404)], - ); - - try { - await sut.get( - requestOptions, - options: Options(headers: {'Cookie': 'foo=bar'}), - ); - } on DioError catch (_) { - // a 404 throws an exception with dio - } - - expect(fixture.transport.calls, 1); - - final eventCall = fixture.transport.events.first; - final exception = eventCall.exceptions?.first; - final mechanism = exception?.mechanism; - - expect(mechanism?.type, 'SentryHttpClient'); - expect( - mechanism?.description, - 'Event was captured because the request status code was 404', - ); - - expect(exception?.type, 'SentryHttpClientError'); - expect( - exception?.value, - 'Exception: Event was captured because the request status code was 404', - ); - - final request = eventCall.request; - expect(request, isNotNull); - expect(request?.method, 'GET'); - expect(request?.url, 'https://example.com?'); - expect(request?.queryString, 'foo=bar'); - expect(request?.cookies, 'foo=bar'); - expect(request?.headers, {'Cookie': 'foo=bar'}); - expect(request?.other.keys.contains('duration'), true); - expect(request?.other.keys.contains('content_length'), false); - }); - - test( - 'just one report on status code reporting with failing requests enabled', - () async { - final sut = fixture.getSut( - client: fixture.getClient(statusCode: 404, reason: 'Not Found'), - badStatusCodes: [SentryStatusCode(404)], - captureFailedRequests: true, - ); - - try { - await sut.get( - requestOptions, - options: Options(headers: {'Cookie': 'foo=bar'}), - ); - } on DioError catch (_) { - // dio throws on 404 - } - - expect(fixture.transport.calls, 1); - }); - - test('close does get called for user defined client', () async { - final mockHub = MockHub(); - - final mockClient = CloseableMockClient(); - - final client = FailedRequestClient(client: mockClient, hub: mockHub); - client.close(); - - expect(mockHub.addBreadcrumbCalls.length, 0); - expect(mockHub.captureExceptionCalls.length, 0); - verify(mockClient.close()); - }); - - test('pii is not send on exception', () async { - final sut = fixture.getSut( - client: createThrowingClient(), - captureFailedRequests: true, - sendDefaultPii: false, - ); - - await expectLater( - () async => await sut.get( - requestOptions, - options: Options(headers: {'Cookie': 'foo=bar'}), - ), - throwsException, - ); - - final event = fixture.transport.events.first; - expect(fixture.transport.calls, 1); - expect(event.request?.headers.isEmpty, true); - expect(event.request?.cookies, isNull); - }); - - test('pii is not send on invalid status code', () async { - final sut = fixture.getSut( - client: fixture.getClient(statusCode: 404, reason: 'Not Found'), - badStatusCodes: [SentryStatusCode(404)], - captureFailedRequests: false, - sendDefaultPii: false, - ); - - try { - await sut.get( - requestOptions, - options: Options(headers: {'Cookie': 'foo=bar'}), - ); - } on DioError catch (_) { - // dio throws on 404 - } - - final event = fixture.transport.events.first; - expect(fixture.transport.calls, 1); - expect(event.request?.headers.isEmpty, true); - expect(event.request?.cookies, isNull); - }); - }); -} - -MockHttpClientAdapter createThrowingClient() { - return MockHttpClientAdapter( - (options, _, __) async { - expect(options.uri, requestUri); - throw TestException(); - }, - ); -} - -class CloseableMockClient extends Mock implements BaseClient {} - -class Fixture { - final _options = SentryOptions(dsn: fakeDsn); - late Hub _hub; - final transport = MockTransport(); - Fixture() { - _options.transport = transport; - _hub = Hub(_options); - } - - Dio getSut({ - MockHttpClientAdapter? client, - bool captureFailedRequests = false, - MaxRequestBodySize maxRequestBodySize = MaxRequestBodySize.small, - List badStatusCodes = const [], - bool sendDefaultPii = true, - }) { - final mc = client ?? getClient(); - final dio = Dio(BaseOptions(baseUrl: 'https://example.com')); - dio.httpClientAdapter = FailedRequestClientAdapter( - client: mc, - hub: _hub, - captureFailedRequests: captureFailedRequests, - failedRequestStatusCodes: badStatusCodes, - maxRequestBodySize: maxRequestBodySize, - sendDefaultPii: sendDefaultPii, - ); - return dio; - } - - MockHttpClientAdapter getClient({int statusCode = 200, String? reason}) { - return MockHttpClientAdapter((options, requestStream, cancelFuture) async { - expect(options.uri, requestUri); - return ResponseBody.fromString('', statusCode); - }); - } -} - -class TestException implements Exception {} - -class MaxRequestBodySizeTestConfig { - MaxRequestBodySizeTestConfig( - this.maxRequestBodySize, - this.contentLength, - this.shouldBeIncluded, - ); - - final MaxRequestBodySize maxRequestBodySize; - final int contentLength; - final bool shouldBeIncluded; -} diff --git a/dio/test/failed_request_interceptor_test.dart b/dio/test/failed_request_interceptor_test.dart new file mode 100644 index 0000000000..96049c9939 --- /dev/null +++ b/dio/test/failed_request_interceptor_test.dart @@ -0,0 +1,46 @@ +import 'package:dio/dio.dart'; +import 'package:sentry_dio/src/failed_request_interceptor.dart'; +import 'package:test/test.dart'; + +import 'mocks/mock_hub.dart'; + +void main() { + late Fixture fixture; + + setUp(() { + fixture = Fixture(); + }); + + test('interceptor send error', () { + final interceptor = fixture.getSut(); + interceptor.onError( + DioError(requestOptions: RequestOptions(path: '')), + fixture.errorInterceptorHandler, + ); + + expect(fixture.errorInterceptorHandler.nextWasCalled, true); + expect(fixture.hub.captureExceptionCalls.length, 1); + }); +} + +class Fixture { + MockHub hub = MockHub(); + MockedErrorInterceptorHandler errorInterceptorHandler = + MockedErrorInterceptorHandler(); + + FailedRequestInterceptor getSut() { + return FailedRequestInterceptor(hub: hub); + } +} + +class MockedErrorInterceptorHandler implements ErrorInterceptorHandler { + bool nextWasCalled = false; + + @override + void next(DioError err) { + nextWasCalled = true; + } + + @override + void noSuchMethod(Invocation invocation) {} +} diff --git a/dio/test/sentry_dio_client_adapter_test.dart b/dio/test/sentry_dio_client_adapter_test.dart index 864f091cf4..d81620ee3b 100644 --- a/dio/test/sentry_dio_client_adapter_test.dart +++ b/dio/test/sentry_dio_client_adapter_test.dart @@ -2,7 +2,6 @@ import 'package:dio/dio.dart'; import 'package:http/http.dart'; import 'package:mockito/mockito.dart'; import 'package:sentry/sentry.dart'; -import 'package:sentry/src/http_client/failed_request_client.dart'; import 'package:sentry_dio/src/sentry_dio_client_adapter.dart'; import 'package:test/test.dart'; @@ -47,27 +46,6 @@ void main() { expect(fixture.hub.addBreadcrumbCalls.length, 1); }); - test('one captured event with when enabling $FailedRequestClient', - () async { - final sut = fixture.getSut( - client: createThrowingClient(), - captureFailedRequests: true, - recordBreadcrumbs: true, - ); - - await expectLater( - () async => await sut.get('/'), - throwsException, - ); - - expect(fixture.hub.captureEventCalls.length, 1); - // The event should not have breadcrumbs from the BreadcrumbClient - expect(fixture.hub.captureEventCalls.first.event.breadcrumbs, null); - // The breadcrumb for the request should still be added for every - // following event. - expect(fixture.hub.addBreadcrumbCalls.length, 1); - }); - test('close does get called for user defined client', () async { final mockHub = MockHub(); @@ -134,9 +112,6 @@ class Fixture { dio.httpClientAdapter = SentryDioClientAdapter( client: mc, hub: hub, - captureFailedRequests: captureFailedRequests, - failedRequestStatusCodes: badStatusCodes, - maxRequestBodySize: maxRequestBodySize, recordBreadcrumbs: recordBreadcrumbs, networkTracing: networkTracing, );