From e3cbafb6038f0aa90b1cc837f8e8b3a44144ad66 Mon Sep 17 00:00:00 2001 From: Istvan Soos Date: Mon, 16 Dec 2024 11:02:56 +0100 Subject: [PATCH 1/5] Report unmapped fields in integrity checks + delete Package.isWithheld and withheldReason. --- CHANGELOG.md | 1 + app/lib/shared/integrity.dart | 25 ++++++++++++++++++- .../tool/backfill/backfill_new_fields.dart | 18 +++++++++++++ 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d254f870bd..e5930dac0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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`. diff --git a/app/lib/shared/integrity.dart b/app/lib/shared/integrity.dart index bc90eea83a..39b1b9c329 100644 --- a/app/lib/shared/integrity.dart +++ b/app/lib/shared/integrity.dart @@ -40,6 +40,11 @@ class IntegrityChecker { final DatastoreDB _db; final int _concurrency; + static const _knownUnmappedFields = { + 'Package.isWithheld', + 'Package.withheldReason', + }; + final _unmappedFields = {}; final _userToOauth = {}; final _oauthToUser = {}; final _deletedUsers = {}; @@ -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.'; + } + } } finally { _httpClient.close(); } @@ -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); @@ -475,6 +487,7 @@ class IntegrityChecker { ..filter('package =', p.name); final foundAssetIds = {}; 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) { @@ -907,6 +920,15 @@ 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 _queryWithPool( Stream Function(R model) fn) async* { final query = _db.query(); @@ -914,6 +936,7 @@ class IntegrityChecker { final futures = >>[]; try { await for (final m in query.run()) { + _updateUnmappedFields(m); final f = pool.withResource(() => fn(m).toList()); futures.add(f); } diff --git a/app/lib/tool/backfill/backfill_new_fields.dart b/app/lib/tool/backfill/backfill_new_fields.dart index 2871f30097..7cf1a53df4 100644 --- a/app/lib/tool/backfill/backfill_new_fields.dart +++ b/app/lib/tool/backfill/backfill_new_fields.dart @@ -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'; @@ -20,9 +21,11 @@ final _logger = Logger('backfill_new_fields'); /// release could remove the backfill from here. Future backfillNewFields() async { await migrateIsBlocked(); + await _removeKnownUnmappedFields(); } /// Migrates entities from the `isBlocked` fields to the new `isModerated` instead. +@visibleForTesting Future migrateIsBlocked() async { _logger.info('Migrating isBlocked...'); final pkgQuery = dbService.query()..filter('isBlocked =', true); @@ -88,3 +91,18 @@ Future migrateIsBlocked() async { _logger.info('isBlocked migration completed.'); } + +Future _removeKnownUnmappedFields() async { + await for (final p in dbService.query().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(p.key); + pkg.additionalProperties.remove('isWithheld'); + pkg.additionalProperties.remove('withheldReason'); + tx.insert(pkg); + }); + } + } +} From a4e9087f63541897058ef6a212e64e35552b0fe5 Mon Sep 17 00:00:00 2001 From: Istvan Soos Date: Mon, 16 Dec 2024 11:16:45 +0100 Subject: [PATCH 2/5] dart format --- app/lib/shared/integrity.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/lib/shared/integrity.dart b/app/lib/shared/integrity.dart index 39b1b9c329..2b52b71006 100644 --- a/app/lib/shared/integrity.dart +++ b/app/lib/shared/integrity.dart @@ -102,7 +102,7 @@ class IntegrityChecker { if (_unmappedFields.isNotEmpty) { for (final field in _unmappedFields) { if (_knownUnmappedFields.contains(field)) continue; - yield 'Unmapped field found: $field.'; + yield 'Unmapped field found: $field.'; } } } finally { From 9d9ae4afa193de1207c584478212b1890756ab50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Istv=C3=A1n=20So=C3=B3s?= Date: Mon, 16 Dec 2024 11:06:15 +0100 Subject: [PATCH 3/5] Update CHANGELOG.md Co-authored-by: Sigurd Meldgaard --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e5930dac0e..6b12885c76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,8 @@ 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`. + * Note: Started reporting unmapped fields + * Started and deleting `Package.isWithheld` and `Package.withheldReason`. ## `20241212t111200-all` * Bump runtimeVersion to `2024.12.11`. From 383d78fa50c8eb540fdb43769621b6e703a0324b Mon Sep 17 00:00:00 2001 From: Istvan Soos Date: Mon, 16 Dec 2024 15:54:37 +0100 Subject: [PATCH 4/5] Rename to _allowedUnmappedFields + documentation. --- app/lib/shared/integrity.dart | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/app/lib/shared/integrity.dart b/app/lib/shared/integrity.dart index 2b52b71006..d98d9ec2ce 100644 --- a/app/lib/shared/integrity.dart +++ b/app/lib/shared/integrity.dart @@ -35,15 +35,19 @@ import 'utils.dart' show canonicalizeVersion, ByteArrayEqualsExt; final _logger = Logger('integrity.check'); final _random = math.Random.secure(); +/// The unmapped/unused fields that we expect to be present on some entities. +/// The presence of such fields won't be reported as integrity issue, only +/// the absent ones will be reported. +const _allowedUnmappedFields = { + 'Package.isWithheld', + 'Package.withheldReason', +}; + /// Checks the integrity of the datastore. class IntegrityChecker { final DatastoreDB _db; final int _concurrency; - static const _knownUnmappedFields = { - 'Package.isWithheld', - 'Package.withheldReason', - }; final _unmappedFields = {}; final _userToOauth = {}; final _oauthToUser = {}; @@ -101,7 +105,7 @@ class IntegrityChecker { if (_unmappedFields.isNotEmpty) { for (final field in _unmappedFields) { - if (_knownUnmappedFields.contains(field)) continue; + if (_allowedUnmappedFields.contains(field)) continue; yield 'Unmapped field found: $field.'; } } From f4f2481cb4ce7a9dd2049a183d470b5409d658ed Mon Sep 17 00:00:00 2001 From: Istvan Soos Date: Mon, 16 Dec 2024 15:59:05 +0100 Subject: [PATCH 5/5] retain and expose Model.id with _unmappedFieldsToObject --- app/lib/shared/integrity.dart | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/app/lib/shared/integrity.dart b/app/lib/shared/integrity.dart index d98d9ec2ce..b300a56f8c 100644 --- a/app/lib/shared/integrity.dart +++ b/app/lib/shared/integrity.dart @@ -48,7 +48,9 @@ class IntegrityChecker { final DatastoreDB _db; final int _concurrency; - final _unmappedFields = {}; + /// Maps an unmapped field in the form of `.` to an + /// object identifier (usually the `id` value of the entity). + final _unmappedFieldsToObject = {}; final _userToOauth = {}; final _oauthToUser = {}; final _deletedUsers = {}; @@ -103,10 +105,10 @@ class IntegrityChecker { yield* _checkModerationCases(); yield* _reportPubspecVersionIssues(); - if (_unmappedFields.isNotEmpty) { - for (final field in _unmappedFields) { - if (_allowedUnmappedFields.contains(field)) continue; - yield 'Unmapped field found: $field.'; + if (_unmappedFieldsToObject.isNotEmpty) { + for (final entry in _unmappedFieldsToObject.entries) { + if (_allowedUnmappedFields.contains(entry.key)) continue; + yield 'Unmapped field found: "${entry.key}" on entity "${entry.value}".'; } } } finally { @@ -927,8 +929,9 @@ 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); + final qualifiedField = [m.runtimeType.toString(), key].join('.'); + if (_unmappedFieldsToObject.containsKey(qualifiedField)) continue; + _unmappedFieldsToObject[qualifiedField] = m.id.toString(); } } }