Skip to content
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

[app_dart] github webhook for create events and branch class initialization #1688

Merged
merged 10 commits into from
Mar 31, 2022
Merged

[app_dart] github webhook for create events and branch class initialization #1688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b7510a7
store branch from create webhook event
XilaiZhang Mar 23, 2022
26cfd2a
incorporate feedbacks
XilaiZhang Mar 23, 2022
8d5a569
files clean up
XilaiZhang Mar 25, 2022
2366432
format files
XilaiZhang Mar 25, 2022
6da2be6
address feedbacks
XilaiZhang Mar 28, 2022
8fc9653
use github 9.1.0 and cleanup class doc
XilaiZhang Mar 30, 2022
a39e46d
remove proto from this pr to frontend pr later
XilaiZhang Mar 30, 2022
29106b1
refactor tests
XilaiZhang Mar 30, 2022
6346639
remove immutable meta to serve tests
XilaiZhang Mar 30, 2022
68bd0af
if null expression
XilaiZhang Mar 31, 2022
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
44 changes: 44 additions & 0 deletions app_dart/lib/src/model/appengine/branch.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Copyright 2021 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:gcloud/db.dart';
import 'package:github/github.dart';

@Kind(name: 'Branch', idType: IdType.String)
class Branch extends Model<String> {
Branch({Key<String>? key, this.lastActivity}) {
parentKey = key?.parent;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no parents for Branch, so this is dead code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

umm maybe I am wrong, but my understanding is it if we keep the code this way, when we initialize the key using final Key<String> key = datastore.db.emptyKey.append<String>(Branch, id: id);, the parentKey will be set to datastore.db.emptyKey , which is a bit different from a null parentKey (what we would obtain if we remove this line).

When we are looking up and deleting entities, and providing a currentBranch.key, we are calling on the getter function of Key<T> get key => parentKey!.append(runtimeType, id: id); (which is not modifiable) part of the code to retrieve the key. This line of code asserts on parentKey not being null. If we initialized the value to be datastore.db.emptyKey we can pass this check. If we remove this line, parentKey will be defaulted to null, and causing the assertion of parentKey! to fail when we lookup or delete entities.

My understanding is based on how we currently generate and lookup keys. Maybe there is a way to workaround this. I could be wrong but I thought with the current topology we need this line to properly initialize a non null parentKey.

Copy link
Contributor

Choose a reason for hiding this comment

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

What you're describing is an issue in the tests. We should update the fake datastore to handle null parent keys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

umm so I have the logic at L115 - L120 of githubwebhook.dart currently looking like this https://github.com/flutter/cocoon/pull/1688/files#diff-3a0003489aad220cc50ef5c753a82ed536839ae6ed0ada1ac8b45c12e3ad1f2aR115-R123 . Maybe there is a workaround for this part but I am not sure how to use null as a base key for append to work

Copy link
Contributor

Choose a reason for hiding this comment

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

The linked pasted doesn't work.

Can you share the snippet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one of the sample errors looks like this

  Null check operator used on a null value
  package:gcloud/src/db/models.dart 106:30                               Model.key
  package:cocoon_service/src/service/branch_service.dart 42:59           BranchService.handleCreateRequest
  ===== asynchronous gap ===========================
  package:cocoon_service/src/request_handlers/github_webhook.dart 88:11  GithubWebhook.post
  ===== asynchronous gap ===========================
  test/request_handlers/github_webhook_test.dart 2053:7                  main.<fn>.<fn>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the line 106 it points to is from the key model class which is not modifiable

abstract class Model<T> {
  T? id;
  Key? parentKey;

  Key<T> get key => parentKey!.append(runtimeType, id: id);       //this is the line 106
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for investigating! This is TD and the real issue is that we allow parent key to be nullable. Can you file a Todo to make parent key non-nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I opened an issue at flutter/flutter#100981. In our case, should we just keep the line parentKey = key?.parent; to pass tests, or maybe there is a better workaround? Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

id = key?.id;
}

/// The timestamp (in milliseconds since the Epoch) of the last time
/// when current branch had activity.
@IntProperty(propertyName: 'lastActivity', required: false)
int? lastActivity;

/// The channel of current branch
@StringProperty(propertyName: 'channel', required: false)
String? channel;

/// [RepositorySlug] of where this commit exists.
RepositorySlug get slug => RepositorySlug.full(repository);

String get repository => key.id!.substring(0, key.id!.lastIndexOf('/'));

String get branch => key.id!.substring(key.id!.lastIndexOf('/') + 1);
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 add a channel field as well? In the future, we can use this to get the current candidate branch for CPs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added. I wasn't very sure about the use of channel. I thought if we have a channel name such as beta, then it would already manifest itself as being 'beta' in the branch name field? umm I am just wondering what's the difference between branch name and channel


@override
String toString() {
final StringBuffer buf = StringBuffer()
..write('$runtimeType(')
..write('id: $id')
..write(', key: ${parentKey == null ? null : key.id}')
..write(', branch: $branch')
..write(', channel: $channel')
..write(', repository: $repository')
..write(', lastActivity: $lastActivity')
..write(')');
return buf.toString();
}
}
19 changes: 15 additions & 4 deletions app_dart/lib/src/request_handlers/github_webhook.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,17 @@
import 'dart:async';
import 'dart:convert';

import 'package:cocoon_service/src/service/branch_service.dart';
import 'package:crypto/crypto.dart';
import 'package:github/github.dart';
import 'package:github/github.dart' show PullRequest, RepositorySlug, GitHub, PullRequestFile, IssueComment;
import 'package:github/hooks.dart';
import 'package:meta/meta.dart';

import '../model/github/checks.dart' as cocoon_checks;
import '../request_handling/body.dart';
import '../request_handling/exceptions.dart';
import '../request_handling/request_handler.dart';
import '../service/config.dart';
import '../service/datastore.dart';
import '../service/github_checks_service.dart';
import '../service/logging.dart';
import '../service/scheduler.dart';
Expand All @@ -35,12 +36,13 @@ const Set<String> kNeedsCheckLabelsAndTests = <String>{
final RegExp kEngineTestRegExp = RegExp(r'(tests?|benchmarks?)\.(dart|java|mm|m|cc)$');
final List<String> kNeedsTestsLabels = <String>['needs tests'];

@immutable
class GithubWebhook extends RequestHandler<Body> {
const GithubWebhook(
GithubWebhook(
Config config, {
required this.scheduler,
this.githubChecksService,
this.datastoreProvider = DatastoreService.defaultProvider,
this.branchService,
}) : super(config: config);

/// Cocoon scheduler to trigger tasks against changes from GitHub.
Expand All @@ -49,8 +51,12 @@ class GithubWebhook extends RequestHandler<Body> {
/// Github checks service. Used to provide build status to github.
final GithubChecksService? githubChecksService;

final DatastoreServiceProvider datastoreProvider;
BranchService? branchService;

@override
Future<Body> post() async {
final DatastoreService datastore = datastoreProvider(config.db);
final String? gitHubEvent = request!.headers.value('X-GitHub-Event');

if (gitHubEvent == null || request!.headers.value('X-Hub-Signature') == null) {
Expand All @@ -76,6 +82,11 @@ class GithubWebhook extends RequestHandler<Body> {
if (await scheduler.processCheckRun(checkRunEvent) == false) {
throw InternalServerError('Failed to process $checkRunEvent');
}
break;
case 'create':
branchService ??= BranchService(datastore, rawRequest: stringRequest);
await branchService!.handleCreateRequest();
break;
}

return Body.empty;
Expand Down
2 changes: 1 addition & 1 deletion app_dart/lib/src/request_handling/request_handler.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import 'exceptions.dart';
/// (before serialization). Subclasses whose HTTP responses don't include a
/// body should extend `RequestHandler<Body>` and return null in their service
/// handlers ([get] and [post]).
@immutable

abstract class RequestHandler<T extends Body> {
/// Creates a new [RequestHandler].
const RequestHandler({
Expand Down
58 changes: 58 additions & 0 deletions app_dart/lib/src/service/branch_service.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Copyright 2021 The Flutter Authors. 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:async';
import 'dart:convert';

import 'package:cocoon_service/src/service/datastore.dart';
import 'package:gcloud/db.dart';
import 'package:github/hooks.dart';

import '../model/appengine/branch.dart';
import '../request_handling/exceptions.dart';

class RetryException implements Exception {}

/// A class to manage GitHub branches.
///
/// Track branch activities such as branch creation, and helps manage release branches.
class BranchService {
BranchService(this.datastore, {this.rawRequest});

DatastoreService datastore;
String? rawRequest;

/// Parse a create github webhook event, and add it to datastore.
Future<void> handleCreateRequest() async {
final CreateEvent? createEvent = await _getCreateRequestEvent(rawRequest!);
if (createEvent == null) {
throw const BadRequestException('Expected create request event.');
}

final String? refType = createEvent.refType;
if (refType == 'tag') {
return;
}
final String? branch = createEvent.ref;
final String? repository = createEvent.repository!.slug().fullName;
final int lastActivity = createEvent.repository!.pushedAt!.millisecondsSinceEpoch;

final String id = '$repository/$branch';
final Key<String> key = datastore.db.emptyKey.append<String>(Branch, id: id);
final Branch currentBranch = Branch(key: key, lastActivity: lastActivity);
try {
await datastore.lookupByValue<Branch>(currentBranch.key);
} on KeyNotFoundException {
await datastore.insert(<Branch>[currentBranch]);
}
}

Future<CreateEvent?> _getCreateRequestEvent(String request) async {
try {
return CreateEvent.fromJson(json.decode(request) as Map<String, dynamic>);
} on FormatException {
return null;
}
}
}
2 changes: 1 addition & 1 deletion app_dart/pubspec.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ packages:
name: github
url: "https://pub.dartlang.org"
source: hosted
version: "8.5.0"
version: "9.1.0"
glob:
dependency: transitive
description:
Expand Down
2 changes: 1 addition & 1 deletion app_dart/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ dependencies:
file: ^6.1.2
fixnum: ^1.0.0
gcloud: ^0.8.4
github: ^8.3.0
github: ^9.1.0
googleapis: ^7.0.0
googleapis_auth: ^1.1.0
gql: ^0.13.0
Expand Down
29 changes: 23 additions & 6 deletions app_dart/test/request_handlers/github_webhook_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@ import 'package:cocoon_service/cocoon_service.dart';
import 'package:cocoon_service/src/model/appengine/commit.dart';
import 'package:cocoon_service/src/model/luci/buildbucket.dart';
import 'package:cocoon_service/src/request_handling/exceptions.dart';
import 'package:cocoon_service/src/service/datastore.dart';

import 'package:crypto/crypto.dart';
import 'package:github/github.dart';
import 'package:github/github.dart' hide Branch;
import 'package:googleapis/bigquery/v2.dart';
import 'package:mockito/mockito.dart';
import 'package:test/test.dart';
Expand All @@ -35,6 +36,7 @@ void main() {
late FakeGithubService githubService;
late FakeHttpRequest request;
late FakeScheduler scheduler;
late MockBranchService branchService;
late MockGitHub gitHubClient;
late MockGithubChecksUtil mockGithubChecksUtil;
late MockGithubChecksService mockGithubChecksService;
Expand Down Expand Up @@ -66,6 +68,7 @@ void main() {
tabledataResource: tabledataResource,
githubClient: gitHubClient,
);
branchService = MockBranchService();
issuesService = MockIssuesService();
when(issuesService.addLabelsToIssue(any, any, any)).thenAnswer((_) async => <IssueLabel>[]);
when(issuesService.createComment(any, any, any)).thenAnswer((_) async => IssueComment());
Expand Down Expand Up @@ -96,11 +99,11 @@ void main() {
});
});

webhook = GithubWebhook(
config,
githubChecksService: mockGithubChecksService,
scheduler: scheduler,
);
webhook = GithubWebhook(config,
datastoreProvider: (_) => DatastoreService(config.db, 5),
githubChecksService: mockGithubChecksService,
scheduler: scheduler,
branchService: branchService);

config.wrongHeadBranchPullRequestMessageValue = 'wrongHeadBranchPullRequestMessage';
config.wrongBaseBranchPullRequestMessageValue = '{{target_branch}} -> {{default_branch}}';
Expand Down Expand Up @@ -2037,6 +2040,20 @@ void foo() {
});
});

group('github webhook create branch event', () {
test('process create branch event', () async {
request.headers.set('X-GitHub-Event', 'create');
request.body = generateCreateBranchEvent('flutter-2.12-candidate.4', 'flutter/flutter');
final Uint8List body = utf8.encode(request.body!) as Uint8List;
final Uint8List key = utf8.encode(keyString) as Uint8List;
final String hmac = getHmac(body, key);
request.headers.set('X-Hub-Signature', 'sha1=$hmac');
await tester.post(webhook);

verify(branchService.handleCreateRequest()).called(1);
});
});

group('github webhook check_run event', () {
setUp(() {
request.headers.set('X-GitHub-Event', 'check_run');
Expand Down
83 changes: 83 additions & 0 deletions app_dart/test/service/branch_service_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
// Copyright 2021 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:cocoon_service/src/model/appengine/branch.dart';
import 'package:cocoon_service/src/service/branch_service.dart';
import 'package:cocoon_service/src/service/datastore.dart';

import 'package:gcloud/db.dart';
import 'package:test/test.dart';

import '../src/datastore/fake_config.dart';
import '../src/datastore/fake_datastore.dart';
import '../src/utilities/webhook_generators.dart';

void main() {
late FakeConfig config;
late FakeDatastoreDB db;
late DatastoreService datastoreService;
late BranchService branchService;

setUp(() {
db = FakeDatastoreDB();
config = FakeConfig(
dbValue: db,
);
datastoreService = DatastoreService(config.db, 5);
});

group('branch service test', () {
test('should add branch to db if db is empty', () async {
expect(db.values.values.whereType<Branch>().length, 0);
final String request = generateCreateBranchEvent('flutter-2.12-candidate.4', 'flutter/flutter');
branchService = BranchService(datastoreService, rawRequest: request);
await branchService.handleCreateRequest();

expect(db.values.values.whereType<Branch>().length, 1);
final Branch branch = db.values.values.whereType<Branch>().single;
expect(branch.repository, 'flutter/flutter');
expect(branch.branch, 'flutter-2.12-candidate.4');
});

test('should not add duplicate entity if branch already exists in db', () async {
expect(db.values.values.whereType<Branch>().length, 0);

const String id = 'flutter/flutter/flutter-2.12-candidate.4';
int lastActivity = DateTime.tryParse("2019-05-15T15:20:56Z")!.millisecondsSinceEpoch;
final Key<String> branchKey = db.emptyKey.append<String>(Branch, id: id);
final Branch currentBranch = Branch(key: branchKey, lastActivity: lastActivity);
db.values[currentBranch.key] = currentBranch;
expect(db.values.values.whereType<Branch>().length, 1);

final String request = generateCreateBranchEvent('flutter-2.12-candidate.4', 'flutter/flutter');
branchService = BranchService(datastoreService, rawRequest: request);
await branchService.handleCreateRequest();

expect(db.values.values.whereType<Branch>().length, 1);
final Branch branch = db.values.values.whereType<Branch>().single;
expect(branch.repository, 'flutter/flutter');
expect(branch.branch, 'flutter-2.12-candidate.4');
});

test('should add branch if it is different from previously existing branches', () async {
expect(db.values.values.whereType<Branch>().length, 0);

const String id = 'flutter/flutter/flutter-2.12-candidate.4';
int lastActivity = DateTime.tryParse("2019-05-15T15:20:56Z")!.millisecondsSinceEpoch;
final Key<String> branchKey = db.emptyKey.append<String>(Branch, id: id);
final Branch currentBranch = Branch(key: branchKey, lastActivity: lastActivity);
db.values[currentBranch.key] = currentBranch;

expect(db.values.values.whereType<Branch>().length, 1);

final String request = generateCreateBranchEvent('flutter-2.12-candidate.5', 'flutter/flutter');
branchService = BranchService(datastoreService, rawRequest: request);
await branchService.handleCreateRequest();

expect(db.values.values.whereType<Branch>().length, 2);
expect(db.values.values.whereType<Branch>().map<String>((Branch b) => b.branch),
containsAll(<String>['flutter-2.12-candidate.4', 'flutter-2.12-candidate.5']));
});
});
}
2 changes: 2 additions & 0 deletions app_dart/test/src/utilities/mocks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import 'package:cocoon_service/src/foundation/github_checks_util.dart';
import 'package:cocoon_service/src/service/access_client_provider.dart';
import 'package:cocoon_service/src/service/access_token_provider.dart';
import 'package:cocoon_service/src/service/bigquery.dart';
import 'package:cocoon_service/src/service/branch_service.dart';
import 'package:cocoon_service/src/service/buildbucket.dart';
import 'package:cocoon_service/src/service/gerrit_service.dart';
import 'package:cocoon_service/src/service/github_checks_service.dart';
Expand Down Expand Up @@ -59,6 +60,7 @@ const List<MockSpec<dynamic>> _mocks = <MockSpec<dynamic>>[
AccessClientProvider,
AccessTokenService,
BigqueryService,
BranchService,
BuildBucketClient,
FakeEntry,
IssuesService,
Expand Down
Loading