Skip to content

Sync exported APIs on moderation + delete a moderated version's exported files. #8282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 18, 2024
Merged

Sync exported APIs on moderation + delete a moderated version's exported files. #8282

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9764b40
Sync exported APIs on moderation + delete a moderated version's expor…
isoos Nov 13, 2024
09aa954
Use forceDelete flag instead of direct deletion.
isoos Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/lib/admin/actions/moderate_package.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import '../../admin/backend.dart';
import '../../admin/models.dart';
import '../../package/api_export/api_exporter.dart';
import '../../package/backend.dart';
import '../../package/models.dart';
import '../../shared/datastore.dart';
Expand Down Expand Up @@ -81,6 +82,9 @@ Note: the action may take a longer time to complete as the public archive bucket
return pkg;
});

// sync exported API(s)
await apiExporter?.synchronizePackage(package);

// retract or re-populate public archive files
await packageBackend.tarballStorage.updatePublicArchiveBucket(
package: package,
Expand Down
4 changes: 4 additions & 0 deletions app/lib/admin/actions/moderate_package_versions.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import 'package:_pub_shared/utils/sdk_version_cache.dart';
import 'package:clock/clock.dart';

import '../../package/api_export/api_exporter.dart';
import '../../package/backend.dart';
import '../../package/models.dart';
import '../../scorecard/backend.dart';
Expand Down Expand Up @@ -114,6 +115,9 @@ Set the moderated flag on a package version (updating the flag and the timestamp
return v;
});

// sync exported API(s)
await apiExporter?.synchronizePackage(package);

// retract or re-populate public archive files
await packageBackend.tarballStorage.updatePublicArchiveBucket(
package: package,
Expand Down
13 changes: 13 additions & 0 deletions app/lib/package/api_export/api_exporter.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,19 @@ final class ApiExporter {
final versions = await packageBackend.tarballStorage
.listVersionsInCanonicalBucket(package);

// Check versions that are not exposed in the public API and if they are moderated, delete them.
for (final cv in versions.entries) {
final version = cv.key;
if (versionListing.versions.any((v) => v.version == version)) {
continue;
}
final pv = await packageBackend.lookupPackageVersion(package, version);
if (pv != null && pv.isModerated) {
Copy link
Member

@jonasfj jonasfj Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this.

versions is the list of archives objects that exist in tarballStorage.

Later we do:

    // Remove versions that are not exposed in the public API.
    versions.removeWhere(
      (version, _) => !versionListing.versions.any((v) => v.version == version),
    );

which ensures that versions only contains entries that are explicitly listed versionListing.

And synchronizeTarballs will delete files not present in versions.

So we don't need to delete files, the only case where synchronizeTarballs doesn't delete an archive not in versions, is if the object was recently modified.
This is to avoid race conditions, but recently modified is 3 hours -- we could set that lower if we're at all concerned here.

// We only delete the package if it is explicitly moderated.
// If we can't find it, then it's safer to assume that it's a bug.
Copy link
Contributor

@sigurdm sigurdm Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean we should log something in that case?

Copy link
Collaborator Author

@isoos isoos Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just leave it there. If it is a stray file, it will get GC-d soon enough.

await _api.package(package).deleteVersion(version);
}
}
// Remove versions that are not exposed in the public API.
versions.removeWhere(
(version, _) => !versionListing.versions.any((v) => v.version == version),
Expand Down
7 changes: 7 additions & 0 deletions app/lib/package/api_export/exported_api.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,13 @@ final class ExportedPackage {
}),
]);
}

/// Deletes the files related to this package and [version].
Future<void> deleteVersion(String version) async {
await Future.wait([
tarball(version).delete(),
]);
}
}

/// Information about an object to be used as source in a copy operation.
Expand Down
27 changes: 20 additions & 7 deletions app/test/admin/moderate_package_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ 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/versions.dart';
import 'package:test/test.dart';

import '../admin/models_test.dart';
Expand Down Expand Up @@ -294,15 +295,27 @@ void main() {
expect(docs3!.where((d) => d.package == 'oxygen'), isNotEmpty);
});

