Add some target protection to `RerunProdTask`. (#4357)
Closes https://github.com/flutter/flutter/issues/165522.
diff --git a/app_dart/lib/src/request_handlers/rerun_prod_task.dart b/app_dart/lib/src/request_handlers/rerun_prod_task.dart
index c80bb62..21bd124 100644
--- a/app_dart/lib/src/request_handlers/rerun_prod_task.dart
+++ b/app_dart/lib/src/request_handlers/rerun_prod_task.dart
@@ -30,7 +30,7 @@
///
/// Expects either [taskKeyParam] or a set of params that give enough detail to lookup a task in datastore.
@immutable
-class RerunProdTask extends ApiRequestHandler<Body> {
+final class RerunProdTask extends ApiRequestHandler<Body> {
const RerunProdTask({
required super.config,
required super.authenticationProvider,
@@ -155,9 +155,29 @@
if (ciYaml.isFusion)
...ciYaml.postsubmitTargets(type: CiType.fusionEngine),
];
- final target = targets.singleWhere(
- (Target target) => target.value.name == task.name,
- );
+
+ // Fail "nicely" on missing a cooresponding target.
+ final Target target;
+ {
+ final matched = targets.where((target) => target.value.name == task.name);
+
+ // Could happen (https://github.com/flutter/flutter/issues/165522).
+ if (matched.isEmpty) {
+ log.warn(
+ 'No matching target ("${task.name}") found in ${targets.map((t) => t.value.name).toList()}.',
+ );
+ return;
+ }
+
+ // Can't happen, as it would be an invalid .ci.yaml.
+ if (matched.length > 1) {
+ throw StateError(
+ 'More than one target ("${task.name}") matched in ${targets.map((t) => t.value.name).toList()}.',
+ );
+ }
+
+ target = matched.first;
+ }
// Prepares Firestore task.
final documentName = FirestoreTaskDocumentName(
diff --git a/app_dart/test/request_handlers/rerun_prod_task_test.dart b/app_dart/test/request_handlers/rerun_prod_task_test.dart
index b12a5fb..7636531 100644
--- a/app_dart/test/request_handlers/rerun_prod_task_test.dart
+++ b/app_dart/test/request_handlers/rerun_prod_task_test.dart
@@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+import 'package:cocoon_common_test/cocoon_common_test.dart';
+import 'package:cocoon_server/logging.dart';
import 'package:cocoon_server_test/test_logging.dart';
import 'package:cocoon_service/cocoon_service.dart';
import 'package:cocoon_service/src/model/appengine/commit.dart';
@@ -188,6 +190,82 @@
);
});
+ test('No matching target fails gracefully', () async {
+ final task = generateTask(
+ 2,
+ // This task is in datastore, but not in .ci.yaml.
+ name: 'Windows C',
+ parent: commit,
+ status: Task.statusSucceeded,
+ );
+ config.db.values[task.key] = task;
+ config.db.values[commit.key] = commit;
+ tester.requestData = {...tester.requestData, 'task': 'Windows C'};
+ expect(await tester.post(handler), Body.empty);
+ verifyNever(
+ mockLuciBuildService.checkRerunBuilder(
+ commit: anyNamed('commit'),
+ datastore: anyNamed('datastore'),
+ task: anyNamed('task'),
+ target: anyNamed('target'),
+ tags: anyNamed('tags'),
+ firestoreService: anyNamed('firestoreService'),
+ taskDocument: anyNamed('taskDocument'),
+ ignoreChecks: false,
+ ),
+ );
+
+ expect(
+ log,
+ bufferedLoggerOf(
+ contains(
+ logThat(
+ severity: atLeastWarning,
+ message: contains('No matching target'),
+ ),
+ ),
+ ),
+ );
+ });
+
+ test('Too many matching targets fails forcefully', () async {
+ ciYamlFetcher.ciYaml = exampleNaughtyConfig;
+ final task = generateTask(
+ 2,
+ name: 'Windows A',
+ parent: commit,
+ status: Task.statusSucceeded,
+ );
+
+ config.db.values[task.key] = task;
+ config.db.values[commit.key] = commit;
+ tester.requestData = {...tester.requestData, 'task': 'Windows A'};
+ await expectLater(
+ tester.post(handler),
+ throwsA(
+ isA<StateError>().having(
+ (b) => b.message,
+ 'message',
+ contains(
+ 'More than one target ("Windows A") matched in [Windows A, Windows A]',
+ ),
+ ),
+ ),
+ );
+ verifyNever(
+ mockLuciBuildService.checkRerunBuilder(
+ commit: anyNamed('commit'),
+ datastore: anyNamed('datastore'),
+ task: anyNamed('task'),
+ target: anyNamed('target'),
+ tags: anyNamed('tags'),
+ firestoreService: anyNamed('firestoreService'),
+ taskDocument: anyNamed('taskDocument'),
+ ignoreChecks: false,
+ ),
+ );
+ });
+
test('Re-schedule without any parameters raises exception', () async {
tester.requestData = <String, dynamic>{};
expect(() => tester.post(handler), throwsA(isA<BadRequestException>()));
diff --git a/app_dart/test/src/service/fake_scheduler.dart b/app_dart/test/src/service/fake_scheduler.dart
index 470b4e5..21e815c 100644
--- a/app_dart/test/src/service/fake_scheduler.dart
+++ b/app_dart/test/src/service/fake_scheduler.dart
@@ -133,6 +133,20 @@
},
);
+final exampleNaughtyConfig = CiYamlSet(
+ slug: Config.flutterSlug,
+ branch: Config.defaultBranch(Config.flutterSlug),
+ yamls: {
+ CiType.any: pb.SchedulerConfig(
+ enabledBranches: <String>[Config.defaultBranch(Config.flutterSlug)],
+ targets: <pb.Target>[
+ pb.Target(name: 'Windows A', scheduler: pb.SchedulerSystem.luci),
+ pb.Target(name: 'Windows A', scheduler: pb.SchedulerSystem.luci),
+ ],
+ ),
+ },
+);
+
final exampleFlakyConfig = CiYamlSet(
slug: Config.flutterSlug,
branch: Config.defaultBranch(Config.flutterSlug),
