Repository: dart-lang/pub-dev (dart)

Issue:
Package deletion admin action should just remove packages from public bucket

A background process should remove packages from the canonical bucket after they have been deleted for more than 60 days.

Pull request:
- Fixes https://github.com/dart-lang/pub-dev/issues/8730 following the pattern of #8788
- follows the same admin action pattern as the moderate actions
- reuses most of the update code with moderating and deleting a package
- updated tests with only a few relevant bits, we won't need to duplicate everything, as the main code will be already tested with the moderation action
- does the actual deletion after 2 months

Patch:
diff --git a/app/lib/admin/actions/moderate_package.dart b/app/lib/admin/actions/moderate_package.dart
index 03f659c597..48650a88d3 100644
--- a/app/lib/admin/actions/moderate_package.dart
+++ b/app/lib/admin/actions/moderate_package.dart
@@ -33,80 +33,101 @@ Note: the action may take a longer time to complete as the public archive bucket
     final caseId = options['case'];
 
     final package = options['package'];
-    InvalidInputException.check(
-      package != null && package.isNotEmpty,
-      'package must be given',
-    );
-
     final state = options['state'];
-    bool? valueToSet;
-    switch (state) {
-      case 'true':
-        valueToSet = true;
-        break;
-      case 'false':
-        valueToSet = false;
-        break;
-    }
-
     final note = options['note'];
 
     final refCase =
         await adminBackend.loadAndVerifyModerationCaseForAdminAction(caseId);
 
-    final p = await packageBackend.lookupPackage(package!);
-    if (p == null) {
-      throw NotFoundException.resource(package);
-    }
-
-    Package? p2;
-    if (valueToSet != null) {
-      p2 = await withRetryTransaction(dbService, (tx) async {
-        final pkg = await tx.lookupValue<Package>(p.key);
-        pkg.updateIsModerated(isModerated: valueToSet!);
-        tx.insert(pkg);
+    return await adminMarkPackageVisibility(
+      package,
+      state: state,
+      whenUpdating: (tx, p, valueToSet) async {
+        p.updateIsModerated(isModerated: valueToSet);
 
         if (refCase != null) {
           final mc = await tx.lookupValue<ModerationCase>(refCase.key);
           mc.addActionLogEntry(
-            ModerationSubject.package(package).fqn,
+            ModerationSubject.package(package!).fqn,
             valueToSet ? ModerationAction.apply : ModerationAction.revert,
             note,
           );
           tx.insert(mc);
         }
-
-        return pkg;
-      });
-
-      // make sure visibility cache is updated immediately
-      await purgePackageCache(package);
-
-      // sync exported API(s)
-      await apiExporter.synchronizePackage(package, forceDelete: true);
-
-      // retract or re-populate public archive files
-      await packageBackend.tarballStorage.updatePublicArchiveBucket(
-        package: package,
-        ageCheckThreshold: Duration.zero,
-        deleteIfOlder: Duration.zero,
-      );
-
-      await taskBackend.trackPackage(package);
-      await purgePackageCache(package);
-    }
-
-    return {
-      'package': p.name,
-      'before': {
+      },
+      valueFn: (p) => {
         'isModerated': p.isModerated,
         'moderatedAt': p.moderatedAt?.toIso8601String(),
       },
-      if (p2 != null)
-        'after': {
-          'isModerated': p2.isModerated,
-          'moderatedAt': p2.moderatedAt?.toIso8601String(),
-        },
-    };
+    );
   },
 );
