From 6d7720e647540adc2da084fb18ffcda483d1cb10 Mon Sep 17 00:00:00 2001 From: Istvan Soos Date: Mon, 28 Oct 2024 09:27:19 +0100 Subject: [PATCH] Move object-name methods to PackageStorage. --- app/lib/admin/backend.dart | 18 ++-------- app/lib/package/backend.dart | 7 ---- app/lib/package/package_storage.dart | 36 +++++++++++++++++-- app/lib/shared/integrity.dart | 13 +++---- app/test/admin/moderate_package_test.dart | 10 +++--- .../admin/moderate_package_version_test.dart | 13 ++++--- .../package/tarball_storage_namer_test.dart | 1 + 7 files changed, 52 insertions(+), 46 deletions(-) diff --git a/app/lib/admin/backend.dart b/app/lib/admin/backend.dart index 8dee6ae5fb..3747d2c134 100644 --- a/app/lib/admin/backend.dart +++ b/app/lib/admin/backend.dart @@ -12,7 +12,6 @@ import 'package:clock/clock.dart'; import 'package:collection/collection.dart'; import 'package:convert/convert.dart'; import 'package:gcloud/service_scope.dart' as ss; -import 'package:gcloud/storage.dart'; import 'package:logging/logging.dart'; import 'package:meta/meta.dart'; import 'package:pool/pool.dart'; @@ -24,11 +23,7 @@ import '../account/models.dart'; import '../admin/models.dart'; import '../audit/models.dart'; import '../package/backend.dart' - show - checkPackageVersionParams, - packageBackend, - purgePackageCache, - tarballObjectName; + show checkPackageVersionParams, packageBackend, purgePackageCache; import '../package/models.dart'; import '../publisher/models.dart'; import '../scorecard/backend.dart'; @@ -36,7 +31,6 @@ import '../service/email/email_templates.dart'; import '../shared/configuration.dart'; import '../shared/datastore.dart'; import '../shared/exceptions.dart'; -import '../shared/storage.dart'; import '../shared/versions.dart'; import '../task/backend.dart'; import 'actions/actions.dart' show AdminAction; @@ -794,9 +788,6 @@ class AdminBackend { @visibleForTesting DateTime? before, }) async { before ??= clock.ago(days: 3 * 366).toUtc(); // extra buffer days - final canonicalBucket = - storageService.bucket(activeConfiguration.canonicalPackagesBucketName!); - // delete packages final pQuery = _db.query() ..filter('moderatedAt <', before) @@ -829,11 +820,8 @@ class AdminBackend { 'Deleting moderated package version: ${version.qualifiedVersionKey}'); // deleting from canonical bucket - final objectName = tarballObjectName(version.package, version.version!); - final info = await canonicalBucket.tryInfo(objectName); - if (info != null) { - await canonicalBucket.delete(objectName); - } + await packageBackend.packageStorage + .deleteArchiveFromCanonicalBucket(version.package, version.version!); // deleting from datastore await withRetryTransaction(_db, (tx) async { diff --git a/app/lib/package/backend.dart b/app/lib/package/backend.dart index 924fd74108..0ff39c8ad1 100644 --- a/app/lib/package/backend.dart +++ b/app/lib/package/backend.dart @@ -1938,13 +1938,6 @@ DerivedPackageVersionEntities derivePackageVersionEntities({ return DerivedPackageVersionEntities(versionInfo, assets); } -/// The GCS object name of a tarball object - excluding leading '/'. -String tarballObjectName(String package, String version) => - '${tarballObjectNamePackagePrefix(package)}$version.tar.gz'; - -/// The GCS object prefix of a tarball object for a given [package] - excluding leading '/'. -String tarballObjectNamePackagePrefix(String package) => 'packages/$package-'; - /// The GCS object name of an temporary object [guid] - excluding leading '/'. @visibleForTesting String tmpObjectName(String guid) => 'tmp/$guid'; diff --git a/app/lib/package/package_storage.dart b/app/lib/package/package_storage.dart index 07259eb914..573ab5d2bb 100644 --- a/app/lib/package/package_storage.dart +++ b/app/lib/package/package_storage.dart @@ -11,6 +11,13 @@ import 'package:pub_dev/shared/storage.dart'; final _logger = Logger('package_storage'); +/// The GCS object name of a tarball object - excluding leading '/'. +String tarballObjectName(String package, String version) => + '${_tarballObjectNamePackagePrefix(package)}$version.tar.gz'; + +/// The GCS object prefix of a tarball object for a given [package] - excluding leading '/'. +String _tarballObjectNamePackagePrefix(String package) => 'packages/$package-'; + class PackageStorage { final DatastoreDB _dbService; final Storage _storage; @@ -32,6 +39,30 @@ class PackageStorage { this._publicBucket, ); + /// Gets the object info of the archive file from the public bucket. + Future getCanonicalBucketArchiveInfo( + String package, String version) async { + final objectName = tarballObjectName(package, version); + return await _canonicalBucket.tryInfo(objectName); + } + + /// Gets the object info of the archive file from the public bucket. + Future getPublicBucketArchiveInfo( + String package, String version) async { + final objectName = tarballObjectName(package, version); + return await _publicBucket.tryInfo(objectName); + } + + /// Deletes the package archive file from the canonical bucket. + Future deleteArchiveFromCanonicalBucket( + String package, String version) async { + final objectName = tarballObjectName(package, version); + final info = await _canonicalBucket.tryInfo(objectName); + if (info != null) { + await _canonicalBucket.delete(objectName); + } + } + /// Updates the public package archive: /// - copies missing archive objects from canonical to public bucket, /// - deletes leftover objects from public bucket @@ -92,8 +123,9 @@ class PackageStorage { objectNamesInPublicBucket.add(objectName); } - final filterForNamePrefix = - package == null ? 'packages/' : tarballObjectNamePackagePrefix(package); + final filterForNamePrefix = package == null + ? 'packages/' + : _tarballObjectNamePackagePrefix(package); await for (final entry in _publicBucket.list(prefix: filterForNamePrefix)) { // Skip non-objects. if (!entry.isObject) { diff --git a/app/lib/shared/integrity.dart b/app/lib/shared/integrity.dart index a2957343bb..3b496b5123 100644 --- a/app/lib/shared/integrity.dart +++ b/app/lib/shared/integrity.dart @@ -9,7 +9,6 @@ import 'package:_pub_shared/search/tags.dart'; import 'package:_pub_shared/utils/http.dart'; import 'package:clock/clock.dart'; import 'package:crypto/crypto.dart'; -import 'package:gcloud/storage.dart'; import 'package:http/http.dart' as http; import 'package:logging/logging.dart'; import 'package:pool/pool.dart'; @@ -604,10 +603,8 @@ class IntegrityChecker { Uri archiveDownloadUri, { required bool shouldBeInPublicBucket, }) async* { - final canonicalInfo = await storageService - .bucket(activeConfiguration.canonicalPackagesBucketName!) - // ignore: invalid_use_of_visible_for_testing_member - .tryInfo(tarballObjectName(pv.package, pv.version!)); + final canonicalInfo = await packageBackend.packageStorage + .getCanonicalBucketArchiveInfo(pv.package, pv.version!); if (canonicalInfo == null) { yield 'PackageVersion "${pv.qualifiedVersionKey}" has no matching canonical archive file.'; return; @@ -631,10 +628,8 @@ class IntegrityChecker { yield 'Canonical archive for PackageVersion "${pv.qualifiedVersionKey}" differs from public bucket.'; } - final publicInfo = await storageService - .bucket(activeConfiguration.publicPackagesBucketName!) - // ignore: invalid_use_of_visible_for_testing_member - .tryInfo(tarballObjectName(pv.package, pv.version!)); + final publicInfo = await packageBackend.packageStorage + .getPublicBucketArchiveInfo(pv.package, pv.version!); if (!canonicalInfo.hasSameSignatureAs(publicInfo)) { yield 'Canonical archive for PackageVersion "${pv.qualifiedVersionKey}" differs in the public bucket.'; } diff --git a/app/test/admin/moderate_package_test.dart b/app/test/admin/moderate_package_test.dart index 71aef8abf4..b4840d2d8f 100644 --- a/app/test/admin/moderate_package_test.dart +++ b/app/test/admin/moderate_package_test.dart @@ -8,7 +8,6 @@ import 'package:_pub_shared/data/account_api.dart'; import 'package:_pub_shared/data/admin_api.dart'; import 'package:_pub_shared/data/package_api.dart'; import 'package:clock/clock.dart'; -import 'package:gcloud/storage.dart'; import 'package:http/http.dart' as http; import 'package:pub_dev/account/backend.dart'; import 'package:pub_dev/admin/actions/actions.dart'; @@ -21,7 +20,6 @@ import 'package:pub_dev/scorecard/backend.dart'; import 'package:pub_dev/search/backend.dart'; import 'package:pub_dev/shared/configuration.dart'; import 'package:pub_dev/shared/datastore.dart'; -import 'package:pub_dev/shared/storage.dart'; import 'package:test/test.dart'; import '../admin/models_test.dart'; @@ -382,10 +380,9 @@ void main() { }); // canonical file is present - final bucket = storageService - .bucket(activeConfiguration.canonicalPackagesBucketName!); expect( - await bucket.tryInfo(tarballObjectName('oxygen', '1.2.0')), + await packageBackend.packageStorage + .getCanonicalBucketArchiveInfo('oxygen', '1.2.0'), isNotNull, ); @@ -400,7 +397,8 @@ void main() { isNull, ); expect( - await bucket.tryInfo(tarballObjectName('oxygen', '1.2.0')), + await packageBackend.packageStorage + .getCanonicalBucketArchiveInfo('oxygen', '1.2.0'), isNull, ); diff --git a/app/test/admin/moderate_package_version_test.dart b/app/test/admin/moderate_package_version_test.dart index e44b1ffcc1..babeefb2b4 100644 --- a/app/test/admin/moderate_package_version_test.dart +++ b/app/test/admin/moderate_package_version_test.dart @@ -8,7 +8,6 @@ import 'package:_pub_shared/data/account_api.dart'; import 'package:_pub_shared/data/admin_api.dart'; import 'package:_pub_shared/data/package_api.dart'; import 'package:clock/clock.dart'; -import 'package:gcloud/storage.dart'; import 'package:http/http.dart' as http; import 'package:pub_dev/admin/backend.dart'; import 'package:pub_dev/admin/models.dart'; @@ -20,7 +19,6 @@ import 'package:pub_dev/search/backend.dart'; import 'package:pub_dev/shared/configuration.dart'; import 'package:pub_dev/shared/datastore.dart'; import 'package:pub_dev/shared/exceptions.dart'; -import 'package:pub_dev/shared/storage.dart'; import 'package:pub_dev/task/backend.dart'; import 'package:test/test.dart'; @@ -430,10 +428,9 @@ void main() { 'cleanup deletes datastore entities and canonical archive file', fn: () async { // canonical file is present - final bucket = storageService - .bucket(activeConfiguration.canonicalPackagesBucketName!); expect( - await bucket.tryInfo(tarballObjectName('oxygen', '1.0.0')), + await packageBackend.packageStorage + .getCanonicalBucketArchiveInfo('oxygen', '1.0.0'), isNotNull, ); @@ -462,7 +459,8 @@ void main() { // canonical file is not present expect( - await bucket.tryInfo(tarballObjectName('oxygen', '1.0.0')), + await packageBackend.packageStorage + .getCanonicalBucketArchiveInfo('oxygen', '1.0.0'), isNull, ); @@ -472,7 +470,8 @@ void main() { isNotNull, ); expect( - await bucket.tryInfo(tarballObjectName('oxygen', '1.2.0')), + await packageBackend.packageStorage + .getCanonicalBucketArchiveInfo('oxygen', '1.2.0'), isNotNull, ); }); diff --git a/app/test/package/tarball_storage_namer_test.dart b/app/test/package/tarball_storage_namer_test.dart index b063dd4f39..23675516e7 100644 --- a/app/test/package/tarball_storage_namer_test.dart +++ b/app/test/package/tarball_storage_namer_test.dart @@ -4,6 +4,7 @@ import 'package:gcloud/storage.dart'; import 'package:pub_dev/package/backend.dart'; +import 'package:pub_dev/package/package_storage.dart'; import 'package:pub_dev/shared/configuration.dart'; import 'package:pub_dev/shared/storage.dart'; import 'package:test/test.dart';