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

XilaiZhang · 2022-03-28T15:50:56Z

This smaller PR contains how we create branch event from github webhook, parse the event and store the event in the form of a 'branch' class into google cloud datastore.

This is a split of PR 1680. (some files are later modified, e.g. branch.proto). Context: #1680 (review)

CaseyHillers

Can you update the PR title and body to describe the changes being done? Stating it's part of larger work is great, but the issue should be tracking what small piece this PR is adding to the larger project. This gives the reviewers context of what to give feedback on versus what will be done in the future.

XilaiZhang · 2022-03-28T16:31:31Z

For sure, I have updated the title and description. Thank you!

CaseyHillers · 2022-03-28T16:41:29Z

app_dart/lib/src/model/appengine/branch.dart

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


Can we add a channel field as well? In the future, we can use this to get the current candidate branch for CPs

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

CaseyHillers · 2022-03-28T16:41:51Z

app_dart/lib/src/model/appengine/branch.dart

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


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

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.

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

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

The linked pasted doesn't work.

Can you share the snippet?

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>

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 }

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?

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!

CaseyHillers · 2022-03-28T16:42:29Z

app_dart/lib/src/model/appengine/branch.dart

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


We should avoid making values required, especially for values that are likely to come from GitHub

Sure, in later commits I have a get_branches request handler that relies on the 'lastactivity' timestamp to compare and filter branches later. I will add assertion of not nullable to those

CaseyHillers · 2022-03-28T16:43:04Z

app_dart/lib/src/model/github/create_event.dart

+part 'create_event.g.dart';
+
+@JsonSerializable(fieldRename: FieldRename.snake)
+class CreateEvent extends HookEvent {


This should get upstreamed in package:github first. Feel free to add me as a reviewer there

sure, raised pr at SpinlockLabs/github.dart#304

CaseyHillers · 2022-03-28T16:43:45Z

app_dart/lib/src/request_handlers/github_webhook.dart

@@ -86,6 +98,30 @@ class GithubWebhook extends RequestHandler<Body> {
    }
  }

+  Future<void> _handleCreateRequest(String rawRequest, DatastoreService datastore) async {


Avoid adding code to the RequestHandler classes as they're harder to test.

This should be moved into a BranchService class

Sure, moved

dashboard/lib/model/branch.proto

CaseyHillers · 2022-03-28T16:46:20Z

dashboard/lib/model/branch.proto

+
+import "lib/model/key.proto";
+
+message Branch {


Avoid adding code that isn't used yet. We can add this in the frontend PR

CaseyHillers · 2022-03-28T16:47:26Z

app_dart/lib/src/model/appengine/branch.dart

+
+/// The serialized representation of a [Branch].
+// TODO(tvolkert): Directly serialize [Branch] once frontends migrate to new serialization format.
+@JsonSerializable(createFactory: false, ignoreUnannotated: true)


Avoiding adding code that isn't used. We could simplify by having the backend adapt this to the proto

Thanks for the reply! Yeah in the later commits I updated protos and added request handlers that use json serializable. I will remove them from this PR and into later PRs.

Since this isn't used, it should be removed from this PR

XilaiZhang · 2022-03-28T21:07:13Z

Thanks for the reply! The newest commit addresses the feedbacks except the logic to create a key from null base key, which I wasn't very sure about and left a comment in the comment section. Thank you!

app_dart/lib/src/service/branch_service.dart

CaseyHillers · 2022-03-30T00:23:56Z

app_dart/lib/src/model/appengine/branch.dart

+
+/// The serialized representation of a [Branch].
+// TODO(tvolkert): Directly serialize [Branch] once frontends migrate to new serialization format.
+@JsonSerializable(createFactory: false, ignoreUnannotated: true)


Since this isn't used, it should be removed from this PR

CaseyHillers · 2022-03-30T00:24:47Z

app_dart/test/request_handlers/github_webhook_test.dart

@@ -2037,6 +2041,73 @@ void foo() {
    });
  });