testWithProfile('archives are removed from public bucket', fn: () async {
final publicUri = Uri.parse('${activeConfiguration.storageBaseUrl}'
'/${activeConfiguration.publicPackagesBucketName}'
'/packages/oxygen-1.0.0.tar.gz');
testWithProfile('archives are removed from public buckets', fn: () async {
final publicUrls = [
'${activeConfiguration.storageBaseUrl}'
'/${activeConfiguration.publicPackagesBucketName}'
'/packages/oxygen-1.0.0.tar.gz',
'${activeConfiguration.storageBaseUrl}'
'/${activeConfiguration.exportedApiBucketName}'
'/latest/api/archives/oxygen-1.0.0.tar.gz',
'${activeConfiguration.storageBaseUrl}'
'/${activeConfiguration.exportedApiBucketName}'
'/$runtimeVersion/api/archives/oxygen-1.0.0.tar.gz',
];

Future<Uint8List?> expectStatusCode(int statusCode) async {
final rs1 = await http.get(publicUri);
expect(rs1.statusCode, statusCode);
return rs1.bodyBytes;
final rs = await Future.wait(
publicUrls.map((url) => http.get(Uri.parse(url))));
for (final r in rs) {
expect(r.statusCode, statusCode);
expect(r.bodyBytes, rs.first.bodyBytes);
}
return rs.first.bodyBytes;
}

final bytes = await expectStatusCode(200);
Expand Down
59 changes: 51 additions & 8 deletions app/test/admin/moderate_package_version_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:convert';
import 'dart:io';
import 'dart:typed_data';

import 'package:_pub_shared/data/account_api.dart';
Expand All @@ -19,6 +21,7 @@ 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/versions.dart';
import 'package:pub_dev/task/backend.dart';
import 'package:test/test.dart';

Expand Down Expand Up @@ -141,7 +144,7 @@ void main() {
expect(optionsUpdates.isRetracted, true);
});

testWithProfile('cannot moderated last visible version', fn: () async {
testWithProfile('cannot moderate last visible version', fn: () async {
await _moderate('oxygen', '1.2.0', state: true);
final p1 = await packageBackend.lookupPackage('oxygen');
expect(p1!.latestVersion, '1.0.0');
Expand Down Expand Up @@ -186,15 +189,29 @@ void main() {
);
});

testWithProfile('archive file is removed from public bucket', fn: () async {
testWithProfile('archive file is removed from public buckets',
fn: () async {
Future<Uint8List?> expectStatusCode(int statusCode,
{String version = '1.0.0'}) async {
final publicUri = Uri.parse('${activeConfiguration.storageBaseUrl}'
'/${activeConfiguration.publicPackagesBucketName}'
'/packages/oxygen-$version.tar.gz');
final rs1 = await http.get(publicUri);
expect(rs1.statusCode, statusCode);
return rs1.bodyBytes;
final publicUrls = [
'${activeConfiguration.storageBaseUrl}'
'/${activeConfiguration.publicPackagesBucketName}'
'/packages/oxygen-$version.tar.gz',
'${activeConfiguration.storageBaseUrl}'
'/${activeConfiguration.exportedApiBucketName}'
'/latest/api/archives/oxygen-$version.tar.gz',
'${activeConfiguration.storageBaseUrl}'
'/${activeConfiguration.exportedApiBucketName}'
'/$runtimeVersion/api/archives/oxygen-$version.tar.gz',
];

final rs = await Future.wait(
publicUrls.map((url) => http.get(Uri.parse(url))));
for (final r in rs) {
expect(r.statusCode, statusCode);
expect(r.bodyBytes, rs.first.bodyBytes);
}
return rs.first.bodyBytes;
}

final bytes = await expectStatusCode(200);
Expand All @@ -215,6 +232,32 @@ void main() {
expect(restoredBytes, bytes);
});

testWithProfile('versions file is updated in exported bucket',
fn: () async {
Future<void> expectIncluded(String version, bool isIncluded) async {
final prefixes = ['latest', runtimeVersion];
for (final prefix in prefixes) {
final url = '${activeConfiguration.storageBaseUrl}'
'/${activeConfiguration.exportedApiBucketName}'
'/$prefix/api/packages/oxygen';
final rs = await http.get(Uri.parse(url));
expect(rs.statusCode, 200);
final data = json.decode(utf8.decode(gzip.decode(rs.bodyBytes)))
as Map<String, dynamic>;
final versions = (data['versions'] as List)
.map((i) => (i as Map)['version'])
.toSet();
expect(versions.contains(version), isIncluded);
}
}

await expectIncluded('1.0.0', true);
await _moderate('oxygen', '1.0.0', state: true);
await expectIncluded('1.0.0', false);
await _moderate('oxygen', '1.0.0', state: false);
await expectIncluded('1.0.0', true);
});

testWithProfile('search is updated with new version', fn: () async {
await searchBackend.doCreateAndUpdateSnapshot(
FakeGlobalLockClaim(clock.now().add(Duration(seconds: 3))),
Expand Down
Loading