Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ AppEngine version, listed here to ease deployment and troubleshooting.
## Next Release (replace with git tag when deployed)
* Bump runtimeVersion to `2024.12.12`.
* Upgraded runtime Dart SDK to `3.6.0`.
* Note: Started reporting unmapped fields and deleting `Package.isWithheld` and `Package.withheldReason`.

## `20241212t111200-all`
* Bump runtimeVersion to `2024.12.11`.
Expand Down
25 changes: 24 additions & 1 deletion app/lib/shared/integrity.dart
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ class IntegrityChecker {
final DatastoreDB _db;
final int _concurrency;

static const _knownUnmappedFields = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please document the scope of this (is it only about packages?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

renamed + done.

'Package.isWithheld',
'Package.withheldReason',
};
final _unmappedFields = <String>{};
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure, do we create a new IntegrityChecker for each run?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is a one-off object on each run.

final _userToOauth = <String, String?>{};
final _oauthToUser = <String, String>{};
final _deletedUsers = <String>{};
Expand Down Expand Up @@ -93,7 +98,13 @@ class IntegrityChecker {
yield* _checkAuditLogs();
yield* _checkModerationCases();
yield* _reportPubspecVersionIssues();
// TODO: report unmapped properties

if (_unmappedFields.isNotEmpty) {
for (final field in _unmappedFields) {
if (_knownUnmappedFields.contains(field)) continue;
yield 'Unmapped field found: $field.';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we report more? Like give some entity with this field, and the entitty type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are reporting EntityType.fieldName here. I'm not sure if we need to have a full entity identifier, as these fields are typically unused, but maybe we could report one id for each of the reported entries? That way we can store it in a Map and only keep the first one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Implemented this, PTAL.

}
}
} finally {
_httpClient.close();
}
Expand Down Expand Up @@ -448,6 +459,7 @@ class IntegrityChecker {
}

await for (final pvi in pviQuery.run()) {
_updateUnmappedFields(pvi);
final key = pvi.qualifiedVersionKey;
pviKeys.add(key);
yield* checkPackageVersionKey('PackageVersionInfo', key);
Expand Down Expand Up @@ -475,6 +487,7 @@ class IntegrityChecker {
..filter('package =', p.name);
final foundAssetIds = <String?>{};
await for (final pva in pvaQuery.run()) {
_updateUnmappedFields(pva);
final key = pva.qualifiedVersionKey;
if (pva.id !=
Uri(pathSegments: [pva.package!, pva.version!, pva.kind!]).path) {
Expand Down Expand Up @@ -907,13 +920,23 @@ class IntegrityChecker {
}
}

void _updateUnmappedFields(Model m) {
if (m is ExpandoModel && m.additionalProperties.isNotEmpty) {
for (final key in m.additionalProperties.keys) {
final field = [m.runtimeType.toString(), key].join('.');
_unmappedFields.add(field);
}
}
}

Stream<String> _queryWithPool<R extends Model>(
Stream<String> Function(R model) fn) async* {
final query = _db.query<R>();
final pool = Pool(_concurrency);
final futures = <Future<List<String>>>[];
try {
await for (final m in query.run()) {
_updateUnmappedFields(m);
final f = pool.withResource(() => fn(m).toList());
futures.add(f);
}
Expand Down
18 changes: 18 additions & 0 deletions app/lib/tool/backfill/backfill_new_fields.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import 'package:clock/clock.dart';
import 'package:logging/logging.dart';
import 'package:meta/meta.dart';
import 'package:pub_dev/account/models.dart';
import 'package:pub_dev/package/api_export/api_exporter.dart';
import 'package:pub_dev/package/backend.dart';
Expand All @@ -20,9 +21,11 @@ final _logger = Logger('backfill_new_fields');
/// release could remove the backfill from here.
Future<void> backfillNewFields() async {
await migrateIsBlocked();
await _removeKnownUnmappedFields();
}

/// Migrates entities from the `isBlocked` fields to the new `isModerated` instead.
@visibleForTesting
Future<void> migrateIsBlocked() async {
_logger.info('Migrating isBlocked...');
final pkgQuery = dbService.query<Package>()..filter('isBlocked =', true);
Expand Down Expand Up @@ -88,3 +91,18 @@ Future<void> migrateIsBlocked() async {

_logger.info('isBlocked migration completed.');
}

Future<void> _removeKnownUnmappedFields() async {
await for (final p in dbService.query<Package>().run()) {
if (p.additionalProperties.isEmpty) continue;
if (p.additionalProperties.containsKey('isWithheld') ||
p.additionalProperties.containsKey('withheldReason')) {
await withRetryTransaction(dbService, (tx) async {
final pkg = await tx.lookupValue<Package>(p.key);
pkg.additionalProperties.remove('isWithheld');
pkg.additionalProperties.remove('withheldReason');
tx.insert(pkg);
});
}
}
}
Loading