+
+/// Changes the moderated or the admin-deleted flag and timestamp on a [package].
+Future<Map<String, dynamic>> adminMarkPackageVisibility(
+  String? package, {
+  /// `true`, `false` or `null`
+  required String? state,
+
+  /// The updates to apply during the transaction.
+  required Future<void> Function(
+    TransactionWrapper tx,
+    Package v,
+    bool valueToSet,
+  ) whenUpdating,
+
+  /// The debug information to return.
+  required Map Function(Package v) valueFn,
+}) async {
+  InvalidInputException.check(
+    package != null && package.isNotEmpty,
+    'package must be given',
+  );
+
+  bool? valueToSet;
+  switch (state) {
+    case 'true':
+      valueToSet = true;
+      break;
+    case 'false':
+      valueToSet = false;
+      break;
+  }
+
+  final p = await packageBackend.lookupPackage(package!);
+  if (p == null) {
+    throw NotFoundException.resource(package);
+  }
+
+  Package? p2;
+  if (valueToSet != null) {
+    p2 = await withRetryTransaction(dbService, (tx) async {
+      final pkg = await tx.lookupValue<Package>(p.key);
+      await whenUpdating(tx, pkg, valueToSet!);
+      tx.insert(pkg);
+      return pkg;
+    });
+
+    // make sure visibility cache is updated immediately
+    await purgePackageCache(package);
+
+    // sync exported API(s)
+    await apiExporter.synchronizePackage(package, forceDelete: true);
+
+    // retract or re-populate public archive files
+    await packageBackend.tarballStorage.updatePublicArchiveBucket(
+      package: package,
+      ageCheckThreshold: Duration.zero,
+      deleteIfOlder: Duration.zero,
+    );
+
+    await taskBackend.trackPackage(package);
+    await purgePackageCache(package);
+  }
+
+  return {
+    'package': p.name,
+    'before': valueFn(p),
+    if (p2 != null) 'after': valueFn(p2),
+  };
+}
diff --git a/app/lib/admin/actions/package_delete.dart b/app/lib/admin/actions/package_delete.dart
index 34c7e406bf..1a115b5300 100644
--- a/app/lib/admin/actions/package_delete.dart
+++ b/app/lib/admin/actions/package_delete.dart
@@ -4,54 +4,40 @@
 
 import '../../account/backend.dart';
 import '../../shared/configuration.dart';
-import '../backend.dart';
 import 'actions.dart';
