From 2bb8e2c4e1e6099345608dddcbe1def0a3319edb Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Fri, 15 Sep 2023 13:45:14 +0200 Subject: [PATCH] Don't impose a strict size limit to uploads (#4014) --- lib/src/authentication/client.dart | 7 ++-- lib/src/command/lish.dart | 29 ++++++++++----- lib/src/http.dart | 35 +++++++++++++++++++ lib/src/utils.dart | 10 ++++++ lib/src/validator.dart | 4 +-- lib/src/validator/size.dart | 14 ++++---- ...storage_upload_provides_an_error_test.dart | 22 ++++++++++-- ...taking a --directory~-C parameter work.txt | 2 ++ .../many_files_test/displays all files.txt | 2 ++ test/validator/size_test.dart | 26 +++++++------- 10 files changed, 113 insertions(+), 38 deletions(-) diff --git a/lib/src/authentication/client.dart b/lib/src/authentication/client.dart index c36e52975..ffa7889cd 100644 --- a/lib/src/authentication/client.dart +++ b/lib/src/authentication/client.dart @@ -11,6 +11,7 @@ import 'package:http_parser/http_parser.dart'; import '../http.dart'; import '../log.dart' as log; import '../system_cache.dart'; +import '../utils.dart'; import 'credential.dart'; /// This client authenticates requests by injecting `Authentication` header to @@ -83,11 +84,7 @@ class _AuthenticatedClient extends http.BaseClient { } } if (serverMessage != null) { - // Only allow printable ASCII, map anything else to whitespace, take - // at-most 1024 characters. - serverMessage = String.fromCharCodes( - serverMessage.runes.map((r) => 32 <= r && r <= 127 ? r : 32).take(1024), - ); + serverMessage = sanitizeForTerminal(serverMessage); } throw AuthenticationException(response.statusCode, serverMessage); } diff --git a/lib/src/command/lish.dart b/lib/src/command/lish.dart index 9c4372c72..8b63718f2 100644 --- a/lib/src/command/lish.dart +++ b/lib/src/command/lish.dart @@ -179,9 +179,7 @@ class LishCommand extends PubCommand { } on PubHttpResponseException catch (error) { var url = error.response.request!.url; if (url == cloudStorageUrl) { - // TODO(nweiz): the response may have XML-formatted information about - // the error. Try to parse that out once we have an easily-accessible - // XML parser. + handleGCSError(error.response); fail(log.red('Failed to upload the package.')); } else if (Uri.parse(url.origin) == Uri.parse(host.origin)) { handleJsonError(error.response); @@ -277,14 +275,17 @@ the \$PUB_HOSTED_URL environment variable.''', '${tree.fromFiles(files, baseDir: entrypoint.rootDir, showFileSizes: true)}', ); - var packageBytesFuture = - createTarGz(files, baseDir: entrypoint.rootDir).toBytes(); + var packageBytes = + await createTarGz(files, baseDir: entrypoint.rootDir).toBytes(); + log.message( + '\nTotal compressed archive size: ${_readableFileSize(packageBytes.length)}.\n', + ); // Validate the package. var isValid = skipValidation ? true : await _validate( - packageBytesFuture.then((bytes) => bytes.length), + packageBytes.length, files, ); if (!isValid) { @@ -294,7 +295,7 @@ the \$PUB_HOSTED_URL environment variable.''', log.message('The server may enforce additional checks.'); return; } else { - await _publish(await packageBytesFuture); + await _publish(packageBytes); } } @@ -307,7 +308,7 @@ the \$PUB_HOSTED_URL environment variable.''', /// Validates the package. Completes to false if the upload should not /// proceed. - Future _validate(Future packageSize, List files) async { + Future _validate(int packageSize, List files) async { final hints = []; final warnings = []; final errors = []; @@ -368,3 +369,15 @@ the \$PUB_HOSTED_URL environment variable.''', return true; } } + +String _readableFileSize(int size) { + if (size >= 1 << 30) { + return '${size ~/ (1 << 30)} GB'; + } else if (size >= 1 << 20) { + return '${size ~/ (1 << 20)} MB'; + } else if (size >= 1 << 10) { + return '${size ~/ (1 << 10)} KB'; + } else { + return '<1 KB'; + } +} diff --git a/lib/src/http.dart b/lib/src/http.dart index 294d567f2..d05c51296 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -211,6 +211,41 @@ void handleJsonError(http.BaseResponse response) { fail(log.red(error['message'] as String)); } +/// Handles an unsuccessful XML-formatted response from google cloud storage. +/// +/// Assumes messages are of the form in +/// https://cloud.google.com/storage/docs/xml-api/reference-status +/// +/// This is a poor person's XML parsing with regexps, but this should be +/// sufficient for the specified messages. +void handleGCSError(http.BaseResponse response) { + if (response is http.Response) { + final responseBody = response.body; + if (responseBody.contains('(.*)').firstMatch(responseBody)?[1]; + if (result == null) return null; + return sanitizeForTerminal(result); + } + + final code = getTagText('Code'); + // TODO(sigurdm): we could hard-code nice error messages for known codes. + final message = getTagText('Message'); + // `Details` are not specified in the doc above, but have been observed in actual responses. + final details = getTagText('Details'); + if (code != null) { + log.error('Server error code: $code'); + } + if (message != null) { + log.error('Server message: $message'); + } + if (details != null) { + log.error('Server details: $details'); + } + } + } +} + /// Parses a response body, assuming it's JSON-formatted. /// /// Throws a user-friendly error if the response body is invalid JSON, or if diff --git a/lib/src/utils.dart b/lib/src/utils.dart index 31ca1d188..d43780a30 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -767,3 +767,13 @@ extension RetrieveFlags on ArgResults { String option(String name) => this[name] as String; String? optionWithoutDefault(String name) => this[name] as String?; } + +/// Limits the range of characters and length. +/// +/// Useful for displaying externally provided strings. +/// +/// Only allowd printable ASCII, map anything else to whitespace, take at-most +/// 1024 characters. +String sanitizeForTerminal(String input) => String.fromCharCodes( + input.runes.map((r) => 32 <= r && r <= 127 ? r : 32).take(1024), + ); diff --git a/lib/src/validator.dart b/lib/src/validator.dart index 07ef9b36c..ba923df3a 100644 --- a/lib/src/validator.dart +++ b/lib/src/validator.dart @@ -128,7 +128,7 @@ abstract class Validator { /// upload to the server. static Future runAll( Entrypoint entrypoint, - Future packageSize, + int packageSize, Uri serverUrl, List files, { required List hints, @@ -163,7 +163,7 @@ abstract class Validator { final context = ValidationContext( entrypoint, - await packageSize, + packageSize, serverUrl, files, ); diff --git a/lib/src/validator/size.dart b/lib/src/validator/size.dart index ae3249676..a8a7eae6c 100644 --- a/lib/src/validator/size.dart +++ b/lib/src/validator/size.dart @@ -7,7 +7,7 @@ import 'dart:async'; import '../io.dart'; import '../validator.dart'; -/// The maximum size of the package to upload (100 MB). +/// The preferred maximum size of the package to upload (100 MB). const _maxSize = 100 * 1024 * 1024; /// A validator that validates that a package isn't too big. @@ -19,17 +19,19 @@ class SizeValidator extends Validator { // Current implementation of Package.listFiles skips hidden files var ignoreExists = fileExists(entrypoint.root.path('.gitignore')); - var error = StringBuffer('Your package is $sizeInMb MB. Hosted ' - 'packages must be smaller than 100 MB.'); + var hint = StringBuffer(''' +Your package is $sizeInMb MB. + +Consider the impact large downloads can have on the package consumer.'''); if (ignoreExists && !entrypoint.root.inGitRepo) { - error.write(' Your .gitignore has no effect since your project ' + hint.write('\nYour .gitignore has no effect since your project ' 'does not appear to be in version control.'); } else if (!ignoreExists && entrypoint.root.inGitRepo) { - error.write(' Consider adding a .gitignore to avoid including ' + hint.write('\nConsider adding a .gitignore to avoid including ' 'temporary files.'); } - errors.add(error.toString()); + hints.add(hint.toString()); } } diff --git a/test/lish/cloud_storage_upload_provides_an_error_test.dart b/test/lish/cloud_storage_upload_provides_an_error_test.dart index bd14d84d0..a11ba9542 100644 --- a/test/lish/cloud_storage_upload_provides_an_error_test.dart +++ b/test/lish/cloud_storage_upload_provides_an_error_test.dart @@ -22,14 +22,30 @@ void main() { globalServer.expect('POST', '/upload', (request) { return request.read().drain().then((_) { return shelf.Response.notFound( - 'Your request sucked.', + // Actual example of an error code we get from GCS + "EntityTooLargeYour proposed upload is larger than the maximum object size specified in your Policy Document.
Content-length exceeds upper bound on range
", headers: {'content-type': 'application/xml'}, ); }); }); - // TODO(nweiz): This should use the server's error message once the client - // can parse the XML. + expect( + pub.stderr, + emits( + 'Server error code: EntityTooLarge', + ), + ); + expect( + pub.stderr, + emits( + 'Server message: Your proposed upload is larger than the maximum object size specified in your Policy Document.', + ), + ); + expect( + pub.stderr, + emits('Server details: Content-length exceeds upper bound on range'), + ); + expect(pub.stderr, emits('Failed to upload the package.')); await pub.shouldExit(1); }); diff --git a/test/testdata/goldens/directory_option_test/commands taking a --directory~-C parameter work.txt b/test/testdata/goldens/directory_option_test/commands taking a --directory~-C parameter work.txt index e4da8be6c..f463833fa 100644 --- a/test/testdata/goldens/directory_option_test/commands taking a --directory~-C parameter work.txt +++ b/test/testdata/goldens/directory_option_test/commands taking a --directory~-C parameter work.txt @@ -118,6 +118,8 @@ Publishing test_pkg 1.0.0 to http://localhost:$PORT: ├── lib │ └── test_pkg.dart (<1 KB) └── pubspec.yaml (<1 KB) + +Total compressed archive size: <1 KB. Validating package... The server may enforce additional checks. [STDERR] diff --git a/test/testdata/goldens/lish/many_files_test/displays all files.txt b/test/testdata/goldens/lish/many_files_test/displays all files.txt index 82a9431d4..2d374562e 100644 --- a/test/testdata/goldens/lish/many_files_test/displays all files.txt +++ b/test/testdata/goldens/lish/many_files_test/displays all files.txt @@ -29,6 +29,8 @@ Publishing test_pkg 1.0.0 to http://localhost:$PORT: │ ├── file_9.dart (<1 KB) │ └── test_pkg.dart (<1 KB) └── pubspec.yaml (<1 KB) + +Total compressed archive size: <1 KB. Validating package... Publishing is forever; packages cannot be unpublished. diff --git a/test/validator/size_test.dart b/test/validator/size_test.dart index 02a093cdf..f04245a55 100644 --- a/test/validator/size_test.dart +++ b/test/validator/size_test.dart @@ -11,30 +11,29 @@ import '../descriptor.dart' as d; import '../test_pub.dart'; import 'utils.dart'; -Future expectSizeValidationError(Matcher matcher) async { +Future expectSizeValidationHint(Matcher matcher) async { await expectValidationDeprecated( SizeValidator.new, size: 100 * (1 << 20) + 1, - errors: contains(matcher), + hints: contains(matcher), ); } void main() { - test('considers a package valid if it is <= 100 MB', () async { + test('ho hint if package is <= 100 MB', () async { await d.validPackage().create(); await expectValidationDeprecated(SizeValidator.new, size: 100); await expectValidationDeprecated(SizeValidator.new, size: 100 * (1 << 20)); }); - group('considers a package invalid if it is more than 100 MB', () { + group('hints if package is more than 100 MB', () { test('package is not under source control and no .gitignore exists', () async { await d.validPackage().create(); - await expectSizeValidationError( - equals('Your package is 100.0 MB. Hosted packages must ' - 'be smaller than 100 MB.'), + await expectSizeValidationHint( + contains('Your package is 100.0 MB.'), ); }); @@ -42,9 +41,9 @@ void main() { await d.validPackage().create(); await d.dir(appPath, [d.file('.gitignore', 'ignored')]).create(); - await expectSizeValidationError( + await expectSizeValidationHint( allOf( - contains('Hosted packages must be smaller than 100 MB.'), + contains('Your package is 100.0 MB.'), contains('Your .gitignore has no effect since your project ' 'does not appear to be in version control.'), ), @@ -55,9 +54,9 @@ void main() { await d.validPackage().create(); await d.git(appPath).create(); - await expectSizeValidationError( + await expectSizeValidationHint( allOf( - contains('Hosted packages must be smaller than 100 MB.'), + contains('Your package is 100.0 MB.'), contains('Consider adding a .gitignore to avoid including ' 'temporary files.'), ), @@ -68,9 +67,8 @@ void main() { await d.validPackage().create(); await d.git(appPath, [d.file('.gitignore', 'ignored')]).create(); - await expectSizeValidationError( - equals('Your package is 100.0 MB. Hosted packages must ' - 'be smaller than 100 MB.'), + await expectSizeValidationHint( + contains('Your package is 100.0 MB.'), ); }); });