From d7f3d95de1a03c24bd40cb251ebb7d68adbcb53f Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Thu, 14 Dec 2023 08:10:50 -0800 Subject: [PATCH] Integrate cupertino_http with http_profile --- pkgs/cupertino_http/example/pubspec.yaml | 2 +- .../lib/src/cupertino_client.dart | 62 +++++++++++++++++-- pkgs/cupertino_http/pubspec.yaml | 2 + pkgs/http_profile/lib/http_profile.dart | 60 ++++++++++++++++-- pkgs/http_profile/pubspec.yaml | 3 - 5 files changed, 114 insertions(+), 15 deletions(-) diff --git a/pkgs/cupertino_http/example/pubspec.yaml b/pkgs/cupertino_http/example/pubspec.yaml index be727312a2..eeab4843f6 100644 --- a/pkgs/cupertino_http/example/pubspec.yaml +++ b/pkgs/cupertino_http/example/pubspec.yaml @@ -10,7 +10,7 @@ environment: flutter: '>=3.10.0' dependencies: - cached_network_image: ^3.2.3 + cached_network_image: ^3.3.0 cupertino_http: path: ../ cupertino_icons: ^1.0.2 diff --git a/pkgs/cupertino_http/lib/src/cupertino_client.dart b/pkgs/cupertino_http/lib/src/cupertino_client.dart index a4c7715de5..71176a306f 100644 --- a/pkgs/cupertino_http/lib/src/cupertino_client.dart +++ b/pkgs/cupertino_http/lib/src/cupertino_client.dart @@ -12,6 +12,7 @@ import 'dart:typed_data'; import 'package:async/async.dart'; import 'package:http/http.dart'; +import 'package:http_profile/http_profile.dart'; import 'cupertino_api.dart'; @@ -21,9 +22,10 @@ class _TaskTracker { final responseCompleter = Completer(); final BaseRequest request; final responseController = StreamController(); + final HttpClientRequestProfile? profile; int numRedirects = 0; - _TaskTracker(this.request); + _TaskTracker(this.request, this.profile); void close() { responseController.close(); @@ -152,12 +154,13 @@ class CupertinoClient extends BaseClient { static void _onComplete( URLSession session, URLSessionTask task, Error? error) { final taskTracker = _tracker(task); - + taskTracker.profile?.responseData.endTime = DateTime.now(); if (error != null) { final exception = ClientException( error.localizedDescription ?? 'Unknown', taskTracker.request.url); if (taskTracker.responseCompleter.isCompleted) { taskTracker.responseController.addError(exception); + taskTracker.profile?.responseData.error = exception.toString(); } else { taskTracker.responseCompleter.completeError(exception); } @@ -172,6 +175,7 @@ class CupertinoClient extends BaseClient { static void _onData(URLSession session, URLSessionTask task, Data data) { final taskTracker = _tracker(task); taskTracker.responseController.add(data.bytes); + taskTracker.profile?.responseBodySink.add(data.bytes); } static URLRequest? _onRedirect(URLSession session, URLSessionTask task, @@ -180,6 +184,10 @@ class CupertinoClient extends BaseClient { ++taskTracker.numRedirects; if (taskTracker.request.followRedirects && taskTracker.numRedirects <= taskTracker.request.maxRedirects) { + taskTracker.profile?.responseData.addRedirect(HttpProfileRedirectData( + statusCode: response.statusCode, + method: request.httpMethod, + location: request.url!.toString())); return request; } return null; @@ -250,6 +258,24 @@ class CupertinoClient extends BaseClient { final stream = request.finalize(); + final profile = HttpClientRequestProfile.profile( + requestStartTimestamp: DateTime.now(), + requestMethod: request.method, + requestUri: request.url.toString()); + profile?.requestData + ?..connectionInfo = { + 'package': 'package:cupertino_http', // XXX Set for http_impl.dart + 'client': 'CupertinoClient', + 'configuration': _urlSession!.configuration.toString(), + } + ..contentLength = request.contentLength + ..cookies = request.headers['cookie'] + ..followRedirects = request.followRedirects + ..headers = request.headers + ..maxRedirects = request.maxRedirects; + + // XXX It would be cool to have a "other stuff field" to stick in task + // data. final urlRequest = MutableURLRequest.fromUrl(request.url) ..httpMethod = request.method; @@ -257,24 +283,39 @@ class CupertinoClient extends BaseClient { // Optimize the (typical) `Request` case since assigning to // `httpBodyStream` requires a lot of expensive setup and data passing. urlRequest.httpBody = Data.fromList(request.bodyBytes); + profile?.requestBodySink.add(request.bodyBytes); } else if (await _hasData(stream) case (true, final s)) { // If the request is supposed to be bodyless (e.g. GET requests) // then setting `httpBodyStream` will cause the request to fail - // even if the stream is empty. - urlRequest.httpBodyStream = s; + if (profile == null) { + urlRequest.httpBodyStream = s; + } else { + final splitter = StreamSplitter(s); + urlRequest.httpBodyStream = splitter.split(); + unawaited(profile.requestBodySink.addStream(splitter.split())); + } } // This will preserve Apple default headers - is that what we want? request.headers.forEach(urlRequest.setValueForHttpHeaderField); final task = urlSession.dataTaskWithRequest(urlRequest); - final taskTracker = _TaskTracker(request); + final taskTracker = _TaskTracker(request, profile); _tasks[task] = taskTracker; task.resume(); final maxRedirects = request.followRedirects ? request.maxRedirects : 0; - final result = await taskTracker.responseCompleter.future; + late URLResponse result; + try { + result = await taskTracker.responseCompleter.future; + } catch (e) { + profile?.requestData.error = e.toString(); + rethrow; + } + + profile?.requestEndTimestamp = DateTime.now(); // Not a timestamp! final response = result as HTTPURLResponse; if (request.followRedirects && taskTracker.numRedirects > maxRedirects) { @@ -292,6 +333,15 @@ class CupertinoClient extends BaseClient { ); } + final isRedirect = !request.followRedirects && taskTracker.numRedirects > 0; + profile?.responseData // Good name? + ?..cookies = responseHeaders['cookies'] + ..headers = responseHeaders + ..isRedirect = isRedirect + ..reasonPhrase = _findReasonPhrase(response.statusCode) + ..startTime = DateTime.now() + ..statusCode = response.statusCode; + return StreamedResponse( taskTracker.responseController.stream, response.statusCode, @@ -300,7 +350,7 @@ class CupertinoClient extends BaseClient { : response.expectedContentLength, reasonPhrase: _findReasonPhrase(response.statusCode), request: request, - isRedirect: !request.followRedirects && taskTracker.numRedirects > 0, + isRedirect: isRedirect, headers: responseHeaders, ); } diff --git a/pkgs/cupertino_http/pubspec.yaml b/pkgs/cupertino_http/pubspec.yaml index 9d639a8450..97655e66f7 100644 --- a/pkgs/cupertino_http/pubspec.yaml +++ b/pkgs/cupertino_http/pubspec.yaml @@ -15,6 +15,8 @@ dependencies: flutter: sdk: flutter http: '>=0.13.4 <2.0.0' + http_profile: + path: ../http_profile dev_dependencies: dart_flutter_team_lints: ^1.0.0 diff --git a/pkgs/http_profile/lib/http_profile.dart b/pkgs/http_profile/lib/http_profile.dart index a980d02e7f..d5770b64d5 100644 --- a/pkgs/http_profile/lib/http_profile.dart +++ b/pkgs/http_profile/lib/http_profile.dart @@ -93,13 +93,18 @@ final class HttpProfileRequestData { } /// The content length of the request, in bytes. - set contentLength(int value) { - _data['contentLength'] = value; + set contentLength(int? value) { + if (value == null) { + _data.remove('contentLength'); + } else { + _data['contentLength'] = value; + } _updated(); } /// The cookies presented to the server (in the 'cookie' header). /// + /// XXX Can have multiple values in the same item. /// Usage example: /// /// ```dart @@ -107,13 +112,30 @@ final class HttpProfileRequestData { /// 'sessionId=abc123', /// 'csrftoken=def456', /// ]; + /// + /// or + /// + /// profile.requestData.cookies = ['sessionId=abc123,csrftoken=def456'] /// ``` - set cookies(List value) { + set cookiesList(List? value) { + if (value == null) { + _data.remove('cookies'); + } _data['cookies'] = [...value]; _updated(); } + set cookies(String value) { + if (value == null) { + _data.remove('cookies'); + } + _data['cookies'] = ",".split(RegExp(r'\s*,\s*')) + _updated(); + } + /// The error associated with a failed request. + /// + /// Should this be here? It doesn't just seem asssociated with the request. set error(String value) { _data['error'] = value; _updated(); @@ -125,11 +147,24 @@ final class HttpProfileRequestData { _updated(); } - set headers(Map> value) { + set headersValueList(Map>? value) { + if (value == null) { + _data.remove('headers'); + } _data['headers'] = {...value}; _updated(); } + /// XXX should this include cookies or not? It's not obvious why we seperate + /// cookies from other headers. + set headers(Map? value) { + if (value == null) { + _data.remove('headers'); + } + _data['headers'] = {for (var entry in value.entries) entry.key : entry.value.split(RegExp(r'\s*,\s*'))}; + _updated(); + } + /// The maximum number of redirects allowed during the request. set maxRedirects(int value) { _data['maxRedirects'] = value; @@ -188,6 +223,9 @@ final class HttpProfileResponseData { /// It can contain any arbitrary data as long as the values are of type /// [String] or [int]. For example: /// { 'localPort': 1285, 'remotePort': 443, 'connectionPoolId': '21x23' } + /// + /// XXX what is the difference between the connection info in the request + /// and the response? Don't they use the same connection? set connectionInfo(Map value) { for (final v in value.values) { if (!(v is String || v is int)) { @@ -214,7 +252,10 @@ final class HttpProfileResponseData { _updated(); } - set reasonPhrase(String value) { + set reasonPhrase(String? value) { + if (value == null) { + _data.remove('reasonPhrase'); + } _data['reasonPhrase'] = value; _updated(); } @@ -250,12 +291,17 @@ final class HttpProfileResponseData { /// The time at which the response was completed. Note that DevTools will not /// consider the request to be complete until [endTime] is non-null. + /// + /// This means that all the data has been received, right? set endTime(DateTime value) { _data['endTime'] = value.microsecondsSinceEpoch; _updated(); } /// The error associated with a failed request. + /// + /// This doesn't seem to be just set for failures. For HttpClient, + /// finishResponseWithError('Connection was upgraded') set error(String value) { _data['error'] = value; _updated(); @@ -297,6 +343,10 @@ final class HttpClientRequestProfile { /// The time at which the request was completed. Note that DevTools will not /// consider the request to be complete until [requestEndTimestamp] is /// non-null. + /// + /// What does this mean? Is it when the response data first arrives? Or + /// after the initial request data has been sent? This matters because do + /// redirects count as part of the request time? set requestEndTimestamp(DateTime value) { _data['requestEndTimestamp'] = value.microsecondsSinceEpoch; _updated(); diff --git a/pkgs/http_profile/pubspec.yaml b/pkgs/http_profile/pubspec.yaml index 9e3b69e12f..1ada043a89 100644 --- a/pkgs/http_profile/pubspec.yaml +++ b/pkgs/http_profile/pubspec.yaml @@ -8,8 +8,5 @@ repository: https://github.com/dart-lang/http/tree/master/pkgs/http_profile environment: sdk: ^3.0.0 -dependencies: - test: ^1.24.9 - dev_dependencies: dart_flutter_team_lints: ^2.1.1