Skip to content

Commit

Permalink
[common] stop crashing with range requests in the browser (#468)
Browse files Browse the repository at this point in the history
Remove check that cannot be done in the browser
Also remove invalid header sets for HTTP requests in the browser

Fixes #462
  • Loading branch information
kevmoo authored Nov 4, 2022
1 parent 14ca236 commit f596473
Show file tree
Hide file tree
Showing 8 changed files with 81 additions and 38 deletions.
6 changes: 6 additions & 0 deletions discoveryapis_commons/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 1.0.4

- Eliminate the invalid header warning in the browser.
- Fix issue with range requests from the browser.
([#462](https://github.com/google/googleapis.dart/issues/462))

## 1.0.3

- Throw a more helpful error message when a resumable upload fails.
Expand Down
58 changes: 30 additions & 28 deletions discoveryapis_commons/lib/src/api_requester.dart
Original file line number Diff line number Diff line change
Expand Up @@ -108,14 +108,19 @@ class ApiRequester {
'Content length of response does not match requested range length.',
);
}
final contentRange = response.headers['content-range'];
final expected = 'bytes ${downloadRange.start}-${downloadRange.end}/';
if (contentRange == null || !contentRange.startsWith(expected)) {
throw client_requests.ApiRequestError(
'Attempting partial '
"download but got invalid 'Content-Range' header "
'(was: $contentRange, expected: $expected).',
);

if (!isWeb) {
// TODO(kevmoo) on the web, should check access-control-expose-headers
// but this is easy for now
final contentRange = response.headers[_contentRangeHeader];
final expected = 'bytes ${downloadRange.start}-${downloadRange.end}/';
if (contentRange == null || !contentRange.startsWith(expected)) {
throw client_requests.ApiRequestError(
'Attempting partial '
"download but got invalid '$_contentRangeHeader' header "
'(was: $contentRange, expected: $expected).',
);
}
}
}

Expand Down Expand Up @@ -183,12 +188,16 @@ class ApiRequester {

Future<http.StreamedResponse> simpleUpload() {
final bodyStream = uploadMedia!.stream;
final request = RequestImpl(method, uri, bodyStream);
request.headers.addAll({
..._requestHeaders,
'content-type': uploadMedia.contentType,
'content-length': '${uploadMedia.length}'
});
final request = RequestImpl(
method,
uri,
stream: bodyStream,
headers: {
..._requestHeaders,
'content-type': uploadMedia.contentType,
'content-length': '${uploadMedia.length}'
},
);
return _httpClient.send(request);
}

Expand All @@ -210,13 +219,12 @@ class ApiRequester {
'range': 'bytes=${downloadRange.start}-${downloadRange.end}',
};

// Filter out headers forbidden in the browser (in calling in browser).
// If we don't do this, the browser will complain that we're attempting
// to set a header that we're not allowed to set.
headers.removeWhere((key, value) => _forbiddenHeaders.contains(key));

final request = RequestImpl(method, uri, bodyController.stream);
request.headers.addAll(headers);
final request = RequestImpl(
method,
uri,
stream: bodyController.stream,
headers: headers,
);
return _httpClient.send(request);
}

Expand Down Expand Up @@ -318,10 +326,4 @@ Stream<String>? _decodeStreamAsText(http.StreamedResponse response) {
}
}

/// List of headers that is forbidden in current execution context.
///
/// In a browser context we're not allowed to set `user-agent` and
/// `content-length` headers.
const _forbiddenHeaders = bool.fromEnvironment('dart.library.html')
? <String>{'user-agent', 'content-length'}
: <String>{};
const _contentRangeHeader = 'content-range';
4 changes: 2 additions & 2 deletions discoveryapis_commons/lib/src/multipart_media_uploader.dart
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ class MultipartMediaUploader {
'content-length': '$totalLength'
};
final bodyStream = bodyController.stream;
final request = RequestImpl(_method, _uri, bodyStream);
request.headers.addAll(headers);
final request =
RequestImpl(_method, _uri, stream: bodyStream, headers: headers);
return _httpClient.send(request);
}
}
Expand Down
25 changes: 23 additions & 2 deletions discoveryapis_commons/lib/src/request_impl.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,36 @@ import 'dart:async';

import 'package:http/http.dart' as http;

import 'utils.dart';

class RequestImpl extends http.BaseRequest {
final Stream<List<int>> _stream;

RequestImpl(super.method, super.url, [Stream<List<int>>? stream])
: _stream = stream ?? const Stream.empty();
RequestImpl(
super.method,
super.url, {
Stream<List<int>>? stream,
Map<String, String>? headers,
}) : _stream = stream ?? const Stream.empty() {
if (headers != null) {
for (var entry in headers.entries) {
if (!_forbiddenHeaders.contains(entry.key)) {
this.headers[entry.key] = entry.value;
}
}
}
}

@override
http.ByteStream finalize() {
super.finalize();
return http.ByteStream(_stream);
}
}

/// List of headers that is forbidden in current execution context.
///
/// In a browser context we're not allowed to set `user-agent` and
/// `content-length` headers.
const _forbiddenHeaders =
isWeb ? <String>{'user-agent', 'content-length'} : <String>{};
11 changes: 6 additions & 5 deletions discoveryapis_commons/lib/src/resumable_media_uploader.dart
Original file line number Diff line number Diff line change
Expand Up @@ -139,14 +139,16 @@ class ResumableMediaUploader {
final bodyStream =
bytes == null ? const Stream<List<int>>.empty() : Stream.value(bytes);

final request = RequestImpl(_method, _uri, bodyStream);
request.headers.addAll({
final headers = {
..._requestHeaders,
'content-type': contentTypeJsonUtf8,
'content-length': '$length',
'x-upload-content-type': _uploadMedia.contentType,
'x-upload-content-length': '${_uploadMedia.length}',
});
};

final request =
RequestImpl(_method, _uri, stream: bodyStream, headers: headers);

final response = await _httpClient.send(request);

Expand Down Expand Up @@ -248,8 +250,7 @@ class ResumableMediaUploader {
};

final stream = Stream.fromIterable(chunk.byteArrays);
final request = RequestImpl('PUT', uri, stream);
request.headers.addAll(headers);
final request = RequestImpl('PUT', uri, stream: stream, headers: headers);
return _httpClient.send(request);
}
}
Expand Down
2 changes: 2 additions & 0 deletions discoveryapis_commons/lib/src/utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,5 @@ bool isJson(String? contentType) {

String escapeVariable(String name) =>
Uri.encodeQueryComponent(name).replaceAll('+', '%20');

const isWeb = bool.fromEnvironment('dart.library.html');
2 changes: 1 addition & 1 deletion discoveryapis_commons/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: _discoveryapis_commons
version: 1.0.3
version: 1.0.4
description: Library for use by client APIs generated from Discovery Documents.
repository: https://github.com/google/googleapis.dart/tree/master/discoveryapis_commons

Expand Down
11 changes: 11 additions & 0 deletions test_integration/web/drive_demo.dart
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,17 @@ Future<void> _upload() async {
);

logToTextArea(jsonEncode(newFile));

logToTextArea('try to read part of the file back - asking for 11 bytes!');

// regression check for https://github.com/google/googleapis.dart/issues/462

final partialRequest = await api.get(
newFile.id!,
downloadOptions: drive.PartialDownloadOptions(drive.ByteRange(0, 10)),
) as drive.Media;

logToTextArea('bytes received: ${partialRequest.length}');
} finally {
client.close();
}
Expand Down

0 comments on commit f596473

Please sign in to comment.