Skip to content

Commit

Permalink
feat: remove SentryResponse fields: url, body, redirected, status
Browse files Browse the repository at this point in the history
  • Loading branch information
vaind committed Oct 31, 2022
1 parent 3f8512b commit d433b49
Show file tree
Hide file tree
Showing 6 changed files with 31 additions and 131 deletions.
6 changes: 1 addition & 5 deletions dart/lib/src/http_client/failed_request_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -190,12 +190,8 @@ class FailedRequestClient extends BaseClient {
if (response != null) {
event.contexts.response = SentryResponse(
headers: sendDefaultPii ? response.headers : null,
redirected: response.isRedirect,
// Body not captured yet.
// See https://github.com/getsentry/sentry-dart/pull/1093#issuecomment-1293056244
body: null,
bodySize: 0, // TODO
statusCode: response.statusCode,
status: response.reasonPhrase,
);
}

Expand Down
66 changes: 11 additions & 55 deletions dart/lib/src/protocol/sentry_response.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,13 @@ class SentryResponse {
/// The tpye of this class in the [Contexts] field
static const String type = 'response';

/// The URL of the response if available.
/// This might be the redirected URL
final String? url;

/// Indicates whether or not the response is the result of a redirect
/// (that is, its URL list has more than one entry).
final bool? redirected;

/// The body of the response
final Object? body;
/// The size of the response body.
final int? bodySize;

/// The HTTP status code of the response.
/// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Status
final int? statusCode;

/// The status message for the corresponding [statusCode]
final String? status;

/// An immutable dictionary of submitted headers.
/// If a header appears multiple times it,
/// needs to be merged according to the HTTP standard for header merging.
Expand All @@ -35,76 +24,43 @@ class SentryResponse {

final Map<String, String>? _headers;

Map<String, String> get other => Map.unmodifiable(_other ?? const {});

final Map<String, String>? _other;

SentryResponse({
this.url,
this.body,
this.redirected,
this.bodySize,
this.statusCode,
this.status,
Map<String, String>? headers,
Map<String, String>? other,
}) : _headers = headers != null ? Map.from(headers) : null,
_other = other != null ? Map.from(other) : null;
}) : _headers = headers != null ? Map.from(headers) : null;

/// Deserializes a [SentryResponse] from JSON [Map].
factory SentryResponse.fromJson(Map<String, dynamic> json) {
return SentryResponse(
url: json['url'],
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'],
status: json['status'],
redirected: json['redirected'],
);
headers: json.containsKey('headers') ? Map.from(json['headers']) : null,
bodySize: json['body_size'],
statusCode: json['status_code']);
}

/// Produces a [Map] that can be serialized to JSON.
Map<String, dynamic> toJson() {
return <String, dynamic>{
if (url != null) 'url': url,
if (headers.isNotEmpty) 'headers': headers,
if (other.isNotEmpty) 'other': other,
if (redirected != null) 'redirected': redirected,
if (body != null) 'body': body,
if (status != null) 'status': status,
if (bodySize != null) 'body_size': bodySize,
if (statusCode != null) 'status_code': statusCode,
};
}

SentryResponse copyWith({
String? url,
bool? redirected,
int? statusCode,
String? status,
Object? body,
int? bodySize,
Map<String, String>? headers,
Map<String, String>? other,
}) =>
SentryResponse(
url: url ?? this.url,
headers: headers ?? _headers,
redirected: redirected ?? this.redirected,
other: other ?? _other,
body: body ?? this.body,
status: status ?? this.status,
bodySize: bodySize ?? this.bodySize,
statusCode: statusCode ?? this.statusCode,
);