+  group('github webhook create branch event', () {
+    test('should add branch to db if db is empty', () async {


Most of these tests should be moved into the test for the branch service. This should verify that branch service is called only for the create event

Sure, I moved the main testing logics to branch_service_test.

It is kind of hard to verify branchService is called for create event in github_webhook_test for the following reasons:

verify calls need to be called on a mockito object, and if we create a mock branchService object in test, it is a different instance of branchService from the one created in github_webhook.dart.

if we add branch service as a field to github_webhook, since the class is immutable we have to make branch service a final variable and provide it with a const default value. The final field constraint would prevent us from creating a branchService later in the code. And we don't have a good default const constructor for branchService because it relies on a datastore service.

If we mark the key function in branch service as visible for testing, the function will be constrained to the class and tests, and it can't be called in github_webhook.

If we create a static handling function in github_webhook.dart, we will need a mock instance of github_webhook, which doesn't seem ideal on top of the already non ideal static function.

I ended up creating a default error case for, the switch statement on event types pullrequest, checkrun and create. Therefore if we encounter an event type not conforming to the fore mentioned three types, the program will throw an error. And for github_webhook test, I made create event test similar to checkrun test in that, it won't check for how many times the function is called. Instead it only checks if the test runs without causing an exception. And we trust the switch statement categorizes event types correctly without adding a test to it.

Throwing an error on the webhooks will cause a large number of errors in our backend.

1 is the right idea, and you in the case no branch service was passed, it can be constructed like the previously logic had.

Thanks for the reply! umm does in the case no branch service was passed imply that we are passing branchService as a field variable to github_webhook? I think the challenge is that github_webhook is an immutable class which requires the branchService passed in to be final. If that is the case, we would need something like GithubWebhook(branchService: BranchService(DatastoreService(config.db, maxentitygroup), requestString) when we are initializing GithubWebhook, but at that point we can't properly provide the information of maxentitygroup and requestString?

Correct. Feel free to remove the immutability status of the handlers. For server handlers, ti doesn't matter much, but it's helpful for models

Thanks for the explanations! I removed the immutability of request handler class to better serve tests

app_dart/lib/src/request_handlers/github_webhook.dart

CaseyHillers · 2022-03-31T15:01:19Z

app_dart/lib/src/request_handlers/github_webhook.dart

@@ -84,11 +83,9 @@ class GithubWebhook extends RequestHandler<Body> {
          }
          break;
        case 'create':
-          final BranchService branchService = BranchService(datastore, rawRequest: stringRequest);
-          await branchService.handleCreateRequest();
+          branchService = branchService ?? BranchService(datastore, rawRequest: stringRequest);


Suggested change

branchService = branchService ?? BranchService(datastore, rawRequest: stringRequest);

branchService ??= BranchService(datastore, rawRequest: stringRequest);

Thanks for the reply! Always learning something new

CaseyHillers

XilaiZhang added 4 commits March 28, 2022 08:45

store branch from create webhook event

b7510a7

incorporate feedbacks

26cfd2a

files clean up

8d5a569

format files

2366432

XilaiZhang requested review from CaseyHillers and keyonghan March 28, 2022 15:51

XilaiZhang mentioned this pull request Mar 28, 2022

[release] Full stack logic to store, retrieve, update and display release branches #1680

Closed

CaseyHillers reviewed Mar 28, 2022

View reviewed changes

XilaiZhang changed the title ~~[split pr 1680] changes up to last Thursday~~ [split pr 1680] github webhook for create events and branch class initialization Mar 28, 2022

XilaiZhang requested a review from CaseyHillers March 28, 2022 16:31

CaseyHillers changed the title ~~[split pr 1680] github webhook for create events and branch class initialization~~ [app_dart] github webhook for create events and branch class initialization Mar 28, 2022

CaseyHillers reviewed Mar 28, 2022

View reviewed changes

address feedbacks

6da2be6

XilaiZhang requested a review from CaseyHillers March 28, 2022 21:09

CaseyHillers reviewed Mar 28, 2022

View reviewed changes

app_dart/lib/src/service/branch_service.dart Outdated Show resolved Hide resolved

XilaiZhang mentioned this pull request Mar 29, 2022

[datastore] make parent key non nullable flutter/flutter#100981

Open

XilaiZhang added 2 commits March 29, 2022 17:01

use github 9.1.0 and cleanup class doc

8fc9653

remove proto from this pr to frontend pr later

a39e46d

XilaiZhang requested a review from CaseyHillers March 30, 2022 00:21

CaseyHillers reviewed Mar 30, 2022

View reviewed changes

XilaiZhang added 2 commits March 29, 2022 22:47

refactor tests

29106b1

remove immutable meta to serve tests

6346639

XilaiZhang requested a review from CaseyHillers March 30, 2022 20:38

CaseyHillers reviewed Mar 31, 2022

View reviewed changes

if null expression

68bd0af

XilaiZhang requested a review from CaseyHillers March 31, 2022 16:29

CaseyHillers approved these changes Mar 31, 2022

View reviewed changes

XilaiZhang added the waiting for tree to go green Merge PR when tree becomes green via fluttergithubbot label Mar 31, 2022

fluttergithubbot merged commit 55392d3 into flutter:main Mar 31, 2022

XilaiZhang deleted the xilaizhang-split-1 branch March 31, 2022 17:12


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

		String get branch => key.id!.substring(key.id!.lastIndexOf('/') + 1);

	branchService = branchService ?? BranchService(datastore, rawRequest: stringRequest);
	branchService ??= BranchService(datastore, rawRequest: stringRequest);


		import "lib/model/key.proto";

		message Branch {

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

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

Conversation

XilaiZhang commented Mar 28, 2022 • edited

CaseyHillers left a comment

Choose a reason for hiding this comment

XilaiZhang commented Mar 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XilaiZhang commented Mar 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaseyHillers left a comment

Choose a reason for hiding this comment

XilaiZhang commented Mar 28, 2022 •

edited