Don't impose a strict size limit to uploads (#4014)

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);
   }

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<bool> _validate(Future<int> packageSize, List<String> files) async {
+  Future<bool> _validate(int packageSize, List<String> files) async {
     final hints = <String>[];
     final warnings = <String>[];
     final errors = <String>[];
@@ -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';
+  }
+}

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('<?xml')) {
+      String? getTagText(String tag) {
+        final result = RegExp('<$tag>(.*)</$tag>').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

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),
+    );

lib/src/validator.dart

@@ -128,7 +128,7 @@ abstract class Validator {
   /// upload to the server.
   static Future<void> runAll(
     Entrypoint entrypoint,
-    Future<int> packageSize,
+    int packageSize,
     Uri serverUrl,
     List<String> files, {
     required List<String> hints,
@@ -163,7 +163,7 @@ abstract class Validator {
 
     final context = ValidationContext(
       entrypoint,
-      await packageSize,
+      packageSize,
       serverUrl,
       files,
     );

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());
   }
 }

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(
-          '<Error><Message>Your request sucked.</Message></Error>',
+          // Actual example of an error code we get from GCS
+          "<?xml version='1.0' encoding='UTF-8'?><Error><Code>EntityTooLarge</Code><Message>Your proposed upload is larger than the maximum object size specified in your Policy Document.</Message><Details>Content-length exceeds upper bound on range</Details></Error>",
           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);
   });

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] 

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.

test/validator/size_test.dart

@@ -11,40 +11,39 @@ import '../descriptor.dart' as d;
 import '../test_pub.dart';
 import 'utils.dart';
 
-Future<void> expectSizeValidationError(Matcher matcher) async {
+Future<void> 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.'),
       );
     });
 
     test('package is not under source control and .gitignore exists', () async {
       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.'),
       );
     });
   });