SentryResponse clone() => SentryResponse(
body: body,
bodySize: bodySize,
headers: headers,
other: other,
redirected: redirected,
status: status,
statusCode: statusCode,
url: url,
);
}
23 changes: 9 additions & 14 deletions dart/test/http_client/failed_request_client_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ void main() {

test('no captured events when everything goes well', () async {
final sut = fixture.getSut(
client: fixture.getClient(statusCode: 200, reason: 'OK'),
client: fixture.getClient(statusCode: 200),
);

final response = await sut.get(requestUri);
Expand Down Expand Up @@ -83,7 +83,7 @@ void main() {
test('event reported if bad status code occurs', () async {
final sut = fixture.getSut(
client: fixture.getClient(
statusCode: 404, reason: 'Not Found', headers: {'lorem': 'ipsum'}),
statusCode: 404, body: 'foo', headers: {'lorem': 'ipsum'}),
badStatusCodes: [SentryStatusCode(404)],
);

Expand Down Expand Up @@ -120,20 +120,16 @@ void main() {
expect(request?.other.keys.contains('content_length'), true);

final response = eventCall.contexts.response!;
expect(response.body, isNull);
expect(response.bodySize, 0); // TODO 3
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(
'just one report on status code reporting with failing requests enabled',
() async {
final sut = fixture.getSut(
client: fixture.getClient(statusCode: 404, reason: 'Not Found'),
client: fixture.getClient(statusCode: 404),
badStatusCodes: [SentryStatusCode(404)],
captureFailedRequests: true,
);
Expand Down Expand Up @@ -172,13 +168,12 @@ 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);
expect(event.contexts.response, isNull);
});

test('pii is not send on invalid status code', () async {
final sut = fixture.getSut(
client: fixture.getClient(statusCode: 404, reason: 'Not Found'),
client: fixture.getClient(statusCode: 404),
badStatusCodes: [SentryStatusCode(404)],
captureFailedRequests: false,
sendDefaultPii: false,
Expand All @@ -190,7 +185,7 @@ 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, isNotNull);
expect(event.contexts.response?.headers.isEmpty, true);
});

Expand Down Expand Up @@ -284,11 +279,11 @@ class Fixture {

MockClient getClient(
{int statusCode = 200,
String? reason,
String body = '',
Map<String, String> headers = const {}}) {
return MockClient((request) async {
expect(request.url, requestUri);
return Response('', statusCode, reasonPhrase: reason, headers: headers);
return Response(body, statusCode, headers: headers);
});
}
}
Expand Down
22 changes: 4 additions & 18 deletions dart/test/protocol/sentry_response_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,15 @@ import 'package:test/test.dart';

void main() {
final sentryResponse = SentryResponse(
url: 'url',
body: 'foobar',
redirected: true,
status: 'OK',
bodySize: 42,
statusCode: 200,
headers: {'header_key': 'header_value'},
other: {'other_key': 'other_value'},
);

final sentryResponseJson = <String, dynamic>{
'url': 'url',
'body': 'foobar',
'redirected': true,
'status': 'OK',
'body_size': 42,
'status_code': 200,
'headers': {'header_key': 'header_value'},
'other': {'other_key': 'other_value'},
};

group('json', () {
Expand Down Expand Up @@ -59,20 +51,14 @@ void main() {
final data = sentryResponse;

final copy = data.copyWith(
url: 'url1',
body: 'barfoo',
bodySize: 11,
headers: {'key1': 'value1'},
redirected: false,
statusCode: 301,
status: 'REDIRECT',
);

expect('url1', copy.url);
expect('barfoo', copy.body);
expect(11, copy.bodySize);
expect({'key1': 'value1'}, copy.headers);
expect(false, copy.redirected);
expect(301, copy.statusCode);
expect('REDIRECT', copy.status);
});
});
}
25 changes: 3 additions & 22 deletions dio/lib/src/dio_event_processor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ class DioEventProcessor implements EventProcessor {

final SentryOptions _options;
final MaxRequestBodySize _maxRequestBodySize;
// Will be used again, see https://github.com/getsentry/sentry-dart/issues/624
// ignore: unused_field
final MaxResponseBodySize _maxResponseBodySize;

SentryExceptionFactory get _sentryExceptionFactory =>
Expand Down Expand Up @@ -162,29 +164,8 @@ class DioEventProcessor implements EventProcessor {

return SentryResponse(
headers: _options.sendDefaultPii ? headers : null,
url: response?.realUri.toString(),
redirected: response?.isRedirect,
body: _getResponseData(dioError.response?.data),
bodySize: dioError.response?.data?.length as int?,
statusCode: response?.statusCode,
status: response?.statusMessage,
);
}

/// Returns the request data, if possible according to the users settings.
/// Type checks are based on DIOs [ResponseType].
Object? _getResponseData(dynamic data) {
if (!_options.sendDefaultPii) {
return null;
}
if (data is String) {
if (_maxResponseBodySize.shouldAddBody(data.codeUnits.length)) {
return data;
}
} else if (data is List<int>) {
if (_maxResponseBodySize.shouldAddBody(data.length)) {
return data;
}
}
return null;
}
}
20 changes: 3 additions & 17 deletions dio/test/dio_event_processor_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -115,17 +115,9 @@ void main() {

expect(processedEvent.throwable, event.throwable);
expect(processedEvent.contexts.response, isNotNull);
expect(processedEvent.contexts.response?.body, 'foobar');
expect(processedEvent.contexts.response?.redirected, true);
expect(processedEvent.contexts.response?.status, 'OK');
expect(processedEvent.contexts.response?.bodySize, 6);
expect(processedEvent.contexts.response?.statusCode, 200);
expect(
processedEvent.contexts.response?.url,
'https://example.org/foo/bar?foo=bar',
);
expect(processedEvent.contexts.response?.headers, <String, String>{
'foo': 'bar',
});
expect(processedEvent.contexts.response?.headers, {'foo': 'bar'});
});

test('$DioEventProcessor adds response without PII', () {
Expand Down Expand Up @@ -153,14 +145,8 @@ void main() {

expect(processedEvent.throwable, event.throwable);
expect(processedEvent.contexts.response, isNotNull);
expect(processedEvent.contexts.response?.body, isNull);
expect(processedEvent.contexts.response?.redirected, true);
expect(processedEvent.contexts.response?.status, 'OK');
expect(processedEvent.contexts.response?.bodySize, 6);
expect(processedEvent.contexts.response?.statusCode, 200);
expect(
processedEvent.contexts.response?.url,
'https://example.org/foo/bar?foo=bar',
);
expect(processedEvent.contexts.response?.headers, <String, String>{});
});
});
Expand Down

0 comments on commit d433b49

Please sign in to comment.