+import 'moderate_package.dart';
 
 final packageDelete = AdminAction(
-    name: 'package-delete',
-    options: {
-      'package': 'name of package to delete',
-    },
-    summary: 'Deletes package <package>.',
-    description: '''
-Deletes package <package>.
+  name: 'package-delete',
+  summary: 'Set the admin-deleted flag on a package (making it not visible).',
+  description: '''
+Set the admin-deleted flag on a package (updating the flag and the timestamp).
 
-Deletes all associated resources:
+A package in this state will appear deleted from the public. But its archive file will still exist in the canonical bucket, and the metadata will still be present.
 
-* PackageVersions
-* Likes
-* AuditLogRecords
-* PackageVersionAsset
-* replacedBy references
-* archives (might be retrievable from backup)
+After 2 months it will be fully purged.
 
-The package will be "tombstoned" and no package with the same name can be 
-published later.
+To undo a deletion run the same command with `state: false`.
 ''',
-    invoke: (args) async {
-      final packageName = args['package'];
-      if (packageName == null) {
-        throw InvalidInputException('Missing `package` argument');
-      }
-
-      await requireAuthenticatedAdmin(AdminPermission.removePackage);
-      final result = await adminBackend.removePackage(packageName);
-
-      return {
-        'message': '''
-Package and all associated resources deleted.
-
-A tombstone has been created
-
-'NOTICE: Redis caches referencing the package will expire given time.'
-''',
-        'package': packageName,
-        'deletedPackages': result.deletedPackages,
-        'deletedPackageVersions': result.deletedPackageVersions,
-        'deletedPackageVersionInfos': result.deletedPackageVersionInfos,
-        'deletedPackageVersionAssets': result.deletedPackageVersionAssets,
-        'deletedLikes': result.deletedLikes,
-        'deletedAuditLogs': result.deletedAuditLogs,
-        'replacedByFixes': result.replacedByFixes,
-      };
-    });
+  options: {
+    'package': 'The package name to be deleted',
+    'state':
+        'Set admin-deleted state true / false. Returns current state if omitted.',
+  },
+  invoke: (args) async {
+    await requireAuthenticatedAdmin(AdminPermission.removePackage);
+    final package = args['package'];
+    final state = args['state'];
+    return await adminMarkPackageVisibility(
+      package,
+      state: state,
+      whenUpdating: (tx, p, valueToSet) async {
+        p.updateIsAdminDeleted(isAdminDeleted: valueToSet);
+      },
+      valueFn: (p) => {
+        'isAdminDeleted': p.isAdminDeleted,
+        'adminDeletedAt': p.adminDeletedAt?.toIso8601String(),
+      },
+    );
+  },
+);
diff --git a/app/lib/admin/actions/package_version_delete.dart b/app/lib/admin/actions/package_version_delete.dart
index 56848737e5..a60be12c2e 100644
--- a/app/lib/admin/actions/package_version_delete.dart
+++ b/app/lib/admin/actions/package_version_delete.dart
@@ -12,7 +12,13 @@ final packageVersionDelete = AdminAction(
   summary:
       'Set the admin-deleted flag on a package version (making it not visible).',
   description: '''
-Set the admin-deleted flag on a package version (updating the flag and the timestamp). After 2 months it will be fully deleted.
+Set the admin-deleted flag on a package version (updating the flag and the timestamp).
+
+A package version in this state will appear deleted from the public. But its archive file will still exist in the canonical bucket, and the metadata will still be present.
+
+After 2 months it will be fully purged.
+
+To undo a deletion run the same command with `state: false`.
 ''',
   options: {
     'package': 'The package name to be deleted',
diff --git a/app/lib/admin/backend.dart b/app/lib/admin/backend.dart
index ab6f447ee6..dc5bd63b66 100644
--- a/app/lib/admin/backend.dart
+++ b/app/lib/admin/backend.dart
@@ -881,6 +881,21 @@ class AdminBackend {
   }) async {
     before ??= clock.ago(days: 62).toUtc(); // extra buffer days
 
+    // delete packages
+    final pQuery = _db.query<Package>()
+      ..filter('adminDeletedAt <', before)
+      ..order('adminDeletedAt');
+    await for (final package in pQuery.run()) {
+      // sanity check
+      if (!(package.isAdminDeleted ?? false)) {
+        continue;
+      }
+
+      _logger.info('Deleting admin-deleted package: ${package.name}');
+      await removePackage(package.name!);
+      _logger.info('Deleted moderated package: ${package.name}');
+    }
+
     // delete package versions
     final pvQuery = _db.query<PackageVersion>()
       ..filter('adminDeletedAt <', before)


Test patch:
diff --git a/app/test/admin/api_test.dart b/app/test/admin/api_test.dart
index b9dc7ea802..aedb344ade 100644
--- a/app/test/admin/api_test.dart
+++ b/app/test/admin/api_test.dart
@@ -163,7 +163,85 @@ void main() {
         ),
       );
 
-      testWithProfile('OK', fn: () async {
+      testWithProfile('mark and revert', expectedLogMessages: [
+        'SHOUT Deleting object from public bucket: "packages/oxygen-1.0.0.tar.gz".',
+        'SHOUT Deleting object from public bucket: "packages/oxygen-1.2.0.tar.gz".',
+        'SHOUT Deleting object from public bucket: "packages/oxygen-2.0.0-dev.tar.gz".',
+      ], fn: () async {
+        final client = createPubApiClient(authToken: siteAdminToken);
+        final pkgKey = dbService.emptyKey.append(Package, id: 'oxygen');
+        final package = await dbService.lookupValue<Package>(pkgKey);
+        expect(package, isNotNull);
+
+        // mark for deletion
+        await client.adminInvokeAction(
+          'package-delete',
+          AdminInvokeActionArguments(arguments: {
+            'package': 'oxygen',
+            'state': 'true',
+          }),
+        );
+
+        // repeated call updates the timestamp
+        final rsAgain = await client.adminInvokeAction(
+          'package-delete',
+          AdminInvokeActionArguments(arguments: {
+            'package': 'oxygen',
+            'state': 'true',
+          }),
+        );
+        expect(rsAgain.output, {
+          'package': 'oxygen',
+          'before': {'isAdminDeleted': true, 'adminDeletedAt': isNotNull},
+          'after': {'isAdminDeleted': true, 'adminDeletedAt': isNotNull},
+        });
+        expect(
+          (rsAgain.output['before'] as Map)['adminDeletedAt'] ==
+              (rsAgain.output['after'] as Map)['adminDeletedAt'],
+          isFalse,
+        );
+
+        // package is no longer visible
+        final pkgAfterMarked = await dbService.lookupOrNull<Package>(pkgKey);
+        expect(pkgAfterMarked!.isVisible, false);
+        final archiveAfterMarked = await packageBackend.tarballStorage
+            .getPublicBucketArchiveInfo('oxygen', '1.2.0');
+        expect(archiveAfterMarked, isNull);
+        await expectLater(() => packageBackend.listVersions('oxygen'),
+            throwsA(isA<NotFoundException>()));
+
+        // revert
+        final rsRevert = await client.adminInvokeAction(
+          'package-delete',
+          AdminInvokeActionArguments(
+            arguments: {
+              'package': 'oxygen',
+              'state': 'false',
+            },
+          ),
+        );
+        expect(rsRevert.output, {
+          'package': 'oxygen',
+          'before': {'isAdminDeleted': true, 'adminDeletedAt': isNotNull},
+          'after': {'isAdminDeleted': false, 'adminDeletedAt': null},
+        });
+
+        // package is visible again
+        final pkgAfterReverted = await dbService.lookupOrNull<Package>(pkgKey);
+        expect(pkgAfterReverted!.isVisible, true);
+        final archiveAfterReverted = await packageBackend.tarballStorage
+            .getPublicBucketArchiveInfo('oxygen', '1.2.0');
+        expect(archiveAfterReverted, isNotNull);
+        final versionsAfterReverted =
+            await packageBackend.listVersions('oxygen');
+        expect(versionsAfterReverted, isNotNull);
+      });
+
+      testWithProfile('OK', expectedLogMessages: [
+        'SHOUT Deleting object from public bucket: "packages/oxygen-1.0.0.tar.gz".',
+        'SHOUT Deleting object from public bucket: "packages/oxygen-1.2.0.tar.gz".',
+        'SHOUT Deleting object from public bucket: "packages/oxygen-2.0.0-dev.tar.gz".',
+      ], fn: () async {
         final client = createPubApiClient(authToken: siteAdminToken);
 
         final pkgKey = dbService.emptyKey.append(Package, id: 'oxygen');
@@ -198,22 +276,22 @@ void main() {
         final timeBeforeRemoval = clock.now().toUtc();
         final rs = await client.adminInvokeAction(
           'package-delete',
-          AdminInvokeActionArguments(arguments: {'package': 'oxygen'}),
+          AdminInvokeActionArguments(arguments: {
+            'package': 'oxygen',
+            'state': 'true',
+          }),
         );
-
         expect(rs.output, {
-          'message':
-              contains('Package and all associated resources deleted.\n'),
           'package': 'oxygen',
-          'deletedPackages': 1,
-          'deletedPackageVersions': 3,
-          'deletedPackageVersionInfos': 3,
-          'deletedPackageVersionAssets': 15,
-          'deletedLikes': 1,
-          'deletedAuditLogs': 4,
-          'replacedByFixes': 0
+          'before': {'isAdminDeleted': false, 'adminDeletedAt': null},
+          'after': {'isAdminDeleted': true, 'adminDeletedAt': isNotNull},
         });
 
+        // trigger removal after 60+ days
+        await withClock(Clock.fixed(clock.daysFromNow(63)),
+            () => adminBackend.deleteAdminDeletedEntities());
+
+        // checks after actual removal
         final pkgAfterRemoval = await dbService.lookupOrNull<Package>(pkgKey);
         expect(pkgAfterRemoval, isNull);